908880 - Overflow rect is not updated when assigning a filter dynamically

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Markus Stange [:mstange]]

Attachment: testcase

Similar to bug 879682, but with a filter that's added dynamically to an already existing element.

Comment 1

[Markus Stange [:mstange]]

Attachment: v1

FinishAndStoreOverflow wasn't called on the frame. Sending nsChangeHint_UpdateOverflow calls it.

Comment 2

[Jonathan Watt [:jwatt]]

Comment on attachment 794866 [details] [diff] [review]
v1

Review of attachment 794866 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: layout/style/nsStyleStruct.cpp
@@ +1153,5 @@
>    if (!EqualURIs(mClipPath, aOther.mClipPath) ||
>        !EqualURIs(mMask, aOther.mMask) ||
>        mFilters != aOther.mFilters) {
>      NS_UpdateHint(hint, nsChangeHint_UpdateEffects);
> +    NS_UpdateHint(hint, nsChangeHint_UpdateOverflow);

Can you tack on:

 // for filters only

to this new line?

Comment 3

[Markus Stange [:mstange]]

Sure.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f37447f59817

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Caused assertion failures on Windows like:

https://tbpl.mozilla.org/php/getParsedLog.php?id=26962547&tree=Mozilla-Inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/beba6103e786

Comment 5

[Robert Longson [:longsonr]]

If we have a hint for filters only then it should be for filters only shouldn't it i.e.

if (mFilters != aOther.mFilters) {
    NS_UpdateHint(hint, nsChangeHint_UpdateOverflow);
}

if (!EqualURIs(mClipPath, aOther.mClipPath) ||
    !EqualURIs(mMask, aOther.mMask) ||
    mFilters != aOther.mFilters) {
    NS_UpdateHint(hint, nsChangeHint_UpdateEffects);
    NS_UpdateHint(hint, nsChangeHint_RepaintFrame);
}

Comment 6

[Markus Stange [:mstange]]

I agree, I'll make that change when I land again.

Comment 7

[Markus Stange [:mstange]]

The assertion is:

###!!! ASSERTION: CalcDifference() returned bigger hint than MaxDifference(): 'NS_IsHintSubset( thisSVGReset->CalcDifference(*otherSVGReset), nsStyleSVGReset::MaxDifference())', file e:/builds/moz2_slave/m-in-w32-d-0000000000000000000/build/layout/style/nsStyleContext.cpp, line 483

Comment 8

[Robert Longson [:longsonr]]

You need to update this line:

http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.h#2348

So that nsChangeHint_UpdateOverflow is allowed.

Comment 9

[Markus Stange [:mstange]]

So it turns out that this failure was not an issue with the patch but with the build which did not recompile nsStyleContext.cpp. I filed bug 908993 on that.

Comment 10

[Markus Stange [:mstange]]

Attachment: v2

New patch with the change requested in comment 5.

Comment 11

[Markus Stange [:mstange]]

Comment on attachment 795025 [details] [diff] [review]
v2

I'll open a new bug for this.

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 794866 [details] [diff] [review]
v1

This was relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/48a3aeb26ed6

Comment 13

[Robert Longson [:longsonr]]

This did land on central as part of https://hg.mozilla.org/mozilla-central/rev/48a3aeb26ed6

Comment 14

[Jonathan Watt [:jwatt]]

This fixed bug 886883.

Comment 15

[Jonathan Watt [:jwatt]]

Attachment: patch for beta (v25)

Requesting approval for beta (v25) based on the fact that this fixed tracking-25+ bug 886883.

This is a minimal version of the patch for this bug, plus the tweak made as a follow-up in bug 911310.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 875037 
User impact if declined: Firebug needs to keep their workaround for bug 886883.
Testing completed (on m-c, etc.): baked on nightly and aurora
Risk to taking this patch (and alternatives if risky): very low
String or IDL/UUID changes made by this patch: none

Comment 16

[Alex Keybl [:akeybl]]

Comment on attachment 816870 [details] [diff] [review]
patch for beta (v25)

Sadly, too late in the cycle to take a non-critical fix.