[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