Bug 90405 – CSS url() filters with forward references don't work

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Stephen White]

Referencing an SVG filter declared after the CSS style declaration doesn't seem to work.  Only references to nodes which appear before the style will be applied.

[Keyar Hood]

The code to add the filter, WebCore::FilterEffectRenderer::buildReferenceFilter, makes a call to document->getElementById() and this returns a null pointer in the case of the filter being defined after the element.

The SVG code path does not have this problem as demonstrated by http://jsfiddle.net/rjfJt/18/

[Keyar Hood]

Created attachment 155345 [details]
Patch

[Keyar Hood]

This is a work in progress. I am looking for comments on if there is a better way to proceed though.

[Keyar Hood]

Created attachment 155639 [details]
Patch

[Keyar Hood]

Created attachment 156433 [details]
Patch

[Early Warning System Bot]

Comment on attachment 156433 [details]
Patch

Attachment 156433 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13419995

[Early Warning System Bot]

Comment on attachment 156433 [details]
Patch

Attachment 156433 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13425750

[Keyar Hood]

Created attachment 156449 [details]
Patch

[Early Warning System Bot]

Comment on attachment 156449 [details]
Patch

Attachment 156449 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13434165

[Early Warning System Bot]

Comment on attachment 156449 [details]
Patch

Attachment 156449 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13421940

[Keyar Hood]

Created attachment 156773 [details]
Patch

[Early Warning System Bot]

Comment on attachment 156773 [details]
Patch

Attachment 156773 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13445538

[Early Warning System Bot]

Comment on attachment 156773 [details]
Patch

Attachment 156773 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13447572

[Keyar Hood]

Created attachment 156954 [details]
Patch

[Stephen White]

Comment on attachment 156954 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156954&action=review

This looks pretty good.  I think you're almost there, but I'm not the most familiar with this code.  In addition to my comments, one of the SVG guys (Dirk or Niko) should take a look.

> Source/WebCore/ChangeLog:33
> +        (WebCore::FilterEffectRenderer::buildReferenceFilter): If the referenced filter cannot be found, we add it's id as a pending reference. Signature was changed to handle this.

Nit: it's -> its

> Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.cpp:129
> +    // The renderer parameter is only needed for CSS shaders.

Is this comment still true?  It seems like this parameter is now used for reference filters as well.

(Not your fault, but this call is a layering violation.  Files in platform/ shouldn't reference files outside platform/.)

> Source/WebCore/rendering/FilterEffectRenderer.cpp:186
> +    Document* document = 0;

This could be expressed more compactly as:

Document* document = renderer ? renderer->document() : 0;

> Source/WebCore/rendering/FilterEffectRenderer.cpp:195
> +        operations = &(renderer->style()->filter());

This seems a little ugly, but I suppose it's necessary due to the TextureMapper call site.  We should probably clean that up, remove the operations parameter, and always extract it from the renderer.  That could be done in another patch, though.

> Source/WebCore/rendering/svg/SVGResources.cpp:197
> +        return setFilter(getRenderSVGResourceById<RenderSVGResourceFilter>(document, id));

Seems a little weird that we assume that a non-SVG element in the cache always has a filter.  Probably true due to the calling code right now, but could be fragile.  Could we do a style->hasFilter() check here?

> LayoutTests/platform/chromium/TestExpectations:3488
> +BUGWK93149 WIN MAC SKIP : css3/filters/effect-reference-after.html

Don't mark these as SKIP, or you won't be able to get images off the bots to rebaseline them.

Mark them as 

BUGWK93149 WIN MAC : <testname> = IMAGE

or

BUGWK93149 WIN MAC : <testname> = IMAGE+TEXT

as appropriate.

[Keyar Hood]

Created attachment 157304 [details]
Patch

[Stephen White]

Comment on attachment 157304 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157304&action=review

> Source/WebCore/rendering/FilterEffectRenderer.cpp:123
> +    Document* document = renderer->document();

Could the renderer be NULL here as well?  It seems like it could, from the TextureMapper call site.  I suppose we should just abort in that case, since we won't be able to find the SVG nodes anyway.

> Source/WebCore/rendering/svg/SVGResources.cpp:196
> +        AtomicString id(style->filterResource());

This variable could also be scoped to the if-statement below.

[Noam Rosenthal]

(In reply to comment #17)
> (From update of attachment 157304 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157304&action=review
> 
> > Source/WebCore/rendering/FilterEffectRenderer.cpp:123
> > +    Document* document = renderer->document();
> 
> Could the renderer be NULL here as well?  It seems like it could, from the TextureMapper call site.  I suppose we should just abort in that case, since we won't be able to find the SVG nodes anyway.
AFAIK It can't be null from the TextureMapper call site. Either way, we should remove the layering violation in TextureMapper, I created https://bugs.webkit.org/show_bug.cgi?id=93549.

[Keyar Hood]

(In reply to comment #18)
> (In reply to comment #17)
> > (From update of attachment 157304 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=157304&action=review
> > 
> > > Source/WebCore/rendering/FilterEffectRenderer.cpp:123
> > > +    Document* document = renderer->document();
> > 
> > Could the renderer be NULL here as well?  It seems like it could, from the TextureMapper call site.  I suppose we should just abort in that case, since we won't be able to find the SVG nodes anyway.
> AFAIK It can't be null from the TextureMapper call site. Either way, we should remove the layering violation in TextureMapper, I created https://bugs.webkit.org/show_bug.cgi?id=93549.

Until the layering violation is removed, I will add a check just to be on the safe side.

[Keyar Hood]

Created attachment 157483 [details]
Patch

[Keyar Hood]

Nikolas or Dirk, could you please review this patch or suggest who I should ask to review this patch?

[Dirk Schulze]

Comment on attachment 157483 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157483&action=review

If Niko does not have time, you can ask anttik as well. He usually gives great feedback.

> Source/WebCore/rendering/FilterEffectRenderer.cpp:120
> +PassRefPtr<FilterEffect> FilterEffectRenderer::buildReferenceFilter(RenderBoxModelObject* renderer, PassRefPtr<FilterEffect> previousEffect, ReferenceFilterOperation* op)

I wonder why you pass a RenderBoxModelObject here? Just a few SVG elements inherit from RenderBoxModelObject. Is that compatible to SVG?

> Source/WebCore/rendering/FilterEffectRenderer.h:103
> +    bool build(RenderBoxModelObject* renderer, const FilterOperations*);
> +    PassRefPtr<FilterEffect> buildReferenceFilter(RenderBoxModelObject* renderer, PassRefPtr<FilterEffect> previousEffect, ReferenceFilterOperation*);

see comment above.

> Source/WebCore/svg/SVGDocumentExtensions.cpp:214
> -void SVGDocumentExtensions::addPendingResource(const AtomicString& id, SVGStyledElement* element)
> +void SVGDocumentExtensions::addPendingResource(const AtomicString& id, Element* element)

Why do you replace SVGStyledElement with Element? Why not StyledElement? Seems also be more secure for SVG, since we assume (or assumed?) that we handle styled elements on other places? This comment applies to all changes in this class.

> Source/WebCore/svg/SVGDocumentExtensions.h:43
> -class SVGStyledElement;
> +class Element;

... and classes.

[Stephen White]

Comment on attachment 157483 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157483&action=review

> Source/WebCore/ChangeLog:10
> +        applied needs to be notified when the SVG filter shows. There is

Clarity nit:  "shows" -> "is added to the DOM"?

>> Source/WebCore/rendering/FilterEffectRenderer.cpp:120
>> +PassRefPtr<FilterEffect> FilterEffectRenderer::buildReferenceFilter(RenderBoxModelObject* renderer, PassRefPtr<FilterEffect> previousEffect, ReferenceFilterOperation* op)
> 
> I wonder why you pass a RenderBoxModelObject here? Just a few SVG elements inherit from RenderBoxModelObject. Is that compatible to SVG?

The renderer passed to buildReferenceFilter() is the element (layer) to which a CSS filter is applied, not an SVG element.  So I think this ok, since the renderer passed at the RenderLayer::updateOrRemoveFilterEffect() call site is guaranteed to an RBMO (RenderLayer::renderer()).

[Dirk Schulze]

(In reply to comment #23)
> (From update of attachment 157483 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157483&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        applied needs to be notified when the SVG filter shows. There is
> 
> Clarity nit:  "shows" -> "is added to the DOM"?
> 
> >> Source/WebCore/rendering/FilterEffectRenderer.cpp:120
> >> +PassRefPtr<FilterEffect> FilterEffectRenderer::buildReferenceFilter(RenderBoxModelObject* renderer, PassRefPtr<FilterEffect> previousEffect, ReferenceFilterOperation* op)
> > 
> > I wonder why you pass a RenderBoxModelObject here? Just a few SVG elements inherit from RenderBoxModelObject. Is that compatible to SVG?
> 
> The renderer passed to buildReferenceFilter() is the element (layer) to which a CSS filter is applied, not an SVG element.  So I think this ok, since the renderer passed at the RenderLayer::updateOrRemoveFilterEffect() call site is guaranteed to an RBMO (RenderLayer::renderer()).

Maybe I don't get your point, but the CSS Filter chain should work on SVG elements (like <rect>) as well. But <rect> inherits from RenderSVGModelObject, which does not inherit from RenderBoxModelObject at all. So just using RenderObject would be fine. Am I wrong?

[Stephen White]

Comment on attachment 157483 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157483&action=review

>>> Source/WebCore/ChangeLog:10
>>> +        applied needs to be notified when the SVG filter shows. There is
>> 
>> Clarity nit:  "shows" -> "is added to the DOM"?
> 
> Maybe I don't get your point, but the CSS Filter chain should work on SVG elements (like <rect>) as well. But <rect> inherits from RenderSVGModelObject, which does not inherit from RenderBoxModelObject at all. So just using RenderObject would be fine. Am I wrong?

If RenderObject works fine for these purposes, then we could definitely make that change now (Keyar?).

I don't think CSS filters currently work on SVG elements, since -webkit-filter can only apply to RenderLayers, and I don't think SVG elements can be promoted to a RenderLayer.  But maybe I'm wrong about that?

In any event, it shouldn't collide with the existing SVG filters on SVG elements, since they use filter: rather than -webkit-filter: and don't end up in this code path.  However, once the vendor prefix drops, there may be some collisions we have to resolve.

[Dirk Schulze]

(In reply to comment #25)
> (From update of attachment 157483 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157483&action=review
> 
> >>> Source/WebCore/ChangeLog:10
> >>> +        applied needs to be notified when the SVG filter shows. There is
> >> 
> >> Clarity nit:  "shows" -> "is added to the DOM"?
> > 
> > Maybe I don't get your point, but the CSS Filter chain should work on SVG elements (like <rect>) as well. But <rect> inherits from RenderSVGModelObject, which does not inherit from RenderBoxModelObject at all. So just using RenderObject would be fine. Am I wrong?
> 
> If RenderObject works fine for these purposes, then we could definitely make that change now (Keyar?).
> 
> I don't think CSS filters currently work on SVG elements, since -webkit-filter can only apply to RenderLayers, and I don't think SVG elements can be promoted to a RenderLayer.  But maybe I'm wrong about that?
SVG does not support layers yet. And maybe SVG elements renderer will inherit from RenderBoxModelObject in the future. Right now they don't and I would rather not exclude SVG elements from the beginning, since The Filter Effects spec explicitly says that these filters should be applicable to SVG elements as well.

> 
> In any event, it shouldn't collide with the existing SVG filters on SVG elements, since they use filter: rather than -webkit-filter: and don't end up in this code path.  However, once the vendor prefix drops, there may be some collisions we have to resolve.
They don't collide, but that is not my point.

[Keyar Hood]

Comment on attachment 157483 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157483&action=review

>> Source/WebCore/rendering/FilterEffectRenderer.cpp:120
>> +PassRefPtr<FilterEffect> FilterEffectRenderer::buildReferenceFilter(RenderBoxModelObject* renderer, PassRefPtr<FilterEffect> previousEffect, ReferenceFilterOperation* op)
> 
> I wonder why you pass a RenderBoxModelObject here? Just a few SVG elements inherit from RenderBoxModelObject. Is that compatible to SVG?

It looks the SVG code uses the renderer in the following way:
Node associated with renderer is added to a hash via SVGDocumentExtensions::addPendingResources().
In RenderSVGResourceContainer::registerResource, SVGResourceCached::clientStyleChanged is called with the renderer.

There, the functions that are called don't rely on the type of renderer other than it being a RenderObject. However, SVGResourcesCache::addResourcesFromRenderObject does depend on the node associated with the renderer being a SVG element for cycle detection, but I skip that part if we do not have a SVG element.

Similarly, I skip the appropriate parts in SVGResources::buildCachedResources if we do not have a SVG element.

That is, I believe that RenderBoxModelObject will be compatible with the SVG code.

>> Source/WebCore/svg/SVGDocumentExtensions.cpp:214
>> +void SVGDocumentExtensions::addPendingResource(const AtomicString& id, Element* element)
> 
> Why do you replace SVGStyledElement with Element? Why not StyledElement? Seems also be more secure for SVG, since we assume (or assumed?) that we handle styled elements on other places? This comment applies to all changes in this class.

No particular reason. There is a toElement (and no toStyledElement), which is convenient. The functions I changed do not seem to take advantage of the fact that they were specifically dealing with a StyledElemnt. If you would prefer, I believe I should be able to change these to StyledElements.

[Keyar Hood]

(In reply to comment #24)
> (In reply to comment #23)
> > (From update of attachment 157483 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=157483&action=review
> > 
> > > Source/WebCore/ChangeLog:10
> > > +        applied needs to be notified when the SVG filter shows. There is
> > 
> > Clarity nit:  "shows" -> "is added to the DOM"?
> > 
> > >> Source/WebCore/rendering/FilterEffectRenderer.cpp:120
> > >> +PassRefPtr<FilterEffect> FilterEffectRenderer::buildReferenceFilter(RenderBoxModelObject* renderer, PassRefPtr<FilterEffect> previousEffect, ReferenceFilterOperation* op)
> > > 
> > > I wonder why you pass a RenderBoxModelObject here? Just a few SVG elements inherit from RenderBoxModelObject. Is that compatible to SVG?
> > 
> > The renderer passed to buildReferenceFilter() is the element (layer) to which a CSS filter is applied, not an SVG element.  So I think this ok, since the renderer passed at the RenderLayer::updateOrRemoveFilterEffect() call site is guaranteed to an RBMO (RenderLayer::renderer()).
> 
> Maybe I don't get your point, but the CSS Filter chain should work on SVG elements (like <rect>) as well. But <rect> inherits from RenderSVGModelObject, which does not inherit from RenderBoxModelObject at all. So just using RenderObject would be fine. Am I wrong?

FilterEffectRenderer::buildReferenceFilter is called by FilterEffectRenderer::build is called by RenderLayer::updateOrRemoveFilterEffect and at this point we know it is a RenderBoxModelObject. This is why I was passing in a RenderBoxModelObject to build and buildReferenceFilter. 

It should be easy to change this to a RenderObject if this is preferred though.

[Dirk Schulze]

(In reply to comment #28)
> (In reply to comment #24)
> > (In reply to comment #23)
> > > (From update of attachment 157483 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=157483&action=review
> > > 
> > > > Source/WebCore/ChangeLog:10
> > > > +        applied needs to be notified when the SVG filter shows. There is
> > > 
> > > Clarity nit:  "shows" -> "is added to the DOM"?
> > > 
> > > >> Source/WebCore/rendering/FilterEffectRenderer.cpp:120
> > > >> +PassRefPtr<FilterEffect> FilterEffectRenderer::buildReferenceFilter(RenderBoxModelObject* renderer, PassRefPtr<FilterEffect> previousEffect, ReferenceFilterOperation* op)
> > > > 
> > > > I wonder why you pass a RenderBoxModelObject here? Just a few SVG elements inherit from RenderBoxModelObject. Is that compatible to SVG?
> > > 
> > > The renderer passed to buildReferenceFilter() is the element (layer) to which a CSS filter is applied, not an SVG element.  So I think this ok, since the renderer passed at the RenderLayer::updateOrRemoveFilterEffect() call site is guaranteed to an RBMO (RenderLayer::renderer()).
> > 
> > Maybe I don't get your point, but the CSS Filter chain should work on SVG elements (like <rect>) as well. But <rect> inherits from RenderSVGModelObject, which does not inherit from RenderBoxModelObject at all. So just using RenderObject would be fine. Am I wrong?
> 
> FilterEffectRenderer::buildReferenceFilter is called by FilterEffectRenderer::build is called by RenderLayer::updateOrRemoveFilterEffect and at this point we know it is a RenderBoxModelObject. This is why I was passing in a RenderBoxModelObject to build and buildReferenceFilter. 
That seems not to be correct. We can just assume that we have a RenderLayer. It would not be really future proof if we just assume that a RenderLayer always implies a RenderBoxModelObject.

> 
> It should be easy to change this to a RenderObject if this is preferred though.
I would think so at the moment.

[Keyar Hood]

Created attachment 158120 [details]
Patch

[WebKit Review Bot]

Comment on attachment 158120 [details]
Patch

Attachment 158120 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13487490

New failing tests:
css3/filters/effect-reference-after.html

[WebKit Review Bot]

Created attachment 158152 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[WebKit Review Bot]

Comment on attachment 158120 [details]
Patch

Attachment 158120 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13491382

New failing tests:
css3/filters/effect-reference-after.html

[WebKit Review Bot]

Created attachment 158180 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Keyar Hood]

Created attachment 158601 [details]
Patch

[Keyar Hood]

I changed FilterEffectRenderer::build and FilterEffectRenderer::buildReferenceFilter to take in a RenderObject.

Also, I stopped the failing EWS on cr-linux by updating the baseline.

If you could look at it again Dirk, it would be appreciated.

[Dirk Schulze]

I'll take a look tomorrow. Feel free to ping me on IRC.

[Dirk Schulze]

Talked with anttik, and he seems to be unhappy about the changes in Element. He asked if we need to include elements in the resource handling at all. I am unsure how an alternative could look like atm.

[Dirk Schulze]

Comment on attachment 158601 [details]
Patch

I was looking into another problem with mask-image and wonder if we shouldn't think about using StyleResolver. This seems to be the proper root for resource handling and cleaner as well. Do you have time to investigate?

[Stephen White]

(In reply to comment #39)
> (From update of attachment 158601 [details])
> I was looking into another problem with mask-image and wonder if we shouldn't think about using StyleResolver. This seems to be the proper root for resource handling and cleaner as well. Do you have time to investigate?

Keyar's internship with us ended some time ago, but perhaps I can find someone to look into this.

[Dirk Schulze]

(In reply to comment #40)
> (In reply to comment #39)
> > (From update of attachment 158601 [details] [details])
> > I was looking into another problem with mask-image and wonder if we shouldn't think about using StyleResolver. This seems to be the proper root for resource handling and cleaner as well. Do you have time to investigate?
> 
> Keyar's internship with us ended some time ago, but perhaps I can find someone to look into this.

Oh, ok. Thanks for the information.

[Stephen White]

(In reply to comment #41)
> (In reply to comment #40)
> > (In reply to comment #39)
> > > (From update of attachment 158601 [details] [details] [details])
> > > I was looking into another problem with mask-image and wonder if we shouldn't think about using StyleResolver. This seems to be the proper root for resource handling and cleaner as well. Do you have time to investigate?
> > 
> > Keyar's internship with us ended some time ago, but perhaps I can find someone to look into this.
> 
> Oh, ok. Thanks for the information.

OK, Alexis Hetu has been looking into this.

I don't think StyleResolver is enough, since we need some way of observing the adding and removing of nodes from the DOM, so that SVG filter nodes added programmatically (for example) can be matched up with the styles which are looking for them.  Unless there is an existing mechanism we can hang this off of, we'll need to put in something pretty low-level.

I'm not sure what to do with node renames, though:  the spec doesn't seem to specify what should happen if a filter node which is referenced by a style is renamed to something else, or if another node is renamed into the thing we are looking for.   (My gut instinct is to check what Firefox's behaviour is here, but it should probably have a spec clarification too).

[Dirk Schulze]

(In reply to comment #42)
> (In reply to comment #41)
> > (In reply to comment #40)
> > > (In reply to comment #39)
> > > > (From update of attachment 158601 [details] [details] [details] [details])
> > > > I was looking into another problem with mask-image and wonder if we shouldn't think about using StyleResolver. This seems to be the proper root for resource handling and cleaner as well. Do you have time to investigate?
> > > 
> > > Keyar's internship with us ended some time ago, but perhaps I can find someone to look into this.
> > 
> > Oh, ok. Thanks for the information.
> 
> OK, Alexis Hetu has been looking into this.
> 
> I don't think StyleResolver is enough, since we need some way of observing the adding and removing of nodes from the DOM, so that SVG filter nodes added programmatically (for example) can be matched up with the styles which are looking for them.  Unless there is an existing mechanism we can hang this off of, we'll need to put in something pretty low-level.

Well, that is right. SVG elements can be dynamically updated, including its id.

> I'm not sure what to do with node renames, though:  the spec doesn't seem to specify what should happen if a filter node which is referenced by a style is renamed to something else, or if another node is renamed into the thing we are looking for.   (My gut instinct is to check what Firefox's behaviour is here, but it should probably have a spec clarification too).

The spec doesn't need to explicitly mention this. It is absolutely clear what happens and SVGReosurces is solving this in a very good way IMO. But everything was still in SVGStyledElements/SVGDocumentExtension. This moves it up to element directly. I wanted to make sure that this could or should be solved in the CSS code itself. But the dynamic nature might avoid that (at least doing it in a proper way). Maybe this patch is fine as is. But still would need an update.

Created attachment 176513 [details]
Patch

Hi all,

  I did a quick investigation for this bug and I implemented an observer pattern that seems to fix the issue. I think it's a bit simpler than the original solution. Whether you prefer the original solution or this new one is up to you. I am aware that the tests are missing, I am only uploading this patch to get comments from reviewers before I put any more effort into this. The solution as it is implemented here works for the purpose of solving the issue.

[Stephen White]

(In reply to comment #45)
> Hi all,
> 
>   I did a quick investigation for this bug and I implemented an observer pattern that seems to fix the issue. I think it's a bit simpler than the original solution. Whether you prefer the original solution or this new one is up to you. I am aware that the tests are missing, I am only uploading this patch to get comments from reviewers before I put any more effort into this. The solution as it is implemented here works for the purpose of solving the issue.

Dirk, Antti:  I would welcome your comments on this issue.  We have two proposed patches, and would like to converge on one that's acceptable.

Alexis's patch is very small and seems to handle the test case above, with minimal intrusive changes.  One thing that it does not address is the node renaming issue (Alexis, correct me if I'm wrong).

The original patch handles all these cases via the SVGResourcesCache, but it was deemed too intrusive.
Please let me know how you'd prefer we proceed.

[Dirk Schulze]

I think we should solve the issue together with bug 103811. There is no need to have two independent solutions. Niko invested in a lot of issues to create SVGResources: dynamic changes of resources (changed id, removed from document, insert into document, moves in document) as well as animations on resources like filters or animations on the filtered objects. You need to test all that before we can get to a conclusion.

[Stephen White]

(In reply to comment #47)
> I think we should solve the issue together with bug 103811. There is no need to have two independent solutions. Niko invested in a lot of issues to create SVGResources: dynamic changes of resources (changed id, removed from document, insert into document, moves in document) as well as animations on resources like filters or animations on the filtered objects. You need to test all that before we can get to a conclusion.

I'm fine with using SVGResourcesCache, it's just that the previous change was rejected due to the Element changes.  Alexis's patch minimizes those changes, in an attempt to satisfy the given constraints.  I'll update Keyar's patch so we have the option.

Antti, since it was your objection that we're trying to overcome here, how would you like us to proceed?

[Dirk Schulze]

(In reply to comment #48)
> (In reply to comment #47)
> > I think we should solve the issue together with bug 103811. There is no need to have two independent solutions. Niko invested in a lot of issues to create SVGResources: dynamic changes of resources (changed id, removed from document, insert into document, moves in document) as well as animations on resources like filters or animations on the filtered objects. You need to test all that before we can get to a conclusion.
> 
> I'm fine with using SVGResourcesCache, it's just that the previous change was rejected due to the Element changes.  Alexis's patch minimizes those changes, in an attempt to satisfy the given constraints.  I'll update Keyar's patch so we have the option.
> 
> Antti, since it was your objection that we're trying to overcome here, how would you like us to proceed?

The new proposal does not proof that it works with the named requirements. Adding new tests that do all that would be great. Take a look at svg/animations or svg/dynamic-updates for existing tests that can be adapted for -webkit-filter.

Until we don't have these tests for the new proposal, we don't have a choice yet.

[Stephen White]

Created attachment 177329 [details]
Patch

[Stephen White]

(In reply to comment #50)
> Created an attachment (id=177329) [details]
> Patch

This is Keyar's patch updated to ToT.

[Stephen White]

Created attachment 177359 [details]
Fix custom filters; de-virtualize Element accessors.

[Stephen White]

Created attachment 177607 [details]
Move clearHasPendingResourcesIfPossible() to SVGDocumentExtensions

[Stephen Chenney]

Comment on attachment 177607 [details]
Move clearHasPendingResourcesIfPossible() to SVGDocumentExtensions

View in context: https://bugs.webkit.org/attachment.cgi?id=177607&action=review

> Source/WebCore/rendering/FilterEffectRenderer.cpp:204
> +    Document* document = renderer ? renderer->document() : 0;

You only need this in one case. Move it down to FilterOperation::VALIDATED_CUSTOM

> Source/WebCore/rendering/FilterEffectRenderer.cpp:207
> +        ASSERT(renderer);

This and the following two lines are redundant. There's no way at all to get here without renderer existing.

> Source/WebCore/rendering/svg/SVGResourcesCache.cpp:64
> +    if (object->node()->isSVGElement()) {

Can a foreign object with a filter referencing the SVG in which is is located cause a loop? Seems very plausible.

[Stephen Chenney]

So far an anttik's concerns about using the resources trackign method, there are other cases in new specs that also have this problem with nodes refering to other nodes later in the tree. In particular, shadow DOM may be affected.

When this is all over I suspect that SVGResourcesCache will need to move into regular dom code, rather than SVG specific.

[Stephen White]

Comment on attachment 177607 [details]
Move clearHasPendingResourcesIfPossible() to SVGDocumentExtensions

View in context: https://bugs.webkit.org/attachment.cgi?id=177607&action=review

>> Source/WebCore/rendering/FilterEffectRenderer.cpp:204
>> +    Document* document = renderer ? renderer->document() : 0;
> 
> You only need this in one case. Move it down to FilterOperation::VALIDATED_CUSTOM

Done (and sanity check added).

>> Source/WebCore/rendering/FilterEffectRenderer.cpp:207
>> +        ASSERT(renderer);
> 
> This and the following two lines are redundant. There's no way at all to get here without renderer existing.

Yes there is.  See TextureMapperImageBuffer:152:

    renderer->build(0 /*renderer */, &filters);

>> Source/WebCore/rendering/svg/SVGResourcesCache.cpp:64
>> +    if (object->node()->isSVGElement()) {
> 
> Can a foreign object with a filter referencing the SVG in which is is located cause a loop? Seems very plausible.

Could you provide a test case?  Just using foreignObject doesn't seem to be enough, since there's no way to reference it from inside the filter, AFAICT:

Are you thinking with an feImage that contains an <img> containing the parent SVG?  That's the only way I could see to make a reference to an SVG object from inside the filter.

[Stephen White]

Created attachment 177757 [details]
Patch

[Stephen Chenney]

Comment on attachment 177607 [details]
Move clearHasPendingResourcesIfPossible() to SVGDocumentExtensions

View in context: https://bugs.webkit.org/attachment.cgi?id=177607&action=review

The feImage referencing its own root was a recently fixed bug. So we've got that case covered for code that runs through SVG's applyResource and postApplyResource paths. If there are other ways to use feImage we might have to fix those too. See http://trac.webkit.org/changeset/132856

I agree that I was wrong on filters causing cycles. The real issue is with markers, clips and masks that draw/use geometry via resources.

Still, it's worth adding a test for feImage applied to a dom element in a foreign object that is referencing the svg that contains the foreign object. I have no idea what will happen, or even whether foreign object will work inside of feImage.

>>> Source/WebCore/rendering/FilterEffectRenderer.cpp:207
>>> +        ASSERT(renderer);
>> 
>> This and the following two lines are redundant. There's no way at all to get here without renderer existing.
> 
> Yes there is.  See TextureMapperImageBuffer:152:
> 
>     renderer->build(0 /*renderer */, &filters);

I meant that you are checking it already above. You know from line 201 that you either have a renderer or operations, so if you don't have operations, you must have renderer.

[Stephen White]

(In reply to comment #58)
> (From update of attachment 177607 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177607&action=review
> 
> The feImage referencing its own root was a recently fixed bug. So we've got that case covered for code that runs through SVG's applyResource and postApplyResource paths. If there are other ways to use feImage we might have to fix those too. See http://trac.webkit.org/changeset/132856
> 
> I agree that I was wrong on filters causing cycles. The real issue is with markers, clips and masks that draw/use geometry via resources.
> 
> Still, it's worth adding a test for feImage applied to a dom element in a foreign object that is referencing the svg that contains the foreign object. I have no idea what will happen, or even whether foreign object will work inside of feImage.
> 
> >>> Source/WebCore/rendering/FilterEffectRenderer.cpp:207
> >>> +        ASSERT(renderer);
> >> 
> >> This and the following two lines are redundant. There's no way at all to get here without renderer existing.
> > 
> > Yes there is.  See TextureMapperImageBuffer:152:
> > 
> >     renderer->build(0 /*renderer */, &filters);
> 
> I meant that you are checking it already above. You know from line 201 that you either have a renderer or operations, so if you don't have operations, you must have renderer.

Ahh, good point.  Fixed.

[Stephen White]

Created attachment 177781 [details]
Patch

[Stephen Chenney]

I also looked into Dirk's request that you add tests for dynamic changes to the filter attribute or to the ID of the filter. I could not find any such tests at all in LayoutTests/svg. It is no surprise that these things do not work reliably.

While it's true that this code should have tests for the same dynamic changes, I believe that by using SVG pending resources the code will both share problems and benefit from improvements. So as we add tests for the SVG cases, we can also add tests for the CSS cases.

I do not think you need to hold up the patch.

Still, it is worth considering what will happen if a filter ID is changed or the webkit-filter property is changed. Do you believe the current patch will update the filter in those circumstances?

[Stephen White]

(In reply to comment #61)
> I also looked into Dirk's request that you add tests for dynamic changes to the filter attribute or to the ID of the filter. I could not find any such tests at all in LayoutTests/svg. It is no surprise that these things do not work reliably.
> 
> While it's true that this code should have tests for the same dynamic changes, I believe that by using SVG pending resources the code will both share problems and benefit from improvements. So as we add tests for the SVG cases, we can also add tests for the CSS cases.
> 
> I do not think you need to hold up the patch.
> 
> Still, it is worth considering what will happen if a filter ID is changed or the webkit-filter property is changed. Do you believe the current patch will update the filter in those circumstances?

Yes, it seems to.  I've added another test for that.

[Stephen White]

Created attachment 177837 [details]
Patch

[Stephen White]

Comment on attachment 177837 [details]
Patch

Cleaned up some useless #includes, and turned the FilterOperations parameter back into a const-ref.  I don't think there's a need for it to be a pointer anymore, especially since the custom filters now want us to call computeFilterOperations() before assignment.

[Early Warning System Bot]

Comment on attachment 177837 [details]
Patch

Attachment 177837 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15170238

[Early Warning System Bot]

Comment on attachment 177837 [details]
Patch

Attachment 177837 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15154585

[Stephen White]

Created attachment 177842 [details]
Patch

[Dirk Schulze]

Comment on attachment 177842 [details]
Patch

Two more note, before I review it tomorrow.

[Dirk Schulze]

Comment on attachment 177842 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177842&action=review

I love when that happens :( So here the note:

> Source/WebCore/rendering/svg/SVGResources.cpp:202
> +        if (style->hasFilter()) {
> +            AtomicString id(style->filterResource());
> +            return setFilter(getRenderSVGResourceById<RenderSVGResourceFilter>(document, id));
> +        }
> +
> +        return false;

Early return please.

> Source/WebCore/rendering/svg/SVGResourcesCache.cpp:68
> +    if (object->node()->isSVGElement()) {
> +        // Run cycle-detection _afterwards_, so self-references can be caught as well.
> +        SVGResourcesCycleSolver solver(object, resources);
> +        solver.resolveCycles();
> +    }

I am a bit worried about this one. We have cycle detection in SVG already and nothing crashed. Why is that necessary here now?

> Source/WebCore/svg/SVGDocumentExtensions.h:43
> +class Element;

Is class Element needed here?

[Stephen Chenney]

Comment on attachment 177842 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177842&action=review

>> Source/WebCore/rendering/svg/SVGResources.cpp:202
>> +        return false;
> 
> Early return please.

How? All I see is to move the Document fetch inside the block that uses it.

>> Source/WebCore/rendering/svg/SVGResourcesCache.cpp:68
>> +    }
> 
> I am a bit worried about this one. We have cycle detection in SVG already and nothing crashed. Why is that necessary here now?

The code for SVG content has not changed. He's just moved the pre-existing cycle detection inside the if statement. See the comments for a previous patch as to why the detection is not needed for webkit-filter on HTML nodes.

And I was unable to find any tests for cycles in filters, in part because I don't think you can construct cycles on filters. We do have tests for markers and patterns, but they are very far from exhaustive.

[Stephen White]

(In reply to comment #69)
> (From update of attachment 177842 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177842&action=review
> 
> I love when that happens :( So here the note:
> 
> > Source/WebCore/rendering/svg/SVGResources.cpp:202
> > +        if (style->hasFilter()) {
> > +            AtomicString id(style->filterResource());
> > +            return setFilter(getRenderSVGResourceById<RenderSVGResourceFilter>(document, id));
> > +        }
> > +
> > +        return false;
> 
> Early return please.

Done.

> > Source/WebCore/rendering/svg/SVGResourcesCache.cpp:68
> > +    if (object->node()->isSVGElement()) {
> > +        // Run cycle-detection _afterwards_, so self-references can be caught as well.
> > +        SVGResourcesCycleSolver solver(object, resources);
> > +        solver.resolveCycles();
> > +    }
> 
> I am a bit worried about this one. We have cycle detection in SVG already and nothing crashed. Why is that necessary here now?

Cycle detection isn't needed for filters, AFAIK (there isn't a way to construct one).  As for why Keyar disabled it outright for non-SVG Elements, I'm not sure.  My guess is that SVGResourcesCycleSolver makes assumptions that all objects are SVGElements, and crashed, although I'm not certain of that.
 
> > Source/WebCore/svg/SVGDocumentExtensions.h:43
> > +class Element;
> 
> Is class Element needed here?

Yes, it is; for the signature changes below.  The only #includes here are WTF ones, and everything else is forward-declared.

[Stephen White]

Created attachment 178041 [details]
Patch

[Dirk Schulze]

(In reply to comment #71)
> (In reply to comment #69)
> > (From update of attachment 177842 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=177842&action=review
> > 
> > I love when that happens :( So here the note:
> > 
> > > Source/WebCore/rendering/svg/SVGResources.cpp:202
> > > +        if (style->hasFilter()) {
> > > +            AtomicString id(style->filterResource());
> > > +            return setFilter(getRenderSVGResourceById<RenderSVGResourceFilter>(document, id));
> > > +        }
> > > +
> > > +        return false;
> > 
> > Early return please.
> 
> Done.
> 
> > > Source/WebCore/rendering/svg/SVGResourcesCache.cpp:68
> > > +    if (object->node()->isSVGElement()) {
> > > +        // Run cycle-detection _afterwards_, so self-references can be caught as well.
> > > +        SVGResourcesCycleSolver solver(object, resources);
> > > +        solver.resolveCycles();
> > > +    }
> > 
> > I am a bit worried about this one. We have cycle detection in SVG already and nothing crashed. Why is that necessary here now?
> 
> Cycle detection isn't needed for filters, AFAIK (there isn't a way to construct one).  As for why Keyar disabled it outright for non-SVG Elements, I'm not sure.  My guess is that SVGResourcesCycleSolver makes assumptions that all objects are SVGElements, and crashed, although I'm not certain of that.

Well, there might be problems in the future. This doesn't sound right. In the future we might have cross references independent of the type of the element.

And filters can reference other filters:

<filter id="f" xlink:href="url(#f)"/>

as example. Would be great if you can test that and investigate why this change was done and if we can undo it.

> 
> > > Source/WebCore/svg/SVGDocumentExtensions.h:43
> > > +class Element;
> > 
> > Is class Element needed here?
> 
> Yes, it is; for the signature changes below.  The only #includes here are WTF ones, and everything else is forward-declared.

[Stephen White]

Created attachment 178116 [details]
Patch

[Stephen White]

(In reply to comment #73)
> (In reply to comment #71)
> > (In reply to comment #69)
> > > (From update of attachment 177842 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=177842&action=review
> > > 
> > > I love when that happens :( So here the note:
> > > 
> > > > Source/WebCore/rendering/svg/SVGResources.cpp:202
> > > > +        if (style->hasFilter()) {
> > > > +            AtomicString id(style->filterResource());
> > > > +            return setFilter(getRenderSVGResourceById<RenderSVGResourceFilter>(document, id));
> > > > +        }
> > > > +
> > > > +        return false;
> > > 
> > > Early return please.
> > 
> > Done.
> > 
> > > > Source/WebCore/rendering/svg/SVGResourcesCache.cpp:68
> > > > +    if (object->node()->isSVGElement()) {
> > > > +        // Run cycle-detection _afterwards_, so self-references can be caught as well.
> > > > +        SVGResourcesCycleSolver solver(object, resources);
> > > > +        solver.resolveCycles();
> > > > +    }
> > > 
> > > I am a bit worried about this one. We have cycle detection in SVG already and nothing crashed. Why is that necessary here now?
> > 
> > Cycle detection isn't needed for filters, AFAIK (there isn't a way to construct one).  As for why Keyar disabled it outright for non-SVG Elements, I'm not sure.  My guess is that SVGResourcesCycleSolver makes assumptions that all objects are SVGElements, and crashed, although I'm not certain of that.
> 
> Well, there might be problems in the future. This doesn't sound right. In the future we might have cross references independent of the type of the element.
> 
> And filters can reference other filters:
> 
> <filter id="f" xlink:href="url(#f)"/>
> 
> as example. Would be great if you can test that and investigate why this change was done and if we can undo it.

OK, I ran all the tests (in Debug and Release) with cycle detection unconditionally enabled, and it seems to be ok.  So perhaps the problem has been fixed since Keyar ran into it.

Also removed some comments which are no longer applicable.

> 
> > 
> > > > Source/WebCore/svg/SVGDocumentExtensions.h:43
> > > > +class Element;
> > > 
> > > Is class Element needed here?
> > 
> > Yes, it is; for the signature changes below.  The only #includes here are WTF ones, and everything else is forward-declared.

[Dirk Schulze]

Comment on attachment 178116 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178116&action=review

Please add the self referencing test as DRT crash test. That is my last request for this patch. Everything else looks fine :)

> Source/WebCore/ChangeLog:4
> +        CSS url() filters with forward references don't work
> +        https://bugs.webkit.org/show_bug.cgi?id=90405

Can you mention the original author as well please? (based on ...)

[Stephen White]

Created attachment 178227 [details]
Patch

[Stephen White]

(In reply to comment #76)
> (From update of attachment 178116 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178116&action=review
> 
> Please add the self referencing test as DRT crash test. That is my last request for this patch. Everything else looks fine :)

Done.  I put it in svg/filters, since it's not CSS-specific, and made it a reftest.

> 
> > Source/WebCore/ChangeLog:4
> > +        CSS url() filters with forward references don't work
> > +        https://bugs.webkit.org/show_bug.cgi?id=90405
> 
> Can you mention the original author as well please? (based on ...)

Done.

[Dirk Schulze]

Comment on attachment 178227 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178227&action=review

r=me

> LayoutTests/svg/filters/filter-cycle.html:6
> +      <filter id="filter1" xlink:href="url(#filter1)">
> +        <feColorMatrix type="matrix" values="0 1 0 0 0  1 0 0 0 0  0 0 1 0 0  0 0 0 1 0"/>
> +      </filter>

<filter id="filter1" xlink:href="url(#filter1)"/> should be enough.

[Stephen White]

Created attachment 178243 [details]
Patch for landing

[Stephen White]

(In reply to comment #79)
> (From update of attachment 178227 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178227&action=review
> 
> r=me
> 
> > LayoutTests/svg/filters/filter-cycle.html:6
> > +      <filter id="filter1" xlink:href="url(#filter1)">
> > +        <feColorMatrix type="matrix" values="0 1 0 0 0  1 0 0 0 0  0 0 1 0 0  0 0 0 1 0"/>
> > +      </filter>
> 
> <filter id="filter1" xlink:href="url(#filter1)"/> should be enough.

I stuck an feOffset in there, otherwise the filter doesn't render anything, and I wanted to make sure it wasn't being optimized out which might hide the bug.

[Dirk Schulze]

(In reply to comment #81)
> (In reply to comment #79)
> > (From update of attachment 178227 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=178227&action=review
> > 
> > r=me
> > 
> > > LayoutTests/svg/filters/filter-cycle.html:6
> > > +      <filter id="filter1" xlink:href="url(#filter1)">
> > > +        <feColorMatrix type="matrix" values="0 1 0 0 0  1 0 0 0 0  0 0 1 0 0  0 0 0 1 0"/>
> > > +      </filter>
> > 
> > <filter id="filter1" xlink:href="url(#filter1)"/> should be enough.
> 
> I stuck an feOffset in there, otherwise the filter doesn't render anything, and I wanted to make sure it wasn't being optimized out which might hide the bug.

Hm. This might mean that referencing SVG filters does not work for us, even if it should work according to SVG 1.1. Interesting. Needs a test and maybe another bug report.

Thanks for investigating in this bug Stephen!

[WebKit Review Bot]

Comment on attachment 178243 [details]
Patch for landing

Clearing flags on attachment: 178243

Committed r136975: <http://trac.webkit.org/changeset/136975>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dirk Schulze]

Comment on attachment 178243 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=178243&action=review

> Source/WebCore/rendering/svg/SVGResources.cpp:202
> +    if (!node->isSVGElement()) {
> +        Document* document = object->document();
> +        ASSERT(document);
> +
> +        if (!style->hasFilter())
> +            return false;
> +
> +        AtomicString id(style->filterResource());
> +        return setFilter(getRenderSVGResourceById<RenderSVGResourceFilter>(document, id));
> +    }

Argh, I fully misunderstood these lines. style is SVGRenderStyle, and hasFilters() checks for the 'filter' property, not '-webkit-filter' property. These lines do actually nothing!

That means we don't create a SVGResources at all; SVGResourcesCache does not cache filters and therefore, updates on the filter itself are not recognized by the filtered object. (Will upload test.)

The patch in the current state just seem to solve the problem, but doesn't actually. That is why we need real dynamic update tests (and we have a lot for the 'filter' property). This is really, really bad. It might even be that we run into serious issues on moving, deleting, renaming ids of elements within the filter. The tests might actually hide this fact, since they specify the 'filter' property as well.

Stephen, can you investigate more please?

[Dirk Schulze]

Reopen the bug.

[Dirk Schulze]

Created attachment 178363 [details]
Filter with dynamic update (on click on div)

Filter with dynamic update. On clicking the div, the color of the div should change to something blueish, but doesn't.

[Dirk Schulze]

(In reply to comment #87)
> Created an attachment (id=178363) [details]
> Filter with dynamic update (on click on div)
> 
> Filter with dynamic update. On clicking the div, the color of the div should change to something blueish, but doesn't.

(In reply to comment #85)
> (From update of attachment 178243 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178243&action=review
> 
> > Source/WebCore/rendering/svg/SVGResources.cpp:202
> > +    if (!node->isSVGElement()) {
> > +        Document* document = object->document();
> > +        ASSERT(document);
> > +
> > +        if (!style->hasFilter())
> > +            return false;
> > +
> > +        AtomicString id(style->filterResource());
> > +        return setFilter(getRenderSVGResourceById<RenderSVGResourceFilter>(document, id));
> > +    }
> 
> Argh, I fully misunderstood these lines. style is SVGRenderStyle, and hasFilters() checks for the 'filter' property, not '-webkit-filter' property. These lines do actually nothing!
> 
> That means we don't create a SVGResources at all; SVGResourcesCache does not cache filters and therefore, updates on the filter itself are not recognized by the filtered object. (Will upload test.)
> 
> The patch in the current state just seem to solve the problem, but doesn't actually. That is why we need real dynamic update tests (and we have a lot for the 'filter' property). This is really, really bad. It might even be that we run into serious issues on moving, deleting, renaming ids of elements within the filter. The tests might actually hide this fact, since they specify the 'filter' property as well.
> 
> Stephen, can you investigate more please?


Just looked a bit. To get resource tracking working in a proper way, we need to update SVGResourcesCache on HTML renderer as well. In theory this could be done in RenderBox, since all necessary element renderer inherit from RenderBox. This could look like this:

void RenderBox::willBeDestroyed()
{
#if ENABLE(SVG)
    SVGResourcesCache::clientDestroyed(this);
#endif
...



void RenderBox::styleDidChange(StyleDifference diff, const RenderStyle* oldStyle)
...
#if ENABLE(SVG)
    SVGResourcesCache::clientStyleChanged(this, diff, style());
#endif
}

void RenderBox::addChild(RenderObject* child, RenderObject* beforeChild)
{
    RenderBoxModelObject::addChild(child, beforeChild);
#if ENABLE(SVG)
    SVGResourcesCache::clientWasAddedToTree(child, child->style());
#endif
}

void RenderBox::removeChild(RenderObject* child)
{
#if ENABLE(SVG)
    SVGResourcesCache::clientWillBeRemovedFromTree(child);
#endif
    RenderBoxModelObject::removeChild(child);
}

and so on. The problem are SVG documents. Some SVG renderer already call the SVGResourcesCache data earlier in the inheritance hierarchy and inherit directly or indirectly from RenderBox.

This would mean that the SVGResourcesCache functions get called twice for some renderers (like RenderSVGRoot or RenderSVGText). I am not sure if this causes any problems in SVGResourcesCache, or if it can be modified to still work in a proper way. But calling these functions twice doesn't sound good anyway.

We can't remove the calls from the SVG renderer easily as well, since they are mostly encapseled by other function calls and actions, see:

RenderSVGRoot::paintReplaced
RenderSVGRoot::willBeDestroyed

Another problem might be that SVG code always assumes that every renderer represents one node in the DOM tree. All graphical nodes always have exactly one renderer. This is not necessarily the case in HTML IIRC.

[Stephen White]

(In reply to comment #88)
> (In reply to comment #87)
> > Created an attachment (id=178363) [details] [details]
> > Filter with dynamic update (on click on div)
> > 
> > Filter with dynamic update. On clicking the div, the color of the div should change to something blueish, but doesn't.
> 
> (In reply to comment #85)
> > (From update of attachment 178243 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=178243&action=review
> > 
> > > Source/WebCore/rendering/svg/SVGResources.cpp:202
> > > +    if (!node->isSVGElement()) {
> > > +        Document* document = object->document();
> > > +        ASSERT(document);
> > > +
> > > +        if (!style->hasFilter())
> > > +            return false;
> > > +
> > > +        AtomicString id(style->filterResource());
> > > +        return setFilter(getRenderSVGResourceById<RenderSVGResourceFilter>(document, id));
> > > +    }
> > 
> > Argh, I fully misunderstood these lines. style is SVGRenderStyle, and hasFilters() checks for the 'filter' property, not '-webkit-filter' property. These lines do actually nothing!
> > 
> > That means we don't create a SVGResources at all; SVGResourcesCache does not cache filters and therefore, updates on the filter itself are not recognized by the filtered object. (Will upload test.)
> > 
> > The patch in the current state just seem to solve the problem, but doesn't actually. That is why we need real dynamic update tests (and we have a lot for the 'filter' property). This is really, really bad. It might even be that we run into serious issues on moving, deleting, renaming ids of elements within the filter. The tests might actually hide this fact, since they specify the 'filter' property as well.
> > 
> > Stephen, can you investigate more please?
> 
> 
> Just looked a bit. To get resource tracking working in a proper way, we need to update SVGResourcesCache on HTML renderer as well. In theory this could be done in RenderBox, since all necessary element renderer inherit from RenderBox. This could look like this:
> 
> void RenderBox::willBeDestroyed()
> {
> #if ENABLE(SVG)
>     SVGResourcesCache::clientDestroyed(this);
> #endif
> ...
> 
> 
> 
> void RenderBox::styleDidChange(StyleDifference diff, const RenderStyle* oldStyle)
> ...
> #if ENABLE(SVG)
>     SVGResourcesCache::clientStyleChanged(this, diff, style());
> #endif
> }
> 
> void RenderBox::addChild(RenderObject* child, RenderObject* beforeChild)
> {
>     RenderBoxModelObject::addChild(child, beforeChild);
> #if ENABLE(SVG)
>     SVGResourcesCache::clientWasAddedToTree(child, child->style());
> #endif
> }
> 
> void RenderBox::removeChild(RenderObject* child)
> {
> #if ENABLE(SVG)
>     SVGResourcesCache::clientWillBeRemovedFromTree(child);
> #endif
>     RenderBoxModelObject::removeChild(child);
> }
> 
> and so on. The problem are SVG documents. Some SVG renderer already call the SVGResourcesCache data earlier in the inheritance hierarchy and inherit directly or indirectly from RenderBox.
> 
> This would mean that the SVGResourcesCache functions get called twice for some renderers (like RenderSVGRoot or RenderSVGText). I am not sure if this causes any problems in SVGResourcesCache, or if it can be modified to still work in a proper way. But calling these functions twice doesn't sound good anyway.
> 
> We can't remove the calls from the SVG renderer easily as well, since they are mostly encapseled by other function calls and actions, see:
> 
> RenderSVGRoot::paintReplaced
> RenderSVGRoot::willBeDestroyed
> 
> Another problem might be that SVG code always assumes that every renderer represents one node in the DOM tree. All graphical nodes always have exactly one renderer. This is not necessarily the case in HTML IIRC.

I'm not sure I understand completely.  If you're talking about SVG attribute changes (e.g., changes via JS, SVG animations) causing CSS style invalidations (via filters), it should be handled in a different way by my earlier patch (r121513).  See m_clientLayers in RenderSVGResourceContainer.

I think the SVGResourcesCache is only necessary for nodes which transition in and out of relevance for a particular style (ie., renamed in, renamed out, added, deleted), although I could be wrong.

At any rate, I'll take a look at the cases you mentioned.  Thanks for your diligence.

[Stephen Chenney]

We really should just land this and deal with improving the test coverage in a separate patch. The Chromium SVG team suspects that there are numerous problems with dynamic updates of resources (filters, clips, masks). There are also known bugs with failed painting when a referenced clip path appears after the user in DOM order.

I think it is unreasonable to weigh this bug, and Stephen, down with the task of fixing all testing for these scenarios. It is not the case that this bug is introducing new problems. Better to have a clean bug where it's easier to track.

[Stephen White]

Reverted r136975 for reason:

Notification problems.

Committed r137463: <http://trac.webkit.org/changeset/137463>

[Stephen White]

Created attachment 179178 [details]
Patch

[Stephen White]

(In reply to comment #92)
> Created an attachment (id=179178) [details]
> Patch

Mostly this is just EWS-food, but feel free to take a look.  This version includes the changes Dirk suggested above, although applied to RenderObject instead of RenderBox.  This seems more symmetrical, since the SVGResourcesCache deals with RenderObject.

Another crucial missing piece was to remove the Element from the pending resources list on destruction, and when the node is removed from its container.  This code was relocated from SVGElement.  Without this, if a node is moved from one Document to another, the old document will still have references to the Element in its pending resource list, and will no longer be notified when it is destroyed.

[WebKit Review Bot]

Comment on attachment 179178 [details]
Patch

Attachment 179178 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15314079

New failing tests:
http/tests/appcache/different-origin-manifest.html
accessibility/aria-checkbox-checked.html
http/tests/appcache/main-resource-hash.html
http/tests/appcache/deferred-events-delete-while-raising.html
accessibility/aria-list-and-listitem.html
http/tests/appcache/cyrillic-uri.html
accessibility/aria-presentational-role.html
accessibility/aria-labelledby-on-input.html
canvas/philip/tests/2d.clearRect.globalalpha.html
accessibility/aria-disabled.html
accessibility/accessibility-node-reparent.html
animations/animation-border-overflow.html
animations/animation-direction-alternate-reverse.html
accessibility/aria-hidden.html
http/tests/appcache/destroyed-frame.html
accessibility/aria-roles.html
accessibility/aria-toggle-button-with-title.html
canvas/philip/tests/2d.clearRect.transform.html
accessibility/aria-hidden-updates-alldescendants.html
canvas/philip/tests/2d.clearRect.negative.html
canvas/philip/tests/2d.clearRect.path.html
canvas/philip/tests/2d.canvas.reference.html
canvas/philip/tests/2d.clearRect.basic.html
accessibility/aria-labelledby-stay-within.html
accessibility/aria-slider-value.html
http/tests/appcache/foreign-fallback.html
http/tests/appcache/disabled.html
accessibility/adjacent-continuations-cause-assertion-failure.html
accessibility/aria-help.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html

[Build Bot]

Comment on attachment 179178 [details]
Patch

Attachment 179178 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15314087

[WebKit Review Bot]

Comment on attachment 179178 [details]
Patch

Attachment 179178 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15313092

New failing tests:
http/tests/appcache/different-origin-manifest.html
accessibility/aria-checkbox-checked.html
http/tests/appcache/main-resource-hash.html
http/tests/appcache/deferred-events-delete-while-raising.html
accessibility/aria-list-and-listitem.html
http/tests/appcache/cyrillic-uri.html
accessibility/aria-presentational-role.html
accessibility/aria-labelledby-on-input.html
canvas/philip/tests/2d.clearRect.globalalpha.html
accessibility/aria-disabled.html
accessibility/accessibility-node-reparent.html
animations/animation-border-overflow.html
animations/animation-direction-alternate-reverse.html
accessibility/aria-hidden.html
http/tests/appcache/destroyed-frame.html
accessibility/aria-roles.html
accessibility/aria-toggle-button-with-title.html
canvas/philip/tests/2d.clearRect.transform.html
accessibility/aria-hidden-updates-alldescendants.html
canvas/philip/tests/2d.clearRect.negative.html
canvas/philip/tests/2d.clearRect.path.html
canvas/philip/tests/2d.canvas.reference.html
canvas/philip/tests/2d.clearRect.basic.html
accessibility/aria-labelledby-stay-within.html
accessibility/aria-slider-value.html
http/tests/appcache/foreign-fallback.html
http/tests/appcache/disabled.html
accessibility/adjacent-continuations-cause-assertion-failure.html
accessibility/aria-help.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html

[Stephen White]

Created attachment 179189 [details]
Fix typo in debug build.

[WebKit Review Bot]

Comment on attachment 179189 [details]
Fix typo in debug build.

Attachment 179189 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15319123

New failing tests:
http/tests/appcache/different-origin-manifest.html
accessibility/aria-checkbox-checked.html
http/tests/appcache/main-resource-hash.html
http/tests/appcache/deferred-events-delete-while-raising.html
accessibility/aria-list-and-listitem.html
http/tests/appcache/cyrillic-uri.html
accessibility/aria-presentational-role.html
accessibility/aria-labelledby-on-input.html
canvas/philip/tests/2d.clearRect.globalalpha.html
accessibility/aria-disabled.html
accessibility/accessibility-node-reparent.html
accessibility/button-title-uses-inner-img-alt.html
animations/animation-border-overflow.html
animations/animation-direction-alternate-reverse.html
accessibility/aria-hidden.html
http/tests/appcache/destroyed-frame.html
accessibility/aria-roles.html
accessibility/aria-toggle-button-with-title.html
accessibility/aria-slider-value.html
accessibility/aria-hidden-updates-alldescendants.html
canvas/philip/tests/2d.clearRect.negative.html
canvas/philip/tests/2d.clearRect.path.html
canvas/philip/tests/2d.canvas.reference.html
canvas/philip/tests/2d.clearRect.basic.html
accessibility/aria-labelledby-stay-within.html
http/tests/appcache/foreign-fallback.html
http/tests/appcache/disabled.html
accessibility/adjacent-continuations-cause-assertion-failure.html
accessibility/aria-help.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html

[Stephen Chenney]

Comment on attachment 179189 [details]
Fix typo in debug build.

View in context: https://bugs.webkit.org/attachment.cgi?id=179189&action=review

> Source/WebCore/svg/SVGElement.cpp:182
> +    if (wasInDocument)

Thought I commented, but can't see the comment on the bug ...

The order of operations has been changed here. Now the element references will be removed before the call to rebuild them. That means that things using the reference will not be notified of its removal. I don't see how notifications are otherwise performed, but I woudl expect more tests to fail. Maybe ews is not as comprehensive as I thought.

> LayoutTests/css3/filters/effect-reference-after-expected.txt:1
> + 

This should contain something, even if just "PASSED".

> LayoutTests/css3/filters/effect-reference-delete-expected.txt:1
> + 

This should contain something, even if just "PASSED".

> LayoutTests/css3/filters/effect-reference-rename-expected.txt:1
> + 

This should contain something, even if just "PASSED".

[Stephen White]

Comment on attachment 179189 [details]
Fix typo in debug build.

I think the RenderObject changes are not a good idea at this time.   The SVGResources class seems to cause asserts and crashes all over the place, because it's not ready for non-SVG renderers.  And I don't think they're necessary to fix the forward-references problem.  They may be necessary to fix other problems, but I think we can deal with those in a subsequent patch.

My suspicion is that Keyar added the RenderObject::willBeDestroyed() call to fix crashes, but it just papered over the deeper problem of the Element not removing itself from the appropriate pending resources lists, as SVGElement does.  I'll do some further investigation and tests to convince myself of that.

[Stephen White]

(In reply to comment #100)
> (From update of attachment 179189 [details])
> I think the RenderObject changes are not a good idea at this time.   The SVGResources class seems to cause asserts and crashes all over the place, because it's not ready for non-SVG renderers.  And I don't think they're necessary to fix the forward-references problem.  They may be necessary to fix other problems, but I think we can deal with those in a subsequent patch.
> 
> My suspicion is that Keyar added the RenderObject::willBeDestroyed() call to fix crashes, but it just papered over the deeper problem of the Element not removing itself from the appropriate pending resources lists, as SVGElement does.  I'll do some further investigation and tests to convince myself of that.

OK, I think I've managed to convince myself that the RenderObject changes aren't necessary.  For -webkit-filter, an HTML element can only depend on an SVG element (a filter), not another HTML element.  So the existing notification infrastructure works, and there's no need to move it up to RenderObject.

For example, node renames for filters go through RenderSVGResourceContainer::idChanged(), which calls RenderSVGResourceContainer::registerResource(), which calls SVGResourcesCache::clientStyleChanged(), which (for non-SVG elements) now sends a synthetic style change.  Similarly for newly-added nodes.

[Stephen White]

Created attachment 179333 [details]
Patch

[Stephen White]

Comment on attachment 179189 [details]
Fix typo in debug build.

View in context: https://bugs.webkit.org/attachment.cgi?id=179189&action=review

Thanks for your review.

>> Source/WebCore/svg/SVGElement.cpp:182
>> +    if (wasInDocument)
> 
> Thought I commented, but can't see the comment on the bug ...
> 
> The order of operations has been changed here. Now the element references will be removed before the call to rebuild them. That means that things using the reference will not be notified of its removal. I don't see how notifications are otherwise performed, but I woudl expect more tests to fail. Maybe ews is not as comprehensive as I thought.

I think pending resources (Element -> string, later resolved to SVGElement) and element references (SVGElement -> SVGElement) are independent.

That said, it looks like I messed up a merge (should be both a rebuild and remove for element references).  Will fix.

>> LayoutTests/css3/filters/effect-reference-after-expected.txt:1
>> + 
> 
> This should contain something, even if just "PASSED".

Since I need the pixel results, and I'd rather not put text in them (in order to share baselines across platforms), I've changed the tests to output render trees, since that's what most of the other effect-foo.html tests use.

[Stephen White]

Created attachment 179338 [details]
Patch

[Build Bot]

Comment on attachment 179338 [details]
Patch

Attachment 179338 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15311430

New failing tests:
css3/filters/effect-reference-delete.html
css3/filters/effect-reference-after.html
css3/filters/effect-reference-rename.html

[Stephen White]

Created attachment 179484 [details]
Patch

[Stephen White]

(In reply to comment #106)
> Created an attachment (id=179484) [details]
> Patch

Changed all tests to reftests or text-only tests, so no rebaselines are necessary.

[Dirk Schulze]

I'll take a look tomorrow morning. Please ping me if I forget to look at it Stephen.

[Dirk Schulze]

Comment on attachment 179484 [details]
Patch

Ok, looked at the patch and have a couple of questions. I think the patch makes it possible to detect if an element was removed or added, but has some other open issues.

[Dirk Schulze]

(In reply to comment #109)
> (From update of attachment 179484 [details])
> Ok, looked at the patch and have a couple of questions. I think the patch makes it possible to detect if an element was removed or added, but has some other open issues.

Ahhhh. My comments were not uploaded :(

[Tony Chang]

(In reply to comment #110)
> (In reply to comment #109)
> > (From update of attachment 179484 [details] [details])
> > Ok, looked at the patch and have a couple of questions. I think the patch makes it possible to detect if an element was removed or added, but has some other open issues.
> 
> Ahhhh. My comments were not uploaded :(

https://bugs.webkit.org/show_bug.cgi?id=105252

We're looking for a repro.

[Stephen White]

(In reply to comment #110)
> (In reply to comment #109)
> > (From update of attachment 179484 [details] [details])
> > Ok, looked at the patch and have a couple of questions. I think the patch makes it possible to detect if an element was removed or added, but has some other open issues.
> 
> Ahhhh. My comments were not uploaded :(

Sorry to hear you're having trouble, Dirk.  Do you think you could take another stab at it?

(BTW, you could try ^C'ing your comments before posting.  I often do that just because of the bugzilla mid-air-collision thing; might help in this situation too).

[Dirk Schulze]

(In reply to comment #112)
> (In reply to comment #110)
> > (In reply to comment #109)
> > > (From update of attachment 179484 [details] [details] [details])
> > > Ok, looked at the patch and have a couple of questions. I think the patch makes it possible to detect if an element was removed or added, but has some other open issues.
> > 
> > Ahhhh. My comments were not uploaded :(
> 
> Sorry to hear you're having trouble, Dirk.  Do you think you could take another stab at it?

Yes of course!

> 
> (BTW, you could try ^C'ing your comments before posting.  I often do that just because of the bugzilla mid-air-collision thing; might help in this situation too).

Yes, I just forget it to do sometimes.

[Dirk Schulze]

Comment on attachment 179484 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179484&action=review

So, the concept of the patch looks better.

> Source/WebCore/rendering/svg/SVGResources.cpp:196
> +    // For now, non-SVG elements cannot have SVG resources. Filters only
> +    // need to marked for invalidation; they will rebuild themselves
> +    // appropriately. Other styles may need further treatment here.

I didn't correctly read these lines on my first review (so good that I needed to review it twice :) ). This is actually the main sense of SVGResources and SVGResourcesCache, to notify the client when something changes. The dynamic removal and adding from resources is important, but repaint on resource changes is important as well. This patch is not doing it.

> Source/WebCore/rendering/svg/SVGResourcesCache.cpp:131
> +    return renderer->node() && renderer->node()->isSVGElement() && !renderer->isSVGInlineText();

Not sure if this needs to be in an extra inline function. Looks like it is only used once.

> Source/WebCore/rendering/svg/SVGResourcesCache.cpp:147
> +    if (rendererCanHaveResources(renderer)) {

When I saw this the first time, I wondered why we couldn't just do it for SVG elements as well. There is basically no difference between HTML and SVG elements.

> Source/WebCore/rendering/svg/SVGResourcesCache.cpp:156
> +        renderer->node()->setNeedsStyleRecalc(SyntheticStyleChange);

Especially after reading this line. But Ok, you just don't handle the case of style updates on the resource (actually not only style updates). Do you think you can at least outline a concept how it should be done in the near future? If we don't fix it, this patch and the previous one don't make a big difference. We are moving everything to Element and Document nonRareData but don't use the main concept.

Do you think that we can fix the problems in comment 88? Especially the problems with RenderBox inheriting elements, the one to one mapping between elements and renderer that we have in SVG but might not have in HTML (not so sure ATM)?

[Stephen White]

Comment on attachment 179484 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179484&action=review

Thanks for your review.

>> Source/WebCore/rendering/svg/SVGResources.cpp:196
>> +    // appropriately. Other styles may need further treatment here.
> 
> I didn't correctly read these lines on my first review (so good that I needed to review it twice :) ). This is actually the main sense of SVGResources and SVGResourcesCache, to notify the client when something changes. The dynamic removal and adding from resources is important, but repaint on resource changes is important as well. This patch is not doing it.

As I said on an earlier patch "If you're talking about SVG attribute changes (e.g., changes via JS, SVG animations) causing CSS style invalidations (via filters), it should be handled in a different way by my earlier patch (r121513).  See m_clientLayers in RenderSVGResourceContainer."

There may be things that notification is not handling, or a way to unify it with SVGResources, but I think that's orthogonal to this patch.

>> Source/WebCore/rendering/svg/SVGResourcesCache.cpp:131
>> +    return renderer->node() && renderer->node()->isSVGElement() && !renderer->isSVGInlineText();
> 
> Not sure if this needs to be in an extra inline function. Looks like it is only used once.

This function existed before; I just added the isSVGElement() clause and moved it up so clientStyleChanged() could call it.

>> Source/WebCore/rendering/svg/SVGResourcesCache.cpp:147
>> +    if (rendererCanHaveResources(renderer)) {
> 
> When I saw this the first time, I wondered why we couldn't just do it for SVG elements as well. There is basically no difference between HTML and SVG elements.

We are doing it for SVG elements; we're skipping it for non-SVG elements.  See my earlier patch http://queues.webkit.org/patch/179189, which triggered a whole mess of asserts and crashing tests when I tried to do so.  I don't think this part of the code is ready for non-SVG elements yet.

>> Source/WebCore/rendering/svg/SVGResourcesCache.cpp:156
>> +        renderer->node()->setNeedsStyleRecalc(SyntheticStyleChange);
> 
> Especially after reading this line. But Ok, you just don't handle the case of style updates on the resource (actually not only style updates). Do you think you can at least outline a concept how it should be done in the near future? If we don't fix it, this patch and the previous one don't make a big difference. We are moving everything to Element and Document nonRareData but don't use the main concept.
> 
> Do you think that we can fix the problems in comment 88? Especially the problems with RenderBox inheriting elements, the one to one mapping between elements and renderer that we have in SVG but might not have in HTML (not so sure ATM)?

This line fixes the case of adding, renaming, and removing SVG filters referenced by HTML elements, so the useful part to me is the pending resources hash map in SVGDocumentExtensions, and its modification to handle any Element.  When that occurs, the synthetic style change above causes the node to re-check the reference and rebuild the filter chain on the layer.

Notifications for animation and JS should be handled by m_clientLayers as I mentioned above, but if that's not enough or could be done better, we can look at that as a separate issue.  I'm pretty sure we *won't* need to move all the willBeDestroyed(), addChild(), removeChild() code to RenderBox, as suggested in comment #88, since filters at least only ever depend on SVG elements (HTML -> SVG), never on other HTML elements (HTML -> HTML), so they only need to be notified about changes to SVG nodes.  See my comment #101.

[Stephen White]

Comment on attachment 179484 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179484&action=review

>>> Source/WebCore/rendering/svg/SVGResources.cpp:196
>>> +    // appropriately. Other styles may need further treatment here.
>> 
>> I didn't correctly read these lines on my first review (so good that I needed to review it twice :) ). This is actually the main sense of SVGResources and SVGResourcesCache, to notify the client when something changes. The dynamic removal and adding from resources is important, but repaint on resource changes is important as well. This patch is not doing it.
> 
> As I said on an earlier patch "If you're talking about SVG attribute changes (e.g., changes via JS, SVG animations) causing CSS style invalidations (via filters), it should be handled in a different way by my earlier patch (r121513).  See m_clientLayers in RenderSVGResourceContainer."
> 
> There may be things that notification is not handling, or a way to unify it with SVGResources, but I think that's orthogonal to this patch.

Now that I think about it, the early-out here is no longer necessary, since we don't call buildCachedResources() on non-SVG elements anymore.  (This was a change I added when I was trying to stem the tide of asserts.)  And the comment is technically incorrect:  non-SVG elements *can* have SVG resources, they're just added by code elsewhere (e.g., by FilterEffectRenderer::buildReferenceFilter).  I'm going to revert this part of the change and re-upload.

[Stephen White]

Created attachment 180380 [details]
Patch

[Dirk Schulze]

Comment on attachment 179484 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179484&action=review

>>>> Source/WebCore/rendering/svg/SVGResources.cpp:196
>>>> +    // appropriately. Other styles may need further treatment here.
>>> 
>>> I didn't correctly read these lines on my first review (so good that I needed to review it twice :) ). This is actually the main sense of SVGResources and SVGResourcesCache, to notify the client when something changes. The dynamic removal and adding from resources is important, but repaint on resource changes is important as well. This patch is not doing it.
>> 
>> As I said on an earlier patch "If you're talking about SVG attribute changes (e.g., changes via JS, SVG animations) causing CSS style invalidations (via filters), it should be handled in a different way by my earlier patch (r121513).  See m_clientLayers in RenderSVGResourceContainer."
>> 
>> There may be things that notification is not handling, or a way to unify it with SVGResources, but I think that's orthogonal to this patch.
> 
> Now that I think about it, the early-out here is no longer necessary, since we don't call buildCachedResources() on non-SVG elements anymore.  (This was a change I added when I was trying to stem the tide of asserts.)  And the comment is technically incorrect:  non-SVG elements *can* have SVG resources, they're just added by code elsewhere (e.g., by FilterEffectRenderer::buildReferenceFilter).  I'm going to revert this part of the change and re-upload.

What do you mean with r121513. Can you add a link? If you mean RenderSVGResourceContainer::markAllClientsForInvalidation then you are right, but this is controlled by SVGResources (the reason why we have it), which you don't use in your patch at all.

[Stephen White]

(In reply to comment #118)
> (From update of attachment 179484 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179484&action=review
> 
> >>>> Source/WebCore/rendering/svg/SVGResources.cpp:196
> >>>> +    // appropriately. Other styles may need further treatment here.
> >>> 
> >>> I didn't correctly read these lines on my first review (so good that I needed to review it twice :) ). This is actually the main sense of SVGResources and SVGResourcesCache, to notify the client when something changes. The dynamic removal and adding from resources is important, but repaint on resource changes is important as well. This patch is not doing it.
> >> 
> >> As I said on an earlier patch "If you're talking about SVG attribute changes (e.g., changes via JS, SVG animations) causing CSS style invalidations (via filters), it should be handled in a different way by my earlier patch (r121513).  See m_clientLayers in RenderSVGResourceContainer."
> >> 
> >> There may be things that notification is not handling, or a way to unify it with SVGResources, but I think that's orthogonal to this patch.
> > 
> > Now that I think about it, the early-out here is no longer necessary, since we don't call buildCachedResources() on non-SVG elements anymore.  (This was a change I added when I was trying to stem the tide of asserts.)  And the comment is technically incorrect:  non-SVG elements *can* have SVG resources, they're just added by code elsewhere (e.g., by FilterEffectRenderer::buildReferenceFilter).  I'm going to revert this part of the change and re-upload.
> 
> What do you mean with r121513. Can you add a link? If you mean RenderSVGResourceContainer::markAllClientsForInvalidation then you are right, but this is controlled by SVGResources (the reason why we have it), which you don't use in your patch at all.

http://trac.webkit.org/changeset/121513

When an SVG filter node is invalidated, it calls filterNeedsRepaint() on its client layers (on the RenderLayer side).

[Dirk Schulze]

(In reply to comment #119)
> (In reply to comment #118)
> > (From update of attachment 179484 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=179484&action=review
> > 
> > >>>> Source/WebCore/rendering/svg/SVGResources.cpp:196
> > >>>> +    // appropriately. Other styles may need further treatment here.
> > >>> 
> > >>> I didn't correctly read these lines on my first review (so good that I needed to review it twice :) ). This is actually the main sense of SVGResources and SVGResourcesCache, to notify the client when something changes. The dynamic removal and adding from resources is important, but repaint on resource changes is important as well. This patch is not doing it.
> > >> 
> > >> As I said on an earlier patch "If you're talking about SVG attribute changes (e.g., changes via JS, SVG animations) causing CSS style invalidations (via filters), it should be handled in a different way by my earlier patch (r121513).  See m_clientLayers in RenderSVGResourceContainer."
> > >> 
> > >> There may be things that notification is not handling, or a way to unify it with SVGResources, but I think that's orthogonal to this patch.
> > > 
> > > Now that I think about it, the early-out here is no longer necessary, since we don't call buildCachedResources() on non-SVG elements anymore.  (This was a change I added when I was trying to stem the tide of asserts.)  And the comment is technically incorrect:  non-SVG elements *can* have SVG resources, they're just added by code elsewhere (e.g., by FilterEffectRenderer::buildReferenceFilter).  I'm going to revert this part of the change and re-upload.
> > 
> > What do you mean with r121513. Can you add a link? If you mean RenderSVGResourceContainer::markAllClientsForInvalidation then you are right, but this is controlled by SVGResources (the reason why we have it), which you don't use in your patch at all.
> 
> http://trac.webkit.org/changeset/121513
> 
> When an SVG filter node is invalidated, it calls filterNeedsRepaint() on its client layers (on the RenderLayer side).

See example https://bugs.webkit.org/attachment.cgi?id=178363 in this bug, it doesn't work.

[Stephen White]

(In reply to comment #120)
> (In reply to comment #119)
> > (In reply to comment #118)
> > > (From update of attachment 179484 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=179484&action=review
> > > 
> > > >>>> Source/WebCore/rendering/svg/SVGResources.cpp:196
> > > >>>> +    // appropriately. Other styles may need further treatment here.
> > > >>> 
> > > >>> I didn't correctly read these lines on my first review (so good that I needed to review it twice :) ). This is actually the main sense of SVGResources and SVGResourcesCache, to notify the client when something changes. The dynamic removal and adding from resources is important, but repaint on resource changes is important as well. This patch is not doing it.
> > > >> 
> > > >> As I said on an earlier patch "If you're talking about SVG attribute changes (e.g., changes via JS, SVG animations) causing CSS style invalidations (via filters), it should be handled in a different way by my earlier patch (r121513).  See m_clientLayers in RenderSVGResourceContainer."
> > > >> 
> > > >> There may be things that notification is not handling, or a way to unify it with SVGResources, but I think that's orthogonal to this patch.
> > > > 
> > > > Now that I think about it, the early-out here is no longer necessary, since we don't call buildCachedResources() on non-SVG elements anymore.  (This was a change I added when I was trying to stem the tide of asserts.)  And the comment is technically incorrect:  non-SVG elements *can* have SVG resources, they're just added by code elsewhere (e.g., by FilterEffectRenderer::buildReferenceFilter).  I'm going to revert this part of the change and re-upload.
> > > 
> > > What do you mean with r121513. Can you add a link? If you mean RenderSVGResourceContainer::markAllClientsForInvalidation then you are right, but this is controlled by SVGResources (the reason why we have it), which you don't use in your patch at all.
> > 
> > http://trac.webkit.org/changeset/121513
> > 
> > When an SVG filter node is invalidated, it calls filterNeedsRepaint() on its client layers (on the RenderLayer side).
> 
> See example https://bugs.webkit.org/attachment.cgi?id=178363 in this bug, it doesn't work.

I've opened (In reply to comment #120)
> (In reply to comment #119)
> > (In reply to comment #118)
> > > (From update of attachment 179484 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=179484&action=review
> > > 
> > > >>>> Source/WebCore/rendering/svg/SVGResources.cpp:196
> > > >>>> +    // appropriately. Other styles may need further treatment here.
> > > >>> 
> > > >>> I didn't correctly read these lines on my first review (so good that I needed to review it twice :) ). This is actually the main sense of SVGResources and SVGResourcesCache, to notify the client when something changes. The dynamic removal and adding from resources is important, but repaint on resource changes is important as well. This patch is not doing it.
> > > >> 
> > > >> As I said on an earlier patch "If you're talking about SVG attribute changes (e.g., changes via JS, SVG animations) causing CSS style invalidations (via filters), it should be handled in a different way by my earlier patch (r121513).  See m_clientLayers in RenderSVGResourceContainer."
> > > >> 
> > > >> There may be things that notification is not handling, or a way to unify it with SVGResources, but I think that's orthogonal to this patch.
> > > > 
> > > > Now that I think about it, the early-out here is no longer necessary, since we don't call buildCachedResources() on non-SVG elements anymore.  (This was a change I added when I was trying to stem the tide of asserts.)  And the comment is technically incorrect:  non-SVG elements *can* have SVG resources, they're just added by code elsewhere (e.g., by FilterEffectRenderer::buildReferenceFilter).  I'm going to revert this part of the change and re-upload.
> > > 
> > > What do you mean with r121513. Can you add a link? If you mean RenderSVGResourceContainer::markAllClientsForInvalidation then you are right, but this is controlled by SVGResources (the reason why we have it), which you don't use in your patch at all.
> > 
> > http://trac.webkit.org/changeset/121513
> > 
> > When an SVG filter node is invalidated, it calls filterNeedsRepaint() on its client layers (on the RenderLayer side).
> 
> See example https://bugs.webkit.org/attachment.cgi?id=178363 in this bug, it doesn't work.

Logged as https://bugs.webkit.org/show_bug.cgi?id=105635, along with some diagnosis.  That problem is orthogonal to this patch, so I'd prefer to fix it separately.

[Dirk Schulze]

Comment on attachment 180380 [details]
Patch

LGTM. Looking forward to review the upcoming style and dynamic update patches :) r=me.

[WebKit Review Bot]

Comment on attachment 180380 [details]
Patch

Clearing flags on attachment: 180380

Committed r138823: <http://trac.webkit.org/changeset/138823>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Philip Rogers]

(In reply to comment #124)
> All reviewed patches have been landed.  Closing bug.

This patch resulted in a 4.7% performance improvement on PerformanceTests/SVG/Debian.svg! Nice work :)

http://webkit-perf.appspot.com/graph.html#tests=[[10711011,2001,173262],[10756660,2001,173262]]&sel=1357320745898.4849,1357343599682.4673,15.625,121.875&displayrange=30&datatype=running