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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 395157: Patch

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




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

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

Looks good overall. Lots of coding style comments. Generally confused about the
threading strategy; seems likely there are mistakes in that. It’s tricky to
correctly use a RefCounted object from multiple threads and this doesn’t seem
to have a disciplined approach to that.

review- for now; maybe Chris Dumez can help us vet the threading part of a
future cut at the patch

> Source/WebCore/html/HTMLCanvasElement.cpp:746
> +    RefPtr<OffscreenCanvas> canvas = OffscreenCanvas::create(context,
width(), height());

This should use auto. And it should be a Ref, not RefPtr.

> Source/WebCore/html/HTMLCanvasElement.cpp:747
> +    canvas->setPlaceholderCanvas(makeWeakPtr(*this));

Can this be passed to an overload of the create function instead?

> Source/WebCore/html/HTMLCanvasElement.cpp:753
> +#if USE(TEXTURE_MAPPER_GL)
> +    // Need to make sure a RenderLayer and compositing layer get created for
the Canvas.
> +    invalidateStyleAndLayerComposition();
> +#endif

This seems like oblique side effect programming. How does "invalidate" make
sure something is created? Can we do better?

Should not capitalize "Canvas".

> Source/WebCore/html/HTMLCanvasElement.h:101
> +    ExceptionOr<RefPtr<OffscreenCanvas>>
transferControlToOffscreen(ScriptExecutionContext&);

This should be ExceptionOr<Ref> instead. RefPtr means the result can be null
even when there is no exception; this result can’t be.

> Source/WebCore/html/OffscreenCanvas.cpp:80
> +    callOnMainThread([canvas = detachedCanvas->takePlaceholderCanvas(),
offscreenCanvas = clone.ptr()] () mutable {

Seems peculiar and unsafe to use .ptr() here, turning offscreenCanvas into a
raw pointer; normally I would have expected copyRef to pass a Ref<> instead.
But also unclear how it’s thread-safe to pass a non-threadsafe-ref-counted
offscreen canvas from one thread to another. What prevents this lambda from
using a stale pointer after the canvas has been destroyed?

Not clear at all to me the role of the main thread here and threading strategy.

> Source/WebCore/html/OffscreenCanvas.cpp:84
> +		   PlaceholderRenderingContext& placeholderContext =
downcast<PlaceholderRenderingContext>(*renderingContext);
> +		   placeholderContext.setOffscreenCanvas(offscreenCanvas);

This should use auto& or be a one-liner.

   
downcast<PlaceholderRenderingContext>(*renderingContext).setOffscreenCanvas(off
screenCanvas);

Is there some way to write this code without the downcast? I am unclear on
where the type guarantee comes from.

> Source/WebCore/html/OffscreenCanvas.cpp:149
> +		   return Optional<OffscreenRenderingContext> { WTF::nullopt };

I’ve seen some other people have success with multiple sets of braces without
the type names. For example, this might work here:

    return { { WTF::nullopt } };

Maybe give it a try? The many other places here we had to write out that long
type name, instead we might just be able to add one more set of parentheses.
And even remove some of the other type names.

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

auto

Also doesn’t make sense that ImageBuffer supplies a create function that
returns a unique_ptr. Normally we use create functions when something is
reference counted. If we are using unique_ptr and not intrusive reference
counting then we should instead have public constructors and use makeUnique at
the call sites.

> Source/WebCore/html/OffscreenCanvas.cpp:357
> +    // TODO: Transfer texture over if we're using accelerated compositing

WebKit project uses FIXME, not TODO. Lets not mix them in.

> Source/WebCore/html/OffscreenCanvas.cpp:382
> +	   ScriptExecutionContext* context = scriptExecutionContext();

auto& context = *scriptExecutionContext();

> Source/WebCore/html/OffscreenCanvas.h:38
> +#include <JavaScriptCore/Uint8ClampedArray.h>

Mysterious include addition. Why?

> Source/WebCore/html/OffscreenCanvas.h:119
> +    HTMLCanvasElement* placeholderCanvas() const;

What is this public function for? I can’t find any uses of it?

> Source/WebCore/html/OffscreenCanvas.h:120
> +    void commitToPlaceholderCanvas();

Why is this public?

> Source/WebCore/html/OffscreenCanvas.h:163
> +    bool m_pendingCommitTask { false };

This local variable name sounds like it is a variable that holds a "pending
commit task". For best naming of boolean, think of how to explain it to
someone. One example could be m_hasPendingCommitTask. But you could probably
come up with an even better name.

> Source/WebCore/html/OffscreenCanvas.h:168
> +    bool m_pendingCommit { false };
> +    RefPtr<ImageData> m_pendingCommitData;

Very unclear the relationship between the boolean and the data.

Also, I never see m_pendingCommitData set to nullptr after the commit. I think
it probably should be. I suspect we can get rid of the boolean and use the
nullity instead.

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

Should write this as a one-liner:

    canvas().commitToPlaceholderCanvas();

Also might consider just putting this in the header as long as it doesn’t
create the need for a new include.

> Source/WebCore/html/canvas/PlaceholderRenderingContext.cpp:57
> +void PlaceholderRenderingContext::setOffscreenCanvas(OffscreenCanvas*
offscreenCanvas)
> +{
> +    ASSERT(offscreenCanvas);
> +    m_offscreenCanvas = offscreenCanvas;
>  }

When we take a pointer and assert it’s not null, that means we should be taking
a reference instead.

> Source/WebCore/html/canvas/PlaceholderRenderingContext.h:39
> +    PlaceholderRenderingContext(CanvasBase&, OffscreenCanvas*);

This should take OffscreenCanvas&.

> Source/WebCore/html/canvas/PlaceholderRenderingContext.h:43
> +    void setOffscreenCanvas(OffscreenCanvas*);

This should take OffscreenCanvas&.

> Source/WebCore/html/canvas/PlaceholderRenderingContext.h:48
> +    OffscreenCanvas* m_offscreenCanvas;

Unclear how it’s safe to use a raw pointer here. What guarantees this object
won’t outlive the OffscreenCanvas?