Invalidate ancestor resources in when relative lengths change

Invalidate ancestor resources in when relative lengths change

If a LayoutObject within a resource had relative lengths, and the viewport
changed, only the object itself would be invalidated - not any of the
clients of the resource. This would fail to invalidate the cached display
item with Slimming Paint.
Modify SVGClipPathElement::selfHasRelativeLengths() to indicate if any
of the children have relative lengths, to make sure the clients are
invalidated in that case. (SVGClipPathElement will otherwise never indicate
that it has relative lengths.)

Fixes svg/custom/viewport-resource-inval.svg with Slimming Paint enabled.

BUG=477743
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194949

[fs, 2015-04-30 13:36:46]

fs@opera.com changed reviewers:
+ pdr@chromium.org

[fs, 2015-04-30 13:36:47]

https://codereview.chromium.org/1111223002/diff/20001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/1111223002/diff/20001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:1204: crbug.com/477743
svg/W3C-SVG-1.1/masking-path-04-b.svg [ NeedsRebaseline ]
This puzzled me a bit at first, but this has a <clipPath> with a <text> (always
flagged as having relative stuffs). Not quite sure why the bbox of the <text>
changes slightly between the initial layout and the content size change. I
stepped though it though and saw nothing strange. I suppose the scale factor
could've change or something...

https://codereview.chromium.org/1111223002/diff/20001/Source/core/layout/svg/...
File Source/core/layout/svg/LayoutSVGResourceContainer.cpp (right):

https://codereview.chromium.org/1111223002/diff/20001/Source/core/layout/svg/...
Source/core/layout/svg/LayoutSVGResourceContainer.cpp:283:
object->setNeedsLayoutAndFullPaintInvalidation(LayoutInvalidationReason::SvgResourceInvalidated,
MarkContainerChain, layoutScope);
I suppose we should work to thread the layout scope even further. Pretty big
change that though. *puts on mental TODO*

https://codereview.chromium.org/1111223002/diff/20001/Source/core/svg/SVGElem...
File Source/core/svg/SVGElement.cpp (left):

https://codereview.chromium.org/1111223002/diff/20001/Source/core/svg/SVGElem...
Source/core/svg/SVGElement.cpp:426:
renderer->setNeedsLayoutAndFullPaintInvalidation(LayoutInvalidationReason::Unknown,
MarkContainerChain, layoutScope);
So Unknown -> SvgResourceInvalidated; bad -> weird. Having
SvgResourceInvalidated where it is is weird too though. I'll look into
refactoring that function a bit to unweird this a bit.

[pdr., 2015-05-01 04:13:17]

pdr@chromium.org changed reviewers:
+ fmalita@chromium.org

[pdr., 2015-05-01 04:14:44]

pdr@chromium.org changed reviewers:
- fmalita@chromium.org

[pdr., 2015-05-01 04:14:44]

I spent some time reviewing this and I don't understand why this is required.
Why isn't this invalidation handled through the existing call to
markClientForInvalidation which should invalidate the display item through
setNeedsBoundariesUpdate?

[fs, 2015-05-04 13:37:06]

On 2015/05/01 04:14:44, pdr wrote:
> I spent some time reviewing this and I don't understand why this is required.
> Why isn't this invalidation handled through the existing call to
> markClientForInvalidation which should invalidate the display item through
> setNeedsBoundariesUpdate?

Because (SVGClipPathElement::)selfHasRelativeLengths() returns false, so
invalidateCacheAndMarkForLayout etc is never called. Maybe a artificially
flagging when SVGClipPathElement has relative children would be a reasonable
alternative (see PS3)?

[fs, 2015-05-04 14:21:30]

On 2015/05/04 13:37:06, fs wrote:
> On 2015/05/01 04:14:44, pdr wrote:
> > I spent some time reviewing this and I don't understand why this is
required.
> > Why isn't this invalidation handled through the existing call to
> > markClientForInvalidation which should invalidate the display item through
> > setNeedsBoundariesUpdate?
> 
> Because (SVGClipPathElement::)selfHasRelativeLengths() returns false, so
> invalidateCacheAndMarkForLayout etc is never called. Maybe a artificially
> flagging when SVGClipPathElement has relative children would be a reasonable
> alternative (see PS3)?

Refined further in PS4 (<mask> and <filter> should suffer from this too, but
less likely to show because of the default "dimensions" being percentages.)

[pdr., 2015-05-05 06:26:13]

On 2015/05/04 at 14:21:30, fs wrote:
> On 2015/05/04 13:37:06, fs wrote:
> > On 2015/05/01 04:14:44, pdr wrote:
> > > I spent some time reviewing this and I don't understand why this is
required.
> > > Why isn't this invalidation handled through the existing call to
> > > markClientForInvalidation which should invalidate the display item through
> > > setNeedsBoundariesUpdate?
> > 
> > Because (SVGClipPathElement::)selfHasRelativeLengths() returns false, so
> > invalidateCacheAndMarkForLayout etc is never called. Maybe a artificially
> > flagging when SVGClipPathElement has relative children would be a reasonable
> > alternative (see PS3)?
> 
> Refined further in PS4 (<mask> and <filter> should suffer from this too, but
less likely to show because of the default "dimensions" being percentages.)

I'm sorry for giving you a hard time on this one but I still don't quite
understand. I agree that we appear to be missing an invalidation here but the
test from this patch is already passing
(https://codereview.chromium.org/1126703003). Could a more serious
over-invalidation bug be masking this issue?

I'm not sure we should override selfHasRelativeLengths to return
hasRelativeLengths. With the exception of SVGUseElement, all other
selfHasRelativeLength functions only consider the element itself.

Should that if statement just be this?
if (layoutObject) {
    if (layoutObject->isSVGResourceContainer() && hasRelativeLengths())
       
toLayoutSVGResourceContainer(layoutObject)->invalidateCacheAndMarkForLayout(layoutScope);
    else if (selfHasRelativeLengths())
       
layoutObject->setNeedsLayoutAndFullPaintInvalidation(LayoutInvalidationReason::Unknown,
MarkContainerChain, layoutScope);
}

[fs, 2015-05-05 08:57:57]

On 2015/05/05 06:26:13, pdr (Slow May 5th - May 18th) wrote:
> On 2015/05/04 at 14:21:30, fs wrote:
> > On 2015/05/04 13:37:06, fs wrote:
> > > On 2015/05/01 04:14:44, pdr wrote:
> > > > I spent some time reviewing this and I don't understand why this is
> required.
> > > > Why isn't this invalidation handled through the existing call to
> > > > markClientForInvalidation which should invalidate the display item
through
> > > > setNeedsBoundariesUpdate?
> > > 
> > > Because (SVGClipPathElement::)selfHasRelativeLengths() returns false, so
> > > invalidateCacheAndMarkForLayout etc is never called. Maybe a artificially
> > > flagging when SVGClipPathElement has relative children would be a
reasonable
> > > alternative (see PS3)?
> > 
> > Refined further in PS4 (<mask> and <filter> should suffer from this too, but
> less likely to show because of the default "dimensions" being percentages.)
> 
> I'm sorry for giving you a hard time on this one but I still don't quite
> understand. I agree that we appear to be missing an invalidation here but the
> test from this patch is already passing
> (https://codereview.chromium.org/1126703003). Could a more serious
> over-invalidation bug be masking this issue?

The entire viewport will be repainted - which will likely mask a lot of things.
Additionally the <clipPath> will still be layouted - and because it's simple
enough to hit the "path-only-clipping" path there's no state to invalidate. With
Slimming Paint though the constraints are tighter.

> I'm not sure we should override selfHasRelativeLengths to return
> hasRelativeLengths.

Yes, I agree this has a bad feel to it... (But then again, the viewport handling
for resources is monumentally broken [to exaggerate slightly], and the paint
outside the corner would need a lot of help to dry...)

> With the exception of SVGUseElement, all other
> selfHasRelativeLength functions only consider the element itself.
> 
> Should that if statement just be this?
> if (layoutObject) {
>     if (layoutObject->isSVGResourceContainer() && hasRelativeLengths())
>        
>
toLayoutSVGResourceContainer(layoutObject)->invalidateCacheAndMarkForLayout(layoutScope);
>     else if (selfHasRelativeLengths())
>        
>
layoutObject->setNeedsLayoutAndFullPaintInvalidation(LayoutInvalidationReason::Unknown,
> MarkContainerChain, layoutScope);
> }

Yes, that ought to work too (with some added over-invalidation in some cases
[oBB vs. uSOU] - but probably not worse than what is there already). Uploaded.

[fs, 2015-05-05 09:03:54]

On 2015/05/05 08:57:57, fs wrote:
> On 2015/05/05 06:26:13, pdr (Slow May 5th - May 18th) wrote:
> > On 2015/05/04 at 14:21:30, fs wrote:
> > > On 2015/05/04 13:37:06, fs wrote:
> > > > On 2015/05/01 04:14:44, pdr wrote:
> > > > > I spent some time reviewing this and I don't understand why this is
> > required.
> > > > > Why isn't this invalidation handled through the existing call to
> > > > > markClientForInvalidation which should invalidate the display item
> through
> > > > > setNeedsBoundariesUpdate?
> > > > 
> > > > Because (SVGClipPathElement::)selfHasRelativeLengths() returns false, so
> > > > invalidateCacheAndMarkForLayout etc is never called. Maybe a
artificially
> > > > flagging when SVGClipPathElement has relative children would be a
> reasonable
> > > > alternative (see PS3)?
> > > 
> > > Refined further in PS4 (<mask> and <filter> should suffer from this too,
but
> > less likely to show because of the default "dimensions" being percentages.)
> > 
> > I'm sorry for giving you a hard time on this one but I still don't quite
> > understand. I agree that we appear to be missing an invalidation here but
the
> > test from this patch is already passing
> > (https://codereview.chromium.org/1126703003). Could a more serious
> > over-invalidation bug be masking this issue?
> 
> The entire viewport will be repainted - which will likely mask a lot of
things.
> Additionally the <clipPath> will still be layouted - and because it's simple
> enough to hit the "path-only-clipping" path there's no state to invalidate.
With
> Slimming Paint though the constraints are tighter.

Here's an illustration: http://jsfiddle.net/xdrntm1q/2/

[pdr., 2015-05-06 00:01:53]

The CQ bit was checked by pdr@chromium.org

[pdr., 2015-05-06 00:01:54]

lgtm

LGTM, thanks for the patience

[commit-bot: I haz the power, 2015-05-06 00:02:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1111223002/120001

[commit-bot: I haz the power, 2015-05-06 00:05:49]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194949