Bug 234954 - [LBSE] Begin layer-aware RenderSVGShape implementation
Summary: [LBSE] Begin layer-aware RenderSVGShape implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords: InRadar
Depends on: 234877
Blocks: 90738 234632 234992
  Show dependency treegraph
 
Reported: 2022-01-07 04:56 PST by Nikolas Zimmermann
Modified: 2022-01-10 01:48 PST (History)
23 users (show)

See Also:


Attachments
Patch, v1 (54.04 KB, patch)
2022-01-07 05:07 PST, Nikolas Zimmermann
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch, v2 (70.75 KB, patch)
2022-01-07 06:31 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v3 (74.58 KB, patch)
2022-01-07 13:59 PST, Nikolas Zimmermann
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch, v4 (74.64 KB, patch)
2022-01-07 15:13 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v5 (74.62 KB, patch)
2022-01-08 02:10 PST, Nikolas Zimmermann
rbuis: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2022-01-07 04:56:56 PST
Now that RenderSVGShape was renamed to LegacyRenderSVGShape, we can re-introduce RenderSVGShape for LBSE.
Comment 1 Nikolas Zimmermann 2022-01-07 05:07:01 PST
Created attachment 448582 [details]
Patch, v1
Comment 2 Nikolas Zimmermann 2022-01-07 06:31:35 PST
Created attachment 448584 [details]
Patch, v2
Comment 3 Rob Buis 2022-01-07 06:40:22 PST
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?
Comment 4 Nikolas Zimmermann 2022-01-07 12:38:49 PST
(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).
Comment 5 Nikolas Zimmermann 2022-01-07 12:57:54 PST
> > > 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.
Comment 6 Nikolas Zimmermann 2022-01-07 13:59:39 PST
Created attachment 448628 [details]
Patch, v3
Comment 7 Nikolas Zimmermann 2022-01-07 15:13:01 PST
Created attachment 448637 [details]
Patch, v4
Comment 8 Nikolas Zimmermann 2022-01-08 02:08:10 PST
(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.
Comment 9 Nikolas Zimmermann 2022-01-08 02:10:24 PST
Created attachment 448666 [details]
Patch, v5
Comment 10 Rob Buis 2022-01-10 00:33:40 PST
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?
Comment 11 Nikolas Zimmermann 2022-01-10 01:29:22 PST
(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.
Comment 12 Rob Buis 2022-01-10 01:31:32 PST
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.
Comment 13 Nikolas Zimmermann 2022-01-10 01:47:46 PST
Committed r287832 (245884@trunk): <https://commits.webkit.org/245884@trunk>
Comment 14 Radar WebKit Bug Importer 2022-01-10 01:48:21 PST
<rdar://problem/87330324>