Closed Bug 1395936 Opened 2 years ago Closed 2 years ago

Avoid child index usage in HTMLEditor::DoContentInserted

Categories

(Core :: DOM: Core & HTML, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: catalinb, Assigned: catalinb)

References

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
Attachment #8903562 - Flags: review?(ehsan) → review?(masayuki)
Comment on attachment 8903562 [details] [diff] [review]
Avoid child index usage in HTMLEditor::DoContentInserted.

>@@ -3270,30 +3273,23 @@ HTMLEditor::DoContentInserted(nsIDocument* aDocument,
>     }
>     // Protect the edit rules object from dying
>     nsCOMPtr<nsIEditRules> rules(mRules);
>     rules->DocumentModified();
> 
>     // Update spellcheck for only the newly-inserted node (bug 743819)
>     if (mInlineSpellChecker) {
>       RefPtr<nsRange> range = new nsRange(aChild);

Although, not the scope of this bug, we could cache the range for reducing the allocation cost.

>-      int32_t endIndex = aIndexInContainer + 1;
>+      nsIContent* endRef = aChild;

nit: I like endContent since |Ref| is usually used for C++ reference (variable name for Foo&).

>       if (aInsertedOrAppended == eAppended) {
>-        // Count all the appended nodes
>-        nsIContent* sibling = aChild->GetNextSibling();
>-        while (sibling) {
>-          endIndex++;
>-          sibling = sibling->GetNextSibling();
>-        }
>-      }
>-      nsresult rv = range->SetStartAndEnd(aContainer, aIndexInContainer,
>-                                          aContainer, endIndex);
>-      if (NS_SUCCEEDED(rv)) {
>-        mInlineSpellChecker->SpellCheckRange(range);
>+        // Maybe more than 1 child was appended.
>+        endRef = aContainer->GetLastChild();
>       }
>+      range->SelectNodesInContainer(aContainer, aChild->GetPreviousSibling(), endRef);

Hmm, from the view of the point of the caller, this really looks like that the range will be from aChild->GetPreviousSibling() to aChild or aContainer->GetLastChild() rather than from aChild.

So, how about to retrieve previous sibling *in* SelectNodesInContainer()?

>+void
>+nsRange::SelectNodesInContainer(nsINode* aContainer,
>+                                nsIContent* aStartRef,
>+                                nsIContent* aEndRef)

So, I like aStartContent and aEndContent better.

>+{
>+  MOZ_ASSERT(aContainer);
>+  MOZ_ASSERT(aContainer->IndexOf(aStartRef) <= aContainer->IndexOf(aEndRef));
>+  MOZ_ASSERT_IF(aStartRef, aContainer->IndexOf(aStartRef) != -1);
>+  MOZ_ASSERT_IF(aEndRef, aContainer->IndexOf(aEndRef) != -1);
>+
>+  RawRangeBoundary start(aContainer, aStartRef);

So, this should be:

RawRangeBoundary start(aContainer, aStartContent->GetPreviousSibling());

>diff --git a/dom/base/nsRange.h b/dom/base/nsRange.h
>index 64882b7bbdd1..87f54f8ef8eb 100644
>--- a/dom/base/nsRange.h
>+++ b/dom/base/nsRange.h
>@@ -164,16 +164,27 @@ public:
>    * the start point or the end point is invalid point.
>    * If the specified start point is after the end point, the range will be
>    * collapsed at the end point.  Similarly, if they are in different root,
>    * the range will be collapsed at the end point.
>    */
>   nsresult SetStartAndEnd(nsINode* aStartContainer, uint32_t aStartOffset,
>                           nsINode* aEndContainer, uint32_t aEndOffset);
> 
>+  /**
>+   * The start offset will be set immediately after |aStartRef|, while
>+   * the end offset will be set immediately after |aEndRef|. Passing null
>+   * will correspond to a 0 offset. Caller must guarantee both refs are
>+   * children of |aContainer| and that aEndRef is after aStartRef.
>+   */

Then, you need to update this comment. aStartContent shouldn't be nullptr.

>+  void
>+  SelectNodesInContainer(nsINode* aContainer,
>+                         nsIContent* aStartRef,
>+                         nsIContent* aEndRef);

So, I like aStartContent and aEndContent.
Attachment #8903562 - Flags: review?(masayuki) → review-
Attachment #8903562 - Attachment is obsolete: true
Attachment #8903903 - Flags: review?(masayuki)
Oh, sorry, I missed to catch the new review request.
Comment on attachment 8903903 [details] [diff] [review]
Avoid child index usage in HTMLEditor::DoContentInserted.

># HG changeset patch
># User Catalin Badea <catalin.badea392@gmail.com>
>
>Bug 1395936 - Avoid child index usage in HTMLEditor::DoContentInserted. r=masayuki
>
>diff --git a/dom/base/nsRange.cpp b/dom/base/nsRange.cpp
>index d495215a2a8e..04cb97192eb7 100644
>--- a/dom/base/nsRange.cpp
>+++ b/dom/base/nsRange.cpp
>@@ -1482,16 +1482,31 @@ nsRange::SetEnd(nsINode* aContainer, uint32_t aOffset)
>   }
> 
>   RawRangeBoundary newEnd(aContainer, aOffset);
>   DoSetRange(mStart.AsRaw(), newEnd, mRoot);
> 
>   return NS_OK;
> }
> 
>+void
>+nsRange::SelectNodesInContainer(nsINode* aContainer,
>+                                nsIContent* aStartContent,
>+                                nsIContent* aEndContent)
>+{
>+  MOZ_ASSERT(aContainer);
>+  MOZ_ASSERT(aContainer->IndexOf(aStartContent) <= aContainer->IndexOf(aEndContent));

nit: too long line. Perhaps:

  MOZ_ASSERT(aContainer->IndexOf(aStartContent) <=
               aContainer->IndexOf(aEndContent));

>diff --git a/dom/base/nsRange.h b/dom/base/nsRange.h
>@@ -167,16 +167,29 @@ public:
>    * the start point or the end point is invalid point.
>    * If the specified start point is after the end point, the range will be
>    * collapsed at the end point.  Similarly, if they are in different root,
>    * the range will be collapsed at the end point.
>    */
>   nsresult SetStartAndEnd(nsINode* aStartContainer, uint32_t aStartOffset,
>                           nsINode* aEndContainer, uint32_t aEndOffset);
> 
>+  /**
>+   * Adds all nodes between |aStartContent| and |aEndContent| to the range.
>+   * The start offset will be set before |aStartContent|,
>+   * while the end offset will be set immediately after |aEndContent|.
>+   *
>+   * Caller must guarantee both nodes are non null and
>+   * children of |aContainer| and that |aEndContent| is after |aStartContent|.
>+   */
>+  void
>+  SelectNodesInContainer(nsINode* aContainer,

nit: you don't need to break after the result type at declaration. I.e.,

  void SelectNodesInContainer(nsINode* aContainer,

>+                         nsIContent* aStartContent,
>+                         nsIContent* aEndContent);

Then, adjust these indent too.
Attachment #8903903 - Flags: review?(masayuki) → review+
Pushed by catalin.badea392@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbb4fb7aa5bb
Avoid child index usage in HTMLEditor::DoContentInserted. r=masayuki
Attachment #8903903 - Attachment is obsolete: true
Set the correct root. I think this fixes the rest of the test failures.
Attachment #8905111 - Attachment is obsolete: true
Pushed by catalin.badea392@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dc4c247094c
Avoid child index usage in HTMLEditor::DoContentInserted. r=masayuki
https://hg.mozilla.org/mozilla-central/rev/8dc4c247094c
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.