[webkit-reviews] review denied: [Bug 88384] [chromium] Certain settings in CCSettings could be global : [Attachment 146191] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jun 7 12:46:16 PDT 2012

James Robinson <jamesr at chromium.org> has denied vollick at chromium.org's request
for review:
Bug 88384: [chromium] Certain settings in CCSettings could be global

Attachment 146191: Patch

------- Additional Comments from James Robinson <jamesr at chromium.org>
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 =

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 at jamesr:~$ g++ test.cc -o test -O2
jamesr at jamesr:~$
/path/to/chromium/checkout/src/tools/linux/dump-static-initializers.py test
mybar (initializer offset 0x4005d0 size 0xb)

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

> 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

> Source/WebKit/chromium/tests/CCTestCommon.h:33
> +// If you have a test that modifies or uses global settings, keep an
> +// 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?

More information about the webkit-reviews mailing list