Bug 105458 – [chromium] Add ContinuousPainter to call setNeedsDisplay on all layers recursively in continuous painting mode

• Component: New Bugs
• Status: RESOLVED FIXED

[chromium] Pass the overlayLayer to WebLayerTreeView

Created attachment 180217 [details]
Patch

Created attachment 180263 [details]
Patch

James, can you review you this please?

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[James Robinson]

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.

[Nat Duca]

@egraether, lets try the approach where we add a 

  WebLayer::setIgnoresForceRedrawMode(bool)

And propagate that through to the redrawing system.

[Nat Duca]

(pick a better name, though... I forget what we've called this cc-side haha)

Created attachment 181560 [details]
Patch

[James Robinson]

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

Created attachment 181704 [details]
Patch

(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.

[James Robinson]

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

(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()

[Nat Duca]

No strong opinions, but steer clear of graphics layer.

[James Robinson]

(In reply to comment #14)
> No strong opinions, but steer clear of graphics layer.

+1 to that

Created attachment 182184 [details]
Patch

Uploaded a new draft with the separate class taking care of continuous painting. What do you think?

[James Robinson]

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?

Created attachment 182430 [details]
Patch

(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.

[James Robinson]

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

Created attachment 182445 [details]
Patch

[WebKit Review Bot]

Comment on attachment 182445 [details]
Patch

Clearing flags on attachment: 182445

Committed r139531: <http://trac.webkit.org/changeset/139531>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.