Closed
Bug 1395936
Opened 7 years ago
Closed 7 years ago
Avoid child index usage in HTMLEditor::DoContentInserted
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: catalinb, Assigned: catalinb)
References
Details
Attachments
(1 file, 3 obsolete files)
No description provided.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8903562 -
Flags: review?(ehsan)
Updated•7 years ago
|
Attachment #8903562 -
Flags: review?(ehsan) → review?(masayuki)
Comment 2•7 years ago
|
||
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-
Assignee | ||
Comment 3•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8903562 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8903903 -
Flags: review?(masayuki)
Comment 4•7 years ago
|
||
Oh, sorry, I missed to catch the new review request.
Comment 5•7 years ago
|
||
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
Comment 7•7 years ago
|
||
Backed out for assertions and crashes in a11y test, mochitests, reftests, crashtest, ...: https://hg.mozilla.org/integration/mozilla-inbound/rev/c8b7472de17bf8d1183883a309cda6fef41a93cd A push which ran most of the failing tests: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d80fa4d123620fd099db9b0e790e5d5daed24590&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Flags: needinfo?(catalin.badea392)
Assignee | ||
Comment 8•7 years ago
|
||
Fix failures.
Assignee | ||
Updated•7 years ago
|
Attachment #8903903 -
Attachment is obsolete: true
Assignee | ||
Comment 9•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfcb55cf7d4214976570b0a228f238a02f5c3f87
Flags: needinfo?(catalin.badea392)
Assignee | ||
Comment 10•7 years ago
|
||
Set the correct root. I think this fixes the rest of the test failures.
Assignee | ||
Updated•7 years ago
|
Attachment #8905111 -
Attachment is obsolete: true
Comment 11•7 years ago
|
||
Pushed by catalin.badea392@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8dc4c247094c Avoid child index usage in HTMLEditor::DoContentInserted. r=masayuki
Comment 12•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8dc4c247094c
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•