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

[bugzilla-daemon at webkit.org]

vollick at chromium.org has asked	for review:
Bug 88384: [chromium] Certain settings in CCSettings could be global
https://bugs.webkit.org/show_bug.cgi?id=88384

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

------- Additional Comments from vollick at chromium.org
(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 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);
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.