[webkit-reviews] review granted: [Bug 208597] Canvas drawing commands have to be flushed to the GPUProcess in batches : [Attachment 392479] Patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 208597: Canvas drawing commands have to be flushed to the GPUProcess in
batches
https://bugs.webkit.org/show_bug.cgi?id=208597

Attachment 392479: Patch

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




--- Comment #3 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 392479
  --> https://bugs.webkit.org/attachment.cgi?id=392479
Patch

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

Elegant.

> Source/WebCore/platform/graphics/displaylists/DisplayListObserver.h:31
> +class Observer {

Calling this by the general name DisplayList::Observer seems misleading as it
really only observes the recorder. Please consider either giving it a longer
name, or making it an inner class (DisplayList::Recorder::Observer).

Personally, I prefer DisplayList::Recorder::Observer because it keeps the two
classes close together, rather than being spread to two separate files.

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBuffer.h:85
> +	      
RemoteImageBufferMessageHandler::flushDrawingContextAndWaitCommit(displayList);

I like the design of this better than what I was doing in
https://bugs.webkit.org/show_bug.cgi?id=208560. I've updated that patch to
match this design.

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBuffer.h:94
> +	   if (displayList.itemCount() >= DisplayListBatchSize) {

This doesn't really matter much, but shouldn't this condition be checked after
the item gets appended? The action of appending the item is the trigger which
can cause this condition to become true.