Refactor SynchronousCompositor out of SynchronousCompositorOutputSurface.

Refactor SynchronousCompositor out of SynchronousCompositorOutputSurface.

This creates a separate SynchronousCompositorImpl class that is owned
by the RenderWidget. This will be needed for hooking up the root layer
scroll offset delegate.

BUG=None

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

[mkosiba (inactive), 2013-05-23 18:12:58]

Hey!
  This was part of https://codereview.chromium.org/15029006/ but it makes sense
to do this change as a separate CL and land it first. PTAL.

[joth, 2013-05-23 19:20:57]

https://codereview.chromium.org/15875009/diff/2001/content/renderer/android/s...
File content/renderer/android/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/15875009/diff/2001/content/renderer/android/s...
content/renderer/android/synchronous_compositor_impl.cc:33: return false;
gracefully handling something you DCHECKed for is antipattern in the style
guide.
As writting the hook-up means it's not possible for the
SynchronousCompositorImpl to exist without a OutputSurface? in which case DCHECK
alone is fine.
if you do want to handle it though, you can log instead of DCEHCK

if (!output_surface) {
  LOG(WARN) 
  return false;
}

+2 more places below

https://codereview.chromium.org/15875009/diff/2001/content/renderer/android/s...
content/renderer/android/synchronous_compositor_impl.cc:78: output_surface_ =
NULL;
you're not calling compositor_client_->DidDestroyCompositor any more. 
doing here maybe premature?
but doing it in the destructor will be on the wrong thread?

https://codereview.chromium.org/15875009/diff/2001/content/renderer/android/s...
File content/renderer/android/synchronous_compositor_output_surface.h (right):

https://codereview.chromium.org/15875009/diff/2001/content/renderer/android/s...
content/renderer/android/synchronous_compositor_output_surface.h:11: #include
"base/memory/weak_ptr.h"
not needed?

https://codereview.chromium.org/15875009/diff/2001/content/renderer/android/s...
content/renderer/android/synchronous_compositor_output_surface.h:25: virtual
void DidDestroyCompositor() = 0;
Shouldn't these be Did{Create|Destroy}OutputSurface?

https://codereview.chromium.org/15875009/diff/2001/content/renderer/android/s...
content/renderer/android/synchronous_compositor_output_surface.h:76: bool
vsync_enabled_;
unused

[mkosiba (inactive), 2013-05-24 14:42:40]

https://codereview.chromium.org/15875009/diff/2001/content/renderer/android/s...
File content/renderer/android/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/15875009/diff/2001/content/renderer/android/s...
content/renderer/android/synchronous_compositor_impl.cc:33: return false;
On 2013/05/23 19:20:57, joth wrote:
> gracefully handling something you DCHECKed for is antipattern in the style
> guide.
> As writting the hook-up means it's not possible for the
> SynchronousCompositorImpl to exist without a OutputSurface? in which case
DCHECK
> alone is fine.
> if you do want to handle it though, you can log instead of DCEHCK
> 
> if (!output_surface) {
>   LOG(WARN) 
>   return false;
> }
> 
> +2 more places below

Ah, yes, the DCHECK should be sufficient. The a_w layer shouldn't poke at these
methods of the SCI before DidCreateSynchronousCompositor is called.

https://codereview.chromium.org/15875009/diff/2001/content/renderer/android/s...
content/renderer/android/synchronous_compositor_impl.cc:78: output_surface_ =
NULL;
On 2013/05/23 19:20:57, joth wrote:
> you're not calling compositor_client_->DidDestroyCompositor any more. 
> doing here maybe premature?
> but doing it in the destructor will be on the wrong thread?

simple oversight on my end. Added the call.

https://codereview.chromium.org/15875009/diff/2001/content/renderer/android/s...
File content/renderer/android/synchronous_compositor_output_surface.h (right):

https://codereview.chromium.org/15875009/diff/2001/content/renderer/android/s...
content/renderer/android/synchronous_compositor_output_surface.h:11: #include
"base/memory/weak_ptr.h"
On 2013/05/23 19:20:57, joth wrote:
> not needed?

Done.

https://codereview.chromium.org/15875009/diff/2001/content/renderer/android/s...
content/renderer/android/synchronous_compositor_output_surface.h:25: virtual
void DidDestroyCompositor() = 0;
On 2013/05/23 19:20:57, joth wrote:
> Shouldn't these be Did{Create|Destroy}OutputSurface?

Done.

https://codereview.chromium.org/15875009/diff/2001/content/renderer/android/s...
content/renderer/android/synchronous_compositor_output_surface.h:76: bool
vsync_enabled_;
On 2013/05/23 19:20:57, joth wrote:
> unused

Done.

[joth, 2013-05-24 15:05:06]

lgtm

On Friday, 24 May 2013,  <mkosiba@chromium.org> wrote:
>
>
https://codereview.chromium.org/15875009/diff/2001/content/renderer/android/s...
> File content/renderer/android/synchronous_compositor_impl.cc (right):
>
>
https://codereview.chromium.org/15875009/diff/2001/content/renderer/android/s...
> content/renderer/android/synchronous_compositor_impl.cc:33: return
> false;
> On 2013/05/23 19:20:57, joth wrote:
>>
>> gracefully handling something you DCHECKed for is antipattern in the
>
> style
>>
>> guide.
>> As writting the hook-up means it's not possible for the
>> SynchronousCompositorImpl to exist without a OutputSurface? in which
>
> case DCHECK
>>
>> alone is fine.
>> if you do want to handle it though, you can log instead of DCEHCK
>
>> if (!output_surface) {
>>    LOG(WARN)
>>    return false;
>> }
>
>> +2 more places below
>
> Ah, yes, the DCHECK should be sufficient. The a_w layer shouldn't poke
> at these methods of the SCI before DidCreateSynchronousCompositor is
> called.
>
>
https://codereview.chromium.org/15875009/diff/2001/content/renderer/android/s...
> content/renderer/android/synchronous_compositor_impl.cc:78:
> output_surface_ = NULL;
> On 2013/05/23 19:20:57, joth wrote:
>>
>> you're not calling compositor_client_->DidDestroyCompositor any more.
>> doing here maybe premature?
>> but doing it in the destructor will be on the wrong thread?
>
> simple oversight on my end. Added the call.
>
>
https://codereview.chromium.org/15875009/diff/2001/content/renderer/android/s...
> File content/renderer/android/synchronous_compositor_output_surface.h
> (right):
>
>
https://codereview.chromium.org/15875009/diff/2001/content/renderer/android/s...
> content/renderer/android/synchronous_compositor_output_surface.h:11:
> #include "base/memory/weak_ptr.h"
> On 2013/05/23 19:20:57, joth wrote:
>>
>> not needed?
>
> Done.
>
>
https://codereview.chromium.org/15875009/diff/2001/content/renderer/android/s...
> content/renderer/android/synchronous_compositor_output_surface.h:25:
> virtual void DidDestroyCompositor() = 0;
> On 2013/05/23 19:20:57, joth wrote:
>>
>> Shouldn't these be Did{Create|Destroy}OutputSurface?
>
> Done.
>
>
https://codereview.chromium.org/15875009/diff/2001/content/renderer/android/s...
> content/renderer/android/synchronous_compositor_output_surface.h:76:
> bool vsync_enabled_;
> On 2013/05/23 19:20:57, joth wrote:
>>
>> unused
>
> Done.
>
> https://codereview.chromium.org/15875009/
>

[jdduke (slow), 2013-05-24 15:16:41]

Sorry, kind of a drive-by =/

https://codereview.chromium.org/15875009/diff/7001/content/renderer/android/s...
File content/renderer/android/synchronous_compositor_impl.h (right):

https://codereview.chromium.org/15875009/diff/7001/content/renderer/android/s...
content/renderer/android/synchronous_compositor_impl.h:17: class InputHandler;
Nit: I think you can remove this forward decl now?

[jdduke (slow), 2013-05-24 15:26:26]

https://codereview.chromium.org/15875009/diff/7001/content/public/renderer/an...
File content/public/renderer/android/synchronous_compositor.h (right):

https://codereview.chromium.org/15875009/diff/7001/content/public/renderer/an...
content/public/renderer/android/synchronous_compositor.h:21: // Interface for
embedders that which to direct compositing operations
Nit: which -> wish?

https://codereview.chromium.org/15875009/diff/7001/content/public/renderer/an...
content/public/renderer/android/synchronous_compositor.h:32: // Returns true is
the compositor is fully initialized and ready to receive
Nit: true is the -> true if the

[mkosiba (inactive), 2013-05-24 16:22:33]

https://codereview.chromium.org/15875009/diff/7001/content/public/renderer/an...
File content/public/renderer/android/synchronous_compositor.h (right):

https://codereview.chromium.org/15875009/diff/7001/content/public/renderer/an...
content/public/renderer/android/synchronous_compositor.h:21: // Interface for
embedders that which to direct compositing operations
On 2013/05/24 15:26:26, jdduke wrote:
> Nit: which -> wish?

Done.

https://codereview.chromium.org/15875009/diff/7001/content/public/renderer/an...
content/public/renderer/android/synchronous_compositor.h:32: // Returns true is
the compositor is fully initialized and ready to receive
On 2013/05/24 15:26:26, jdduke wrote:
> Nit: true is the -> true if the

Done.

https://codereview.chromium.org/15875009/diff/7001/content/renderer/android/s...
File content/renderer/android/synchronous_compositor_impl.h (right):

https://codereview.chromium.org/15875009/diff/7001/content/renderer/android/s...
content/renderer/android/synchronous_compositor_impl.h:17: class InputHandler;
On 2013/05/24 15:16:41, jdduke wrote:
> Nit: I think you can remove this forward decl now?

Done.

[mkosiba (inactive), 2013-05-28 10:28:07]

James - could you PTAL @ the render_widget changes?

[jamesr, 2013-05-29 23:00:50]

lgtm

https://codereview.chromium.org/15875009/diff/36001/content/renderer/render_w...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/15875009/diff/36001/content/renderer/render_w...
content/renderer/render_widget.h:57: class InputHandlerClient;
this doesn't appear to be used, please revert

[joth, 2013-05-29 23:42:55]

+piman for content_renderer.gypi rubberstamp

https://codereview.chromium.org/15875009/diff/36001/content/renderer/render_w...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/15875009/diff/36001/content/renderer/render_w...
content/renderer/render_widget.h:57: class InputHandlerClient;
On 2013/05/29 23:00:50, jamesr wrote:
> this doesn't appear to be used, please revert

Done.

[piman, 2013-05-30 00:07:40]

lgtm

[commit-bot: I haz the power, 2013-05-30 00:35:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/15875009/52001

[commit-bot: I haz the power, 2013-05-30 01:22:53]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-30 01:28:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/15875009/52001

[commit-bot: I haz the power, 2013-05-30 05:02:01]

Change committed as 203078