Bug 239527

rdar://91113628

Created attachment 457949 [details]
Patch

Created attachment 457963 [details]
Patch

Created attachment 458006 [details]
Patch

Created attachment 458014 [details]
Patch

Created attachment 458114 [details]
Patch

Created attachment 458240 [details]
Patch

Created attachment 458243 [details]
Patch

Created attachment 458244 [details]
Patch

Created attachment 458331 [details]
Patch

Created attachment 458333 [details]
Patch

Created attachment 458338 [details]
Patch

Created attachment 458477 [details]
Patch

(In reply to Said Abou-Hallawa from comment #13)
> Created attachment 458477 [details]
> Patch

This patch fixes the failures in the api-mac tests. The change in WebChromeClient::createImageBuffer() fixes the timeout in two api tests.

Comment on attachment 458477 [details]
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"?

Created attachment 458484 [details]
Patch

Comment on attachment 458477 [details]
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?

Fixed. This was changed to
    Ref document = *frame.document();

>> 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?

Unfortunately we can't do that. A lambda can be used as a pointer to function as long as it doesn't capture variables.

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

Fixed.

>> 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).

I agree. Comments were removed.

>> 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"?

Fixed. Default color space was removed.

Committed r293570 (250084@main): <https://commits.webkit.org/250084@main>

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