[webkit-reviews] review denied: [Bug 104990] Coordinated Graphics: Manage the lifecycle of CoordinatedGraphicsLayer explicitly. : [Attachment 180485] 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 104990: Coordinated Graphics: Manage the lifecycle of
CoordinatedGraphicsLayer explicitly.
https://bugs.webkit.org/show_bug.cgi?id=104990

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

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


> Source/WebKit2/ChangeLog:12
> +	   Currently, Web Process does not send a layer creation message to UI
Process, so
> +	   LayerTreeRenderer creates GraphicsLayer lazily. We need to
deliberatively decide
> +	   when we can create layer lazily, and it is hard to understand the
lifecycle of
> +	   GraphicsLayer. This patch manages the lifecycle of layer explicitly,
and it
> +	   increases readability.

Send explicit commands to the UI process to create/delete compositing layers,
instead of having the UI process decide lazily when to create them.
Avoid creating a compositing layer at all if it was deleted in the same cycle.

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:321
> +    layer->setReplicatedByLayer((layerInfo.replica !=
InvalidCoordinatedLayerID) ? layerByID(layerInfo.replica) : 0);
> +    layer->setMaskLayer((layerInfo.mask != InvalidCoordinatedLayerID) ?
layerByID(layerInfo.mask) : 0);

This is really verbose. I liked the old way better.

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.h:126
> +    WebCore::GraphicsLayer* layerByID(CoordinatedLayerID id)
> +    {
> +	   ASSERT(m_layers.contains(id));
> +	   ASSERT(id != InvalidCoordinatedLayerID);
> +	   return m_layers.get(id);

Why are you changing this function? It's not more readable.