Factor common code into native_viewport_controller.cc

Factor common code into native_viewport_controller.cc

The platform-specific implementations of NativeViewport have a bunch of
duplicated code. This CL centralizes that code in the cross-platform
controller class.

R=ben@chromium.org

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

[Ben Goodger (Google), 2013-11-06 07:17:16]

https://codereview.chromium.org/59383011/diff/50001/mojo/services/native_view...
File mojo/services/native_viewport/native_viewport.cc (right):

https://codereview.chromium.org/59383011/diff/50001/mojo/services/native_view...
mojo/services/native_viewport/native_viewport.cc:25: false, widget_,
bounds_.size(), false,
nit: 4-space indent here

https://codereview.chromium.org/59383011/diff/50001/mojo/services/native_view...
mojo/services/native_viewport/native_viewport.cc:28:
&NativeViewport::OnGLContextLost, base::Unretained(this)));
and here

https://codereview.chromium.org/59383011/diff/50001/mojo/services/native_view...
File mojo/services/native_viewport/native_viewport.h (right):

https://codereview.chromium.org/59383011/diff/50001/mojo/services/native_view...
mojo/services/native_viewport/native_viewport.h:54: void
OnAcceleratedWidgetAvailable();
wdyt about this stuff being the responsibility of this object's delegate instead
of making this type concrete?

[abarth-chromium, 2013-11-06 15:40:54]

https://codereview.chromium.org/59383011/diff/50001/mojo/services/native_view...
File mojo/services/native_viewport/native_viewport.cc (right):

https://codereview.chromium.org/59383011/diff/50001/mojo/services/native_view...
mojo/services/native_viewport/native_viewport.cc:25: false, widget_,
bounds_.size(), false,
On 2013/11/06 07:17:17, Ben Goodger (Google) wrote:
> nit: 4-space indent here

Done.

https://codereview.chromium.org/59383011/diff/50001/mojo/services/native_view...
mojo/services/native_viewport/native_viewport.cc:28:
&NativeViewport::OnGLContextLost, base::Unretained(this)));
On 2013/11/06 07:17:17, Ben Goodger (Google) wrote:
> and here

Done.

https://codereview.chromium.org/59383011/diff/50001/mojo/services/native_view...
File mojo/services/native_viewport/native_viewport.h (right):

https://codereview.chromium.org/59383011/diff/50001/mojo/services/native_view...
mojo/services/native_viewport/native_viewport.h:54: void
OnAcceleratedWidgetAvailable();
On 2013/11/06 07:17:17, Ben Goodger (Google) wrote:
> wdyt about this stuff being the responsibility of this object's delegate
instead
> of making this type concrete?

It depends on whether we're going to have the object's delegate be a concrete
CPP class or whether the delegate is going to be remote over IPC.  I was
imagining that NativeViewport and NativeViewportDelegate would actually be the
IPC end points, which would mean it was NativeViewport's object to instantiate
the command buffer.

[Ben Goodger (Google), 2013-11-06 18:25:33]

On 2013/11/06 15:40:54, abarth wrote:
>
https://codereview.chromium.org/59383011/diff/50001/mojo/services/native_view...
> mojo/services/native_viewport/native_viewport.h:54: void
> OnAcceleratedWidgetAvailable();
> On 2013/11/06 07:17:17, Ben Goodger (Google) wrote:
> > wdyt about this stuff being the responsibility of this object's delegate
> instead
> > of making this type concrete?
> 
> It depends on whether we're going to have the object's delegate be a concrete
> CPP class or whether the delegate is going to be remote over IPC.  I was
> imagining that NativeViewport and NativeViewportDelegate would actually be the
> IPC end points, which would mean it was NativeViewport's object to instantiate
> the command buffer.

So with that in mind, should the stuff in NativeViewportController move into
this base class then?

The reason originally I asked about this is, having worked with several
iterations of these widget hierarchy classes (Aura, Views, etc) I have a
preference for composition over inheritance.

[abarth-chromium, 2013-11-06 18:28:45]

On 2013/11/06 18:25:33, Ben Goodger (Google) wrote:
> So with that in mind, should the stuff in NativeViewportController move into
> this base class then?

Currently NativeViewportController serializes NativeViewportDelegate calls into
mojo::WriteMessage calls.  That code will be autogenerated by the IDL compiler.

> The reason originally I asked about this is, having worked with several
> iterations of these widget hierarchy classes (Aura, Views, etc) I have a
> preference for composition over inheritance.

Ok.  I'll rework the CL along those lines.

Thanks!

[Ben Goodger (Google), 2013-11-06 20:07:38]

On Wed, Nov 6, 2013 at 10:28 AM, <abarth@chromium.org> wrote:

> On 2013/11/06 18:25:33, Ben Goodger (Google) wrote:
>
>> So with that in mind, should the stuff in NativeViewportController move
>> into
>> this base class then?
>>
>
> Currently NativeViewportController serializes NativeViewportDelegate calls
> into
> mojo::WriteMessage calls.  That code will be autogenerated by the IDL
> compiler.


I'm not so concerned about where the current string serialization/sending
ends up since I'm assuming we'll rip it all out shortly. I'm more
interested in having the right set of relationships on the service impl
side.


> The reason originally I asked about this is, having worked with several
>> iterations of these widget hierarchy classes (Aura, Views, etc) I have a
>> preference for composition over inheritance.
>>
>
> Ok.  I'll rework the CL along those lines.
>

Cool.

-Ben

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[abarth-chromium, 2013-11-06 20:12:33]

Done.  PTAL

[Ben Goodger (Google), 2013-11-06 20:14:22]

lgtm !`

[commit-bot: I haz the power, 2013-11-06 20:59:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/59383011/220001

[commit-bot: I haz the power, 2013-11-06 21:46:14]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
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-11-06 22:05:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/59383011/490001

[commit-bot: I haz the power, 2013-11-07 00:03:56]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
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-11-07 00:21:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/59383011/680001

[commit-bot: I haz the power, 2013-11-07 06:48:18]

Change committed as 233511