Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Description • 8 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 1 • 7 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 2 • 7 years ago

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

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 3 • 7 years ago

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

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 4 • 7 years ago

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

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 5 • 7 years ago

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

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 6 • 7 years ago

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

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 16 • 7 years ago
mozreview-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/#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.

	Makoto Kato [:m_kato]
	Comment 17 • 7 years ago
mozreview-review

Comment on attachment 8895700 [details]
Bug 1319340 - part1 Move AsTextEditor() and AsHTMLEditor() to nsIEditor

https://reviewboard.mozilla.org/r/166990/#review172158

	Makoto Kato [:m_kato]
	Comment 18 • 7 years ago
mozreview-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?

	Makoto Kato [:m_kato]
	Comment 19 • 7 years ago
mozreview-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.

	Makoto Kato [:m_kato]
	Comment 20 • 7 years ago
mozreview-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

	Makoto Kato [:m_kato]
	Comment 21 • 7 years ago
mozreview-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.

	Makoto Kato [:m_kato]
	Comment 22 • 7 years ago
mozreview-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.

	Makoto Kato [:m_kato]
	Comment 23 • 7 years ago
mozreview-review

Comment on attachment 8895706 [details]
Bug 1319340 - part7: Fix some warnings in nsComposerCommands.h

https://reviewboard.mozilla.org/r/167002/#review172198

	Makoto Kato [:m_kato]
	Comment 24 • 7 years ago
mozreview-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

	Makoto Kato [:m_kato]
	Comment 25 • 7 years ago
mozreview-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

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 26 • 7 years ago
mozreview-review-reply

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.

	Pulsebot
	Comment 35 • 7 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 36 • 7 years ago
bugherder

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