Bug 78005 - [Qt] Control the lifetime of TiledBackingStores in WebGraphicsLayer.
Summary: [Qt] Control the lifetime of TiledBackingStores in WebGraphicsLayer.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jocelyn Turcotte
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-07 11:27 PST by Jocelyn Turcotte
Modified: 2012-02-09 06:39 PST (History)
5 users (show)

See Also:


Attachments
Patch (13.29 KB, patch)
2012-02-07 11:37 PST, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (12.60 KB, patch)
2012-02-08 04:57 PST, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (13.32 KB, patch)
2012-02-08 09:00 PST, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (12.84 KB, patch)
2012-02-09 03:37 PST, Jocelyn Turcotte
noam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jocelyn Turcotte 2012-02-07 11:27:47 PST
[Qt] Control the lifetime of TiledBackingStores in WebGraphicsLayer.
Comment 1 Jocelyn Turcotte 2012-02-07 11:37:31 PST
Created attachment 125886 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2012-02-07 13:16:24 PST
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
Comment 3 Noam Rosenthal 2012-02-07 15:31:21 PST
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.
Comment 4 Jocelyn Turcotte 2012-02-08 04:57:42 PST
Created attachment 126061 [details]
Patch

- extract updateTileBuffersRecursively removal to a different patch
- split comment in two lines
- iter -> it
Comment 5 Jocelyn Turcotte 2012-02-08 05:02:42 PST
(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.
Comment 6 Noam Rosenthal 2012-02-08 06:08:58 PST
> 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.
Comment 7 Noam Rosenthal 2012-02-08 06:19:54 PST
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.
Comment 8 Jocelyn Turcotte 2012-02-08 09:00:47 PST
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
Comment 9 Jocelyn Turcotte 2012-02-08 09:02:23 PST
(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.
Comment 10 Noam Rosenthal 2012-02-08 14:29:08 PST
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.
Comment 11 Jocelyn Turcotte 2012-02-09 03:37:45 PST
Created attachment 126274 [details]
Patch

Sounds right, moved it to updateContentsBuffers, it fits better there too.
Comment 12 Jocelyn Turcotte 2012-02-09 06:39:20 PST
Committed r107236: <http://trac.webkit.org/changeset/107236>