Closed
Bug 1456428
Opened 7 years ago
Closed 5 years ago
Don't reset Selection::mCachedRange from Clear() simply
Categories
(Core :: DOM: Selection, enhancement, P3)
Core
DOM: Selection
Tracking
()
RESOLVED
INVALID
| Tracking | Status | |
|---|---|---|
| firefox61 | --- | affected |
People
(Reporter: masayuki, Assigned: masayuki)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
|
(deleted),
text/x-review-board-request
|
Details |
Currently, Selection caches a DOM range with mCachedRange to avoid reallocating nsRange instance. However, this is unset from Selection by Selection::Clear(). Therefore, even if this is used later, it needs to be reset related information again even if it refers same frame/node.
|
Assignee | |
Comment 1•7 years ago
|
||
|
|
Assignee | |
Comment 2•7 years ago
|
||
|
|
Assignee | |
Comment 3•7 years ago
|
||
|
|
Assignee | |
Comment 4•7 years ago
|
||
|
|
Assignee | |
Comment 5•7 years ago
|
||
|
|
Assignee | |
Comment 6•7 years ago
|
||
| Comment hidden (mozreview-request) |
|
||
Comment 8•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8971468 [details]
Bug 1456428 - Don't unset selection of mCachedRange in Selection::Clear() since setting selection again is too expensive
https://reviewboard.mozilla.org/r/240092/#review246134
Sorry, I don't have time to review this b/c I'm busy with grid stuff ATM,
so I suggest you get review from a DOM peer instead (smaug probably).
Attachment #8971468 -
Flags: review?(mats)
|
|
Assignee | |
Comment 9•7 years ago
|
||
Comment on attachment 8971468 [details]
Bug 1456428 - Don't unset selection of mCachedRange in Selection::Clear() since setting selection again is too expensive
Thanks, mats. Smaug, could you review this?
Attachment #8971468 -
Flags: review?(bugs)
|
||
Comment 10•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8971468 [details]
Bug 1456428 - Don't unset selection of mCachedRange in Selection::Clear() since setting selection again is too expensive
https://reviewboard.mozilla.org/r/240092/#review246264
::: dom/base/Selection.cpp:2560
(Diff revision 1)
> - RefPtr<nsRange> range;
> + RefPtr<nsRange> range;
> - // If the old range isn't referred by anybody other than this method,
> - // we should reuse it for reducing the recreation cost.
> - if (oldRange && oldRange->GetRefCount() == 1) {
> - range = Move(oldRange);
> - } else if (mCachedRange) {
> + if (mCachedRange) {
> + // Don't move mCachedRange to |range| now because a call of CollapseTo()
> + // makes nsRange check if it's actually associated with this selection
> + // with referring GetCachedRange(). If it returns nullptr, nsRange
> + // will fire unexpected selection change notification.
Hmm, why CollapseTo method's selection change notifications depend on whether or not there is cached range. Sounds like a bug...
oh, I see, because mSelection is non-null.
Ok, so nsRange::IsInSelection starts to sort of lie after this patch.
Attachment #8971468 -
Flags: review?(bugs)
|
|
||
Comment 11•7 years ago
|
||
Comment on attachment 8971468 [details]
Bug 1456428 - Don't unset selection of mCachedRange in Selection::Clear() since setting selection again is too expensive
Oops, I wasn't going to clear the request.
Just wondering if there is any simpler way to deal with this.
Attachment #8971468 -
Flags: review?(bugs)
|
|
Assignee | |
Comment 12•7 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8971468 [details]
Bug 1456428 - Don't unset selection of mCachedRange in Selection::Clear() since setting selection again is too expensive
https://reviewboard.mozilla.org/r/240092/#review246264
> Hmm, why CollapseTo method's selection change notifications depend on whether or not there is cached range. Sounds like a bug...
> oh, I see, because mSelection is non-null.
>
> Ok, so nsRange::IsInSelection starts to sort of lie after this patch.
> Ok, so nsRange::IsInSelection starts to sort of lie after this patch.
Oh, right. Perhaps, nsRange::IsInSelection() should also check if it's NOT cached range. Then, nsRange should check IsInSelection() instead of mSelection before notifying selection listeners. However, I need to investigate it won't cause problem with any callers of IsInSelection().
|
|
||
Comment 13•7 years ago
|
||
Could you explain how IsDescendantOfCommonAncestorForRangeInSelection() works after this change?
That flag isn't telling the truth either, since we don't always unset that flag.
And that will then affect things like nsIFrame::IsVisibleInSelection.
|
|
||
Updated•7 years ago
|
Attachment #8971468 -
Flags: review?(bugs)
|
|
Assignee | |
Comment 14•7 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #13)
> Could you explain how IsDescendantOfCommonAncestorForRangeInSelection()
> works after this change?
> That flag isn't telling the truth either, since we don't always unset that
> flag.
> And that will then affect things like nsIFrame::IsVisibleInSelection.
Yeah, unfortunately, you're right. I've not given up to fix this bug yet since the cost is expensive, however, currently, I have no idea how to skip only expensive code without making the code spaghetti since there are a lot of callers of RegisterCommonAncestor().
Sorry for the bug spam and thank you for your review.
|
||
Updated•7 years ago
|
Priority: -- → P3
|
|
Assignee | |
Comment 15•5 years ago
|
||
This is wrong approach, and we should remove mCachedRange anyway for keeping our implementation simpler (see bug 1612085).
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → INVALID
You need to log in
before you can comment on or make changes to this bug.
Description
•