There are a number of easy improvements we can make here.

In this case we are soon going to collapse the selection anyway, so there is no reason to collapse it once here also.

Comment on attachment 8892730 [details] [diff] [review] Part 3: Devirtualize and inline EditorBase::IsEditable() >+ bool IsEditable(nsINode* aNode) >+ { >+ NS_ENSURE_TRUE(aNode, false); >+ >+ if (!aNode->IsNodeOfType(nsINode::eCONTENT) || IsMozEditorBogusNode(aNode) || >+ !IsModifiableNode(aNode)) { >+ return false; >+ } >+ >+ switch (aNode->NodeType()) { >+ case nsIDOMNode::ELEMENT_NODE: >+ // In HTML editors, if we're dealing with an element, then ask it >+ // whether it's editable. >+ return IsPlaintextEditor() ? true : aNode->IsEditable(); No, this changes the behavior. IsPlaintextEditor() may return true even if the instance is HTMLEditor. E.g., plaintext email editor of Thunderbird. So, |!AsHTMLEditor()| returns what you want. However, it's a virtual method. So, you might want to create a bool member which indicates whether it's an HTMLEditor instance.

Comment on attachment 8892732 [details] [diff] [review] Part 5: Avoid collapsing the selection at the end of CreateBogusNodeIfNeeded() when called from TextEditRules::AfterEdit() > nsresult >-TextEditRules::CreateBogusNodeIfNeeded(Selection* aSelection) >+TextEditRules::CreateBogusNodeIfNeeded(Selection* aSelection, bool aSkipSelectionCollapse) nit: over 80 characters. But please see next point. >@@ -1465,18 +1465,20 @@ TextEditRules::CreateBogusNodeIfNeeded(Selection* aSelection) > // Give it a special attribute. > newContent->SetAttr(kNameSpaceID_None, kMOZEditorBogusNodeAttrAtom, > kMOZEditorBogusNodeValue, false); > > // Put the node in the document. > nsresult rv = mTextEditor->InsertNode(*mBogusNode, *body, 0); > NS_ENSURE_SUCCESS(rv, rv); > >- // Set selection. >- aSelection->Collapse(body, 0); >+ if (!aSkipSelectionCollapse) { >+ // Set selection. >+ aSelection->Collapse(body, 0); >+ } I don't like this change since additional bool argument isn't clear what it means when I see the callers. How about to remove this collapsing simply and do it by callers if it's necessary? There are only 4 callers and the method name, CreateBogusNodeIfNeeded, doesn't sound like that it will change selection.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #7) > Comment on attachment 8892732 [details] [diff] [review] > Part 5: Avoid collapsing the selection at the end of > CreateBogusNodeIfNeeded() when called from TextEditRules::AfterEdit() > > > nsresult > >-TextEditRules::CreateBogusNodeIfNeeded(Selection* aSelection) > >+TextEditRules::CreateBogusNodeIfNeeded(Selection* aSelection, bool aSkipSelectionCollapse) > > nit: over 80 characters. But please see next point. > > >@@ -1465,18 +1465,20 @@ TextEditRules::CreateBogusNodeIfNeeded(Selection* aSelection) > > // Give it a special attribute. > > newContent->SetAttr(kNameSpaceID_None, kMOZEditorBogusNodeAttrAtom, > > kMOZEditorBogusNodeValue, false); > > > > // Put the node in the document. > > nsresult rv = mTextEditor->InsertNode(*mBogusNode, *body, 0); > > NS_ENSURE_SUCCESS(rv, rv); > > > >- // Set selection. > >- aSelection->Collapse(body, 0); > >+ if (!aSkipSelectionCollapse) { > >+ // Set selection. > >+ aSelection->Collapse(body, 0); > >+ } > > I don't like this change since additional bool argument isn't clear what it > means when I see the callers. > > How about to remove this collapsing simply and do it by callers if it's > necessary? There are only 4 callers and the method name, > CreateBogusNodeIfNeeded, doesn't sound like that it will change selection. I thought about doing that first but the issue is that we don't have access to the body at the callers and getting it again seems like wasted work. How about adding an enum or something like that?

Adding enum sounds good. On the other hand, the |body| is result of GetRoot(). https://searchfox.org/mozilla-central/rev/f0e4ae5f8c40ba742214e89aba3f554da0b89a33/editor/libeditor/EditorBase.cpp#4981,4983,4986,4989 Its result is cached at first use. So, I don't think that this causes performance issue.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #9) > On the other hand, the |body| is result of GetRoot(). > https://searchfox.org/mozilla-central/rev/ > f0e4ae5f8c40ba742214e89aba3f554da0b89a33/editor/libeditor/EditorBase. > cpp#4981,4983,4986,4989 > Its result is cached at first use. So, I don't think that this causes > performance issue. You're right, good point. I'll do that! That is actually my preferred approach as well. It's always better make it more obvious where expensive work happens.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #6) > So, |!AsHTMLEditor()| returns what you want. However, it's a virtual > method. So, you might want to create a bool member which indicates whether > it's an HTMLEditor instance. Even better, I'm going to devirtualize AsHTMLEditor() first (so that it's more efficient for all consumers) and then use !AsHTMLEditor() here. :-)

In this case we are soon going to collapse the selection anyway, so there is no reason to collapse it once here also.

Comment on attachment 8893576 [details] [diff] [review] Part 3: Devirtualize EditorBase::AsTextEditor()/AsHTMLEditor() >diff --git a/editor/libeditor/EditorBase.h b/editor/libeditor/EditorBase.h >index d014c8f40a86..5243f76576ad 100644 >--- a/editor/libeditor/EditorBase.h >+++ b/editor/libeditor/EditorBase.h >@@ -232,20 +232,22 @@ public: > }; > > /** > * The default constructor. This should suffice. the setting of the > * interfaces is done after the construction of the editor class. > */ > EditorBase(); > >- virtual TextEditor* AsTextEditor() = 0; >- virtual const TextEditor* AsTextEditor() const = 0; >- virtual HTMLEditor* AsHTMLEditor() = 0; >- virtual const HTMLEditor* AsHTMLEditor() const = 0; >+ // Please include TextEditor.h. >+ inline TextEditor* AsTextEditor(); >+ inline const TextEditor* AsTextEditor() const; >+ // Please include HTMLEditor.h. >+ inline HTMLEditor* AsHTMLEditor(); >+ inline const HTMLEditor* AsHTMLEditor() const; > > protected: > /** > * The default destructor. This should suffice. Should this be pure virtual > * for someone to derive from the EditorBase later? I don't believe so. > */ > virtual ~EditorBase(); > >@@ -1219,16 +1221,18 @@ protected: > bool mDidPostCreate; > bool mDispatchInputEvent; > // True while the instance is handling an edit action. > bool mIsInEditAction; > // Whether caret is hidden forcibly. > bool mHidingCaret; > // Whether spellchecker dictionary is initialized after focused. > bool mSpellCheckerDictionaryUpdated; >+ // Whether we are a plaintext editor class. >+ bool mIsPlaintextEditorClass; The word, "PlaintextEditor" is used for nsIPlaintextEditor and the editing mode of HTMLEditor. So, could you rename this to mIsTextEditorClass? >diff --git a/editor/libeditor/HTMLEditor.cpp b/editor/libeditor/HTMLEditor.cpp >index 9b7acee608a1..87e42f6b00fe 100644 >--- a/editor/libeditor/HTMLEditor.cpp >+++ b/editor/libeditor/HTMLEditor.cpp >@@ -127,16 +127,17 @@ HTMLEditor::HTMLEditor() > , mPositionedObjectMarginTop(0) > , mPositionedObjectBorderLeft(0) > , mPositionedObjectBorderTop(0) > , mGridSize(0) > , mDefaultParagraphSeparator( > Preferences::GetBool("editor.use_div_for_default_newlines", true) > ? ParagraphSeparator::div : ParagraphSeparator::br) > { >+ mIsPlaintextEditorClass = false; Hmm, so, this means that, while TextEditor::TextEditor() and EditorBase::EditorBase() are running, AsHTMLEditor() returns nullptr? Shouldn't we create another constructor to initialize the bool flag as expected? >diff --git a/editor/libeditor/HTMLEditor.h b/editor/libeditor/HTMLEditor.h >@@ -1055,11 +1052,21 @@ private: > */ > already_AddRefed<Element> CreateAnonymousElement( > nsIAtom* aTag, > nsIDOMNode* aParentNode, > const nsAString& aAnonClass, > bool aIsCreatedHidden); > }; > >+inline HTMLEditor* EditorBase::AsHTMLEditor() >+{ >+ return mIsPlaintextEditorClass ? nullptr : static_cast<HTMLEditor*>(this); >+} >+ >+inline const HTMLEditor* EditorBase::AsHTMLEditor() const >+{ >+ return mIsPlaintextEditorClass ? nullptr : static_cast<const HTMLEditor*>(this); >+} Wow, tricky! HTMLEditor.h will have the implementation of EditorBase. Do you need |inline| keyword here? You specified it in EditorBase.h. And could you wrap after |HTMLEditor*|? I mean the method should begins with: HTMLEditor* EditorBase::AsHTMLEditor() { and const HTMLEditor* EditorBase::AsHTMLEditor() const { or inline HTMLEditor* EditorBase::AsHTMLEditor() { and inline const HTMLEditor* EditorBase::AsHTMLEditor() const { >diff --git a/editor/libeditor/TextEditor.h b/editor/libeditor/TextEditor.h >@@ -242,11 +237,21 @@ protected: > int32_t mNewlineHandling; > int32_t mCaretStyle; > > friend class AutoEditInitRulesTrigger; > friend class HTMLEditRules; > friend class TextEditRules; > }; > >+inline TextEditor* EditorBase::AsTextEditor() Same as AsHTMLEditor(), please wrap this line after |TextEditor*|. >+{ >+ return mIsPlaintextEditorClass ? static_cast<TextEditor*>(this) : nullptr; No, this is wrong. Even if it's HTMLEditor instance, it implements TextEditor too. So, this should be: return static_cast<TextEditor*>(this); >+} >+ >+inline const TextEditor* EditorBase::AsTextEditor() const Same. Please wrap this line after |const TextEditor*|. >+{ >+ return mIsPlaintextEditorClass ? static_cast<const TextEditor*>(this) : nullptr; Same. This is wrong. Should be: return static_cast<const TextEditor*>(this); Ah, from the point of view from these methods, mIsPlaintextEditorClass (mIsTextEditorClass) shold be mIsHTMLEditorClass?

Comment on attachment 8893577 [details] [diff] [review] Part 4: Devirtualize and inline EditorBase::IsEditable() If you'll rename mIsPlaintextEditorClass to mIsHTMLEditorClass, I should check if you revert bool value check here. So, canceling the review request to review new patch.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #15) > >+ // Whether we are a plaintext editor class. > >+ bool mIsPlaintextEditorClass; > > The word, "PlaintextEditor" is used for nsIPlaintextEditor and the editing > mode of HTMLEditor. So, could you rename this to mIsTextEditorClass? Sure. > > { > >+ mIsPlaintextEditorClass = false; > > Hmm, so, this means that, while TextEditor::TextEditor() and > EditorBase::EditorBase() are running, AsHTMLEditor() returns nullptr? Yes. > Shouldn't we create another constructor to initialize the bool flag as > expected? Not unless we are seeking a behavior change here. I just implemented what we currently do. According to the C++ standard, when creating a Derived object, as the Base constructor is running, the identity of *this is Base, not Derived. The identity will switch over to become Derived when the ctor for Derived starts to run. Indeed, it is the constructor that switches the vtable pointer to point to the vtable of the class, so if you call a Base virtual function that Derived overrides inside the Base constructor, you will call the Base version. So in the example above, currently inside EditorBase::EditorBase() when you call AsHTMLEditor(), you are guaranteed to run EditorBase::AsHTMLEditor() which returns false. > >diff --git a/editor/libeditor/HTMLEditor.h b/editor/libeditor/HTMLEditor.h > >@@ -1055,11 +1052,21 @@ private: > > */ > > already_AddRefed<Element> CreateAnonymousElement( > > nsIAtom* aTag, > > nsIDOMNode* aParentNode, > > const nsAString& aAnonClass, > > bool aIsCreatedHidden); > > }; > > > >+inline HTMLEditor* EditorBase::AsHTMLEditor() > >+{ > >+ return mIsPlaintextEditorClass ? nullptr : static_cast<HTMLEditor*>(this); > >+} > >+ > >+inline const HTMLEditor* EditorBase::AsHTMLEditor() const > >+{ > >+ return mIsPlaintextEditorClass ? nullptr : static_cast<const HTMLEditor*>(this); > >+} > > Wow, tricky! HTMLEditor.h will have the implementation of EditorBase. Yeah, I had to do it like this for the static_cast's to work... (Sorry, it's kind of gross.) > Do you need |inline| keyword here? You specified it in EditorBase.h. That is a good question. We do this in a whole bunch of other places, but I just checked cppreference.com and apparently this is only needed on declarations, so should be OK to drop here. > And could you wrap after |HTMLEditor*|? I mean the method should begins with: Of course. > >+{ > >+ return mIsPlaintextEditorClass ? static_cast<TextEditor*>(this) : nullptr; > > No, this is wrong. Even if it's HTMLEditor instance, it implements > TextEditor too. So, this should be: > > return static_cast<TextEditor*>(this); Aha, right. I misunderstood the meaning of this function, sorry about that! > Ah, from the point of view from these methods, mIsPlaintextEditorClass > (mIsTextEditorClass) shold be mIsHTMLEditorClass? Yeah I suppose that makes more sense now in hindsight.

Comment on attachment 8893578 [details] [diff] [review] Part 6: Avoid collapsing the selection at the end of CreateBogusNodeIfNeeded() when called from TextEditRules::AfterEdit() After testing this on try, it turns out that I was wrong, and this isn't a no-op. Apparently there are existing tests that rely on this behavior in various ways, and this part breaks them, so I will retract this part of the series.

When I only applied part3 to my local tree, I got this error on Windows. > 2:46.14 Unified_cpp_dom_base6.obj : error LNK2019: unresolved external symbol "public: class mozilla::HTMLEditor * __cdecl mozilla::EditorBase::AsHTMLEditor(void)" (?AsHTMLEditor@EditorBase@mozilla@@QEAAPEAVHTMLEditor@2@XZ) referenced in function "public: class nsIContent * __cdecl nsINode::GetTextEditorRootContent(class mozilla::TextEditor * *)" (?GetTextEditorRootContent@nsINode@@QEAAPEAVnsIContent@@PEAPEAVTextEditor@mozilla@@@Z)

Okay, please include mozilla/HTMLEditor.h in dom/base/nsINode.cpp.

Pushed by eakhgari@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3c630870cc7c Part 1: Inline EditorBase::IsMozEditorBogusNode(); r=masayuki https://hg.mozilla.org/integration/mozilla-inbound/rev/2a06201254fd Part 2: Hoist the body editablity check out of the loop; r=masayuki https://hg.mozilla.org/integration/mozilla-inbound/rev/9bc5ba5a521a Part 3: Devirtualize EditorBase::AsTextEditor()/AsHTMLEditor(); r=masayuki https://hg.mozilla.org/integration/mozilla-inbound/rev/7a07154c27e9 Part 4: Devirtualize and inline EditorBase::IsEditable(); r=masayuki https://hg.mozilla.org/integration/mozilla-inbound/rev/b605b5224f61 Part 5: Avoid manipulating the refcount of all visited nodes in CreateBogusNodeIfNeeded(); r=masayuki

https://hg.mozilla.org/mozilla-central/rev/3c630870cc7c https://hg.mozilla.org/mozilla-central/rev/2a06201254fd https://hg.mozilla.org/mozilla-central/rev/9bc5ba5a521a https://hg.mozilla.org/mozilla-central/rev/7a07154c27e9 https://hg.mozilla.org/mozilla-central/rev/b605b5224f61