1540043 - Get rid of nsIPlaintextEditor interface

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement, P3)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Once bug 1540037 is fixed, we can get rid of nsIPlaintextEditor instance which is currently not used by so many places.
https://searchfox.org/comm-central/search?q=nsIPlaintextEditor&case=false&regexp=false&path=
https://github.com/therealglazou/bluegriffon/search?q=nsIPlaintextEditor&type=Code

Even though comm-central and BlueGriffon need some changes, I think that we should move all methods and constants in nsIPlaintextEditor to nsIEditor and get rid of nsIPlaintextEditor.

Fortunately, QI cost from nsIEditorto nsIPlaintextEditor is really cheap in Firefox, Thunderbird and BlueGriffon even now. But we can make it zero cost and avoid maintaining similar interfaces for (always) accessing same instance. I.e., yes, I'd like to reduce the number of interfaces around editor.

Comment 1

[Jorg K (CEST = GMT+2)]

Note that editor/ui is not used by Thunderbird any more. Only these call sites are relevant to TB:
https://searchfox.org/comm-central/search?q=nsIPlaintextEditor&case=false&regexp=false&path=%5Email

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

First, just move all members of nsIPlaintextEditor to nsIEditor and make it inherits nsIEditor.

After landing it and after a while (comm-central replaces nsIPlaintextEditor for its contents to nsIEditor and fix each QI), get rid of nsIPlaintextEditor completely.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1540043 - part 1: Move all constants and methods of `nsIPlaintextEditor` to `nsIEditor` and make `nsIPlaintextEditor` inherit `nsIEditor` r=m_kato!

For preparing to remove nsIPlaintextEditor interface, this patch moves
all of them to nsIEditor, but for avoiding bustage in comm-central, makes
nsIPlaintextEditor inherit nsIEditor for now (i.e., even with this patch,
script can access old nsIPlaintextEditor members with the interface.

In C++ code, this patch moves SetWrapColumn(), InsertTextAsAction(),
InsertTextAsSubAction() and InsertLineBreakAsSubAction() because
they do common things between TextEditor and HTMLEditor. On the other
hand, this does not move TextEditor::GetTextLength() because it's designed
only for TextEditor.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1160e6ba7a5d
part 1: Move all constants and methods of `nsIPlaintextEditor` to `nsIEditor` and make `nsIPlaintextEditor` inherit `nsIEditor` r=m_kato

Comment 5

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/1160e6ba7a5d

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1540043 - part 2: Make comm-central stop using `nsIPlaintextEditor` interface

I'd like somebody to review the patch for comm-central...

Comment 7

[Alexandru Ionescu (needinfo me) [:alexandrui]]

== Change summary for alert #24799 (as of Sat, 25 Jan 2020 04:22:24 GMT) ==

Improvements:

2% raptor-motionmark-htmlsuite-firefox macosx1014-64-shippable opt 50.09 -> 51.25

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=24799

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Could you review attachment 9123284 [details] [diff] [review]? Or redirect this review request to somebody who can do it?

(All nsIPlaintextEditor members have been moved to nsIEditor and nsIPlaintextEditor inherits nsIEditor for keeping its users accessible to the moved members via nsIPlaintextEditor interface, but I'd like to remove nsIPlaintextEditor interface completely as soon as possible. So, JS in comm-central should stop QI from nsIEditor to nsIPlaintextEditor and refer nsIEditor interface for accessing its constants.)

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9123284 [details] [diff] [review]
Bug 1540043 - part 2: Make comm-central stop using `nsIPlaintextEditor` interface

Review of attachment 9123284 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, thx! r=mkmelin

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you!

I'll post new patch which removes nsIPlaintextEditor completely.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/857a1d5c4507
part 2: Make comm-central stop using nsIPlaintextEditor interface r=jorgk

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Pulsebot from comment #11)

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/857a1d5c4507
part 2: Make comm-central stop using nsIPlaintextEditor interface r=jorgk

Oops, I forgot to change the reviewer, sorry for that.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1540043 - part 3: Remove `nsIPlaintextEditor` interface r=m_kato!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a489fb692c9f
part 3: Remove `nsIPlaintextEditor` interface r=m_kato

Comment 15

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/a489fb692c9f