[webkit-reviews] review denied: [Bug 218401] [GPU Process] Flush canvas displayList from doAfterUpdateRendering : [Attachment 412791] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Rini Patel
<rini_patel at apple.com>'s request for review:
Bug 218401: [GPU Process] Flush canvas displayList from doAfterUpdateRendering
https://bugs.webkit.org/show_bug.cgi?id=218401

Attachment 412791: Patch

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




--- Comment #2 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 412791
  --> https://bugs.webkit.org/attachment.cgi?id=412791
Patch

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

> Source/WebCore/ChangeLog:8
> +
> +	   No new tests (OOPS!).

This needs some explanation for the change. Remove the "No new tests" line.

> Source/WebCore/html/HTMLCanvasElement.cpp:373
> +    if (m_context)
> +	   addObserver(document());

I don't think you should do this unconditionally. You should only do this when
GPU Process is active. In fact, you should only do this when GPU Process is
active, and drawing has occurred since the last flush.

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.h:310
> +    bool needsPreparationForDisplay() const final { return true; }

This can't be unconditional. You should only return true when using the GPU
backend. Ideally it would only return true if there is new drawing.