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.

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

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?

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.

Comment on attachment 8895703 [details] Bug 1319340 - part4: SetState() of nsComposerCommands should take HTMLEditor* instead of nsIEditor* https://reviewboard.mozilla.org/r/166996/#review172184

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.

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.

Comment on attachment 8895706 [details] Bug 1319340 - part7: Fix some warnings in nsComposerCommands.h https://reviewboard.mozilla.org/r/167002/#review172198

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

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

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.

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

https://hg.mozilla.org/mozilla-central/rev/af626178b4de https://hg.mozilla.org/mozilla-central/rev/c612ed5072bd https://hg.mozilla.org/mozilla-central/rev/33412f3fd23b https://hg.mozilla.org/mozilla-central/rev/ba3a07622a50 https://hg.mozilla.org/mozilla-central/rev/2762a672be69 https://hg.mozilla.org/mozilla-central/rev/1fbb78c53ea8 https://hg.mozilla.org/mozilla-central/rev/b88824858b72 https://hg.mozilla.org/mozilla-central/rev/c6d10ba42666 https://hg.mozilla.org/mozilla-central/rev/53b88290184c