Closed Bug 1367683 Opened 3 years ago Closed 3 years ago

Optimize callers of nsRange::SetStart() and nsRange::SetEnd()

Categories

(Core :: DOM: Selection, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

A lot of callers of nsRange::SetStart() calls nsRange::SetEnd() immediately. Then, nsRange::SetEnd() needs redundant processing. Therefore, nsRange should have a method to set both of them once.

Now, nsRange::Set() is that, but that is just calling SetStart() and SetEnd(). So, we should optimize it and rename it to better name, perhaps, SetStartAndEnd().

The coming patch changes a lot of callers. So, I'd like to land this in 55 since such change makes harder to uplift when you work on security bugs.
The patch improves the score of attachment 8848015 [details] (bug 1346723) 3%~.
Comment on attachment 8871187 [details]
Bug 1367683 Optimize initializing nsRange

https://reviewboard.mozilla.org/r/142670/#review146332

Looks like mostly DOM stuff- I'm punting the review to smaug instead.
Attachment #8871187 - Flags: review?(mats) → review?(bugs)
I agree with the change in general though - nice perf improvement!
Comment on attachment 8871187 [details]
Bug 1367683 Optimize initializing nsRange

https://reviewboard.mozilla.org/r/142670/#review147688

::: dom/base/nsRange.h:172
(Diff revision 1)
> +   * 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* aParent, int32_t aOffset)

I don't understand how this method works. It takes just parent and offset. How is that setting both start and end?
Could we call this CollapseTo(aParent, aOffset)

::: dom/base/nsRange.h:176
(Diff revision 1)
> +   */
> +  nsresult SetStartAndEnd(nsINode* aParent, int32_t aOffset)
>    {
> -    // If this starts being hot, we may be able to optimize this a bit,
> -    // but for now just set start and end separately.
> -    nsresult rv = SetStart(aStartParent, aStartOffset);
> +    return SetStartAndEnd(aParent, aOffset, aParent, aOffset);
> +  }
> +  nsresult SetStartAndEnd(nsINode* aStartParent, int32_t aStartOffset,

This variant is easy to understand

::: dom/base/nsRange.h:183
(Diff revision 1)
>  
> -    return SetEnd(aEndParent, aEndOffset);
> +  /**
> +   * Retrieves node and offset for setting start or end of a range to
> +   * before or after aNode.
> +   */
> +  static nsINode* GetNodeAndOffsetAfter(nsINode* aNode, int32_t* aOffset)

Shouldn't this be called GetParentAndOffsetAfter

::: dom/base/nsRange.h:194
(Diff revision 1)
> +    if (*aOffset >= 0) {
> +      (*aOffset)++;
> +    }
> +    return parentNode;
> +  }
> +  static nsINode* GetNodeAndOffsetBefore(nsINode* aNode, int32_t* aOffset)

GetParentAndOffsetBefore

::: dom/base/nsRange.cpp:1275
(Diff revision 1)
>      return;
>    }
>  
>    AutoInvalidateSelection atEndOfBlock(this);
> -  aRv = SetStart(aNode.GetParentNode(), IndexOf(&aNode));
> +  int32_t offset = -1;
> +  nsINode* node = GetNodeAndOffsetBefore(&aNode, &offset);

s/node/parent/

::: dom/base/nsRange.cpp:1311
(Diff revision 1)
>      return;
>    }
>  
>    AutoInvalidateSelection atEndOfBlock(this);
> -  aRv = SetStart(aNode.GetParentNode(), IndexOf(&aNode) + 1);
> +  int32_t offset = -1;
> +  nsINode* node = GetNodeAndOffsetAfter(&aNode, &offset);

s/node/parent/

::: dom/base/nsRange.cpp:1465
(Diff revision 1)
>      return;
>    }
>  
>    AutoInvalidateSelection atEndOfBlock(this);
> -  aRv = SetEnd(aNode.GetParentNode(), IndexOf(&aNode));
> +  int32_t offset = -1;
> +  nsINode* node = GetNodeAndOffsetBefore(&aNode, &offset);

s/node/parent/

::: dom/base/nsRange.cpp:1501
(Diff revision 1)
>      return;
>    }
>  
>    AutoInvalidateSelection atEndOfBlock(this);
> -  aRv = SetEnd(aNode.GetParentNode(), IndexOf(&aNode) + 1);
> +  int32_t offset = -1;
> +  nsINode* node = GetNodeAndOffsetAfter(&aNode, &offset);

s/node/parent/

::: dom/events/ContentEventHandler.cpp:988
(Diff revision 1)
>      *aLastTextNode = nullptr;
>    }
>  
>    // Special case like <br contenteditable>
>    if (!mRootContent->HasChildren()) {
> -    nsresult rv = aRange->SetStart(mRootContent, 0);
> +    nsresult rv = aRange->SetStartAndEnd(mRootContent, 0);

CollapseTo

::: dom/events/ContentEventHandler.cpp:2882
(Diff revision 1)
>    if (!childNode || !childNode->IsNodeOfType(nsINode::eTEXT) ||
>        NS_WARN_IF(offsetInChildNode < 0)) {
>      return NS_OK;
>    }
>  
> -  nsresult rv = aRange->Set(childNode, offsetInChildNode,
> +  nsresult rv = aRange->SetStartAndEnd(childNode, offsetInChildNode,

Why is this using the 4 param version.
Couldn't this be CollapseTo

::: editor/libeditor/HTMLEditRules.cpp:1457
(Diff revision 1)
> +                                           curNode, curOffset);
>        if (NS_WARN_IF(NS_FAILED(rv))) {
>          return rv;
>        }
>      } else {
> -      rv = mDocChangeRange->SetEnd(selNode, selOffset);
> +      rv = mDocChangeRange->SetStartAndEnd(selNode, selOffset,

This could be CollapseTo, right?

::: editor/libeditor/HTMLEditRules.cpp:8342
(Diff revision 1)
>    if (!mListenerEnabled) {
>      return NS_OK;
>    }
> +  nsCOMPtr<nsINode> rightNode = do_QueryInterface(aRightNode);
>    // assumption that Join keeps the righthand node
> -  nsresult rv = mUtilRange->SetStart(aRightNode, mJoinOffset);
> +  nsresult rv = mUtilRange->SetStartAndEnd(rightNode, mJoinOffset);

CollapseTo

::: editor/libeditor/HTMLEditRules.cpp:8394
(Diff revision 1)
>  {
>    if (!mListenerEnabled) {
>      return NS_OK;
>    }
> -  nsCOMPtr<nsIDOMNode> theNode = do_QueryInterface(aTextNode);
> -  nsresult rv = mUtilRange->SetStart(theNode, aOffset);
> +  nsCOMPtr<nsINode> theNode = do_QueryInterface(aTextNode);
> +  nsresult rv = mUtilRange->SetStartAndEnd(theNode, aOffset);

CollapseTo

::: extensions/spellcheck/src/mozInlineSpellChecker.cpp:1222
(Diff revision 1)
> -    rv = range->SetEndAfter(aEndNode);
> -  NS_ENSURE_SUCCESS(rv, rv);
> +      return rv;
> +    }
> +  } else {
> +    int32_t endOffset = -1;
> +    nsCOMPtr<nsINode> startNode = do_QueryInterface(aStartNode);
> +    nsCOMPtr<nsINode> endNode = do_QueryInterface(aEndNode);

Couldn't you move nsCOMPtrs to be before if, then there would be less copy-pasting.

::: layout/generic/nsSelection.cpp:1798
(Diff revision 1)
>        // non-anchor/focus collapsed ranges.
>        mDomSelections[index]->RemoveCollapsedRanges();
>  
>        RefPtr<nsRange> newRange = new nsRange(aNewFocus);
>  
> -      newRange->SetStart(aNewFocus, aContentOffset);
> +      newRange->SetStartAndEnd(aNewFocus, aContentOffset);

CollapseTo

::: layout/generic/nsSelection.cpp:5244
(Diff revision 1)
>        }
>      }
>    }
>  
>    RefPtr<nsRange> range = new nsRange(parentNode);
> -  result = range->SetEnd(parentNode, aOffset);
> +  result = range->SetStartAndEnd(parentNode, aOffset);

CollapseTo
Attachment #8871187 - Flags: review?(bugs) → review+
Attachment #8871187 - Flags: review?(mats)
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/6736d54ffd89
Optimize initializing nsRange r=smaug
https://hg.mozilla.org/mozilla-central/rev/6736d54ffd89
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.