1386485 - CreateBogusNodeIfNeeded() is too expensive

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Description

[(no longer active)]

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

Comment 1

[(no longer active)]

Attachment: Part 1: Inline EditorBase::IsMozEditorBogusNode()

Comment 2

[(no longer active)]

Attachment: Part 2: Hoist the body editablity check out of the loop

Comment 3

[(no longer active)]

Attachment: Part 3: Devirtualize and inline EditorBase::IsEditable()

Comment 4

[(no longer active)]

Attachment: Part 4: Avoid manipulating the refcount of all visited nodes in CreateBogusNodeIfNeeded()

Comment 5

[(no longer active)]

Attachment: Part 5: Avoid collapsing the selection at the end of CreateBogusNodeIfNeeded() when called from TextEditRules::AfterEdit()

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

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 8

[(no longer active)]

(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?

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 10

[(no longer active)]

(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.

Comment 11

[(no longer active)]

(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.  :-)

Comment 12

[(no longer active)]

Attachment: Part 3: Devirtualize EditorBase::AsTextEditor()/AsHTMLEditor()

Comment 13

[(no longer active)]

Attachment: Part 4: Devirtualize and inline EditorBase::IsEditable()

Comment 14

[(no longer active)]

Attachment: Part 6: Avoid collapsing the selection at the end of CreateBogusNodeIfNeeded() when called from TextEditRules::AfterEdit()

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

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 17

[(no longer active)]

(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 18

[(no longer active)]

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.

Comment 19

[(no longer active)]

Attachment: Part 3: Devirtualize EditorBase::AsTextEditor()/AsHTMLEditor()

Comment 20

[(no longer active)]

Attachment: Part 4: Devirtualize and inline EditorBase::IsEditable()

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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)

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 23

[Pulsebot]

Pushed by [e-mail]:
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

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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