Bug 230296 – setNeedsLayout() should not be called when changing the SVG properties

• Component: SVG
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

We should call SVGElement::setNeedsParentAndResourceInvalidation() which sets a flag to be checked by RenderTreeUpdater::updateRenderTree(). If this flag is true setNeedsLayout() will be called.

[Said Abou-Hallawa]

Created attachment 438211 [details]
WIP

[Said Abou-Hallawa]

Created attachment 438212 [details]
WIP

[Radar WebKit Bug Importer]

<rdar://problem/83383150>

[Rob Buis]

Created attachment 453188 [details]
Patch

[Rob Buis]

Created attachment 453194 [details]
Patch

[Rob Buis]

Created attachment 453199 [details]
Patch

[Rob Buis]

Created attachment 453292 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 453292 [details]
Patch

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

> Source/WebCore/svg/SVGSVGElement.cpp:214
>              RenderSVGResource::markForLayoutAndParentResourceInvalidation(*renderer);

Why do we still call RenderSVGResource::markForLayoutAndParentResourceInvalidation() here?

[Rob Buis]

Created attachment 454103 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 454103 [details]
Patch

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

Unofficial review:

First of all: this is what we need! :-) I'm glad both Said and you worked on that topic, since I haven't touched any non-rendering related stuff in LBSE -- however what we have right now is hacky, racy and unpredictable -- the way "parent resources" are invalidated, etc. The terminology is hard to understand, what is a 'resource' etc - quite confusing overall. In LBSE the dependency on the code in the RenderSVGResource* classes is greatly reduced (RenderSVGResourceClipper is dumb -- applyresource/postApplyResources are all no-ops, for Clipper/Masker/Filter, and they are stateless -- no more caching and maps from renderer pointers to temporary state data or even rendered image buffers). However I haven't started to improve anything related to the core concepts as how resources invalidate themselves, or how elements invalidate resources in their ancestor chain, I can only say: that is a grey area, when to use markForParentResourceInvalidation() / moreConfusingMethods()...

This patch is a huge leap in the right direction: avoiding setNeedsLayout() in random places -- I do know we're able to enter layout cycles, that we have to break (various security issues in the past...).

Obviously this is WIP, and too large for a review. And there are things you can easily break out in individual mechanical preparation patches, after everyone agreed on the design.

> Source/WebCore/ChangeLog:8
> +        Make SVG not call setNeedsLayout() from outside RenderTreeUpdater::updateRenderTree() when an attribute changes.

The importance of this patch should be reflected in the future ChangeLog :-) aka. please expand what kind of issues/races... we avoid by this approach.

> Source/WebCore/dom/ElementData.h:134
> +    static const uint32_t s_flagParentAndResourceInvalidationNeeded = 1 << 5;

That needs a better name, totally unclear if that is SVG related or not, and what Parent and Resource means in that context.
(That's a comment to myself probably, I guess I'm guilty for that awful name).

> Source/WebCore/dom/ElementData.h:165
> +    void setParentAndResourceInvalidationNeeded(bool dirty) const { updateFlag(s_flagParentAndResourceInvalidationNeeded, dirty); }

First try:
void setSVGResourcesInAncestorChainAreDirty(bool dirty)?

> Source/WebCore/rendering/svg/RenderSVGImage.cpp:254
> +    imageElement().setNeedsParentAndResourceInvalidation();

I am not sure about that change -- in the LBSE variant of RenderSVGImage, the imageChanged() method is basically a copy of the RenderImage::imageChanged() method and that does include a 'repaintOrMarkForLayout(rect)' call.
If you trace that further RenderImage::repaintOrMarkForLayout() reacts on intrinsic size changes with a setNeedsLayout() call, see setNeedsLayoutIfNeededAfterIntrinsicSizeChange(). Therefore the removal of the reaction on 'updateImageViewport()' seems fishy.

> Source/WebCore/svg/SVGAnimateMotionElement.cpp:253
> +    auto invalidate = [](SVGElement& element) {

invalidate what? :-) I'd go for invalidateTargetElement().

However: this is also legacy terminology, what is invalidation? :-) setNeeds.... setFooIsDirty... is more descriptive.
This should be adapted to whatever naming scheme is chosen instead of 'setNeedsParentAndResourceInvalidation'.

> Source/WebCore/svg/SVGCircleElement.cpp:71
> +        ASSERT(PropertyRegistry::isAnimatedLengthAttribute(attrName));

That is guaranteed by the generated code, and only to instruct the developer?

> Source/WebCore/svg/SVGComponentTransferFunctionElement.cpp:95
> +        invalidateFilterPrimitiveParent(*this);

I would consistently get rid of the 'invalidate' terminology here as you did in other places.

> Source/WebCore/svg/SVGElement.cpp:613
> +    setAnimatedSVGAttributesAreDirty();

That reads so much better than invalidateSVGAttributes().

> Source/WebCore/svg/SVGElement.cpp:886
> +void SVGElement::setPresentationalHintStyleIsDirty()

You de-inlined this -- I guess this is hot enough to justify inlining.

> Source/WebCore/svg/SVGElement.cpp:889
> +    // Trigger style recalculation for "elements as resource" (e.g. referenced by feImage).

Huh? setPresentationalHintStyleIsDirty() is also used to propagate e.g. cxAttr changes of SVGEllipseElement, since that's mapped to the CSS cx property too.

> Source/WebCore/svg/SVGElement.cpp:893
> +void SVGElement::setNeedsParentAndResourceInvalidation()

Won't repeat the comment, let's find a better replacment for 'NeedsParentAndResourceInvalidation'.

Also if you re-inline setPresantationalHintStyleIsDirty(), this one should be inlined too.

> Source/WebCore/svg/SVGFilterPrimitiveStandardAttributes.h:83
> +void invalidateFilterPrimitiveParent(SVGElement&);

Might be a good idea to encapsulate this somewhere else than in the namespace.

> Source/WebCore/svg/SVGGeometryElement.cpp:150
>      if (attrName == SVGNames::pathLengthAttr) {

The PropertyRegistry::isKnownAttribute() check is omitted on purpose?

> Source/WebCore/svg/SVGImageElement.cpp:101
> +            if (!downcast<RenderSVGImage>(*renderer).updateImageViewport())

Aha, ic -- regarding my previous RenderSVGImage::imageChanged() comment. You're now triggering the setNeedsParentAndResourceInvalidation() here for SVG attribute changes -- I wonder if we still handle "intrincic size changes" correctly -- not sure if we have test coverage for that.

> Source/WebCore/svg/SVGSVGElement.cpp:223
> +                    renderer->view().setNeedsLayout(MarkOnlyThis);

Hmmm, I am not fully sure about these changes, if that covers all cases or not. Need to think about it.
Is width/height CSS property changes the only case when we have to mark the RenderView explicitely? How does it work for other cases, e.g. <iframe>?e

[Rob Buis]

Comment on attachment 454103 [details]
Patch

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

>> Source/WebCore/svg/SVGSVGElement.cpp:223
>> +                    renderer->view().setNeedsLayout(MarkOnlyThis);
> 
> Hmmm, I am not fully sure about these changes, if that covers all cases or not. Need to think about it.
> Is width/height CSS property changes the only case when we have to mark the RenderView explicitely? How does it work for other cases, e.g. <iframe>?e

After experimenting a bit, I think there is a difference since iframe seems to dictate its dimension whereas object can take the dimension of the (image) contents. Of course object can't reference html, so I am not sure there is a html equivalent to the object + embedded svg scenario.

[Rob Buis]

Comment on attachment 453292 [details]
Patch

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

>> Source/WebCore/svg/SVGSVGElement.cpp:214
>>              RenderSVGResource::markForLayoutAndParentResourceInvalidation(*renderer);
> 
> Why do we still call RenderSVGResource::markForLayoutAndParentResourceInvalidation() here?

Well spotted :) After more debugging, this is only needed for the case where we set width/height and the svg is embedded through object (and some other containers). In this case it is important to mark the container chain dirty (in reality only the container RenderView) so document.defaultView.getComputedStyle(object).width will detect this and restyle/relayout to get up to date contents and image dimension.

AFAIK this is a special case and nothing like this is possible in HTML (I could be wrong). My current idea is to special case it and add a FIXME to see if anything nicer can be implemented.

[Rob Buis]

Comment on attachment 454103 [details]
Patch

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

>> Source/WebCore/svg/SVGElement.cpp:886
>> +void SVGElement::setPresentationalHintStyleIsDirty()
> 
> You de-inlined this -- I guess this is hot enough to justify inlining.

I am taking this to https://bugs.webkit.org/show_bug.cgi?id=237716.

>> Source/WebCore/svg/SVGElement.cpp:889
>> +    // Trigger style recalculation for "elements as resource" (e.g. referenced by feImage).
> 
> Huh? setPresentationalHintStyleIsDirty() is also used to propagate e.g. cxAttr changes of SVGEllipseElement, since that's mapped to the CSS cx property too.

Good point! Will address in https://bugs.webkit.org/show_bug.cgi?id=237716.

[Rob Buis]

Created attachment 454469 [details]
Patch

[Rob Buis]

Created attachment 454482 [details]
Patch

[Rob Buis]

Comment on attachment 454103 [details]
Patch

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

>> Source/WebCore/dom/ElementData.h:165
>> +    void setParentAndResourceInvalidationNeeded(bool dirty) const { updateFlag(s_flagParentAndResourceInvalidationNeeded, dirty); }
> 
> First try:
> void setSVGResourcesInAncestorChainAreDirty(bool dirty)?

Not bad, I go with that for now.

>> Source/WebCore/rendering/svg/RenderSVGImage.cpp:254
>> +    imageElement().setNeedsParentAndResourceInvalidation();
> 
> I am not sure about that change -- in the LBSE variant of RenderSVGImage, the imageChanged() method is basically a copy of the RenderImage::imageChanged() method and that does include a 'repaintOrMarkForLayout(rect)' call.
> If you trace that further RenderImage::repaintOrMarkForLayout() reacts on intrinsic size changes with a setNeedsLayout() call, see setNeedsLayoutIfNeededAfterIntrinsicSizeChange(). Therefore the removal of the reaction on 'updateImageViewport()' seems fishy.

Not sure either, this was Said's code, I did not look into this yet.

>> Source/WebCore/svg/SVGAnimateMotionElement.cpp:253
>> +    auto invalidate = [](SVGElement& element) {
> 
> invalidate what? :-) I'd go for invalidateTargetElement().
> 
> However: this is also legacy terminology, what is invalidation? :-) setNeeds.... setFooIsDirty... is more descriptive.
> This should be adapted to whatever naming scheme is chosen instead of 'setNeedsParentAndResourceInvalidation'.

Good point, I changed the name but it is probably still not great.

>> Source/WebCore/svg/SVGCircleElement.cpp:71
>> +        ASSERT(PropertyRegistry::isAnimatedLengthAttribute(attrName));
> 
> That is guaranteed by the generated code, and only to instruct the developer?

Just a sanity check I think, they do not hurt and should be now all over the place.

>> Source/WebCore/svg/SVGComponentTransferFunctionElement.cpp:95
>> +        invalidateFilterPrimitiveParent(*this);
> 
> I would consistently get rid of the 'invalidate' terminology here as you did in other places.

Right.

>> Source/WebCore/svg/SVGElement.cpp:613
>> +    setAnimatedSVGAttributesAreDirty();
> 
> That reads so much better than invalidateSVGAttributes().

Good!

>> Source/WebCore/svg/SVGFilterPrimitiveStandardAttributes.h:83
>> +void invalidateFilterPrimitiveParent(SVGElement&);
> 
> Might be a good idea to encapsulate this somewhere else than in the namespace.

I did not see how, so far.... SVGRenderSupport?

>> Source/WebCore/svg/SVGGeometryElement.cpp:150
>>      if (attrName == SVGNames::pathLengthAttr) {
> 
> The PropertyRegistry::isKnownAttribute() check is omitted on purpose?

Fixed already in ToT.

>> Source/WebCore/svg/SVGImageElement.cpp:101
>> +            if (!downcast<RenderSVGImage>(*renderer).updateImageViewport())
> 
> Aha, ic -- regarding my previous RenderSVGImage::imageChanged() comment. You're now triggering the setNeedsParentAndResourceInvalidation() here for SVG attribute changes -- I wonder if we still handle "intrincic size changes" correctly -- not sure if we have test coverage for that.

So is this part of the change acceptable?

[Nikolas Zimmermann]

(In reply to Rob Buis from comment #16)
> Comment on attachment 454103 [details]

> >> Source/WebCore/rendering/svg/RenderSVGImage.cpp:254
> >> +    imageElement().setNeedsParentAndResourceInvalidation();
> > 
> > I am not sure about that change -- in the LBSE variant of RenderSVGImage, the imageChanged() method is basically a copy of the RenderImage::imageChanged() method and that does include a 'repaintOrMarkForLayout(rect)' call.
> > If you trace that further RenderImage::repaintOrMarkForLayout() reacts on intrinsic size changes with a setNeedsLayout() call, see setNeedsLayoutIfNeededAfterIntrinsicSizeChange(). Therefore the removal of the reaction on 'updateImageViewport()' seems fishy.
> 
> Not sure either, this was Said's code, I did not look into this yet.
Ok. I think we have tests that do exercise image repainting, but only if they run as pixel tests :-( we have lots of these tests, e.g. in svg/dynamic-updates. If we don't convert them to reftests or tests that dump repaintrects, we lack coverage.


> >> Source/WebCore/svg/SVGFilterPrimitiveStandardAttributes.h:83
> >> +void invalidateFilterPrimitiveParent(SVGElement&);
> > 
> > Might be a good idea to encapsulate this somewhere else than in the namespace.
> 
> I did not see how, so far.... SVGRenderSupport?
Hmm, I'm trying to get rid of classes like SVGRenderSupport / SVGRenderingContext in the future. You could add a simple RAII helper class instead of a free function in its own header file.

Also it should not use the 'invalidate' terminology. Since all it does is calling setSVGResourcesInAncestorChainAreDirty() on the enclosing filter element, how about: MarkFilterAncestorChainAsDirty mark(*this) -- similar to the way 'InstanceInvalidationGuard' is used in WebCore/svg/.

> 
> >> Source/WebCore/svg/SVGImageElement.cpp:101
> >> +            if (!downcast<RenderSVGImage>(*renderer).updateImageViewport())
> > 
> > Aha, ic -- regarding my previous RenderSVGImage::imageChanged() comment. You're now triggering the setNeedsParentAndResourceInvalidation() here for SVG attribute changes -- I wonder if we still handle "intrincic size changes" correctly -- not sure if we have test coverage for that.
> 
> So is this part of the change acceptable?
I am only pointing out that we should check carefully if/how it differs from RenderImage::imageChanged(). However the current RenderSVGImage implementation is already quite different from RenderImage, unlike the RenderImage variant in the LBSE downstream branch -- with that one it would be easier to check/spot the diffs between RenderImage/RenderSVGImage. That said, I am not fully sure -- at least the controversial parts of this patch should be split out to a first preparation patch, that fixes e.g. the RenderSVGImage implementation before the  actual patch that changes the 'updateRenderTree' behaviour for SVG.

[Rob Buis]

Created attachment 454834 [details]
Patch

[Rob Buis]

Comment on attachment 454103 [details]
Patch

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

Re

>>>> Source/WebCore/svg/SVGFilterPrimitiveStandardAttributes.h:83
>>>> +void invalidateFilterPrimitiveParent(SVGElement&);
>>> 
>>> Might be a good idea to encapsulate this somewhere else than in the namespace.
>> 
>> I did not see how, so far.... SVGRenderSupport?
> 
> Hmm, I'm trying to get rid of classes like SVGRenderSupport / SVGRenderingContext in the future. You could add a simple RAII helper class instead of a free function in its own header file.
> 
> Also it should not use the 'invalidate' terminology. Since all it does is calling setSVGResourcesInAncestorChainAreDirty() on the enclosing filter element, how about: MarkFilterAncestorChainAsDirty mark(*this) -- similar to the way 'InstanceInvalidationGuard' is used in WebCore/svg/.

Ah, I did not think of RAII, I like it! Fixed.

[Rob Buis]

The failures look unrelated, so this can be reviewed.

[Said Abou-Hallawa]

Comment on attachment 454834 [details]
Patch

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

> Source/WebCore/dom/ElementData.h:134
> +    static const uint32_t s_flagSVGResourcesInAncestorChainAreDirty = 1 << 5;

I am not sure if this is the right place to add this new flag or not since this is only related to SVGElements. Because the following existing flags are also related to SVGElements

s_flagAnimatedSVGAttributesAreDirty (Only SVGElements)
s_flagPresentationalHintStyleIsDirty (Mostly SVGElements)

I do not have a strong opinion about it but adding more similar flags makes me feel we should do some clean-up here may be in the future. So please add a FIXME comment if you stick with adding it here.

> Source/WebCore/dom/ElementData.h:164
> +    bool areSVGResourcesInAncestorChainDirty() const { return m_arraySizeAndFlags & s_flagSVGResourcesInAncestorChainAreDirty; }

Should this function have a similar name like all the getters above?

areSVGResourcesInAncestorChainDirty() -> svgResourcesInAncestorChainAreDirty()

> Source/WebCore/svg/SVGComponentTransferFunctionElement.cpp:95
>          InstanceInvalidationGuard guard(*this);
> -        invalidateFilterPrimitiveParent(this);
> +        MarkFilterAncestorChainAsDirty mark(*this);

This now really looks weird. We declare two objects of two different classes and and pass them *this. And in their destructors, they do some work. Unless I am missing something, I think this easy implementation looks more obvious and cleaner:

invalidateFilterPrimitiveParent(this); // which can be renamed setFilterPrimitiveSVGResourcesInAncestorChainAreDirty() or something else.
invalidateInstances();

> Source/WebCore/svg/SVGElement.cpp:892
> +void SVGElement::markSVGResourcesInAncestorChainIfNeeded()

This function's name does not say what the SVGResources will be marked for. Should not it be named 

invalidateSVGResourcesInAncestorChainIfNeeded()
markForLayoutAndParentResourceInvalidation()

Or something more obvious.

> Source/WebCore/svg/SVGFilterPrimitiveStandardAttributes.cpp:110
> +MarkFilterAncestorChainAsDirty::~MarkFilterAncestorChainAsDirty()

I think MarkFilterAncestorChainAsDirty is not an accurate name because the parent we call setSVGResourcesInAncestorChainAreDirty() for should be a superclass of SVGFilterPrimitiveStandardAttributes such that SVGFEMerge, SVGFEDiffuseLightingElement, SVGFESpecularLightingElement or SVGFEComponentTransferElement. These superclasses must be children of SVGFilter. And this is why the original function name was invalidateFilterPrimitiveParent() not invalidateFilterParent().

> Source/WebCore/svg/SVGPolyElement.cpp:61
> +        if (auto* renderer = downcast<RenderSVGPath>(this->renderer()))

I think this casting is not needed. All SVGGraphicsElement superclasses create renders of type RenderSVGPath.

> Source/WebCore/svg/SVGSVGElement.cpp:224
> +                if (embeddedThroughFrame)
> +                    renderer->view().setNeedsLayout(MarkOnlyThis);

I do not understand this change. Why do we have to call setNeedsLayout() for the entire RenderView in addition to calling setSVGResourcesInAncestorChainAreDirty() for this particular case?

[Rob Buis]

Created attachment 455357 [details]
Patch

[Nikolas Zimmermann]

(In reply to Said Abou-Hallawa from comment #21)
> Comment on attachment 454834 [details]
> Patch
> 
> > Source/WebCore/svg/SVGComponentTransferFunctionElement.cpp:95
> >          InstanceInvalidationGuard guard(*this);
> > -        invalidateFilterPrimitiveParent(this);
> > +        MarkFilterAncestorChainAsDirty mark(*this);
> 
> This now really looks weird. We declare two objects of two different classes
> and and pass them *this. And in their destructors, they do some work. Unless
> I am missing something, I think this easy implementation looks more obvious
> and cleaner:
> 
> invalidateFilterPrimitiveParent(this); // which can be renamed
> setFilterPrimitiveSVGResourcesInAncestorChainAreDirty() or something else.
> invalidateInstances();

Indeed -- my initial suggestion was just not to place this in namespace WebCore directly, having a non-descriptive top-level function named 'invalidateInstances' would pollute WebCore IMHO.


> > Source/WebCore/svg/SVGElement.cpp:892
> > +void SVGElement::markSVGResourcesInAncestorChainIfNeeded()
> 
> This function's name does not say what the SVGResources will be marked for.
> Should not it be named 
> 
> invalidateSVGResourcesInAncestorChainIfNeeded()
> markForLayoutAndParentResourceInvalidation()
> 
> Or something more obvious.
> 
> > Source/WebCore/svg/SVGFilterPrimitiveStandardAttributes.cpp:110
> > +MarkFilterAncestorChainAsDirty::~MarkFilterAncestorChainAsDirty()
> 
> I think MarkFilterAncestorChainAsDirty is not an accurate name because the
> parent we call setSVGResourcesInAncestorChainAreDirty() for should be a
> superclass of SVGFilterPrimitiveStandardAttributes such that SVGFEMerge,
> SVGFEDiffuseLightingElement, SVGFESpecularLightingElement or
> SVGFEComponentTransferElement. These superclasses must be children of
> SVGFilter. And this is why the original function name was
> invalidateFilterPrimitiveParent() not invalidateFilterParent().
That's a pretty strong compelling reason, thanks Said.

[Rob Buis]

Created attachment 455372 [details]
Patch

[Rob Buis]

Created attachment 455394 [details]
Patch

[Rob Buis]

Comment on attachment 454834 [details]
Patch

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

>> Source/WebCore/dom/ElementData.h:134
>> +    static const uint32_t s_flagSVGResourcesInAncestorChainAreDirty = 1 << 5;
> 
> I am not sure if this is the right place to add this new flag or not since this is only related to SVGElements. Because the following existing flags are also related to SVGElements
> 
> s_flagAnimatedSVGAttributesAreDirty (Only SVGElements)
> s_flagPresentationalHintStyleIsDirty (Mostly SVGElements)
> 
> I do not have a strong opinion about it but adding more similar flags makes me feel we should do some clean-up here may be in the future. So please add a FIXME comment if you stick with adding it here.

I think for SVG we only have a rare data section to store this conveniently, but I think the two flags are not rare data. So for now I left the FIXME.

>> Source/WebCore/dom/ElementData.h:164
>> +    bool areSVGResourcesInAncestorChainDirty() const { return m_arraySizeAndFlags & s_flagSVGResourcesInAncestorChainAreDirty; }
> 
> Should this function have a similar name like all the getters above?
> 
> areSVGResourcesInAncestorChainDirty() -> svgResourcesInAncestorChainAreDirty()

Right, I was going for SVGResourcesInAncestorChainAreDirty first, and it looked off, but lowercasing the svg here seems reasonable. Fixed.

>>> Source/WebCore/svg/SVGComponentTransferFunctionElement.cpp:95
>>> +        MarkFilterAncestorChainAsDirty mark(*this);
>> 
>> This now really looks weird. We declare two objects of two different classes and and pass them *this. And in their destructors, they do some work. Unless I am missing something, I think this easy implementation looks more obvious and cleaner:
>> 
>> invalidateFilterPrimitiveParent(this); // which can be renamed setFilterPrimitiveSVGResourcesInAncestorChainAreDirty() or something else.
>> invalidateInstances();
> 
> Indeed -- my initial suggestion was just not to place this in namespace WebCore directly, having a non-descriptive top-level function named 'invalidateInstances' would pollute WebCore IMHO.

Sure, I removed the MarkFilterAncestorChainAsDirty RAII class.

>>> Source/WebCore/svg/SVGElement.cpp:892
>>> +void SVGElement::markSVGResourcesInAncestorChainIfNeeded()
>> 
>> This function's name does not say what the SVGResources will be marked for. Should not it be named 
>> 
>> invalidateSVGResourcesInAncestorChainIfNeeded()
>> markForLayoutAndParentResourceInvalidation()
>> 
>> Or something more obvious.
> 
> That's a pretty strong compelling reason, thanks Said.

Right, that was a bit inaccurate, I went with invalidateSVGResourcesInAncestorChainIfNeeded.

>> Source/WebCore/svg/SVGFilterPrimitiveStandardAttributes.cpp:110
>> +MarkFilterAncestorChainAsDirty::~MarkFilterAncestorChainAsDirty()
> 
> I think MarkFilterAncestorChainAsDirty is not an accurate name because the parent we call setSVGResourcesInAncestorChainAreDirty() for should be a superclass of SVGFilterPrimitiveStandardAttributes such that SVGFEMerge, SVGFEDiffuseLightingElement, SVGFESpecularLightingElement or SVGFEComponentTransferElement. These superclasses must be children of SVGFilter. And this is why the original function name was invalidateFilterPrimitiveParent() not invalidateFilterParent().

Fair enough, I removed the MarkFilterAncestorChainAsDirty class.

>> Source/WebCore/svg/SVGPolyElement.cpp:61
>> +        if (auto* renderer = downcast<RenderSVGPath>(this->renderer()))
> 
> I think this casting is not needed. All SVGGraphicsElement superclasses create renders of type RenderSVGPath.

Fixed here and in two similar instances.

>> Source/WebCore/svg/SVGSVGElement.cpp:224
>> +                    renderer->view().setNeedsLayout(MarkOnlyThis);
> 
> I do not understand this change. Why do we have to call setNeedsLayout() for the entire RenderView in addition to calling setSVGResourcesInAncestorChainAreDirty() for this particular case?

This is explained in comment 12.

[Said Abou-Hallawa]

Comment on attachment 455394 [details]
Patch

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

> Source/WebCore/svg/SVGSVGElement.cpp:222
> +#if ENABLE(LAYER_BASED_SVG_ENGINE)
> +                if (is<RenderSVGRoot>(renderer) && downcast<RenderSVGRoot>(renderer)->isEmbeddedThroughFrameContainingSVGDocument())
> +                    embeddedThroughFrame = true;
> +#endif
> +                if (is<LegacyRenderSVGRoot>(renderer) && downcast<LegacyRenderSVGRoot>(renderer)->isEmbeddedThroughFrameContainingSVGDocument())
> +                    embeddedThroughFrame = true;

Why we do have to check is<LegacyRenderSVGRoot>(renderer) in the case #if ENABLE(LAYER_BASED_SVG_ENGINE)? Should not the check is<LegacyRenderSVGRoot>(renderer) be inside the #else ... #endif block? I think also we can get rid of the local "embeddedThroughFrame" and call renderer->view().setNeedsLayout(MarkOnlyThis); directly.

[Nikolas Zimmermann]

(In reply to Said Abou-Hallawa from comment #27)
> Comment on attachment 455394 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455394&action=review

Thanks for the r+ Said and starting the work - it's a good move in the right direction! I'd like to chim in regarding the LBSE questions:

> 
> > Source/WebCore/svg/SVGSVGElement.cpp:222
> > +#if ENABLE(LAYER_BASED_SVG_ENGINE)
> > +                if (is<RenderSVGRoot>(renderer) && downcast<RenderSVGRoot>(renderer)->isEmbeddedThroughFrameContainingSVGDocument())
> > +                    embeddedThroughFrame = true;
> > +#endif
> > +                if (is<LegacyRenderSVGRoot>(renderer) && downcast<LegacyRenderSVGRoot>(renderer)->isEmbeddedThroughFrameContainingSVGDocument())
> > +                    embeddedThroughFrame = true;
> 
> Why we do have to check is<LegacyRenderSVGRoot>(renderer) in the case #if
> ENABLE(LAYER_BASED_SVG_ENGINE)? Should not the check
> is<LegacyRenderSVGRoot>(renderer) be inside the #else ... #endif block?
Because you can compile WebKit with LBSE enabled, but disable it at runtime (that's the default). Therefore you need to handle both cases when compiling wiht LBSE. The check if LBSE is actually activated can be omitted (which Rob did), since objects of type 'RenderSVGRoot' are only ever produced, if LBSE is compiled into WebKit and activated at runtime (e.g. via a menu in MiniBrowser).

> I think also we can get rid of the local "embeddedThroughFrame" and call
> renderer->view().setNeedsLayout(MarkOnlyThis); directly.
Nope, that would a huge amount of extra work for inner <svg> elements, whose width/height changes -- you would invalidate the whole documents' RenderView, whenever an inner <svg> element changes its size. SVGSVGElement creates 'RenderSVGRoot' type renderers for outer <svg> elements (such as those SVG documents embedded through a frame) and 'RenderSVGViewportContainer' - for inner <svg> elements.

[Nikolas Zimmermann]

Comment on attachment 455394 [details]
Patch

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

> Source/WebCore/svg/SVGSVGElement.cpp:221
> +                if (is<LegacyRenderSVGRoot>(renderer) && downcast<LegacyRenderSVGRoot>(renderer)->isEmbeddedThroughFrameContainingSVGDocument())

Nitpick: add if (!embeddedThroughFrame... to save work.

[Rob Buis]

Created attachment 455553 [details]
Patch

[EWS]

Committed r291788 (248816@main): <https://commits.webkit.org/248816@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 455553 [details].

[Antti Koivisto]

This was 9% regression in Motion Mark Suits subtest, bug 240112