Move synchronous compositor into content/browser

Move synchronous compositor into content/browser

This means all the hookup can remain in content/ so less internal
details need to be revealed in the public API long-term.
Now there is a single API to intialize the sync compositor mode per
process, and per WebContents, rather than a collection of flags and
hooks. The bulk of the browser->renderer hookup is done in content/ so
android_webview InProcessViewRenderer can be greatly simplified.

(depends on pending CL https://codereview.chromium.org/15875009/)

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203351

[joth, 2013-05-30 01:03:44]

This CL supercedes https://codereview.chromium.org/16119003/ and makes all the
content/public API changes in addition to the implementation file moves of that
prior iteration. (Seems less churn in total to land this all as one piece)

jam: content/public
mkosiba: anroid_webview & please look at all the syncrohronous_* files too
jamesr: content/renderer

Thanks!

[jamesr, 2013-05-30 02:30:15]

General shape of this looks pretty reasonable to me.  I'll do a line-by-line
review once someone familiar with android_wv's taken a look at those bits, since
that's where the significant functionality is.

[mkosiba (inactive), 2013-05-30 11:05:28]

LGTM I think this clears stuff up nicely!

https://codereview.chromium.org/15851006/diff/7001/android_webview/browser/in...
File android_webview/browser/in_process_view_renderer.cc (right):

https://codereview.chromium.org/15851006/diff/7001/android_webview/browser/in...
android_webview/browser/in_process_view_renderer.cc:281: SetContents(NULL);
add a DCHECK(compositor_ == NULL)?

https://codereview.chromium.org/15851006/diff/7001/content/browser/android/in...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/15851006/diff/7001/content/browser/android/in...
content/browser/android/in_process/synchronous_compositor_impl.cc:71: return
FromWebContents(contents);
nit: it's a bit hard to figure out that this method is inherited.. add a comment
or maybe write it like this:
WebContentsUserData<SynchronousCompositorImpl>::FromWebContents(contents); ?

https://codereview.chromium.org/15851006/diff/7001/content/browser/android/in...
content/browser/android/in_process/synchronous_compositor_impl.cc:77:
SynchronousCompositorImpl* instance = FromWebContents(contents);
same here

https://codereview.chromium.org/15851006/diff/7001/content/browser/android/in...
content/browser/android/in_process/synchronous_compositor_impl.cc:80:
CreateForWebContents(contents);
and here

https://codereview.chromium.org/15851006/diff/7001/content/browser/android/in...
File content/browser/android/in_process/synchronous_compositor_output_surface.h
(right):

https://codereview.chromium.org/15851006/diff/7001/content/browser/android/in...
content/browser/android/in_process/synchronous_compositor_output_surface.h:70:
SynchronousCompositorOutputSurfaceDelegate* GetDelegate();
uber-nit: might be fine to call this delegate(). I think the style guide says
you can use this naming convention for simple&cheap getters (which this one is)
and I don't think this naming convention is limited to inline functions only.

[jam, 2013-05-30 18:25:00]

lgtm, thanks

[joth, 2013-05-30 19:18:05]

Thanks mkosiba, jam!

jamesr - over to you for a look?

Cheers

https://codereview.chromium.org/15851006/diff/7001/android_webview/browser/in...
File android_webview/browser/in_process_view_renderer.cc (right):

https://codereview.chromium.org/15851006/diff/7001/android_webview/browser/in...
android_webview/browser/in_process_view_renderer.cc:281: SetContents(NULL);
On 2013/05/30 11:05:29, mkosiba wrote:
> add a DCHECK(compositor_ == NULL)?

Done.

https://codereview.chromium.org/15851006/diff/7001/content/browser/android/in...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/15851006/diff/7001/content/browser/android/in...
content/browser/android/in_process/synchronous_compositor_impl.cc:81: return
FromWebContents(contents);
and here?

IKWYM about taking some effort to read this.... but I think commenting each call
is going to be messy, and using WebContentsUserData<SynchronousCompositorImpl>::
scoping is redundant, cumbersome, and causes extra line wrapping. I searched the
other 50+ uses of WebContentsUserData and didn't find any that thought this a
problem....


I do like skia's pattern of adding a "Super" typedef in every class so you can
just say Super::FooBar(), but that's by the by.

I have removed this GetOrCreateForWebContents() though, as we only need it on
SynchronousCompositor (at end of file) as anyone who can see
SynchronousCompositorImpl can just use the base-class methods anyway. This at
least removes most the confusing uses :-)

https://codereview.chromium.org/15851006/diff/7001/content/browser/android/in...
File content/browser/android/in_process/synchronous_compositor_output_surface.h
(right):

https://codereview.chromium.org/15851006/diff/7001/content/browser/android/in...
content/browser/android/in_process/synchronous_compositor_output_surface.h:70:
SynchronousCompositorOutputSurfaceDelegate* GetDelegate();
On 2013/05/30 11:05:29, mkosiba wrote:
> uber-nit: might be fine to call this delegate(). I think the style guide says
> you can use this naming convention for simple&cheap getters (which this one
is)
> and I don't think this naming convention is limited to inline functions only.

I don't like hacker_case() methods calling CamelCase() methods internally. Any
this does do a map lookup, not not entirely as cheap as returning a member
(which hacker_case() is intended for).

[jamesr, 2013-05-30 20:28:56]

lgtm on all but content_renderer.gypi. I think that might just be a
mass-rename.sh misfire

https://codereview.chromium.org/15851006/diff/15001/content/content_renderer....
File content/content_renderer.gypi (right):

https://codereview.chromium.org/15851006/diff/15001/content/content_renderer....
content/content_renderer.gypi:35:
'public/browser/android/synchronous_compositor.h',
this is weird - why do you need these header files in the gyp target for
content_renderer? headers in a gyp file don't have any impact on the compiled
target so i'd strongly prefer not doing this and only listing these headers in
content_browser.gypi

https://codereview.chromium.org/15851006/diff/15001/content/renderer/android/...
File content/renderer/android/synchronous_compositor_factory.h (right):

https://codereview.chromium.org/15851006/diff/15001/content/renderer/android/...
content/renderer/android/synchronous_compositor_factory.h:31: static bool
HasInstance() { return GetInstance() != NULL; }
nit: i don't find this terribly valuable, code could just as easily do

if (GetInstance()) { .... }

or perhaps more usefully

if (SynchronousCompositorFactory* factory = ...GetInstance() {
  factory->Blahblahblah()
}

[joth, 2013-05-30 21:22:14]

Both points addressed, thanks!

https://codereview.chromium.org/15851006/diff/15001/content/content_renderer....
File content/content_renderer.gypi (right):

https://codereview.chromium.org/15851006/diff/15001/content/content_renderer....
content/content_renderer.gypi:35:
'public/browser/android/synchronous_compositor.h',
On 2013/05/30 20:28:57, jamesr wrote:
> this is weird - why do you need these header files in the gyp target for
> content_renderer? headers in a gyp file don't have any impact on the compiled
> target so i'd strongly prefer not doing this and only listing these headers in
> content_browser.gypi

Good spot, many thanks. 
Yes this was misuse of the tools/git/move_source_file.py tool (I fixed it on the
first round of file moves but forgot on the second)

https://codereview.chromium.org/15851006/diff/15001/content/renderer/android/...
File content/renderer/android/synchronous_compositor_factory.h (right):

https://codereview.chromium.org/15851006/diff/15001/content/renderer/android/...
content/renderer/android/synchronous_compositor_factory.h:31: static bool
HasInstance() { return GetInstance() != NULL; }
On 2013/05/30 20:28:57, jamesr wrote:
> nit: i don't find this terribly valuable, code could just as easily do
> 
> if (GetInstance()) { .... }
> 
> or perhaps more usefully
> 
> if (SynchronousCompositorFactory* factory = ...GetInstance() {
>   factory->Blahblahblah()
> }

Done.
In my mind I was thinking of something a little more abstracted here like
"IsEnabled()" but I agree it's basically redundant and I like your second
suggestion so going with that (where applicable)

[commit-bot: I haz the power, 2013-05-30 21:22:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joth@chromium.org/15851006/19002

[commit-bot: I haz the power, 2013-05-30 23:55:00]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-05-31 01:59:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joth@chromium.org/15851006/47036

[commit-bot: I haz the power, 2013-05-31 01:59:11]

Failed to apply patch for content/public/renderer/content_renderer_client.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/public/renderer/content_renderer_client.cc
  Hunk #1 FAILED at 159.
  1 out of 1 hunk FAILED -- saving rejects to file
content/public/renderer/content_renderer_client.cc.rej

Patch:       content/public/renderer/content_renderer_client.cc
Index: content/public/renderer/content_renderer_client.cc
diff --git a/content/public/renderer/content_renderer_client.cc
b/content/public/renderer/content_renderer_client.cc
index
45d08cd99e6fe1d3d4f6eb8b73943876ddb450f0..a56652cd89a45a7f7d5fabf45359691e80cf9f71
100644
--- a/content/public/renderer/content_renderer_client.cc
+++ b/content/public/renderer/content_renderer_client.cc
@@ -159,12 +159,4 @@ bool ContentRendererClient::AllowBrowserPlugin(
   return false;
 }
 
-MessageLoop* ContentRendererClient::OverrideCompositorMessageLoop() const {
-  return NULL;
-}
-
-bool ContentRendererClient::ShouldCreateCompositorInputHandler() const {
-  return true;
-}
-
 }  // namespace content

[commit-bot: I haz the power, 2013-05-31 02:23:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joth@chromium.org/15851006/55003

[commit-bot: I haz the power, 2013-05-31 07:16:09]

Change committed as 203351