911310 - Only raise an nsChangeHint_UpdateOverflow for filters

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Attachment: invalidate.txt

Comment 1

[Markus Stange [:mstange]]

Thanks!

Comment 2

[Daniel Holbert [:dholbert]]

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.)

Comment 3

[Robert Longson [:longsonr]]

(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 4

[Daniel Holbert [:dholbert]]

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.

Comment 5

[Robert Longson [:longsonr]]

https://tbpl.mozilla.org/?tree=Try&rev=97c20459adc1

Comment 6

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/00d15571bb86

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/00d15571bb86