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

[bugzilla-daemon at webkit.org]

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 146086: Patch
https://bugs.webkit.org/attachment.cgi?id=146086&action=review

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