Bug 232369 – [GPU Process] [Filters 1/23] Remove non-platform code form the FEImage class

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Said Abou-Hallawa]

We need to make easy to encode and decode the FEImage members without having any DOM or rendering objects.

[Said Abou-Hallawa]

Created attachment 442574 [details]
Patch

[Said Abou-Hallawa]

Created attachment 442594 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 442594 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [GPU Process] Remove layering violations form FEImage class

This title make me think that I'll see removal of non-platform #includes in platform code, but I don't see any of that here, so maybe this is just a first step? If so, adjust the bug title.

Maybe the plan is to move FEImage down to platform code in future?

> Source/WebCore/svg/SVGFEImageElement.cpp:6
>   * Copyright (C) 2018 Apple Inc. All rights reserved.
> + * Copyright (C) 2018-2021 Apple Inc. All rights reserved.

Duplicate Apple lines.

> Source/WebCore/svg/SVGFEImageElement.cpp:193
> +    // Calculate the absoluteTransform

Comment doesn't add anything.

> Source/WebCore/svg/SVGFEImageElement.cpp:200
> +    auto imageRect = renderer->repaintRectInLocalCoordinates();

Not new, but using something called "repaint rect" for filter geometry sounds wrong.

> Source/WebCore/svg/SVGFEImageElement.h:58
> +    std::tuple<RefPtr<ImageBuffer>, FloatRect> imageBufferForEffect() const;

The problem with returning a tuple is that the rect doesn't have a name. Consider returning a struct instead.

> Source/WebCore/svg/graphics/filters/SVGFEImage.h:40
> +        Ref<ImageBuffer>,
> +        RenderingResourceIdentifier

I'm confused about the we use a raw RenderingResourceIdentifier rather than an ImageBuffer (which can be identified via a RenderingResourceIdentifier). If you don't use the RenderingResourceIdentifier in this patch, maybe add it later.

If the variant holds a ImageBuffer, does it have to be a concrete image buffer?

[Radar WebKit Bug Importer]

<rdar://problem/84966765>

[Said Abou-Hallawa]

Created attachment 443633 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 442594 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        [GPU Process] Remove layering violations form FEImage class
> 
> This title make me think that I'll see removal of non-platform #includes in platform code, but I don't see any of that here, so maybe this is just a first step? If so, adjust the bug title.
> 
> Maybe the plan is to move FEImage down to platform code in future?

Title was changed. And yes the plan should be to move FEImage down to platform/graphics/filters directory.

>> Source/WebCore/svg/SVGFEImageElement.cpp:6
>> + * Copyright (C) 2018-2021 Apple Inc. All rights reserved.
> 
> Duplicate Apple lines.

removed.

>> Source/WebCore/svg/SVGFEImageElement.cpp:193
>> +    // Calculate the absoluteTransform
> 
> Comment doesn't add anything.

removed.

>> Source/WebCore/svg/SVGFEImageElement.cpp:200
>> +    auto imageRect = renderer->repaintRectInLocalCoordinates();
> 
> Not new, but using something called "repaint rect" for filter geometry sounds wrong.

Yes. This name does not seem appropriate. It used by SVG renderers only. I will try to fix in a separate patch.

>> Source/WebCore/svg/SVGFEImageElement.h:58
>> +    std::tuple<RefPtr<ImageBuffer>, FloatRect> imageBufferForEffect() const;
> 
> The problem with returning a tuple is that the rect doesn't have a name. Consider returning a struct instead.

Yes you are right. Using the tuple does not make the return value clear in the prototype only. But the return value is clear in the implementation and when the function is called. I will clean this later.

>> Source/WebCore/svg/graphics/filters/SVGFEImage.h:40
>> +        RenderingResourceIdentifier
> 
> I'm confused about the we use a raw RenderingResourceIdentifier rather than an ImageBuffer (which can be identified via a RenderingResourceIdentifier). If you don't use the RenderingResourceIdentifier in this patch, maybe add it later.
> 
> If the variant holds a ImageBuffer, does it have to be a concrete image buffer?

Yes the RenderingResourceIdentifier was added for GPUProcess. The plan is to encode the RenderingResourceIdentifier of the Image or the ImageBuffer. When decoding the FEImage, this RenderingResourceIdentifier will be replaced with the appropriate resource. But since the RenderingResourceIdentifier is not used in this patch, I removed it for now.

[EWS]

Committed r285481 (244005@main): <https://commits.webkit.org/244005@main>

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