[webkit-reviews] review granted: [Bug 239527] [GPU Process] Make WebImage be backed by ImageBuffer : [Attachment 458477] Patch

[bugzilla-daemon at webkit.org]

Tim Horton <thorton at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 239527: [GPU Process] Make WebImage be backed by ImageBuffer
https://bugs.webkit.org/show_bug.cgi?id=239527

Attachment 458477: Patch

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




--- Comment #15 from Tim Horton <thorton at apple.com> ---
Comment on attachment 458477
  --> https://bugs.webkit.org/attachment.cgi?id=458477
Patch

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

> Source/WebCore/page/FrameSnapshotting.cpp:84
> +    auto& document = *frame.document();

Should this be a strong reference?

> Source/WebCore/platform/graphics/ConcreteImageBuffer.h:296
> +	   ref(); // Balanced by deref below.

Could this be a RefPtr that gets captured by the block, instead of this raw
ref/deref?

> Source/WebKit/Shared/UserData.cpp:402
> +	   result = WebImage::create(*paramters, WTFMove(handle));

Spelling: "paramters"

> Source/WebKit/Shared/WebImage.cpp:38
> +    // Create shareable RemoteImageBuffer if GPU Process is enabled.

I kind of see the need for these comments, but they feel indicative of how
weird some of this code has gotten :)

I don't think they're quite worded correctly, though, because e.g. this code
has NO IDEA why ChromeClient might or might not give you a imagebuffer (doesn't
know that it's about GPUP), and would get stale if that reason changed. So
maybe we leave them out? Or word them differently ("allow the chrome client to
override the image if it wants to" etc. etc. but then it's fairly obvious from
reading the code).

> Source/WebKit/Shared/WebImage.h:48
> +    static RefPtr<WebImage> create(const WebCore::IntSize&, ImageOptions,
const WebCore::DestinationColorSpace& = WebCore::DestinationColorSpace::SRGB(),
WebCore::ChromeClient* = nullptr);

Wasn't Sam pushing us in the direction of "never default to SRGB, always make
clients say it, so you can fix them one-by-one"?