Bug 105458 - [chromium] Add ContinuousPainter to call setNeedsDisplay on all layers recursively in continuous painting mode
Summary: [chromium] Add ContinuousPainter to call setNeedsDisplay on all layers recurs...
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: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-19 13:22 PST by egraether
Modified: 2013-01-11 17:59 PST (History)
8 users (show)

See Also:


Attachments
Patch (3.11 KB, patch)
2012-12-19 13:29 PST, egraether
no flags Details | Formatted Diff | Diff
Patch (3.22 KB, patch)
2012-12-19 19:31 PST, egraether
no flags Details | Formatted Diff | Diff
Patch (4.10 KB, patch)
2013-01-07 14:05 PST, egraether
no flags Details | Formatted Diff | Diff
Patch (4.73 KB, patch)
2013-01-08 10:03 PST, egraether
no flags Details | Formatted Diff | Diff
Patch (10.08 KB, patch)
2013-01-10 11:56 PST, egraether
no flags Details | Formatted Diff | Diff
Patch (10.52 KB, patch)
2013-01-11 15:08 PST, egraether
no flags Details | Formatted Diff | Diff
Patch (11.01 KB, patch)
2013-01-11 16:37 PST, egraether
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description egraether 2012-12-19 13:22:33 PST
[chromium] Pass the overlayLayer to WebLayerTreeView
Comment 1 egraether 2012-12-19 13:29:18 PST
Created attachment 180217 [details]
Patch
Comment 2 egraether 2012-12-19 19:31:00 PST
Created attachment 180263 [details]
Patch
Comment 3 egraether 2012-12-19 19:33:42 PST
James, can you review you this please?
Comment 4 WebKit Review Bot 2012-12-19 21:05:47 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 5 James Robinson 2013-01-02 13:55:42 PST
Comment on attachment 180263 [details]
Patch

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

> Source/Platform/ChangeLog:9
> +        WebLayerTreeView. Thereon this layer can be identified in the layer tree.

typo "Thereon". This doesn't explain why the page overlay layer needs to be treated specially

> Source/Platform/chromium/public/WebLayerTreeView.h:196
> +    // Passes a reference of the PageOverlay's layer to exclude it from continuous painting.
> +    // That way it doesn't affect the totalPaintTime measurement.
> +    virtual void setPageOverlayLayer(const WebLayer*) { }

Is this really necessary?  For general performance measurements there won't be a page overlay layer at all, but if there is some case where the page overlay is typically visible then it's something the user sees and should be counted with all other content.
Comment 6 Nat Duca 2013-01-03 12:11:34 PST
@egraether, lets try the approach where we add a 

  WebLayer::setIgnoresForceRedrawMode(bool)

And propagate that through to the redrawing system.
Comment 7 Nat Duca 2013-01-03 12:11:52 PST
(pick a better name, though... I forget what we've called this cc-side haha)
Comment 8 egraether 2013-01-07 14:05:20 PST
Created attachment 181560 [details]
Patch
Comment 9 James Robinson 2013-01-07 17:39:22 PST
Comment on attachment 181560 [details]
Patch

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

In talking with Nat it came up that rather than exposing per-layer bits and spreading logic for what layers have what behavior in this mode, it might make more sense to to have all of the continuous painting logic dependent on specific layers driven completely by the WebKit side.  Then any changes in what things you want to include or exclude can be made purely in WebKit.  You can see an example of code that iterates through GraphicsLayers to do things in WebViewBenchmarkSupportImpl.cpp (although I suspect that particular file is on its way out, but it's not much code).  If you handled the invalidations in that way instead of doing them in cc/layer_tree_host.cc, you could decide to invalidate or not invalidate layers based on any information available in WebKit.  If it turns out that there isn't too much code spread around then having it split across cc/ and WebKit might be OK, but it will make iterating on it more difficult.

What do you think, Eberhard?

> Source/Platform/chromium/public/WebLayer.h:211
> +    virtual void setIgnoresContinuousPainting(bool) { };

no trailing ; here
Comment 10 egraether 2013-01-08 10:03:29 PST
Created attachment 181704 [details]
Patch
Comment 11 egraether 2013-01-08 10:06:24 PST
(In reply to comment #9)
> (From update of attachment 181560 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181560&action=review
> 
> In talking with Nat it came up that rather than exposing per-layer bits and spreading logic for what layers have what behavior in this mode, it might make more sense to to have all of the continuous painting logic dependent on specific layers driven completely by the WebKit side.  Then any changes in what things you want to include or exclude can be made purely in WebKit.  You can see an example of code that iterates through GraphicsLayers to do things in WebViewBenchmarkSupportImpl.cpp (although I suspect that particular file is on its way out, but it's not much code).  If you handled the invalidations in that way instead of doing them in cc/layer_tree_host.cc, you could decide to invalidate or not invalidate layers based on any information available in WebKit.  If it turns out that there isn't too much code spread around then having it split across cc/ and WebKit might be OK, but it will make iterating on it more difficult.
> 
> What do you think, Eberhard?

Makes sense. I uploaded a new patch with a working draft, please have a look.
Comment 12 James Robinson 2013-01-08 12:32:01 PST
Comment on attachment 181704 [details]
Patch

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

Seems pretty reasonable to me.  I think it'd be a good idea to try to not add all the new code to WebViewImpl but instead have it off in a separate class that WebViewImpl owns and manages, just to keep the code size down.  Nat - WDYT?

> Source/WebKit/chromium/src/WebViewImpl.cpp:1776
> +    Vector<GraphicsLayer*>::const_iterator it;
> +    for (it = children.begin(); it != children.end(); ++it)
> +        setNeedsDisplayOnAllGraphicLayersRecursive(*it);

FYI if you want to hit mask and replica layers you have to do that explicitly as well with something like

if (GraphicsLayer* maskLayer = layer->maskLayer())
  maskLayer->fooBar();

... ditto for replicaLayer
Comment 13 egraether 2013-01-09 09:32:10 PST
(In reply to comment #12)
> (From update of attachment 181704 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181704&action=review
> 
> Seems pretty reasonable to me.  I think it'd be a good idea to try to not add all the new code to WebViewImpl but instead have it off in a separate class that WebViewImpl owns and manages, just to keep the code size down.  Nat - WDYT?

Having a separate file for this one method seems a bit overdone in my opinion. Or do you think there will be other stuff added? What would you call this class?

What about adding the method to the GraphicsLayer instead:
GraphicsLayer::setNeedsDisplayForContinuousPaintingRecursive()

with a member and method to ignore the overlay's layer:
GraphicsLayer::setIgnoresContinuousPainting()
Comment 14 Nat Duca 2013-01-09 12:26:46 PST
No strong opinions, but steer clear of graphics layer.
Comment 15 James Robinson 2013-01-09 19:34:19 PST
(In reply to comment #14)
> No strong opinions, but steer clear of graphics layer.

+1 to that
Comment 16 egraether 2013-01-10 11:56:16 PST
Created attachment 182184 [details]
Patch
Comment 17 egraether 2013-01-10 11:58:03 PST
Uploaded a new draft with the separate class taking care of continuous painting. What do you think?
Comment 18 James Robinson 2013-01-10 13:28:52 PST
Comment on attachment 182184 [details]
Patch

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

Seems reasonable

> Source/WebKit/chromium/src/painting/ContinuousPainter.h:8
> + *     * Redistributions of source code must retain the above copyright

new files should use the 2-clause license header

> Source/WebKit/chromium/src/painting/ContinuousPainter.h:44
> +    explicit ContinuousPainter();

no explicit for zero-argument constructors

> Source/WebKit/chromium/src/painting/ContinuousPainter.h:47
> +    void setExceptionLayer(WebCore::GraphicsLayer*);

Please document what this needs.

We support having multiple overlay layers, do you want to exclude them all?
Comment 19 egraether 2013-01-11 15:08:50 PST
Created attachment 182430 [details]
Patch
Comment 20 egraether 2013-01-11 15:20:07 PST
(In reply to comment #18)
> (From update of attachment 182184 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182184&action=review
> 
> Seems reasonable
> 
> > Source/WebKit/chromium/src/painting/ContinuousPainter.h:8
> > + *     * Redistributions of source code must retain the above copyright
> 
> new files should use the 2-clause license header
> 
> > Source/WebKit/chromium/src/painting/ContinuousPainter.h:44
> > +    explicit ContinuousPainter();
> 
> no explicit for zero-argument constructors
> 
> > Source/WebKit/chromium/src/painting/ContinuousPainter.h:47
> > +    void setExceptionLayer(WebCore::GraphicsLayer*);
> 
> Please document what this needs.
> 
> We support having multiple overlay layers, do you want to exclude them all?

I found out that WebViewImpl::setOverlayLayer() is called for all overlays so the old patch wouldn't work.

The new patch excludes all overlay layers instead. I made the recursive function static now, because it doesn't have any state. Please have a look.
Comment 21 James Robinson 2013-01-11 15:22:01 PST
Comment on attachment 182430 [details]
Patch

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

> Source/WebKit/chromium/src/painting/ContinuousPainter.cpp:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

2013 for new files, please

> Source/WebKit/chromium/src/painting/ContinuousPainter.h:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

2013
Comment 22 egraether 2013-01-11 16:37:20 PST
Created attachment 182445 [details]
Patch
Comment 23 WebKit Review Bot 2013-01-11 17:59:34 PST
Comment on attachment 182445 [details]
Patch

Clearing flags on attachment: 182445

Committed r139531: <http://trac.webkit.org/changeset/139531>
Comment 24 WebKit Review Bot 2013-01-11 17:59:40 PST
All reviewed patches have been landed.  Closing bug.