1374207 - Editor instance should be referred/stored as concrete class as far as possible

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

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

Now, editor classes are exposed as "mozilla/*.h". So, as far as possible, others should use concrete class instead of nsI*Editor interface.

Comment 1

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=990ff3ebca61bdee8c20e4b90b77a294e3191c80

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce4dbf63c2412ee0115b7632377df8eacb3fa4c0

Comment 3

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5376fa80bab20290f515f2ff729554b513e73652

Comment 4

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6e8b07e96c74e8dbee12318a22fab56c3109399

Comment 5

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ad631e1699711e06bdb51e67ec826a72d3dc31e

Comment 6

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3239efa507c27a3aa880828aac27e9d80630206c

Comment 7

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e954c90e161a7e3f4163b9834d11201e13010f4d

Comment 8

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

Oh, unexpectedly, the patches will improve attachment 8848015 [details] of bug 1346723 about 5% ~ 8%.

Comment 9

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

Attachment: Bug 1374207 - part1: nsTextEditorState should use mozilla::TextEditor instead of editor interfaces

Using concrete class rather than interface classes (nsI*Editor) will allow to reduce QI and some virtual calls.  Therefore, Editor classes should be used as concrete class as far as possible.

Unfortunately, if classes referring editor is initialized via scriptable interface, we cannot do this. E.g., inline spellchecker and nsIDocShell.  Such remaining cases should be investigated after XUL addons are banned.

Note that this patch also creates nsIdentifierMapEntry.h which is separated from nsDocument.h because ShadowRoot.h needs the class but exposing nsDocument.h to the global and includes it causes bustage on Linux and Android.  Therefore, for fixing the include hell, this patch touches them and ContentChild.cpp.

Review commit: https://reviewboard.mozilla.org/r/150982/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150982/

Comment 10

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

Attachment: Bug 1374207 - part2: TextComposition, IMEContentObserver and IMEStateManager should use EditorBase instead of nsIEditor

Review commit: https://reviewboard.mozilla.org/r/150984/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150984/

Comment 11

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

Attachment: Bug 1374207 - part3: Editor classes should use concrete classes instead of nsI*Editor

HTMLEditor inherits some interface classes.  Therefore, CachedWeakPtr is confused at ambiguous conversion from HTMLEditor to nsISupports.  Therefore, this patch adds second parameter to the template class.

Review commit: https://reviewboard.mozilla.org/r/150986/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150986/

Comment 12

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

Attachment: Bug 1374207 - part4: Element classes should use TextEditor class instead of nIEditor

Unfortunately, methods which are for webidl bindings and nsGenericHTMLElement::GetAssociatedEditor() cannot use concrete classes because the former needs to be an XPCOM interface and the latter may return nsIEditor which is retrieved from nsIDocShell.  nsIDocShell's editor can be an editor implemented by JS (due to bug 1053048).

Review commit: https://reviewboard.mozilla.org/r/150988/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150988/

Comment 13

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

Attachment: Bug 1374207 - part5: nsTextControlFrame should use TextEditor instead of nsIEditor

Review commit: https://reviewboard.mozilla.org/r/150990/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150990/

Comment 14

[Makoto Kato [:m_kato]]

Comment on attachment 8879818 [details]
Bug 1374207 - part2: TextComposition, IMEContentObserver and IMEStateManager should use EditorBase instead of nsIEditor

https://reviewboard.mozilla.org/r/150984/#review156100

Comment 15

[Makoto Kato [:m_kato]]

Comment on attachment 8879819 [details]
Bug 1374207 - part3: Editor classes should use concrete classes instead of nsI*Editor

https://reviewboard.mozilla.org/r/150986/#review156106

::: editor/libeditor/HTMLEditor.h:148
(Diff revision 1)
>    // nsIHTMLEditor methods
>    NS_DECL_NSIHTMLEDITOR
>  
>    // nsIHTMLObjectResizer methods (implemented in HTMLObjectResizer.cpp)
>    NS_DECL_NSIHTMLOBJECTRESIZER
> +  nsresult MouseMove(nsIDOMMouseEvent* aMouseEvent);

This isn't scope of this issue...

This lines are declare block for NS_DECL\_\*, you shouldn't add declare of non-nsIHTMLObjectResider.

Declare blocks of HTMLEditor is always too confused.  So we should clean up for it.

Comment 16

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

(In reply to Makoto Kato [:m_kato] from comment #15)
> Comment on attachment 8879819 [details]
> Bug 1374207 - part3: Editor classes should use concrete classes instead of
> nsI*Editor
> 
> https://reviewboard.mozilla.org/r/150986/#review156106
> 
> ::: editor/libeditor/HTMLEditor.h:148
> (Diff revision 1)
> >    // nsIHTMLEditor methods
> >    NS_DECL_NSIHTMLEDITOR
> >  
> >    // nsIHTMLObjectResizer methods (implemented in HTMLObjectResizer.cpp)
> >    NS_DECL_NSIHTMLOBJECTRESIZER
> > +  nsresult MouseMove(nsIDOMMouseEvent* aMouseEvent);
> 
> This isn't scope of this issue...

It's now available because of the patch making the pointer from nsIHTMLEditor to HTMLEditor. It's possible to separate replacing slow method calls to another bug but I'd like to do that here for clearing the purpose of these big patches.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8879817 [details]
Bug 1374207 - part1: nsTextEditorState should use mozilla::TextEditor instead of editor interfaces

https://reviewboard.mozilla.org/r/150982/#review156438

::: commit-message-8f9cf:5
(Diff revision 1)
> +Bug 1374207 - part1: nsTextEditorState should use mozilla::TextEditor instead of editor interfaces r?smaug
> +
> +Using concrete class rather than interface classes (nsI*Editor) will allow to reduce QI and some virtual calls.  Therefore, Editor classes should be used as concrete class as far as possible.
> +
> +Unfortunately, if classes referring editor is initialized via scriptable interface, we cannot do this. E.g., inline spellchecker and nsIDocShell.  Such remaining cases should be investigated after XUL addons are banned.

Why? If those are marked as builtinclass in .idl, doing a static_cast from them to concrete class should be fine.

::: dom/html/nsTextEditorState.cpp:1342
(Diff revision 1)
> -  if (mEditor) {
> +  if (mTextEditor) {
>      nsCOMPtr<nsIContent> content = do_QueryInterface(mTextCtrlElement);
>      NS_ENSURE_TRUE(content, NS_ERROR_FAILURE);
>  
>      // Set the correct direction on the newly created root node
> -    uint32_t flags;
> +    if (mTextEditor->IsRightToLeft()) {

unrelated changes just make code review harder. But ok, this looks fine.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8879820 [details]
Bug 1374207 - part4: Element classes should use TextEditor class instead of nIEditor

https://reviewboard.mozilla.org/r/150988/#review156490

::: commit-message-68a74:3
(Diff revision 1)
> +Bug 1374207 - part4: Element classes should use TextEditor class instead of nIEditor r?smaug
> +
> +Unfortunately, methods which are for webidl bindings and nsGenericHTMLElement::GetAssociatedEditor() cannot use concrete classes because the former needs to be an XPCOM interface and the latter may return nsIEditor which is retrieved from nsIDocShell.  nsIDocShell's editor can be an editor implemented by JS (due to bug 1053048).

This comment isn't right. Webidl doesn't need idl interface.

::: dom/html/HTMLTextAreaElement.h:72
(Diff revision 1)
>  
>    // nsIDOMNSEditableElement
>    NS_IMETHOD GetEditor(nsIEditor** aEditor) override
>    {
> -    return nsGenericHTMLElement::GetEditor(aEditor);
> +    nsCOMPtr<nsIEditor> editor = GetEditor();
> +    *aEditor = editor.forget().take();

Why not editor.forget(*aEditor);

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8879821 [details]
Bug 1374207 - part5: nsTextControlFrame should use TextEditor instead of nsIEditor

https://reviewboard.mozilla.org/r/150990/#review156492

Comment 20

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

Comment on attachment 8879817 [details]
Bug 1374207 - part1: nsTextEditorState should use mozilla::TextEditor instead of editor interfaces

https://reviewboard.mozilla.org/r/150982/#review156438

> Why? If those are marked as builtinclass in .idl, doing a static_cast from them to concrete class should be fine.

Both nsIEditor and nsIHTMLEditor are not builtinclass. Therefore, JS can implement an editor class (i.e., there can be editor instance which doesn't inherit EditorBase nor HTMLEditor). If such editor is used at initializing spellchecker or setting editor of nsIDocShell, reinterpret_cast<HTMLEditor*>(editor) is dangerous.

Comment 21

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

Comment on attachment 8879820 [details]
Bug 1374207 - part4: Element classes should use TextEditor class instead of nIEditor

https://reviewboard.mozilla.org/r/150988/#review156490

> This comment isn't right. Webidl doesn't need idl interface.

Oh, really? I'll file a follow up bug (I'll fix only the comment in this bug).

Comment 22

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

Comment on attachment 8879817 [details]
Bug 1374207 - part1: nsTextEditorState should use mozilla::TextEditor instead of editor interfaces

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150982/diff/1-2/

Comment 23

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

Comment on attachment 8879818 [details]
Bug 1374207 - part2: TextComposition, IMEContentObserver and IMEStateManager should use EditorBase instead of nsIEditor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150984/diff/1-2/

Comment 24

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

Comment on attachment 8879819 [details]
Bug 1374207 - part3: Editor classes should use concrete classes instead of nsI*Editor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150986/diff/1-2/

Comment 25

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

Comment on attachment 8879820 [details]
Bug 1374207 - part4: Element classes should use TextEditor class instead of nIEditor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150988/diff/1-2/

Comment 26

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

Comment on attachment 8879821 [details]
Bug 1374207 - part5: nsTextControlFrame should use TextEditor instead of nsIEditor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150990/diff/1-2/

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/621ec6b67f20
part1: nsTextEditorState should use mozilla::TextEditor instead of editor interfaces r=smaug
https://hg.mozilla.org/integration/autoland/rev/8230a1deb27f
part2: TextComposition, IMEContentObserver and IMEStateManager should use EditorBase instead of nsIEditor r=m_kato
https://hg.mozilla.org/integration/autoland/rev/527224116b19
part3: Editor classes should use concrete classes instead of nsI*Editor r=m_kato
https://hg.mozilla.org/integration/autoland/rev/fdfde4187469
part4: Element classes should use TextEditor class instead of nIEditor r=smaug
https://hg.mozilla.org/integration/autoland/rev/3bced40559b6
part5: nsTextControlFrame should use TextEditor instead of nsIEditor r=smaug

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/621ec6b67f20
https://hg.mozilla.org/mozilla-central/rev/8230a1deb27f
https://hg.mozilla.org/mozilla-central/rev/527224116b19
https://hg.mozilla.org/mozilla-central/rev/fdfde4187469
https://hg.mozilla.org/mozilla-central/rev/3bced40559b6