[webkit-reviews] review granted: [Bug 202573] Implement OffscreenCanvas.convertToBlob : [Attachment 385248] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Lord <clord at igalia.com>'s
request for review:
Bug 202573: Implement OffscreenCanvas.convertToBlob
https://bugs.webkit.org/show_bug.cgi?id=202573

Attachment 385248: Patch

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




--- Comment #19 from Darin Adler <darin at apple.com> ---
Comment on attachment 385248
  --> https://bugs.webkit.org/attachment.cgi?id=385248
Patch

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

Looks great, but I’m not 100% happy with every detail.

> Source/WebCore/html/OffscreenCanvas.cpp:212
> +    promise->resolveWithNewlyCreated<IDLInterface<Blob>>(blob);

What about the WTFMove I mentioned last time? Would it not save one little
reference count churn here?

> Source/WebCore/html/OffscreenCanvas.cpp:222
> +    auto* context = canvasBaseScriptExecutionContext();

If context is guaranteed to be non-null, then I suggest making the local
variable be a reference rather than a pointer, and dereferencing here.

> Source/WebCore/html/OffscreenCanvas.cpp:223
> +    if (context->isWorkerGlobalScope())

When paired with downcast, I prefer to use is<>, the two function templates are
designed to be used together, so this should be is<WorkerGlobalScope>(*context)
instead.

> Source/WebCore/html/OffscreenCanvas.cpp:226
> +    ASSERT(context->isDocument());

This assertion isn’t valuable. The point of downcast<Document> is exactly that,
includes the assertion plus a static_cast. So no need to also write a separate
assertion.

> Source/WebCore/platform/ThreadGlobalData.cpp:127
> +static HashSet<String, ASCIICaseInsensitiveHash>
createSupportedImageMIMETypesForEncoding()

Seems inelegant that this entire function ends up here in a file that was
formerly not really filled with specific code. Instead we should find a way to
leave this code in MIMETypeRegistry.cpp. Something like this in
MIMETypeRegistry.h perhaps:

    std::unique_ptr<MIMETypeRegistryThreadGlobalData>
createMIMETypeRegistryThreadGlobalData();

Not sure all the details but generally it would be strange to have all this
code in the ThreadGlobalData source file. And this way we'd have less specifics
in the ThreadGlobalData header too, just the name
MIMETypeRegistryThreadGlobalData and not the specifics of exactly what kind of
HashSet.