[webkit-reviews] review denied: [Bug 108294] Coordinated Graphics: CoordinatedGraphicsLayer makes CoordinatedGraphicsScene perform via CoordinatedGraphicsOperation. : [Attachment 186016] Patch

[bugzilla-daemon at webkit.org]

Noam Rosenthal <noam at webkit.org> has denied Huang Dongsung
<luxtella at company100.net>'s request for review:
Bug 108294: Coordinated Graphics: CoordinatedGraphicsLayer makes
CoordinatedGraphicsScene perform via CoordinatedGraphicsOperation.
https://bugs.webkit.org/show_bug.cgi?id=108294

Attachment 186016: Patch
https://bugs.webkit.org/attachment.cgi?id=186016&action=review

------- Additional Comments from Noam Rosenthal <noam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=186016&action=review


Have some comments, but I think it's a good direction. More input from others
welcome...

>
Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h:
48
> +class CoordinatedGraphicsLayerClient : public
CoordinatedGraphicsOperation::Queue {

I wouldn't use inheritance here.
Instead, CoordinatedGraphicsLayerClient should have a function that returns a
queue.

>
Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsOperatio
n.h:59
> +	   CreateLayersType,
> +	   DeleteLayersType,
> +	   SetRootLayerType,
> +	   SetLayerStateType,
> +	   SetLayerChildrenType,
> +	   CreateTileType,
> +	   UpdateTileType,
> +	   RemoveTileType,
> +	   CreateUpdateAtlasType,
> +	   RemoveUpdateAtlasType,

...
Let's use caps here, CREATE_LAYERS etc., like FilterOperation and
TransformOperation. Then you also don't need the word Type everywhere.

>
Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsOperatio
n.h:83
> +	   NoneType

I don't see the point of NoneType.

>
Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsOperatio
n.h:86
> +    class Operation : public RefCounted<Operation> {

I don't see the point of this inner class... This virtual class can be part of
CoordinatedGraphicsOperation, and the subclasses can be inner classes.
Also, it has to be ThreadSafeRefCounted.

>
Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.
cpp:614
> +void
CoordinatedLayerTreeHost::enqueueCoordinatedGraphicsOperation(PassRefPtr<Coordi
natedGraphicsOperation::Operation> operation)

I think this should be divided to two functions. One called
willEnqueueCoordinatedGraphicsOperation, that handles your switch statement and
is called from this function. This function should only call willEnqueue, and
then append to the vector.