Bug 234954 – [LBSE] Begin layer-aware RenderSVGShape implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Nikolas Zimmermann]

Now that RenderSVGShape was renamed to LegacyRenderSVGShape, we can re-introduce RenderSVGShape for LBSE.

[Nikolas Zimmermann]

Created attachment 448582 [details]
Patch, v1

[Nikolas Zimmermann]

Created attachment 448584 [details]
Patch, v2

[Rob Buis]

Comment on attachment 448582 [details]
Patch, v1

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

> Source/WebCore/ChangeLog:10
> +        layout / paint / hit-testing looks quite, much more similar to RenderBox & co

quite what? :)

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:251
> +        return;

Maybe instead there could be an ASSERT that this does not happen? AFAICS only filter provide nullptr for both but that seems a different code path.

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:254
> +        ASSERT(shape->isSVGShapeOrLegacySVGShape());

How about ASSERT(!shape || shape->isSVGShapeOrLegacySVGShape()); ?

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:32
> +#include "LegacyRenderSVGShape.h"

Is this needed?

> Source/WebCore/svg/SVGAnimateMotionElement.cpp:28
> +#include "RenderElement.h"

Ditto?

> Source/WebCore/svg/SVGLineElement.cpp:25
> +#include "LegacyRenderSVGShape.h"

Ditto?

[Nikolas Zimmermann]

(In reply to Rob Buis from comment #3)
Thanks for the first review.

> > Source/WebCore/ChangeLog:10
> > +        layout / paint / hit-testing looks quite, much more similar to RenderBox & co
> 
> quite what? :)
different :-)
 
> > Source/WebCore/rendering/svg/RenderSVGResource.cpp:251
> > +        return;
> 
> Maybe instead there could be an ASSERT that this does not happen? AFAICS
> only filter provide nullptr for both but that seems a different code path.
> 
> > Source/WebCore/rendering/svg/RenderSVGResource.cpp:254
> > +        ASSERT(shape->isSVGShapeOrLegacySVGShape());
> 
> How about ASSERT(!shape || shape->isSVGShapeOrLegacySVGShape()); ?
Having two statements in an assertion doesn't tell you precisely what failed, which is annoying :-)

However we could turn both the if (!shape && !path) return and the if(shape) ASSERT... into:

    if (shape)
        ASSERT(shape->isSVGShapeOrLegacySVGShape());
    else
        ASSERT(path);

This is even more ugly, IMHO. So I went for:

void RenderSVGResource::fillAndStrokePathOrShape(..) {
{
    if (shape) {
        ASSERT(shape->isSVGShapeOrLegacySVGShape());

        if (resourceMode.contains(RenderSVGResourceMode::ApplyToFill)) {
            if (is<LegacyRenderSVGShape>(shape))
                downcast<LegacyRenderSVGShape>(shape)->fillShape(context);
...
        }

        if (resourceMode.contains(RenderSVGResourceMode::ApplyToStroke)) {
            if (is<LegacyRenderSVGShape>(shape))
                downcast<LegacyRenderSVGShape>(shape)->strokeShape(context);
...
        }

        return;
    }

    if (path) {
        if (resourceMode.contains(RenderSVGResourceMode::ApplyToFill))
            context.fillPath(*path);
        if (resourceMode.contains(RenderSVGResourceMode::ApplyToStroke))
            context.strokePath(*path);

        return;
    }

    ASSERT_NOT_REACHED();
}

That is more pleasant to my eyes :-) Fine with you?

> 
> > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:32
> > +#include "LegacyRenderSVGShape.h"
> 
> Is this needed?
Leftover, probably not.

> 
> > Source/WebCore/svg/SVGAnimateMotionElement.cpp:28
> > +#include "RenderElement.h"
> 
> Ditto?
Nope on purpose, I removed a RenderElement.h include in an unrelated place, and this is fixing the wrong places, that forgot the include before.
 
> > Source/WebCore/svg/SVGLineElement.cpp:25
> > +#include "LegacyRenderSVGShape.h"
> 
> Ditto?
Nope, (caught by WinCairo EWS).

[Nikolas Zimmermann]

> > > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:32
> > > +#include "LegacyRenderSVGShape.h"
> > 
> > Is this needed?
> Leftover, probably not.

./rendering/svg/SVGRenderSupport.cpp:161:16: error: unknown type name 'LegacyRenderSVGShape'; did you mean 'LegacyRenderSVGRoot'?
        if (is<LegacyRenderSVGShape>(current) && downcast<LegacyRenderSVGShape>(current).isRenderingDisabled())
               ^~~~~~~~~~~~~~~~~~~~
               LegacyRenderSVGRoot

The new include is needed, as I've removed the LegacyRenderSVGShape.h include from RenderSVGResource.h, which is no longer needed there, since postApplyResource() is now taking a RenderElement (common-base class for LegacyRenderSVGShape/RenderSVGShape) instead of a LegacyRenderSVGShape.


I've also double-checked that all places that handle LegacyRenderSVGShape, also handle RenderSVGShape now. Previously I missed SVGGeometryElement (getTotalLength() / etc.).

I will include all fixes in a new patch, that also hopefully satisfies win/wincairo builds.

[Nikolas Zimmermann]

Created attachment 448628 [details]
Patch, v3

[Nikolas Zimmermann]

Created attachment 448637 [details]
Patch, v4

[Nikolas Zimmermann]

(In reply to Rob Buis from comment #3)> 
> > Source/WebCore/rendering/svg/RenderSVGResource.cpp:251
> > +        return;
> 
> Maybe instead there could be an ASSERT that this does not happen? AFAICS
> only filter provide nullptr for both but that seems a different code path.

EWS lectured me: shape=path=nullptr is possible.

void SVGInlineTextBox::restoreGraphicsContextAfterTextPainting(GraphicsContext*& context)
{
    releasePaintingResource(context, /* path */nullptr);

->

void SVGInlineTextBox::releasePaintingResource(GraphicsContext*& context, const Path* path)
{
    ASSERT(m_paintingResource);

    m_paintingResource->postApplyResource(parent()->renderer(), context, paintingResourceMode(), path, /*LegacyRenderSVGShape*/ nullptr);


-> both shape and path are nullptr.

The ASSERT_NOT_REACHED() has to go, or we fix the call sites. However, due to the scope of this patch, it's better to remove the ASSERT_NOT_REACHED.

[Nikolas Zimmermann]

Created attachment 448666 [details]
Patch, v5

[Rob Buis]

Comment on attachment 448666 [details]
Patch, v5

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

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:280
> +        return;

Not needed.

> Source/WebCore/rendering/svg/RenderSVGResourceSolidColor.h:23
> +#include "FloatRect.h"

Is this needed?

[Nikolas Zimmermann]

(In reply to Rob Buis from comment #10)
> Comment on attachment 448666 [details]
> Patch, v5
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448666&action=review
> 
> > Source/WebCore/rendering/svg/RenderSVGResource.cpp:280
> > +        return;
> 
> Not needed.
True. But then I could also make it if (!path) return early return..

> 
> > Source/WebCore/rendering/svg/RenderSVGResourceSolidColor.h:23
> > +#include "FloatRect.h"
> 
> Is this needed?

It is, we use FloatRect() in resourceBoundingBox(), and no other class now provides this include.

[Rob Buis]

Comment on attachment 448666 [details]
Patch, v5

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

>>> Source/WebCore/rendering/svg/RenderSVGResource.cpp:280
>>> +        return;
>> 
>> Not needed.
> 
> True. But then I could also make it if (!path) return early return..

Actually, that is probably preferred, it was also remove indentation.

[Nikolas Zimmermann]

Committed r287832 (245884@trunk): <https://commits.webkit.org/245884@trunk>

[Radar WebKit Bug Importer]

<rdar://problem/87330324>