Handle layer clients in LayoutSVGResourceContainer::registerResource

Handle layer clients in LayoutSVGResourceContainer::registerResource

When registering pending clients on a newly discovered (filter) resource,
make sure to invalidate via the DeprectatedPaintLayer if the client has
one, or the client won't be updated properly.
Make SVGResourcesCache::clientStyleChanged handle non-SVG elements.

This was exposed by scheduler changes that caused flaky behavior in
LayoutTests/css3/filters/effect-reference-rename.html, where the
script:

  document.getElementById("NotMyFilter").id = "MyFilter";

would normally execute before the call to registerResource() that added
MyFilter. With the scheduler these could happen in the opposite order
and trigger the "layoutObject->node()->isSVGElement()" assert.

BUG=453019, 432129
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193438

[fs, 2015-04-08 15:44:47]

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

[alex clarke (OOO till 29th), 2015-04-08 17:08:50]

Thanks for picking this up!  Unfortunately I don't know this code well enough to
provide a meaningful review.

[kouhei (in TOK), 2015-04-08 17:14:40]

kouhei@chromium.org changed reviewers:
+ kouhei@chromium.org

[kouhei (in TOK), 2015-04-08 17:14:41]

Thanks for fixing this!

https://codereview.chromium.org/1068073005/diff/1/Source/core/layout/svg/Layo...
File Source/core/layout/svg/LayoutSVGResourceContainer.cpp (right):

https://codereview.chromium.org/1068073005/diff/1/Source/core/layout/svg/Layo...
Source/core/layout/svg/LayoutSVGResourceContainer.cpp:226: // If the client is
has a layer (is a non-SVGElement) we need to signal
s/is has/has/

https://codereview.chromium.org/1068073005/diff/1/Source/core/layout/svg/Layo...
Source/core/layout/svg/LayoutSVGResourceContainer.cpp:229:
toLayoutBoxModelObject(layoutObject)->layer()->filterNeedsPaintInvalidation();
Would it make sense to do a early "continue" here, instead of removing the
assert on SVGResourcesCache?

[fs, 2015-04-08 17:26:04]

https://codereview.chromium.org/1068073005/diff/1/Source/core/layout/svg/Layo...
File Source/core/layout/svg/LayoutSVGResourceContainer.cpp (right):

https://codereview.chromium.org/1068073005/diff/1/Source/core/layout/svg/Layo...
Source/core/layout/svg/LayoutSVGResourceContainer.cpp:226: // If the client is
has a layer (is a non-SVGElement) we need to signal
On 2015/04/08 17:14:41, kouhei wrote:
> s/is has/has/

Done.

https://codereview.chromium.org/1068073005/diff/1/Source/core/layout/svg/Layo...
Source/core/layout/svg/LayoutSVGResourceContainer.cpp:229:
toLayoutBoxModelObject(layoutObject)->layer()->filterNeedsPaintInvalidation();
On 2015/04/08 17:14:41, kouhei wrote:
> Would it make sense to do a early "continue" here, instead of removing the
> assert on SVGResourcesCache?

I believe we still want to do the setNeedsLayoutAndFullPaintInvalidation, so
something like:

if (layoutObject->hasLayer() &&...) {
    ...filterNeedsPaintInval
     idation();
} else {
   ...
   ...clientStyleChanged();
}
layoutObject->setNeeds...

might work I suppose. I wouldn't think it's fundamentally wrong to call
clientStyleChanged with a non-SVG element though.

[kouhei (in TOK), 2015-04-08 17:28:50]

https://codereview.chromium.org/1068073005/diff/1/Source/core/layout/svg/Layo...
File Source/core/layout/svg/LayoutSVGResourceContainer.cpp (right):

https://codereview.chromium.org/1068073005/diff/1/Source/core/layout/svg/Layo...
Source/core/layout/svg/LayoutSVGResourceContainer.cpp:229:
toLayoutBoxModelObject(layoutObject)->layer()->filterNeedsPaintInvalidation();
On 2015/04/08 17:26:04, fs wrote:
> On 2015/04/08 17:14:41, kouhei wrote:
> > Would it make sense to do a early "continue" here, instead of removing the
> > assert on SVGResourcesCache?
> 
> I believe we still want to do the setNeedsLayoutAndFullPaintInvalidation, so
> something like:
> 
> if (layoutObject->hasLayer() &&...) {
>     ...filterNeedsPaintInval
>      idation();
> } else {
>    ...
>    ...clientStyleChanged();
> }
> layoutObject->setNeeds...
> 
> might work I suppose. I wouldn't think it's fundamentally wrong to call
> clientStyleChanged with a non-SVG element though.

Thanks for the explanation. No strong opinion here. lgtm either way.

[fs, 2015-04-08 17:34:16]

https://codereview.chromium.org/1068073005/diff/1/Source/core/layout/svg/Layo...
File Source/core/layout/svg/LayoutSVGResourceContainer.cpp (right):

https://codereview.chromium.org/1068073005/diff/1/Source/core/layout/svg/Layo...
Source/core/layout/svg/LayoutSVGResourceContainer.cpp:229:
toLayoutBoxModelObject(layoutObject)->layer()->filterNeedsPaintInvalidation();
On 2015/04/08 17:28:50, kouhei wrote:
> On 2015/04/08 17:26:04, fs wrote:
> > On 2015/04/08 17:14:41, kouhei wrote:
> > > Would it make sense to do a early "continue" here, instead of removing the
> > > assert on SVGResourcesCache?
> > 
> > I believe we still want to do the setNeedsLayoutAndFullPaintInvalidation, so
> > something like:
> > 
> > if (layoutObject->hasLayer() &&...) {
> >     ...filterNeedsPaintInval
> >      idation();
> > } else {
> >    ...
> >    ...clientStyleChanged();
> > }
> > layoutObject->setNeeds...
> > 
> > might work I suppose. I wouldn't think it's fundamentally wrong to call
> > clientStyleChanged with a non-SVG element though.
> 
> Thanks for the explanation. No strong opinion here. lgtm either way.

Hmm, it's quite possible that the "filter update hack" will be enough though (it
triggers a style-recalc IIRC.)

[fs, 2015-04-09 10:21:19]

The CQ bit was checked by fs@opera.com

[fs, 2015-04-09 10:21:20]

The patchset sent to the CQ was uploaded after l-g-t-m from kouhei@chromium.org
Link to the patchset: https://codereview.chromium.org/1068073005/#ps40001
(title: "Continue in loop")

[fs, 2015-04-09 10:21:33]

https://codereview.chromium.org/1068073005/diff/1/Source/core/layout/svg/Layo...
File Source/core/layout/svg/LayoutSVGResourceContainer.cpp (right):

https://codereview.chromium.org/1068073005/diff/1/Source/core/layout/svg/Layo...
Source/core/layout/svg/LayoutSVGResourceContainer.cpp:229:
toLayoutBoxModelObject(layoutObject)->layer()->filterNeedsPaintInvalidation();
On 2015/04/08 17:14:41, kouhei wrote:
> Would it make sense to do a early "continue" here, instead of removing the
> assert on SVGResourcesCache?

Ended up doing like this suggests, because I concluded that it should be enough
(and is more consistent with the rest of the code.)

[commit-bot: I haz the power, 2015-04-09 10:22:06]

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

[commit-bot: I haz the power, 2015-04-09 11:23:56]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193438