Bug 103215 – Coordinated Graphics: Refactor code managing a backing store in LayerTreeRenderer.

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Layout and Rendering

[Dongseong Hwang]

This patch makes LayerTreeRenderer assign a content backing store to
TextureMapperLayer only in setLayerState(). Currently, createTile() can assign
the backing store to TextureMapperLayer and it can break the invariant condition
of TextureMapperLayer: TextureMapperLayer can have its own backing store only if
the layer has following conditions: drawsContent, contentsVisible and non empty
size.

In addition, the modified code about creating and removing a backing store
matches the same purpose code of CoordinatedGraphicsLayer and
GraphicsLayerTextureMapepr.

[Dongseong Hwang]

Created attachment 175925 [details]
Patch

[Noam Rosenthal]

Comment on attachment 175925 [details]
Patch

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.

[Dongseong Hwang]

Created attachment 175929 [details]
Patch

[Dongseong Hwang]

(In reply to comment #2)

Thanks for review!

> (From update of attachment 175925 [details])
> 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
Done.
> 
> > 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.
Done.
> 
> > 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()
Done.
> 
> > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:439
> >      RefPtr<CoordinatedBackingStore> backingStore = static_cast<CoordinatedBackingStore*>(layer->backingStore().get());
> >      ASSERT(backingStore);
> 
> This is not needed.
You're right! Done.
> 
> > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:465
> > +    // LayerTreeRenderer::setLayerState removes a backing store.
> 
> Unnecessary comment.
Done.

[Noam Rosenthal]

Comment on attachment 175929 [details]
Patch

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

> Source/WebKit2/ChangeLog:39
> +            if needed. Moreover, we must sync a layer state prior to create a

create -> creating.
Also lines are too short...

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:422
> +    if (!shouldHaveBackingStore && getBackingStore(graphicsLayer)) {

The call to getBackingStore(graphicsLayer) is redundant.

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:427
> +    if (shouldHaveBackingStore)

This is not really needed if the above call is redundant.

[Dongseong Hwang]

Created attachment 175935 [details]
Patch

[Dongseong Hwang]

(In reply to comment #5)
> (From update of attachment 175929 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175929&action=review
> 
> > Source/WebKit2/ChangeLog:39
> > +            if needed. Moreover, we must sync a layer state prior to create a
> 
> create -> creating.
> Also lines are too short...
Do you mean writing more words in lines? If so, done.
> 
> > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:422
> > +    if (!shouldHaveBackingStore && getBackingStore(graphicsLayer)) {
> 
> The call to getBackingStore(graphicsLayer) is redundant.
Ok, Done.
> 
> > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:427
> > +    if (shouldHaveBackingStore)
> 
> This is not really needed if the above call is redundant.
Ok, Done.

[Dongseong Hwang]

Created attachment 175937 [details]
Patch

[Dongseong Hwang]

(In reply to comment #8)
> Created an attachment (id=175937) [details]
> Patch

post for running ews.

[Noam Rosenthal]

Comment on attachment 175937 [details]
Patch

LGTM, but EWS is purple so it probably doesn't apply...

[Dongseong Hwang]

Created attachment 176063 [details]
Patch

[Dongseong Hwang]

(In reply to comment #10)
> (From update of attachment 175937 [details])
> LGTM, but EWS is purple so it probably doesn't apply...

Thanks for review!

I don't know why ews is purlple. I tried again.

[Dongseong Hwang]

(In reply to comment #12)
> (In reply to comment #10)
> > (From update of attachment 175937 [details] [details])
> > LGTM, but EWS is purple so it probably doesn't apply...
> 
> Thanks for review!
> 
> I don't know why ews is purlple. I tried again.

EWS is purple too..

I think this bug has some problems...

I'll make a duplicated new bug.

[Dongseong Hwang]

*** This bug has been marked as a duplicate of bug 103298 ***