[webkit-reviews] review granted: [Bug 202797] Implement Canvas.transferControlToOffscreen and OffscreenCanvasRenderingContext2D.commit : [Attachment 395666] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Chris Lord <clord at igalia.com>'s
request for review:
Bug 202797: Implement Canvas.transferControlToOffscreen and
OffscreenCanvasRenderingContext2D.commit
https://bugs.webkit.org/show_bug.cgi?id=202797

Attachment 395666: Patch

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




--- Comment #8 from Dean Jackson <dino at apple.com> ---
Comment on attachment 395666
  --> https://bugs.webkit.org/attachment.cgi?id=395666
Patch

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

It still would be nice if cdumez verified the threading bits.

> Source/WebCore/html/HTMLCanvasElement.cpp:941
> +    if (isControlledByOffscreen() && oldSize != size()) {
> +	   setAttributeWithoutSynchronization(widthAttr,
AtomString::number(width()));
> +	   setAttributeWithoutSynchronization(heightAttr,
AtomString::number(height()));
> +	   notifyObserversCanvasResized();
> +	   invalidateStyleAndRenderersForSubtree();
> +    }

Why is this only for placeholder canvas objects? Does the specification
explicitly say we should update the attributes? I assume this is the end of the
call to transferFromImageBitmap().

> Source/WebCore/html/OffscreenCanvas.cpp:150
>	       if (!is<OffscreenCanvasRenderingContext2D>(*m_context))

I don't think we can ever get this case. Am I mistaken? If not, I wonder if we
should just assert (or let downcast do that).

> Source/WebCore/html/OffscreenCanvas.cpp:164
> +	       if (is<WebGLRenderingContext>(*m_context))

Same here.

> Source/WebCore/html/OffscreenCanvas.cpp:341
> +	   std::unique_ptr<ImageBuffer> buffer =
ImageBuffer::create(FloatSize(m_pendingCommitData->size()),
RenderingMode::Unaccelerated);

Agree with Darin that we should probably update ImageBuffer::create uses to
makeUnique, but not in this patch.

> Source/WebCore/html/OffscreenCanvas.cpp:379
> +	   auto& context = *scriptExecutionContext();
> +	   m_hasScheduledCommit =
context.postTaskTo(context.contextIdentifier(), [this]
(ScriptExecutionContext&) {

Suggest using a different name here for "context", which could be confused with
the rendering context.

> Source/WebCore/html/OffscreenCanvas.h:69
> +    WeakPtr<HTMLCanvasElement> takePlaceholderCanvas();

I wonder if this should be givePlaceholderCanvas() because the action is being
called on the DetachedOffscreenCanvas. (Or, releasePlaceholderCanvas, except it
is only a weakptr so "release" isn't clear)

> Source/WebCore/html/canvas/OffscreenCanvasRenderingContext2D.cpp:54
> +    OffscreenCanvas& offscreenCanvas =
downcast<OffscreenCanvas>(canvasBase());
> +    offscreenCanvas.commitToPlaceholderCanvas();

Darin mentioned doing this in a single line.