Closed Bug 698237 Opened 8 years ago Closed 8 years ago

Invalidate affected frames when a range in a selection is modified

Categories

(Core :: Selection, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: mats, Assigned: mats)

References

Details

(Whiteboard: [inbound])

Attachments

(2 files, 1 obsolete file)

The following Range methods doesn't invalidate the selection properly:
SetStart
  SetStartBefore
  SetStartAfter
SetEnd
  SetEndBefore
  SetEndAfter
Collapse
SelectNode
SelectNodeContents
SurroundContents
Detach
Attached patch fix + tests, v1 (obsolete) — Splinter Review
Call InvalidateFrameSubtree() for all continuations of the
range common ancestor before/after the modification as needed.
Attachment #570516 - Flags: review?(Olli.Pettay)
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-
Attached patch fix + tests, v2Splinter Review
> 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 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+
> >+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.
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
Blocks: 673108
Blocks: 677269
You need to log in before you can comment on or make changes to this bug.