Closed
Bug 698237
Opened 13 years ago
Closed 13 years ago
Invalidate affected frames when a range in a selection is modified
Categories
(Core :: DOM: Selection, defect)
Core
DOM: Selection
Tracking
()
RESOLVED
FIXED
mozilla12
People
(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)
References
Details
(Whiteboard: [inbound])
Attachments
(2 files, 1 obsolete file)
14.24 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
1.30 KB,
patch
|
Details | Diff | Splinter Review |
The following Range methods doesn't invalidate the selection properly: SetStart SetStartBefore SetStartAfter SetEnd SetEndBefore SetEndAfter Collapse SelectNode SelectNodeContents SurroundContents Detach
Assignee | ||
Comment 1•13 years ago
|
||
Call InvalidateFrameSubtree() for all continuations of the range common ancestor before/after the modification as needed.
Attachment #570516 -
Flags: review?(Olli.Pettay)
Comment 2•13 years ago
|
||
Comment on attachment 570516 [details] [diff] [review] fix + tests, v1 >+static void InvalidateAllFrames(nsINode* aNode) >+{ >+ NS_PRECONDITION(aNode, "bad arg"); >+ >+ nsCOMPtr<nsIContent> content = do_QueryInterface(aNode); QI is a bit slow, and adds extra addref/release. Could you use non-virtual nsINode::NodeType() and switch-case. DOCUMENT_NODE should cause invalidation to root frame, ATTRIBUTE_NODE, ENTITY_REFERENCE_NODE, ENTITY_NODE, NOTATION_NODE, DOCUMENT_FRAGMENT_NODE should not cause any validation, and for the rest casting to nsIContent* should be safe. > nsRange::SetStart(nsIDOMNode* aParent, PRInt32 aOffset) > { > VALIDATE_ACCESS(aParent); > > nsCOMPtr<nsINode> parent = do_QueryInterface(aParent); >+ AutoInvalidateSelection atEndOfBlock(this); > return SetStart(parent, aOffset); > } Could we just do the whole AutoInvalidateSelection thing in DoSetRange? Does this affect badly to performance? IIRC we have some selection handling related perf bugs open when one selects a huge table or so. r- because I want QI of cycle collected objects reduced.
Attachment #570516 -
Flags: review?(Olli.Pettay) → review-
Assignee | ||
Comment 3•13 years ago
|
||
> Could you use non-virtual nsINode::NodeType() and switch-case. Fixed. I think we only need to care about TEXT and ELEMENT (nsIContent) and DOCUMENT (nsIDocument). I don't think we can have frames for the other ones(?) > Could we just do the whole AutoInvalidateSelection thing in DoSetRange? That would be overkill. When removing/adding/changing nodes the frames are already invalidated by other code. Same for Selection methods which does its invalidation in selectFrames(). Some methods also calls DoSetRange multiple times. Still, I agree it would be nice to consolidate the invalidation part in one place. I think a better way to do that though is to implement the :selection pseudo-class and let the style system do the updates. > Does this affect badly to performance? The performance impact should be negligible I think, I don't see how we can fix this bug without some sort of frame invalidation code. > IIRC we have some selection handling > related perf bugs open when one selects a huge table or so. Let me know if you have know the specific bug numbers and I'll test it - I'll query Bugzilla to see what I can find. (FYI, I'm in New Zealand time zone the next two weeks)
Attachment #570516 -
Attachment is obsolete: true
Attachment #574179 -
Flags: review?(bugs)
Comment 4•13 years ago
|
||
Comment on attachment 574179 [details] [diff] [review] fix + tests, v2 >+ AutoInvalidateSelection atEndOfBlock(this); Not having AutoInvalidateSelection in DoSetRange looks a bit error prone. But I can see the performance reasons to not do that. >+nsRange::GetRegisteredCommonAncestor() >+{ >+ NS_ASSERTION(IsInSelection(), >+ "GetRegisteredCommonAncestor only valid for range in selection"); >+ nsINode* ancestor = GetNextRangeCommonAncestor(mStartParent); >+ while (ancestor) { >+ RangeHashTable* ranges = >+ static_cast<RangeHashTable*>(ancestor->GetProperty(nsGkAtoms::range)); >+ if (ranges->GetEntry(this)) { >+ break; >+ } >+ ancestor = GetNextRangeCommonAncestor(ancestor->GetNodeParent()); >+ } This could be reasonable slow. We have still plenty of spare boolean flags in nsINode, so would it make sense to add a flag if node may have a range in its RangeHashTable? Though, actually, perhaps there should be flag to indicate if node has any properties. Could you file a followup to investigate how often nodes have properties (I mean any property) and how often GetProperty is used on nodes which don't have any properties. >+ nsINode* GetRegisteredCommonAncestor(); Please document what this method does. >+ struct NS_STACK_CLASS AutoInvalidateSelection { >+ AutoInvalidateSelection(nsRange* aRange) : mRange(aRange) { { should be in the next line. >+#ifdef DEBUG >+ mWasInSelection = mRange->IsInSelection(); >+#endif >+ if (!mRange->IsInSelection() || mIsNested) >+ return; if (expr) { stmt; }
Attachment #574179 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 5•13 years ago
|
||
> >+nsRange::GetRegisteredCommonAncestor() > >+{ > >+ NS_ASSERTION(IsInSelection(), > >+ "GetRegisteredCommonAncestor only valid for range in selection"); > >+ nsINode* ancestor = GetNextRangeCommonAncestor(mStartParent); > >+ while (ancestor) { > >+ RangeHashTable* ranges = > >+ static_cast<RangeHashTable*>(ancestor->GetProperty(nsGkAtoms::range)); > >+ if (ranges->GetEntry(this)) { > >+ break; > >+ } > >+ ancestor = GetNextRangeCommonAncestor(ancestor->GetNodeParent()); > >+ } > This could be reasonable slow. We have still plenty of spare boolean flags > in nsINode, so would it > make sense to add a flag if node may have a range in its RangeHashTable? That shouldn't be a problem here, GetNextRangeCommonAncestor returns the next ancestor with the CommonAncestorForRangeInSelection bit and if non-null that ancestor MUST have this property with at least one range in the hash table. It's rare to have more than one such ancestor (overlapping ranges). > Though, actually, perhaps there should be flag to indicate if node has any > properties. > Could you file a followup to investigate how often nodes have properties (I > mean any property) and how often > GetProperty is used on nodes which don't have any properties. Sure, that could be an optimization in general.
Assignee | ||
Comment 6•13 years ago
|
||
Assignee | ||
Comment 7•13 years ago
|
||
With the nits fixed as suggested, and the wallpaper in an earlier reftest removed: https://hg.mozilla.org/integration/mozilla-inbound/rev/91909393c5a0 https://hg.mozilla.org/integration/mozilla-inbound/rev/4b2c62d75dea
Flags: in-testsuite+
Whiteboard: [inbound]
Target Milestone: --- → mozilla12
Comment 8•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/91909393c5a0 https://hg.mozilla.org/mozilla-central/rev/4b2c62d75dea
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•