Last Comment Bug 698237 - Invalidate affected frames when a range in a selection is modified
: Invalidate affected frames when a range in a selection is modified
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: Selection (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla12
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on: 619273
Blocks: 673108 677269
  Show dependency treegraph
 
Reported: 2011-10-29 19:50 PDT by Mats Palmgren (:mats)
Modified: 2011-12-27 02:28 PST (History)
5 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix + tests, v1 (13.93 KB, patch)
2011-10-29 19:57 PDT, Mats Palmgren (:mats)
bugs: review-
Details | Diff | Splinter Review
fix + tests, v2 (14.24 KB, patch)
2011-11-13 13:35 PST, Mats Palmgren (:mats)
bugs: review+
Details | Diff | Splinter Review
part 2. remove wallpaper for this bug in an earlier reftest (1.30 KB, patch)
2011-12-24 05:31 PST, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review

Description Mats Palmgren (:mats) 2011-10-29 19:50:33 PDT
The following Range methods doesn't invalidate the selection properly:
SetStart
  SetStartBefore
  SetStartAfter
SetEnd
  SetEndBefore
  SetEndAfter
Collapse
SelectNode
SelectNodeContents
SurroundContents
Detach
Comment 1 Mats Palmgren (:mats) 2011-10-29 19:57:09 PDT
Created attachment 570516 [details] [diff] [review]
fix + tests, v1

Call InvalidateFrameSubtree() for all continuations of the
range common ancestor before/after the modification as needed.
Comment 2 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-10-31 16:01:15 PDT
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.
Comment 3 Mats Palmgren (:mats) 2011-11-13 13:35:18 PST
Created attachment 574179 [details] [diff] [review]
fix + tests, v2

> 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)
Comment 4 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-11-16 03:48:34 PST
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;
}
Comment 5 Mats Palmgren (:mats) 2011-11-16 13:13:27 PST
> >+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.
Comment 6 Mats Palmgren (:mats) 2011-12-24 05:31:16 PST
Created attachment 584190 [details] [diff] [review]
part 2.  remove wallpaper for this bug in an earlier reftest
Comment 7 Mats Palmgren (:mats) 2011-12-24 05:34:19 PST
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

Note You need to log in before you can comment on or make changes to this bug.