`HTMLEditor` will be directly derived from `EditorBase`, not via `TextEditor`

[Masayuki Nakano]

Hi,

I've rewritten the editor module a lot in a half of decade, and I reach
a big milestone now. That is, `HTMLEditor` will stop being derived from
`TextEditor`, instead, will be directly derived from `EditorBase`.
Currently, the image is:

`HTMLEditor` <- `TextEditor` <- `EditorBase` <- `nsIEditor`
<- `nsIHTMLEditor` etc.

But this will be:

`HTMLEditor` <- `EditorBase` <- `nsIEditor`
<- `nsIHTMLEditor` etc.

`TextEditor` <- `EditorBase` <- `nsIEditor`

The code shared by both `HTMLEditor` and `TextEditor` have already been
moved to `EditorBase` and `EditorUtils`.

One of the big change is, `nsIEditor::AsTextEditor()` and
`nsIEditor::AsHTMLEditor()` will be unsafe like `nsINode::AsContent()`.
Currently, they check its type and returns `nullptr` if `AsHTMLEditor()`
is called with `TextEditor` instance. But new utility methods will take
same form as DOM nodes' ones.

`nsIEditor::IsTextEditor()` and `nsIEditor::IsHTMLEditor()` check
whether the instance is `TextEditor` or `HTMLEditor` (they are same as
current `EditorBase`'s API).

`nsIEditor::GetAsTextEditor()` and `nsIEditor::GetAsHTMLEditor()` will
check whether the instance is `TextEditor` or `HTMLEditor` and iif it
matches, returns its pointer, and otherwise, `nullptr`.

`nsIEditor::AsTextEditor()` and `nsIEditor::AsHTMLEditor()` will just
cast its pointer without the type check. So, they are faster, but must
be used after `Is*Editor()` check.

For making the code working with `nsIEditor` simpler, there will be new
static methods `TextEditor::GetFrom(nsIEditor*)` and
`HTMLEditor::GetFrom(nsIEditor*)`, and their const versions. They return
non-nullptr if and only if the instance type matches the class.

I hope this changes will help you to read editor code easier!

The former change is handled in bug 1713760
https://bugzilla.mozilla.org/show_bug.cgi?id=1713760

And the latter change is handled in bug 1714918
https://bugzilla.mozilla.org/show_bug.cgi?id=1714918

Thank you Makoto-san for your a lot of review about this kind of changes!

--
Masayuki Nakano <masa...@d-toybox.com>
Working on DOM, Events, editor and IME handling for Gecko

[Mirko Brodesser]

Hello Masayuki,

On 10.06.21 02:29, Masayuki Nakano wrote:
> Hi,
>
> I've rewritten the editor module a lot in a half of decade, and I
> reach a big milestone now. That is, `HTMLEditor` will stop being
> derived from `TextEditor`, instead, will be directly derived from
> `EditorBase`. Currently, the image is:
>
> `HTMLEditor` <- `TextEditor` <- `EditorBase` <- `nsIEditor`
>              <- `nsIHTMLEditor` etc.
>
> But this will be:
>
> `HTMLEditor` <- `EditorBase` <- `nsIEditor`
>              <- `nsIHTMLEditor` etc.
>
> `TextEditor` <- `EditorBase` <- `nsIEditor`

That looks cleaner, thanks!

>
> The code shared by both `HTMLEditor` and `TextEditor` have already
> been moved to `EditorBase` and `EditorUtils`.
>
> One of the big change is, `nsIEditor::AsTextEditor()` and
> `nsIEditor::AsHTMLEditor()` will be unsafe like
> `nsINode::AsContent()`. Currently, they check its type and returns
> `nullptr` if `AsHTMLEditor()` is called with `TextEditor` instance. 
> But new utility methods will take same form as DOM nodes' ones.

Would it be possible to get rid of `AsTextEditor` and `AsHTMLEditor`? It
could be changed in a separate review.

>
> `nsIEditor::IsTextEditor()` and `nsIEditor::IsHTMLEditor()` check
> whether the instance is `TextEditor` or `HTMLEditor` (they are same as
> current `EditorBase`'s API).
>
> `nsIEditor::GetAsTextEditor()` and `nsIEditor::GetAsHTMLEditor()` will
> check whether the instance is `TextEditor` or `HTMLEditor` and iif it
> matches, returns its pointer, and otherwise, `nullptr`.
>
> `nsIEditor::AsTextEditor()` and `nsIEditor::AsHTMLEditor()` will just
> cast its pointer without the type check. So, they are faster, but must
> be used after `Is*Editor()` check.

Presuming efficiency is in practice not affected too much, how about the
following: would it be possible to return something like
`Maybe<TextEditor&>`? That would make misuse less likely.

>
> For making the code working with `nsIEditor` simpler, there will be
> new static methods `TextEditor::GetFrom(nsIEditor*)` and
> `HTMLEditor::GetFrom(nsIEditor*)`, and their const versions. They
> return non-nullptr if and only if the instance type matches the class.

Why are those methods needed?

Mirko

[Masayuki Nakano]

On 21/06/10 17:05, Mirko Brodesser wrote:
>
>> The code shared by both `HTMLEditor` and `TextEditor` have already
>> been moved to `EditorBase` and `EditorUtils`.
>>
>> One of the big change is, `nsIEditor::AsTextEditor()` and
>> `nsIEditor::AsHTMLEditor()` will be unsafe like
>> `nsINode::AsContent()`. Currently, they check its type and returns
>> `nullptr` if `AsHTMLEditor()` is called with `TextEditor` instance. 
>> But new utility methods will take same form as DOM nodes' ones.
>
> Would it be possible to get rid of `AsTextEditor` and `AsHTMLEditor`?
> It could be changed in a separate review.
>>
>> `nsIEditor::IsTextEditor()` and `nsIEditor::IsHTMLEditor()` check
>> whether the instance is `TextEditor` or `HTMLEditor` (they are same
>> as current `EditorBase`'s API).
>>
>> `nsIEditor::GetAsTextEditor()` and `nsIEditor::GetAsHTMLEditor()`
>> will check whether the instance is `TextEditor` or `HTMLEditor` and
>> iif it matches, returns its pointer, and otherwise, `nullptr`.
>>
>> `nsIEditor::AsTextEditor()` and `nsIEditor::AsHTMLEditor()` will just
>> cast its pointer without the type check. So, they are faster, but
>> must be used after `Is*Editor()` check.
> Presuming efficiency is in practice not affected too much, how about
> the following: would it be possible to return something like
> `Maybe<TextEditor&>`? That would make misuse less likely.

First of all, the motivation of this change is to avoid some developers
to misunderstand the meaning of `As*Editor()` at reading the editor
code. Therefore, I'd like to take exactly same naming style as our
internal DOM API.

Additionally, I guess that there is a performance reason or something
keeping unsafe `nsINode::As*()` methods.  If it's a performance reason,
I love using it in editor module too because unfortunately, there are
this kind of ugly hack outside the `TextEditor` and `HTMLEditor`:

https://searchfox.org/mozilla-central/rev/2c991232499e826e46f9d976eb653817340ba389/editor/libeditor/EditorBase.cpp#5341-5342

Therefore, I don't want to use `Maybe` etc here.

(I'd like to rename `WidgetEvent::As*Event()` to `GetAs*Event()` too...)

>>
>> For making the code working with `nsIEditor` simpler, there will be
>> new static methods `TextEditor::GetFrom(nsIEditor*)` and
>> `HTMLEditor::GetFrom(nsIEditor*)`, and their const versions. They
>> return non-nullptr if and only if the instance type matches the class.
>
> Why are those methods needed?

If root caller is JS, we need to access the concrete class via
`nsIEditor` pointer. In the cases, we can avoid the double null check
(one for given point, the other for the result of `GetAs*Editor()`) in
same place.

[Mirko Brodesser]

I'm not sure whether the same style like for the internal DOM API, e.g.
`nsINode::AsContent`
<https://searchfox.org/mozilla-central/rev/af8e5d37fd56be90ccddae2203e7b875d3f3ae87/dom/base/nsINode.h#537>,
is justified. The latter performs a `reinterpret_cast`.
`nsIEditor::AsTextEditor` performs a `static_cast`.

I'm also not sure whether the style of the internal DOM API is indeed
desirable. It seems not, because of the `reinterpret_cast`.

In an ideal world, `nsIEditor` shouldn't know about its derived classes,
so there should be no `nsIEditor::IsHTMLEditor()`. From
https://en.wikipedia.org/wiki/SOLID <https://en.wikipedia.org/wiki/SOLID>:

"The Liskov substitution principle
<https://en.wikipedia.org/wiki/Liskov_substitution_principle>:
"Functions that use pointers or references to base classes must be able
to use objects of derived classes without knowing it"." I guess, that's
not the case here, requiring `IsHTMLEditor()`.

I'm not sufficiently familiar with editor code to propose a working
alternative, unfortunately.

[Masayuki Nakano]

On 21/06/11 17:23, Mirko Brodesser wrote:
>
> On 10.06.21 10:44, Masayuki Nakano wrote:
>> First of all, the motivation of this change is to avoid some
>> developers to misunderstand the meaning of `As*Editor()` at reading
>> the editor code. Therefore, I'd like to take exactly same naming style
>> as our internal DOM API.
>
> I'm not sure whether the same style like for the internal DOM API, e.g.
> `nsINode::AsContent`
> <https://searchfox.org/mozilla-central/rev/af8e5d37fd56be90ccddae2203e7b875d3f3ae87/dom/base/nsINode.h#537>,
> is justified.

It's checked only in debug build (`MOZ_ASSERT`). (Oh, but I realize that
`nsINode::GetAsContent()` has gone? Or am I confused with different API?)

> In an ideal world, `nsIEditor` shouldn't know about its derived classes,
> so there should be no `nsIEditor::IsHTMLEditor()`. From
> https://en.wikipedia.org/wiki/SOLID <https://en.wikipedia.org/wiki/SOLID>:
>
> "The Liskov substitution principle
> <https://en.wikipedia.org/wiki/Liskov_substitution_principle>:
> "Functions that use pointers or references to base classes must be able
> to use objects of derived classes without knowing it"." I guess, that's
> not the case here, requiring `IsHTMLEditor()`.

Yes, it is. Previously, `TextEditor` inherited `nsIPlaintextEditor` and
`EditorBase`, `HTMLEditor` inherited `nsIHTMLEditor` and `TextEditor`.
To access `TextEditor` or `HTMLEditor` (semi-)directly, we did:
* nsCOMPtr<nsIPlaintextEditor> plaintextEditor = do_QueryInterface(aEditor);
* nsCOMPtr<nsIHTMLEditor> htmlEditor = do_QueryInterface(aEditor);

and called only interface methods. However, QI is too slow and every
interface method's virtual call also appeared in profiles. Therefore, I
changed to use concrete classes instead of our old "ideal" style for
performance after changing `nsIEditor` to a builtin class.

I agree with current editor's design is far from ideal one. Actually, we
do a lot of bad things in EditorBase.cpp:
https://searchfox.org/mozilla-central/search?q=Is%28Text%7CHTML%29Editor&path=EditorBase.cpp&case=true&regexp=true

Some of them could be rewritten with slower approach or duplicating code
in both `TextEditor` and `HTMLEditor`, but not all.

[Mirko Brodesser]

On 12.06.21 04:16, Masayuki Nakano wrote:
> On 21/06/11 17:23, Mirko Brodesser wrote:
>>
>> On 10.06.21 10:44, Masayuki Nakano wrote:
>>> First of all, the motivation of this change is to avoid some
>>> developers to misunderstand the meaning of `As*Editor()` at reading
>>> the editor code. Therefore, I'd like to take exactly same naming
>>> style as our internal DOM API.
>>
>> I'm not sure whether the same style like for the internal DOM API,
>> e.g. `nsINode::AsContent`
>> <https://searchfox.org/mozilla-central/rev/af8e5d37fd56be90ccddae2203e7b875d3f3ae87/dom/base/nsINode.h#537>,
>> is justified.
>
> It's checked only in debug build (`MOZ_ASSERT`). (Oh, but I realize
> that `nsINode::GetAsContent()` has gone? Or am I confused with
> different API?)
>

It's gone or never existed.

>> In an ideal world, `nsIEditor` shouldn't know about its derived
>> classes, so there should be no `nsIEditor::IsHTMLEditor()`. From
>> https://en.wikipedia.org/wiki/SOLID
>> <https://en.wikipedia.org/wiki/SOLID>:
>>
>> "The Liskov substitution principle
>> <https://en.wikipedia.org/wiki/Liskov_substitution_principle>:
>> "Functions that use pointers or references to base classes must be
>> able to use objects of derived classes without knowing it"." I guess,
>> that's not the case here, requiring `IsHTMLEditor()`.
>
> Yes, it is. Previously, `TextEditor` inherited `nsIPlaintextEditor`
> and `EditorBase`, `HTMLEditor` inherited `nsIHTMLEditor` and
> `TextEditor`. To access `TextEditor` or `HTMLEditor` (semi-)directly,
> we did:
> * nsCOMPtr<nsIPlaintextEditor> plaintextEditor =
> do_QueryInterface(aEditor);
> * nsCOMPtr<nsIHTMLEditor> htmlEditor = do_QueryInterface(aEditor);
>
> and called only interface methods. However, QI is too slow and every
> interface method's virtual call also appeared in profiles. Therefore,
> I changed to use concrete classes instead of our old "ideal" style for
> performance after changing `nsIEditor` to a builtin class.

Ah, removing calls to `do_QueryInterface` makes sense. I wasn't aware,
that was one of the objectives, too.

>
> I agree with current editor's design is far from ideal one. Actually,
> we do a lot of bad things in EditorBase.cpp:
> https://searchfox.org/mozilla-central/search?q=Is%28Text%7CHTML%29Editor&path=EditorBase.cpp&case=true&regexp=true
>
>
> Some of them could be rewritten with slower approach or duplicating
> code in both `TextEditor` and `HTMLEditor`, but not all.

Would the code really have to be duplicated? I guess, it could be
factored out instead.

Could you please give some context, why the `IsTextEditor()` calls are
needed? I've skimmed through the code, and it seems it's required for
Thunderbird (e.g. here
<https://searchfox.org/mozilla-central/rev/bea2345ce636dc329ac7133fc0a13c9171777e40/editor/libeditor/EditorBase.cpp#1487>).

Is getting rid of these `IsTextEditor()` calls something worth striving
for, or are there reasons why they will have to remain?

Mirko

[Masayuki Nakano]

On 21/06/14 17:32, Mirko Brodesser wrote:
>
> On 12.06.21 04:16, Masayuki Nakano wrote:
>> I agree with current editor's design is far from ideal one. Actually,
>> we do a lot of bad things in EditorBase.cpp:
>> https://searchfox.org/mozilla-central/search?q=Is%28Text%7CHTML%29Editor&path=EditorBase.cpp&case=true&regexp=true
>>
>>
>> Some of them could be rewritten with slower approach or duplicating
>> code in both `TextEditor` and `HTMLEditor`, but not all.
>
> Would the code really have to be duplicated? I guess, it could be
> factored out instead.

The using places `Is*Editor()` in `EditorBase` do only simple thing for
specific editor class, but they may be in a complicated method. Ideally,
each special paths are handled in the concrete classes with virtual
calls for avoiding the duplication, but I don't like to do it due to the
balance of performance vs. ideal coding style.

>
> Could you please give some context, why the `IsTextEditor()` calls are
> needed? I've skimmed through the code, and it seems it's required for
> Thunderbird (e.g. here
> <https://searchfox.org/mozilla-central/rev/bea2345ce636dc329ac7133fc0a13c9171777e40/editor/libeditor/EditorBase.cpp#1487>).

It's the fastest path for the performance of the `value` getter of
`<input>` and `<textarea>`. It's a hot path in benchmarks. Using
document encoder for the case is too expensive.

[Mirko Brodesser]

On 16.06.21 10:23, Masayuki Nakano wrote:
> On 21/06/14 17:32, Mirko Brodesser wrote:
>>
>> On 12.06.21 04:16, Masayuki Nakano wrote:
>>> I agree with current editor's design is far from ideal one.
>>> Actually, we do a lot of bad things in EditorBase.cpp:
>>> https://searchfox.org/mozilla-central/search?q=Is%28Text%7CHTML%29Editor&path=EditorBase.cpp&case=true&regexp=true
>>>
>>>
>>> Some of them could be rewritten with slower approach or duplicating
>>> code in both `TextEditor` and `HTMLEditor`, but not all.
>>
>> Would the code really have to be duplicated? I guess, it could be
>> factored out instead.
> The using places `Is*Editor()` in `EditorBase` do only simple thing
> for specific editor class, but they may be in a complicated method.
> Ideally, each special paths are handled in the concrete classes with
> virtual calls for avoiding the duplication,

I see. The inheritance diagram you provided before,

> `HTMLEditor` <- `EditorBase` <- `nsIEditor`
>              <- `nsIHTMLEditor` etc.
>
> `TextEditor` <- `EditorBase` <- `nsIEditor`

raises the basic question: do both editors, `HTMLEditor` and
`TextEditor` have to inherit from `nsIEditor`?

Why is `nsIEditor` needed anyway?

Ideally, couldn't `HTMLEditor` and `TextEditor` be classes without a
common super-class?

[Masayuki Nakano]

On 21/06/16 18:28, Mirko Brodesser wrote:
>
> On 16.06.21 10:23, Masayuki Nakano wrote:
>> On 21/06/14 17:32, Mirko Brodesser wrote:
>>>
>>> On 12.06.21 04:16, Masayuki Nakano wrote:
>>>> I agree with current editor's design is far from ideal one.
>>>> Actually, we do a lot of bad things in EditorBase.cpp:
>>>> https://searchfox.org/mozilla-central/search?q=Is%28Text%7CHTML%29Editor&path=EditorBase.cpp&case=true&regexp=true
>>>>
>>>>
>>>> Some of them could be rewritten with slower approach or duplicating
>>>> code in both `TextEditor` and `HTMLEditor`, but not all.
>>>
>>> Would the code really have to be duplicated? I guess, it could be
>>> factored out instead.
>> The using places `Is*Editor()` in `EditorBase` do only simple thing
>> for specific editor class, but they may be in a complicated method.
>> Ideally, each special paths are handled in the concrete classes with
>> virtual calls for avoiding the duplication,
>
> I see. The inheritance diagram you provided before,
>
>> `HTMLEditor` <- `EditorBase` <- `nsIEditor`
>>              <- `nsIHTMLEditor` etc.
>>
>> `TextEditor` <- `EditorBase` <- `nsIEditor`
>
> raises the basic question: do both editors, `HTMLEditor` and
> `TextEditor` have to inherit from `nsIEditor`?
>
> Why is `nsIEditor` needed anyway?
>
> Ideally, couldn't `HTMLEditor` and `TextEditor` be classes without a
> common super-class?

To change the design, we need to touch a lot of chrome code in Firefox,
Thunderbird and SeaMonkey...