[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
https://bugs.webkit.org/show_bug.cgi?id=88384

Attachment 146191: Patch
https://bugs.webkit.org/attachment.cgi?id=146191&action=review

------- 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 =
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 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)
  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?


More information about the webkit-reviews mailing list