Prepare Chromium for WebCompositor::initialize.

Prepare Chromium for WebCompositor::initialize.

Currently, we have two duplicate mechanisms for telling the compositor to go into threaded mode. First, you set WebCompositor::setThread, then you pass enableCompositorThread via WebPreferences.

We're moving to a global initialize/shutdown flow on the compositor. This is a breaking change on the WebKit API so this change prepares us for it.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110794

[piman, 2011-11-18 23:21:56]

lgtm

http://codereview.chromium.org/8558030/diff/1/ui/gfx/compositor/compositor_cc.cc
File ui/gfx/compositor/compositor_cc.cc (right):

http://codereview.chromium.org/8558030/diff/1/ui/gfx/compositor/compositor_cc...
ui/gfx/compositor/compositor_cc.cc:152: DCHECK(g_compositor_thread);
remove DCHECK, since it'll fail if use_thread == false.

[sky, 2011-11-18 23:27:29]

LGTM

http://codereview.chromium.org/8558030/diff/1/content/renderer/gpu/compositor...
File content/renderer/gpu/compositor_thread.h (right):

http://codereview.chromium.org/8558030/diff/1/content/renderer/gpu/compositor...
content/renderer/gpu/compositor_thread.h:37: webkit_glue::WebThreadImpl*
GetWebThread() const { return thread_.get(); }
nit: according to style guide this should be named thread().

http://codereview.chromium.org/8558030/diff/1/ui/gfx/compositor/compositor_cc.h
File ui/gfx/compositor/compositor_cc.h (right):

http://codereview.chromium.org/8558030/diff/1/ui/gfx/compositor/compositor_cc...
ui/gfx/compositor/compositor_cc.h:90: static void Initialize(bool useThread);
nit: useThread -> use_thread and document what it means.

[tony, 2011-11-18 23:30:08]

Can you elaborate on the change description a bit?  What are you doing to
prepare chromium?

rubber stamp webkit/glue LGTM since the change there is minor.

http://codereview.chromium.org/8558030/diff/1/webkit/glue/webpreferences.cc
File webkit/glue/webpreferences.cc (right):

http://codereview.chromium.org/8558030/diff/1/webkit/glue/webpreferences.cc#n...
webkit/glue/webpreferences.cc:11: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebCompositor.h"
Is this needed for this change or you're just doing it preemptively?

http://codereview.chromium.org/8558030/diff/1/webkit/glue/webpreferences.cc#n...
webkit/glue/webpreferences.cc:263:
settings->setUseThreadedCompositor(threaded_compositing_enabled);
It feels weird to conditionally set this.  Should we conditionally be giving a
value to threaded_compositing_enabled instead?

[nduca, 2011-11-18 23:32:37]

On 2011/11/18 23:30:08, tony wrote:
> Can you elaborate on the change description a bit?  What are you doing to
> prepare chromium?
Good point, elaboratifying. :)

http://codereview.chromium.org/8558030/diff/1/webkit/glue/webpreferences.cc#n...
> webkit/glue/webpreferences.cc:11: #include
> "third_party/WebKit/Source/WebKit/chromium/public/WebCompositor.h"
> Is this needed for this change or you're just doing it preemptively?

This pulls in the define we need for the ifdef that follows.

http://codereview.chromium.org/8558030/diff/1/webkit/glue/webpreferences.cc#n...
> webkit/glue/webpreferences.cc:263:
> settings->setUseThreadedCompositor(threaded_compositing_enabled);
> It feels weird to conditionally set this.  Should we conditionally be giving a
> value to threaded_compositing_enabled instead?

When the initialize feature is landed, the setting goes away. I.e. this would be
a compile error to even call.

There will be a follow up patch on chromium to clear out the thread enable field
from the webpreferences side of the struct entirely.

[tony, 2011-11-19 00:10:36]

On 2011/11/18 23:32:37, nduca wrote:
> On 2011/11/18 23:30:08, tony wrote:
>
http://codereview.chromium.org/8558030/diff/1/webkit/glue/webpreferences.cc#n...
> > webkit/glue/webpreferences.cc:11: #include
> > "third_party/WebKit/Source/WebKit/chromium/public/WebCompositor.h"
> > Is this needed for this change or you're just doing it preemptively?
> 
> This pulls in the define we need for the ifdef that follows.

I see, WEBCOMPOSITOR_HAS_INITIALIZE doesn't exist yet, but it will soon.  Got
it.

LGTM