Closed Bug 1367690 Opened 7 years ago Closed 7 years ago

Optimize Selection::selectFrames()

Categories

(Core :: DOM: Selection, defect)

Product:
Core
Component:
DOM: Selection
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

Bug 1367690 Optimize Selection::selectFrames()
(deleted), text/x-review-board-request
MatsPalmgren_bugz
: review+
Details 
Selection::selectFrames() creates two content iterators and sets selection ranges to text frames and invalidate other frames. However, this wastes time a lot.

It doesn't need to create content iterators if the given range is in a node and the node doesn't have children.

It sets selection to first frame (when it's a text frame) twice. One is from selectFrames(), the other is from Selection::SelectAllFramesForContent(). We should avoid to do such redundant thing.

The coming patch improves the score of attachment 8848015 [details] (bug 1346723) 13%~.
Comment on attachment 8871195 [details]
Bug 1367690 Optimize Selection::selectFrames()

https://reviewboard.mozilla.org/r/142684/#review146442

r=mats with these issues addressed

::: layout/generic/Selection.h:350
(Diff revision 1)
> -  nsresult     SelectAllFramesForContent(nsIContentIterator *aInnerIter,
> +  void SelectFramesForContent(nsIContent* aContent, bool aSelected);
> +  nsresult SelectAllFramesForContent(nsIContentIterator *aInnerIter,
> -                               nsIContent *aContent,
> +                                     nsIContent *aContent,
> -                               bool aSelected);
> +                                     bool aSelected);
> -  nsresult     selectFrames(nsPresContext* aPresContext, nsRange *aRange, bool aSelect);
> +  nsresult SelectFrames(nsPresContext* aPresContext,
> +                        nsRange* aRange,

Please don't make naming changes like this in the same patch as functional changes.  It makes both reviewing and future code archeology harder.
I'll review this anyway, because those changes are fairly separate from the other changes in this patch, but please make a "part 2" containing only that naming change before landing.  (r+ on that part 2)

Perhaps it would be better to file a separate bug for that name change and then fix the other methods as well, setAnchorFocusRange, getTableCellLocationFromRange, addTableCellRange etc?

::: layout/generic/nsSelection.cpp:4498
(Diff revision 1)
>  
> +  if (NS_WARN_IF(NS_FAILED(aInnerIter->Init(aContent)))) {
> -  return NS_ERROR_FAILURE;
> +    return NS_ERROR_FAILURE;
> -}
> +  }
>  
> +  for (aInnerIter->First(); !aInnerIter->IsDone(); aInnerIter->Next()) {

I think the First() call is redundant right after an Init() call.  If so, please remove it.

::: layout/generic/nsSelection.cpp:4501
(Diff revision 1)
> -}
> +  }
>  
> +  for (aInnerIter->First(); !aInnerIter->IsDone(); aInnerIter->Next()) {
> +    nsINode* node = aInnerIter->GetCurrentNode();
> +    nsIContent* innercontent =
> +      node && node->IsContent() ? node->AsContent() : nullptr;

Do we really need to null-check |node| here?  I would expect that !IsDone() guarantees that GetCurrentNode() is non-null.

::: layout/generic/nsSelection.cpp:4536
(Diff revision 1)
> -  iter->Init(aRange);
>  
>    // Loop through the content iterator for each content node; for each text
>    // node, call SetSelected on it:
> -  nsCOMPtr<nsIContent> content = do_QueryInterface(aRange->GetStartParent());
> +  nsINode* node = aRange->GetStartParent();
> +  nsIContent* content = node->IsContent() ? node->AsContent() : nullptr;

Here you assume that |node| is non-null, which is fine, but the old code did not depend on that.  For clarity, please change the existing MOZ_ASSERT(aRange) a few lines up to MOZ_ASSERT(aRange && aRange->IsPositioned()).

::: layout/generic/nsSelection.cpp:4546
(Diff revision 1)
>  
>    // We must call first one explicitly
> -  if (content->IsNodeOfType(nsINode::eTEXT)) {
> +  bool isFirstContentTextNode = content->IsNodeOfType(nsINode::eTEXT);
> +  if (isFirstContentTextNode) {
>      nsIFrame* frame = content->GetPrimaryFrame();
>      // The frame could be an SVG text frame, in which case we'll ignore it.

This code comment says that the code intentionally ignores text nodes which have some other type of frame than "IsTextFrame", but now you're adding an else-branch which calls InvalidateFrameSubtree().  The change seems reasonable, but I think it makes this comment obsolete so it should be removed.  (Please investigate where this comment comes from though, to make sure we're not misunderstanding something.)

::: layout/generic/nsSelection.cpp:4569
(Diff revision 1)
>  
> +  // If the range is in a node and the node is a leaf node, we don't need to
> +  // walk the subtree.
> +  if (aRange->Collapsed() ||
> +      (aRange->GetStartParent() == aRange->GetEndParent() &&
> +       !aRange->GetStartParent()->HasChildren())) {

|aRange->GetStartParent()| can be replaced with |node| here (twice).

::: layout/generic/nsSelection.cpp:4578
(Diff revision 1)
> +    return NS_OK;
> +  }
> +
> +  nsCOMPtr<nsIContentIterator> iter = NS_NewContentSubtreeIterator();
> +  iter->Init(aRange);
>    iter->First();

I think First() after Init() is redundant.

::: layout/generic/nsSelection.cpp:4585
(Diff revision 1)
> +    iter->Next(); // first content has already been handled.
> +  }
>    nsCOMPtr<nsIContentIterator> inneriter = NS_NewContentIterator();
> -  for (iter->First(); !iter->IsDone(); iter->Next()) {
> -    content = do_QueryInterface(iter->GetCurrentNode());
> +  for (; !iter->IsDone(); iter->Next()) {
> +    node = iter->GetCurrentNode();
> +    content = node && node->IsContent() ? node->AsContent() : nullptr;

Per my previous comment, null-checking |node| here seems redundant.

::: layout/generic/nsSelection.cpp:4592
(Diff revision 1)
>    }
>  
>    // We must now do the last one if it is not the same as the first
>    if (aRange->GetEndParent() != aRange->GetStartParent()) {
> -    nsresult res;
> -    content = do_QueryInterface(aRange->GetEndParent(), &res);
> +    node = aRange->GetEndParent();
> +    content = node && node->IsContent() ? node->AsContent() : nullptr;

No need to null-check |node| if we assert that aRange is positioned.

::: layout/generic/nsSelection.cpp:5715
(Diff revision 1)
> -    selectFrames(presContext, difRange, false); // deselect now
> +    // deselect now
> +    SelectFrames(presContext, difRange, false);

nit: I prefer these comments at the end of the line as they were
Attachment #8871195 - Flags: review?(mats) → review+
Just a general note on selectFrames() unrelated to the review:
selectFrames() currently returns NS_ERROR_UNEXPECTED if IsContent() is false
for either the start or end node of the range.  I find that surprising
because having the document as a range boundary point is perfectly fine.
(I don't understand how this code can work for that case.)
Comment on attachment 8871195 [details]
Bug 1367690 Optimize Selection::selectFrames()

https://reviewboard.mozilla.org/r/142684/#review147276

::: layout/generic/Selection.h:350
(Diff revision 1)
> -  nsresult     SelectAllFramesForContent(nsIContentIterator *aInnerIter,
> +  void SelectFramesForContent(nsIContent* aContent, bool aSelected);
> +  nsresult SelectAllFramesForContent(nsIContentIterator *aInnerIter,
> -                               nsIContent *aContent,
> +                                     nsIContent *aContent,
> -                               bool aSelected);
> +                                     bool aSelected);
> -  nsresult     selectFrames(nsPresContext* aPresContext, nsRange *aRange, bool aSelect);
> +  nsresult SelectFrames(nsPresContext* aPresContext,
> +                        nsRange* aRange,

Sure, due to the limitation of autoland of MozReview, I'll file a new bug to rename them.

::: layout/generic/nsSelection.cpp:4498
(Diff revision 1)
>  
> +  if (NS_WARN_IF(NS_FAILED(aInnerIter->Init(aContent)))) {
> -  return NS_ERROR_FAILURE;
> +    return NS_ERROR_FAILURE;
> -}
> +  }
>  
> +  for (aInnerIter->First(); !aInnerIter->IsDone(); aInnerIter->Next()) {

Yeah, I thought so too. But I don't fully understand the detail of nsContentIterator. Therefore, I didn't change it. However, we could try to remove it because selection behavior change is really obvious. So, if there would be regressions, we'd get some regression reports soon.

::: layout/generic/nsSelection.cpp:4501
(Diff revision 1)
> -}
> +  }
>  
> +  for (aInnerIter->First(); !aInnerIter->IsDone(); aInnerIter->Next()) {
> +    nsINode* node = aInnerIter->GetCurrentNode();
> +    nsIContent* innercontent =
> +      node && node->IsContent() ? node->AsContent() : nullptr;

Yeah, basically, should be so. However, I saw some crash reports which hits nullptr accesses like this loop. So, nsContentIterator might have a bug (IIRC, I fixed some such crash bugs, though).

So, I'd like to keep this nullptr check.

::: layout/generic/nsSelection.cpp:4536
(Diff revision 1)
> -  iter->Init(aRange);
>  
>    // Loop through the content iterator for each content node; for each text
>    // node, call SetSelected on it:
> -  nsCOMPtr<nsIContent> content = do_QueryInterface(aRange->GetStartParent());
> +  nsINode* node = aRange->GetStartParent();
> +  nsIContent* content = node->IsContent() ? node->AsContent() : nullptr;

Sure.

::: layout/generic/nsSelection.cpp:4546
(Diff revision 1)
>  
>    // We must call first one explicitly
> -  if (content->IsNodeOfType(nsINode::eTEXT)) {
> +  bool isFirstContentTextNode = content->IsNodeOfType(nsINode::eTEXT);
> +  if (isFirstContentTextNode) {
>      nsIFrame* frame = content->GetPrimaryFrame();
>      // The frame could be an SVG text frame, in which case we'll ignore it.

Okay. I'll check it.

::: layout/generic/nsSelection.cpp:4585
(Diff revision 1)
> +    iter->Next(); // first content has already been handled.
> +  }
>    nsCOMPtr<nsIContentIterator> inneriter = NS_NewContentIterator();
> -  for (iter->First(); !iter->IsDone(); iter->Next()) {
> -    content = do_QueryInterface(iter->GetCurrentNode());
> +  for (; !iter->IsDone(); iter->Next()) {
> +    node = iter->GetCurrentNode();
> +    content = node && node->IsContent() ? node->AsContent() : nullptr;

As I said above, I'd like to keep the nullptr check due to my experience. If you don't like it, let me know. I'll remove the nullptr check in new bug.

::: layout/generic/nsSelection.cpp:4592
(Diff revision 1)
>    }
>  
>    // We must now do the last one if it is not the same as the first
>    if (aRange->GetEndParent() != aRange->GetStartParent()) {
> -    nsresult res;
> -    content = do_QueryInterface(aRange->GetEndParent(), &res);
> +    node = aRange->GetEndParent();
> +    content = node && node->IsContent() ? node->AsContent() : nullptr;

Same above. I'd like it to be done in another bug.
Comment on attachment 8871195 [details]
Bug 1367690 Optimize Selection::selectFrames()

https://reviewboard.mozilla.org/r/142684/#review146442

> This code comment says that the code intentionally ignores text nodes which have some other type of frame than "IsTextFrame", but now you're adding an else-branch which calls InvalidateFrameSubtree().  The change seems reasonable, but I think it makes this comment obsolete so it should be removed.  (Please investigate where this comment comes from though, to make sure we're not misunderstanding something.)

The comment was added by bug 371839. However, the patch just explain it's possible the frame to be an SVG text frame when redesigning the method. So, the code coming older code. However, it's 8 years ago. I guess that SVG text won't be selectable at that time and the code has been existed for nsTextFrame. And the frame invalidation was added by you at bug 619273 when you move selection bit from frame to content. So, I think that the comment should just explain it could be an SVG text frame.
Comment on attachment 8871195 [details]
Bug 1367690 Optimize Selection::selectFrames()

https://reviewboard.mozilla.org/r/142684/#review147316

::: layout/generic/nsSelection.cpp:5715
(Diff revision 1)
> -    selectFrames(presContext, difRange, false); // deselect now
> +    // deselect now
> +    SelectFrames(presContext, difRange, false);

Okay, but when the comment is overflown from 80 characters, what should we do? Actually, the following setFrames() line is overflown. Therefore, I moved the comment to its previous line. And then, I moved other comments for consistency.
Comment on attachment 8871195 [details]
Bug 1367690 Optimize Selection::selectFrames()

https://reviewboard.mozilla.org/r/142684/#review146442

> Here you assume that |node| is non-null, which is fine, but the old code did not depend on that.  For clarity, please change the existing MOZ_ASSERT(aRange) a few lines up to MOZ_ASSERT(aRange && aRange->IsPositioned()).

Oops, this is my mistake. It could be nullptr if the range is specified by JS. For example, the range starts/ends from/to a comment node.
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/429241f2e18f
Optimize Selection::selectFrames() r=mats
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/26) from comment #4)
>(In reply to Mats Palmgren (:mats) from comment #2)
>> > +  for (aInnerIter->First(); !aInnerIter->IsDone(); aInnerIter->Next()) {
>> > +    nsINode* node = aInnerIter->GetCurrentNode();
>> > +    nsIContent* innercontent =
>> > +      node && node->IsContent() ? node->AsContent() : nullptr;
>> 
>> Do we really need to null-check |node| here?  I would expect that !IsDone()
>> guarantees that GetCurrentNode() is non-null.
> 
> Yeah, basically, should be so. However, I saw some crash reports which hits
> nullptr accesses like this loop. So, nsContentIterator might have a bug
> (IIRC, I fixed some such crash bugs, though).

Please file a follow-up bug on removing these null-checks.  It adds FUD
in the mind of the reader to suggest that GetCurrentNode() can be null
even after we tested !IsDone().
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/26) from comment #7)
> Comment on attachment 8871195 [details]
> > Here you assume that |node| is non-null, which is fine, but the old code did not depend on that.  For clarity, please change the existing MOZ_ASSERT(aRange) a few lines up to MOZ_ASSERT(aRange && aRange->IsPositioned()).
> 
> Oops, this is my mistake. It could be nullptr if the range is specified by
> JS. For example, the range starts/ends from/to a comment node.

No, I believe you're mistaken.  Comment nodes are nsIContent too:
http://searchfox.org/mozilla-central/source/dom/base/Comment.h
(The only case where a Range boundary point isn't nsIContent is when
it's the document node.)
https://hg.mozilla.org/mozilla-central/rev/429241f2e18f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(In reply to Mats Palmgren (:mats) from comment #12)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/26) from
> comment #7)
> > Comment on attachment 8871195 [details]
> > > Here you assume that |node| is non-null, which is fine, but the old code did not depend on that.  For clarity, please change the existing MOZ_ASSERT(aRange) a few lines up to MOZ_ASSERT(aRange && aRange->IsPositioned()).
> > 
> > Oops, this is my mistake. It could be nullptr if the range is specified by
> > JS. For example, the range starts/ends from/to a comment node.
> 
> No, I believe you're mistaken.  Comment nodes are nsIContent too:
> http://searchfox.org/mozilla-central/source/dom/base/Comment.h
> (The only case where a Range boundary point isn't nsIContent is when
> it's the document node.)

Oh, okay. But we can add a range whose start (and/or end, perhaps) is a document node to Selection:
https://jsfiddle.net/d_toybox/f14LLucc/

And I was confused at what you pointed. I was thinking about failing to cast to nsIContent, but the problem is, whether the nsINode can be nullptr. I filed new bugs for remaining issues.
Depends on: 1368521
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: