[webkit-reviews] review denied: [Bug 103215] Coordinated Graphics: Refactor code managing a backing store in LayerTreeRenderer. : [Attachment 175925] 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 103215: Coordinated Graphics: Refactor code managing a backing store in
LayerTreeRenderer.
https://bugs.webkit.org/show_bug.cgi?id=103215

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

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


> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:416
> +    if (graphicsLayer->drawsContent() && graphicsLayer->contentsAreVisible()
&& !graphicsLayer->size().isEmpty())

Put this in a static function layerShouldHaveBackingStore

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:419
> +    else if (getBackingStore(graphicsLayer))
> +	   removeBackingStore(graphicsLayer);

I would just have removeBackingStore return early if there is no backing store.


> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:427
> +    if (getBackingStore(graphicsLayer))
> +	   return;
> +
>      TextureMapperLayer* layer = toTextureMapperLayer(graphicsLayer);

I'd prefer it if this function didn't call getBackingStore, but returned early
on if layer->backingStore()

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:439
>      RefPtr<CoordinatedBackingStore> backingStore =
static_cast<CoordinatedBackingStore*>(layer->backingStore().get());
>      ASSERT(backingStore);

This is not needed.

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:465
> +    // LayerTreeRenderer::setLayerState removes a backing store.

Unnecessary comment.