Certain settings (e.g., enabling per-tile painting) could be made global. To accomplish this, CCSettings should be moved to its own file, the current CCSettings should be renamed CCLayerTreeSettings and retain only those settings that may vary from tab to tab. This will depend on a chromium patch to prepare for the API breakage.
+1
Created attachment 145911 [details] Patch
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 145911 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145911&action=review > Source/WebCore/platform/graphics/chromium/cc/CCSettings.cpp:53 > +static const IntSize defaultTileSizeDefault = IntSize(256, 256); > +static IntSize s_defaultTileSize = defaultTileSizeDefault; This smells like a static initializer - is it? If so that's a no-no. Perhaps we could use a lazy singleton pattern instead? > Source/WebKit/chromium/src/WebViewImpl.cpp:3483 > + CCSettings::setAcceleratedPaintingEnabled(page()->settings()->acceleratedDrawingEnabled()); I think this is (slightly) wrong - we shouldn't have every view clobbering the global settings. Since this is exposed through WebKit API the embedder (chromium) should be making these decisions based on command line flags or whatnot before we even get to here.
Created attachment 146086 [details] Patch (In reply to comment #4) > (From update of attachment 145911 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145911&action=review > > > Source/WebCore/platform/graphics/chromium/cc/CCSettings.cpp:53 > > +static const IntSize defaultTileSizeDefault = IntSize(256, 256); > > +static IntSize s_defaultTileSize = defaultTileSizeDefault; > > This smells like a static initializer - is it? If so that's a no-no. Perhaps we could use a lazy singleton pattern instead? *blush* Fixed. > > > Source/WebKit/chromium/src/WebViewImpl.cpp:3483 > > + CCSettings::setAcceleratedPaintingEnabled(page()->settings()->acceleratedDrawingEnabled()); > > I think this is (slightly) wrong - we shouldn't have every view clobbering the global settings. Since this is exposed through WebKit API the embedder (chromium) should be making these decisions based on command line flags or whatnot before we even get to here. Fixed.
Comment on attachment 146086 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146086&action=review Getting there! > Source/WebCore/platform/graphics/chromium/cc/CCSettings.cpp:113 > + MutexLocker lock(m_mutex); this is the suck :( we query settings all the time, we shouldn't be taking a mutex hit each one! i would define the interface as saying that the embedder is responsible for initializing these settings before the compositor starts running and they have to take care of the appropriate happens-before relationships. one easy way to do this is to set everything before calling WebCompositor::initialize(). then you don't need any internal locking > Source/WebCore/platform/graphics/chromium/cc/CCSettings.cpp:199 > + if (s_instance) looking at it more this seems a bit silly. to avoid static initializers, we just need to avoid having any non-POD globals. the only settings we have are bools, ints, and IntSizes. if we stored the IntSize as pairs of ints and converted them to IntSize at call time then they could all live as statics without any issue > Source/WebKit/chromium/public/WebCompositor.h:55 > + WEBKIT_EXPORT static bool acceleratedPaintingEnabled(); you need to document the rules for these settings. when can they be set? how long are they valid for? what is their relationship with initialize() / shutdown() ? why do we have getters on these? does anyone use the WebCompositor interface for anything other than setting? > Source/WebKit/chromium/src/WebCompositorImpl.cpp:51 > + CCSettings::initialize(); > WebCompositorImpl::initialize(implThread); it's unfortunate to require both
(In reply to comment #6) > > Source/WebCore/platform/graphics/chromium/cc/CCSettings.cpp:113 > > + MutexLocker lock(m_mutex); > i would define the interface as saying that the embedder is responsible for initializing these settings before the compositor starts running and they have to take care of the appropriate happens-before relationships. one easy way to do this is to set everything before calling WebCompositor::initialize(). then you don't need any internal locking This is where having the public methods that affect CCSettings being on WebCompositor would be helpful.... that way you can basically say "these can only be set before you call the initialize function."
(In reply to comment #6) > (From update of attachment 146086 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=146086&action=review > > Getting there! > > > Source/WebCore/platform/graphics/chromium/cc/CCSettings.cpp:113 > > + MutexLocker lock(m_mutex); > > this is the suck :( we query settings all the time, we shouldn't be taking a mutex hit each one! > > i would define the interface as saying that the embedder is responsible for initializing these settings before the compositor starts running and they have to take care of the appropriate happens-before relationships. one easy way to do this is to set everything before calling WebCompositor::initialize(). then you don't need any internal locking > Looking at render_thread_impl.cc, we lazily initialize the WebCompositor in several spots, but we only have the preferences we need in order to initialize the global settings in one of them (when we create a view). This approach would require us to guarantee that the first time we try to initialize the WebCompositor is when we create a view which, given the lazy-initialization approach, is not currently not the case. We _can_ guarantee that the settings are in place before the creation of the first view. Would that be ok? > > Source/WebCore/platform/graphics/chromium/cc/CCSettings.cpp:199 > > + if (s_instance) > > looking at it more this seems a bit silly. to avoid static initializers, we just need to avoid having any non-POD globals. the only settings we have are bools, ints, and IntSizes. if we stored the IntSize as pairs of ints and converted them to IntSize at call time then they could all live as statics without any issue Sounds good. I will do this. > > > Source/WebKit/chromium/public/WebCompositor.h:55 > > + WEBKIT_EXPORT static bool acceleratedPaintingEnabled(); > > you need to document the rules for these settings. when can they be set? how long are they valid for? what is their relationship with initialize() / shutdown() ? Initialize and shutdown will be going away. The rules will be -- you can only start using the getters once you're done with the setters. And you can only use the setters on the main thread. Should I adjust the code to enforce this? > > why do we have getters on these? does anyone use the WebCompositor interface for anything other than setting? Will remove these. > > > Source/WebKit/chromium/src/WebCompositorImpl.cpp:51 > > + CCSettings::initialize(); > > WebCompositorImpl::initialize(implThread); > > it's unfortunate to require both CCSettings::initialize() should be going away.
Created attachment 146191 [details] Patch (In reply to comment #6) > (From update of attachment 146086 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=146086&action=review > > Getting there! > > > Source/WebCore/platform/graphics/chromium/cc/CCSettings.cpp:113 > > + MutexLocker lock(m_mutex); > > this is the suck :( we query settings all the time, we shouldn't be taking a mutex hit each one! > > i would define the interface as saying that the embedder is responsible for initializing these settings before the compositor starts running and they have to take care of the appropriate happens-before relationships. one easy way to do this is to set everything before calling WebCompositor::initialize(). then you don't need any internal locking Done. > > > Source/WebCore/platform/graphics/chromium/cc/CCSettings.cpp:199 > > + if (s_instance) > > looking at it more this seems a bit silly. to avoid static initializers, we just need to avoid having any non-POD globals. the only settings we have are bools, ints, and IntSizes. if we stored the IntSize as pairs of ints and converted them to IntSize at call time then they could all live as statics without any issue Fixed. > > > Source/WebKit/chromium/public/WebCompositor.h:55 > > + WEBKIT_EXPORT static bool acceleratedPaintingEnabled(); > > you need to document the rules for these settings. when can they be set? how long are they valid for? what is their relationship with initialize() / shutdown() ? Done. > > why do we have getters on these? does anyone use the WebCompositor interface for anything other than setting? Removed. > > > Source/WebKit/chromium/src/WebCompositorImpl.cpp:51 > > + CCSettings::initialize(); > > WebCompositorImpl::initialize(implThread); > > it's unfortunate to require both Removed.
Comment on attachment 146191 [details] Patch Attachment 146191 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12919131
Comment on attachment 146191 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146191&action=review Still have one static initializer and this doesn't appear to quite compile, but otherwise I think this is looking good. R- for now since we need another round but it's close. > Source/WebCore/platform/graphics/chromium/cc/CCSettings.cpp:45 > +static size_t s_maxPartialTextureUpdatesDefault = std::numeric_limits<size_t>::max(); hey guess what this is! sadly, it's a static initializer. If you're on linux here's a way to test things like this: $ cat test.cc #include <limits> int mybar = std::numeric_limits<int>::max(); int main() { return 0; } jamesr@jamesr:~$ g++ test.cc -o test -O2 jamesr@jamesr:~$ /path/to/chromium/checkout/src/tools/linux/dump-static-initializers.py test mybar (initializer offset 0x4005d0 size 0xb) mybar Found 1 static initializers in 1 files. works with clang as well. I'd suggest just a really big number or perhaps the slightly uglier static_cast<size_t>(-1); > Source/WebCore/platform/graphics/chromium/cc/CCSettings.h:35 > + // Must be called on the main thread before WebCompostor::initialize. Typo: Compostor. We aren't turning layers into manure here :D I think having one comment describing this would make more sense - and I'd also group all the setters together at the bottom since most people looking at this It's a bit of a (conceptual) layering violation to have this refer to WebCompositor, that's a WebKit client API. I think the comment in WebCompositor.h is sufficient - alternately you could describe the invariants here in terms of other CC concepts > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:639 > + // For these tests, we will enable threaded animations. > + WebCompositor::setAcceleratedAnimationEnabled(true); For all of these tests? Is this a change in behavior, or just shuffling logic around? > Source/WebKit/chromium/tests/CCTestCommon.h:33 > +// If you have a test that modifies or uses global settings, keep an instance > +// of this class to ensure that you start and end with a clean slate. Could we instead have WebCompositor::shutdown() reset the settings? Tests that depend on these things should hopefully be calling ::initialize()/::shutdown() already - right?
Created attachment 146558 [details] Patch (In reply to comment #11) > (From update of attachment 146191 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=146191&action=review > > Still have one static initializer and this doesn't appear to quite compile, but otherwise I think this is looking good. R- for now since we need another round but it's close. > > > Source/WebCore/platform/graphics/chromium/cc/CCSettings.cpp:45 > > +static size_t s_maxPartialTextureUpdatesDefault = std::numeric_limits<size_t>::max(); > > hey guess what this is! sadly, it's a static initializer. If you're on linux here's a way to test things like this: > > $ cat test.cc > #include <limits> > > int mybar = std::numeric_limits<int>::max(); > > int main() { > return 0; > } > jamesr@jamesr:~$ g++ test.cc -o test -O2 > jamesr@jamesr:~$ /path/to/chromium/checkout/src/tools/linux/dump-static-initializers.py test > mybar (initializer offset 0x4005d0 size 0xb) > mybar > > Found 1 static initializers in 1 files. > > > works with clang as well. > > I'd suggest just a really big number or perhaps the slightly uglier static_cast<size_t>(-1); D'oh! Fixed. (Thanks for the tip, too). > > > Source/WebCore/platform/graphics/chromium/cc/CCSettings.h:35 > > + // Must be called on the main thread before WebCompostor::initialize. > > Typo: Compostor. We aren't turning layers into manure here :D > > I think having one comment describing this would make more sense - and I'd also group all the setters together at the bottom since most people looking at this > > It's a bit of a (conceptual) layering violation to have this refer to WebCompositor, that's a WebKit client API. I think the comment in WebCompositor.h is sufficient - alternately you could describe the invariants here in terms of other CC concepts I've updated the comment, but I think I've noticed a problem. in CCLayerTreeHost::initializeLayerRenderer, we update the global settings based on capabilities we get from the layer renderer. This could, I think, happen more than once per process. Is this an issue? If so, do you think it would be better if CCSettings held the 'requested' settings, and CCLayerTreeSettings held the 'effective' settings (based on the real capabilities of the machine)? > > > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:639 > > + // For these tests, we will enable threaded animations. > > + WebCompositor::setAcceleratedAnimationEnabled(true); > > For all of these tests? Is this a change in behavior, or just shuffling logic around? This is not a behavior change -- I just moved the spot where I applied the setting. > > > Source/WebKit/chromium/tests/CCTestCommon.h:33 > > +// If you have a test that modifies or uses global settings, keep an instance > > +// of this class to ensure that you start and end with a clean slate. > > Could we instead have WebCompositor::shutdown() reset the settings? Tests that depend on these things should hopefully be calling ::initialize()/::shutdown() already - right? Done. This won't work for all the tests, though. Some tests do use the settings, but don't initialize or shut down the WebCompositor. E.g., LayerRendererChromiumTest2.initializationDoesNotMakeSynchronousCalls.
Comment on attachment 146558 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146558&action=review > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:147 > + // Update settings based on capabilities that we got back from the renderer. Ah yes, I think this is a (potential) problem - these capabilities are specific to the context used by this compositor instance, so I think it's (slightly) wrong to have them change global settings. I say slightly because it would only matter if different contexts had different capabilities - it's really hard to think of a case where that would happen. I could imagine these changing over time - for instance, on a dual-GPU system if we switched from one to the other I could imagine the maximum texture size changing. Today if that happens we lose all contexts on the "old" device and then have to reinitialize them all. Perhaps some of these really should be CCLayerTreeSettings - such as usingAcceleratedPainting, maxPartialTextureUpdates, maxUntiledLayerSize. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:150 > + CCSettings::setDefaultTileSize(IntSize(min(CCSettings::defaultTileSize().width(), m_proxy->layerRendererCapabilities().maxTextureSize), It's a bit strange to set the default value based on a capability - but maybe this setting is just poorly named? I think we can safely assume that the max texture size will always be >=2k since that's required by DX9, so maybe we don't need this?
Created attachment 146666 [details] Patch (In reply to comment #13) > (From update of attachment 146558 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=146558&action=review > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:147 > > + // Update settings based on capabilities that we got back from the renderer. > > Ah yes, I think this is a (potential) problem - these capabilities are specific to the context used by this compositor instance, so I think it's (slightly) wrong to have them change global settings. I say slightly because it would only matter if different contexts had different capabilities - it's really hard to think of a case where that would happen. > > I could imagine these changing over time - for instance, on a dual-GPU system if we switched from one to the other I could imagine the maximum texture size changing. Today if that happens we lose all contexts on the "old" device and then have to reinitialize them all. > > Perhaps some of these really should be CCLayerTreeSettings - such as usingAcceleratedPainting, maxPartialTextureUpdates, maxUntiledLayerSize. That sounds safe. I've made these CCLayerTreeSettings. > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:150 > > + CCSettings::setDefaultTileSize(IntSize(min(CCSettings::defaultTileSize().width(), m_proxy->layerRendererCapabilities().maxTextureSize), > > It's a bit strange to set the default value based on a capability - but maybe this setting is just poorly named? I think we can safely assume that the max texture size will always be >=2k since that's required by DX9, so maybe we don't need this? I'm not sure about the name, but at first glance it seems reasonable to bound the default tile size by the maximum possible texture size.
Comment on attachment 146666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146666&action=review R=me. > Source/WebCore/platform/graphics/chromium/cc/CCSettings.cpp:29 > +#include <limits> I think this #include is unused now > Source/WebKit/chromium/public/WebCompositor.h:32 > +#include "platform/WebSize.h" this appears to be unused > Source/WebKit/chromium/tests/CCTestCommon.h:2 > + * Copyright (C) 2011 Google Inc. All rights reserved. 2012
Created attachment 146871 [details] =patch for landing (In reply to comment #15) > (From update of attachment 146666 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=146666&action=review > > R=me. > > > Source/WebCore/platform/graphics/chromium/cc/CCSettings.cpp:29 > > +#include <limits> > > I think this #include is unused now Removed. > > > Source/WebKit/chromium/public/WebCompositor.h:32 > > +#include "platform/WebSize.h" > > this appears to be unused Removed. > > > Source/WebKit/chromium/tests/CCTestCommon.h:2 > > + * Copyright (C) 2011 Google Inc. All rights reserved. > > 2012 Done.
Comment on attachment 146871 [details] =patch for landing =cq+
Comment on attachment 146871 [details] =patch for landing Rejecting attachment 146871 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: med 'setPartialSwapEnabled' Source/WebKit/chromium/webkit/glue/webpreferences.cc:372: error: 'class WebKit::WebSettings' has no member named 'setPerTilePaintingEnabled' cc1plus: warnings being treated as errors At global scope: cc1plus: error: unrecognized command line option "-Wno-narrowing" cc1plus: error: unrecognized command line option "-Wno-narrowing" make: *** [out/Debug/obj.target/glue/Source/WebKit/chromium/webkit/glue/webpreferences.o] Error 1 make: *** Waiting for unfinished jobs.... Full output: http://queues.webkit.org/results/12938488
(In reply to comment #18) > (From update of attachment 146871 [details]) > Rejecting attachment 146871 [details] from commit-queue. > > Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 > > Last 500 characters of output: > med 'setPartialSwapEnabled' > Source/WebKit/chromium/webkit/glue/webpreferences.cc:372: error: 'class WebKit::WebSettings' has no member named 'setPerTilePaintingEnabled' Looks like you'll need to land some chromium-side changes first or add more guards.
(In reply to comment #19) > (In reply to comment #18) > > (From update of attachment 146871 [details] [details]) > > Rejecting attachment 146871 [details] [details] from commit-queue. > > > > Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 > > > > Last 500 characters of output: > > med 'setPartialSwapEnabled' > > Source/WebKit/chromium/webkit/glue/webpreferences.cc:372: error: 'class WebKit::WebSettings' has no member named 'setPerTilePaintingEnabled' > > Looks like you'll need to land some chromium-side changes first or add more guards. Strange. The chromium side has landed and does include guards. Has it maybe not rolled?
Check Source/WebKit/chromium/DEPS to see. If it hasn't, please post a separate patch to roll that rev to the value you want and mark that as blocking this one (separate patch to avoid merge conflict hell).
Comment on attachment 146871 [details] =patch for landing Attachment 146871 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12951310
Created attachment 147355 [details] Patch Fix DRT.
Comment on attachment 147355 [details] Patch Clearing flags on attachment: 147355 Committed r120268: <http://trac.webkit.org/changeset/120268>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by 89060
Created attachment 147587 [details] Patch Fix DRT again. Ensure that we enable/disable per tile painting only once before we initialize the WebCompositor.
Comment on attachment 147587 [details] Patch Clearing flags on attachment: 147587 Committed r120360: <http://trac.webkit.org/changeset/120360>