Closed Bug 911310 Opened 6 years ago Closed 6 years ago

Only raise an nsChangeHint_UpdateOverflow for filters

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: longsonr, Assigned: longsonr)

References

Details

(Keywords: perf)

Attachments

(1 file)

Attached patch invalidate.txtSplinter Review
No description provided.
Attachment #797962 - Attachment is patch: true
Attachment #797962 - Flags: review?(dholbert)
Blocks: 908880
Assignee: nobody → longsonr
Thanks!
So with this patch, we won't be sending nsChangeHint_UpdateEffects for filter changes anymore?

If that's correct, we should tweak the comment for UpdateEffects to no longer mention filters:

45   /**
46    * Used when the computed value (a URI) of one or more of an element's
47    * filter/mask/clip/etc CSS properties changes, causing the element's frame
48    * to start/stop referencing (or reference different) SVG resource elements.
49    * (_Not_ used to handle changes to referenced resource elements.) Using this
50    * hint results in nsSVGEffects::UpdateEffects being called on the element's
51    * frame.
52    */
53   nsChangeHint_UpdateEffects = 0x80,

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsChangeHint.h#45

(I haven't dug deeply enough into these particular change hints to feel comfortable r+'ing this yet -- I'll review this when I'm back from vacation next week.)
(In reply to Daniel Holbert [:dholbert] (vacation through 8/31) from comment #2)
> So with this patch, we won't be sending nsChangeHint_UpdateEffects for
> filter changes anymore?

I don't think you've read the patch correctly.
Comment on attachment 797962 [details] [diff] [review]
invalidate.txt

You're quite right, I didn't read it correctly. :)


Just one nit:

> nsChangeHint nsStyleSVGReset::CalcDifference(const nsStyleSVGReset& aOther) const
> {
>   nsChangeHint hint = nsChangeHint(0);
> 
>+  if (mFilters != aOther.mFilters) {
>+    NS_UpdateHint(hint, nsChangeHint_UpdateOverflow);
>+  }
>+
>   if (!EqualURIs(mClipPath, aOther.mClipPath) ||
>       !EqualURIs(mMask, aOther.mMask) ||
>       mFilters != aOther.mFilters) {


So, now we'll be redundantly checking whether "mFilters != aOther.mFilters"  -- that's not necessarily cheap, since mFilters is a nsTArray<nsStyleFilter>, and each nsStyleFilter could contain something bulky like a URL.

So: I'd rather we do something like:

const bool filtersEqual = (mFilters == aOther.mFilter);
and then s/mFilters != aOther.mFilter/!filtersEqual/

(or "filtersNotEqual", if you prefer)

>diff --git a/layout/svg/nsSVGUseFrame.cpp b/layout/svg/nsSVGUseFrame.h
>copy from layout/svg/nsSVGUseFrame.cpp
>copy to layout/svg/nsSVGUseFrame.h

This is at the end of your patch, presumably by mistake?

r=me with the more efficient inequality-check and the unnecessary file-copy reverted.
Attachment #797962 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/mozilla-central/rev/00d15571bb86
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.