Summary: | [chromium] Add ContinuousPainter to call setNeedsDisplay on all layers recursively in continuous painting mode | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | egraether | ||||||||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, egraether, fishd, jamesr, nduca, tkent+wkapi, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
egraether
2012-12-19 13:22:33 PST
Created attachment 180217 [details]
Patch
Created attachment 180263 [details]
Patch
James, can you review you this please? 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 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. @egraether, lets try the approach where we add a WebLayer::setIgnoresForceRedrawMode(bool) And propagate that through to the redrawing system. (pick a better name, though... I forget what we've called this cc-side haha) Created attachment 181560 [details]
Patch
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. 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() No strong opinions, but steer clear of graphics layer. (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? 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. 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
Comment on attachment 182445 [details] Patch Clearing flags on attachment: 182445 Committed r139531: <http://trac.webkit.org/changeset/139531> All reviewed patches have been landed. Closing bug. |