nsIEditor should have |mozilla::EditorBase* AsEditorBase()|, |mozilla::TextEditor* AsTextEditor()|, |mozilla::HTMLEditor* AsHTMLEditor()|

RESOLVED FIXED in Firefox 57

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Blocks 2 bugs)

Trunk
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(9 attachments)

59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
Currently, getting EditorBase, TextEditor or HTMLEditor pointer from nsIEditor pointer is using static_cast. This is not safe and makes the line longer.

We should create As*() methods and returns nullptr if the instance doesn't implement it.
Priority: -- → P3
I counted the number of QI for each interface of EditorBase, TextEditor and HTMLEditor:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b30c9d85c12da1d5eb3ccac76df7b46b6557b0ad
(patch: https://hg.mozilla.org/try/rev/be1067bc3ce65a686b9604612a9f76a2ee7a50ae)

The result on Linux-Debug is here:
https://docs.google.com/spreadsheets/d/1-p__3xxh2gRyp4Y63-JsTmM-ED913hCNisJaqJqSBMU/edit?usp=sharing

Then, QIed with nsIEditor is 59,648 times. with nsIPlaintextEditor is 7,857 times. with nsIHTMLEditor is 4,229 times.

Although, these runtime cost must not appear in profiler, but this might make UI response bad.
Comment on attachment 8895704 [details]
Bug 1319340 - part5: Make nsComposerCommands use concrete class when calling methods of editor

https://reviewboard.mozilla.org/r/166998/#review172128

::: editor/composer/nsComposerCommands.cpp:93
(Diff revision 1)
>  nsBaseStateUpdatingCommand::DoCommand(const char *aCommandName,
>                                        nsISupports *refCon)
>  {
>    nsCOMPtr<nsIEditor> editor = do_QueryInterface(refCon);
>    if (NS_WARN_IF(!editor)) {
> -    return NS_ERROR_INVALID_ARG;
> +    return NS_ERROR_FAILURE;
>    }

FYI: For consistency with other command classes, I think that when coming nsISupports object isn't an editor instance, we should return NS_ERROR_FAILURE instead of NS_ERROR_INVALID_ARG since *trying* to do commands without editor isn't invalid, just not supported. So, generic error, NS_ERROR_FAILURE is better.
Comment on attachment 8895700 [details]
Bug 1319340 - part1 Move AsTextEditor() and AsHTMLEditor() to nsIEditor

https://reviewboard.mozilla.org/r/166990/#review172158
Attachment #8895700 - Flags: review?(m_kato) → review+
Comment on attachment 8895701 [details]
Bug 1319340 - part2: GetCurrentState() of the classes in nsComposerCommands should take HTMLEditor instead of nsIEditor

https://reviewboard.mozilla.org/r/166992/#review172180

::: editor/composer/nsComposerCommands.cpp:745
(Diff revision 1)
>  
>    nsAutoString outStateString;
>    nsCOMPtr<nsIAtom> fontAtom = NS_Atomize("font");
>    bool firstHas, anyHas, allHas;
> -  nsresult rv = htmlEditor->GetInlinePropertyWithAttrValue(fontAtom,
> +  nsresult rv = aHTMLEditor->GetInlinePropertyWithAttrValue(
> +                               fontAtom,

Could we use nsGkAtoms::font instead?
Attachment #8895701 - Flags: review?(m_kato) → review+
Comment on attachment 8895702 [details]
Bug 1319340 - part3: ToggleState() in nsComposerCommands should take HTMLEditor* instead of nsIEditor*

https://reviewboard.mozilla.org/r/166994/#review172182

::: editor/composer/nsComposerCommands.cpp:242
(Diff revision 1)
>    }
>  
>    if (doTagRemoval) {
>      // Also remove equivalent properties (bug 317093)
>      if (mTagName == nsGkAtoms::b) {
> -      rv = RemoveTextProperty(htmlEditor, NS_LITERAL_STRING("strong"));
> +      rv = RemoveTextProperty(aHTMLEditor, NS_LITERAL_STRING("strong"));

Although this isn't scope of this bug, I should add RemoveTextProperty(HTMLEditor*, nsIAtom*) to avoid NS_Atomize.  I will file a bug.
Attachment #8895702 - Flags: review?(m_kato) → review+
Comment on attachment 8895703 [details]
Bug 1319340 - part4: SetState() of nsComposerCommands should take HTMLEditor* instead of nsIEditor*

https://reviewboard.mozilla.org/r/166996/#review172184
Attachment #8895703 - Flags: review?(m_kato) → review+
Comment on attachment 8895704 [details]
Bug 1319340 - part5: Make nsComposerCommands use concrete class when calling methods of editor

https://reviewboard.mozilla.org/r/166998/#review172190

::: editor/composer/nsComposerCommands.cpp:1531
(Diff revision 1)
>      return NS_ERROR_NOT_IMPLEMENTED;
>    }
>  
>    nsCOMPtr<nsIDOMElement> domElem;
> -  rv = editor->CreateElementWithDefaults(nsDependentAtomString(mTagName),
> +  rv = htmlEditor->CreateElementWithDefaults(nsDependentAtomString(mTagName),
> -                                         getter_AddRefs(domElem));
> +                                             getter_AddRefs(domElem));

I should Element version of CreateElementWithDefaults as public method. Then we use Element::SetAttr instead of nsIDOMElement::SetAttribute.  I will file a new bug.
Attachment #8895704 - Flags: review?(m_kato) → review+
Comment on attachment 8895705 [details]
Bug 1319340 - part6: Implement some interface methods as non-virtual methods of EditorBase or HTMLEditor

https://reviewboard.mozilla.org/r/167000/#review172196

::: editor/libeditor/EditorBase.cpp:559
(Diff revision 1)
>    RefPtr<Selection> selection = GetSelection();
> -  NS_ENSURE_TRUE(selection, NS_ERROR_NULL_POINTER);
> +  if (NS_WARN_IF(!selection)) {
> +    return false;
> +  }
>  
> +  if (!mIsHTMLEditorClass) {

This is strange code that EditorBase handles HTMLEditor.  I think that we should use virtual bool HTMLEditor::IsSelectionEditable() as code readability.

But various command controllers use EditorBase as instance..., so we should think better way after landing this.
Attachment #8895705 - Flags: review?(m_kato) → review+
Comment on attachment 8895706 [details]
Bug 1319340 - part7: Fix some warnings in nsComposerCommands.h

https://reviewboard.mozilla.org/r/167002/#review172198
Attachment #8895706 - Flags: review?(m_kato) → review+
Comment on attachment 8895707 [details]
Bug 1319340 - part8: EditorCommands should use TextEditor instead of nsIEditor, nsIPlaintextEditor and nsIEditorMailSupport

https://reviewboard.mozilla.org/r/167004/#review172202
Attachment #8895707 - Flags: review?(m_kato) → review+
Comment on attachment 8895708 [details]
Bug 1319340 - part9: Make nsComposerDocumentCommands use concrete class when calling methods of editor

https://reviewboard.mozilla.org/r/167006/#review172204
Attachment #8895708 - Flags: review?(m_kato) → review+
Comment on attachment 8895705 [details]
Bug 1319340 - part6: Implement some interface methods as non-virtual methods of EditorBase or HTMLEditor

https://reviewboard.mozilla.org/r/167000/#review172196

> This is strange code that EditorBase handles HTMLEditor.  I think that we should use virtual bool HTMLEditor::IsSelectionEditable() as code readability.
> 
> But various command controllers use EditorBase as instance..., so we should think better way after landing this.

The reason of not using virtual method is, it'll be called from a lot of places.

On the other hand, your suggestion is also correct. I'll try to think better way.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/af626178b4de
part1 Move AsTextEditor() and AsHTMLEditor() to nsIEditor r=m_kato
https://hg.mozilla.org/integration/autoland/rev/c612ed5072bd
part2: GetCurrentState() of the classes in nsComposerCommands should take HTMLEditor instead of nsIEditor r=m_kato
https://hg.mozilla.org/integration/autoland/rev/33412f3fd23b
part3: ToggleState() in nsComposerCommands should take HTMLEditor* instead of nsIEditor* r=m_kato
https://hg.mozilla.org/integration/autoland/rev/ba3a07622a50
part4: SetState() of nsComposerCommands should take HTMLEditor* instead of nsIEditor* r=m_kato
https://hg.mozilla.org/integration/autoland/rev/2762a672be69
part5: Make nsComposerCommands use concrete class when calling methods of editor r=m_kato
https://hg.mozilla.org/integration/autoland/rev/1fbb78c53ea8
part6: Implement some interface methods as non-virtual methods of EditorBase or HTMLEditor r=m_kato
https://hg.mozilla.org/integration/autoland/rev/b88824858b72
part7: Fix some warnings in nsComposerCommands.h r=m_kato
https://hg.mozilla.org/integration/autoland/rev/c6d10ba42666
part8: EditorCommands should use TextEditor instead of nsIEditor, nsIPlaintextEditor and nsIEditorMailSupport r=m_kato
https://hg.mozilla.org/integration/autoland/rev/53b88290184c
part9: Make nsComposerDocumentCommands use concrete class when calling methods of editor r=m_kato
You need to log in before you can comment on or make changes to this bug.