Bug 78005 – [Qt] Control the lifetime of TiledBackingStores in WebGraphicsLayer.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jocelyn Turcotte]

[Qt] Control the lifetime of TiledBackingStores in WebGraphicsLayer.

[Jocelyn Turcotte]

Created attachment 125886 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 125886 [details]
Patch

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

> Source/WebKit2/ChangeLog:15
> +        - Remove the unused updateTileBuffersRecursively

Should really be another patch

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:106
> +    if (m_webGraphicsLayerClient) {
> +        purgeBackingStores();
>          m_webGraphicsLayerClient->releaseLayer(this);
> +    }

I noticed today that going to google.com and then to pol.dk and scrolling sidewards would show old google.com contents below. Do yo know whether this patches fixes that?

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:589
> +    // This is the only place we (re)create the main tiled backing store, once we have a remote client and we are ready to send our data to the UI process.

Please split in two lines

> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:450
> +    for (HashSet<WebCore::WebGraphicsLayer*>::iterator iter = m_registeredLayers.begin(); iter != end; ++iter)
> +        (*iter)->purgeBackingStores();

Switch to 'it' when you are at it

[Noam Rosenthal]

Comment on attachment 125886 [details]
Patch

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

Please fix/respond to comments from me and Kenneth :)

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:348
> +
> +    m_mainBackingStore.clear();
> +    m_previousBackingStore.clear();
> +

I think a layer can potentially have both regular contents and an image; so this wouldn't work.

[Jocelyn Turcotte]

Created attachment 126061 [details]
Patch

- extract updateTileBuffersRecursively removal to a different patch
- split comment in two lines
- iter -> it

[Jocelyn Turcotte]

(In reply to comment #2)
> I noticed today that going to google.com and then to pol.dk and scrolling sidewards would show old google.com contents below. Do yo know whether this patches fixes that?

From a broad guess I think that this is caused by the m_previousBackingStore that we used to clear as soon as the main backing store had enough content to fill the viewport. Now we don't clear it anymore (exept when we re-scale and replace it with another one).

(In reply to comment #3)
> I think a layer can potentially have both regular contents and an image; so this wouldn't work.

I might be missing something, but the code I've read around RenderLayerBacking::isDirectlyCompositedImage seems to suggest that we can assume that a layer won't have both.
So drawsContent() == true would mean that we either have containsPaintedContent() == true OR isDirectlyCompositedImage() == true.

[Noam Rosenthal]

> I might be missing something, but the code I've read around RenderLayerBacking::isDirectlyCompositedImage seems to suggest that we can assume that a layer won't have both.
> So drawsContent() == true would mean that we either have containsPaintedContent() == true OR isDirectlyCompositedImage() == true.

Right, but in that case drawsContent() would be changed and we'd purge the backing-store anyway... so this is redundant.

[Noam Rosenthal]

Comment on attachment 126061 [details]
Patch

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

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:347
> +    m_mainBackingStore.clear();
> +    m_previousBackingStore.clear();

I think we should do this as a result of setDrawsContent(false), rather than in setContentsToImage.
Even though RenderLayerBacking doesn't allow for layers with both an image and regular content, it does allow, for example, canvas and regular content. So this feels more like it's based on a current implementation choice, rather than an API contract.

[Jocelyn Turcotte]

Created attachment 126094 [details]
Patch

I thought that drawsContent() was also true when we had a directly composited image.
- Cleared the backing stores in setDrawsContent(false)
- Moved the early return for if not drawsContent under the image case in updateContentsBuffers
- Removed the check for drawsContent in tiledBackingStoreContentsRect since we won't have a backing store in that case

[Jocelyn Turcotte]

(In reply to comment #8)
> I thought that drawsContent() was also true when we had a directly composited image.
But it's not the case, just to clarify.

[Noam Rosenthal]

Comment on attachment 126094 [details]
Patch

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

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:253
>  void WebGraphicsLayer::setDrawsContent(bool b)
>          return;
>      GraphicsLayer::setDrawsContent(b);
>  
> -    if (b)
> -        setNeedsDisplay();
> +    if (!b) {
> +        m_mainBackingStore.clear();
> +        m_previousBackingStore.clear();
> +    }
> +    // The next call to updateContentBuffers will recreate the backing store if needed.
>      notifyChange();
>  }

Looking at this again, this shouldn't happen here, but rather in syncCompositingStateForThisLayer. 
That's because JS might change the drawsContent state of the layer back and forth and we don't want to be too eager about it.

[Jocelyn Turcotte]

Created attachment 126274 [details]
Patch

Sounds right, moved it to updateContentsBuffers, it fits better there too.

[Jocelyn Turcotte]

Committed r107236: <http://trac.webkit.org/changeset/107236>