[webkit-reviews] review granted: [Bug 232369] [GPU Process] Remove layering violations form FEImage class : [Attachment 442594] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 232369: [GPU Process] Remove layering violations form FEImage class
https://bugs.webkit.org/show_bug.cgi?id=232369

Attachment 442594: Patch

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




--- Comment #3 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 442594
  --> https://bugs.webkit.org/attachment.cgi?id=442594
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?