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

RESOLVED FIXED in Firefox 55

Status

()

Core
Selection
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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.
Comment hidden (mozreview-request)
The patch improves the score of attachment 8848015 [details] (bug 1346723) 3%~.

Comment 3

a year ago
mozreview-review
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!
Blocks: 1367740
Blocks: 1367744

Comment 6

a year ago
mozreview-review
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+
Comment hidden (mozreview-request)
Attachment #8871187 - Flags: review?(mats)

Comment 8

a year ago
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/6736d54ffd89
Optimize initializing nsRange r=smaug

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6736d54ffd89
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.