Closed Bug 1374207 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

Bug 1374207 - part1: nsTextEditorState should use mozilla::TextEditor instead of editor interfaces
(deleted), text/x-review-board-request
smaug
: review+
Details
Bug 1374207 - part2: TextComposition, IMEContentObserver and IMEStateManager should use EditorBase instead of nsIEditor
(deleted), text/x-review-board-request
m_kato
: review+
Details
Bug 1374207 - part3: Editor classes should use concrete classes instead of nsI*Editor
(deleted), text/x-review-board-request
m_kato
: review+
Details
Bug 1374207 - part4: Element classes should use TextEditor class instead of nIEditor
(deleted), text/x-review-board-request
smaug
: review+
Details
Bug 1374207 - part5: nsTextControlFrame should use TextEditor instead of nsIEditor
(deleted), text/x-review-board-request
smaug
: review+
Details 
Now, editor classes are exposed as "mozilla/*.h". So, as far as possible, others should use concrete class instead of nsI*Editor interface.
Summary: Editor instance should be referrer/stored as concrete class as far as possible → Editor instance should be referred/stored as concrete class as far as possible
Oh, unexpectedly, the patches will improve attachment 8848015 [details] of bug 1346723 about 5% ~ 8%.
Blocks: 1346723
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
Attachment #8879818 - Flags: review?(m_kato) → review+
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.
Attachment #8879819 - Flags: review?(m_kato) → review+
(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 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.
Attachment #8879817 - Flags: review?(bugs) → review+
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);
Attachment #8879820 - Flags: review?(bugs) → review+
Comment on attachment 8879821 [details]
Bug 1374207 - part5: nsTextControlFrame should use TextEditor instead of nsIEditor

https://reviewboard.mozilla.org/r/150990/#review156492
Attachment #8879821 - Flags: review?(bugs) → review+
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 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).
Pushed by masayuki@d-toybox.com:
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: