[Ozone-GBM] Adding NativeWindowDelegate to IPC window changes to the GPU

[Ozone-GBM] Adding NativeWindowDelegate to IPC window changes to the GPU

This change is the first part in a series of changes to allow better display
configuration and tracking of windows/surfaces.

The browser process uses PlatformWindows to keep track of display surfaces.
The window then has an underlying surface to display content. The surface (in our
context SurfaceOzoneEGL) is created on the GPU process and is associated with
the window via an AcceleratedWidget handle. Each surface is then associated with
a HardwareDisplayController in order to scanout the contents to the monitor.
Since the surface is assumed to be in the window's coordinate system, the GPU
side has no knowledge of how to map a surface to the configured displays. The
NativeWindowDelegate is meant to IPC window information from the browser process
to the GPU process such that we can map surfaces to display controllers.

BUG=392478
NOTRY=true

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

[dnicoara, 2014-08-15 22:11:48]

This CL just introduces the classes used to bridge between the browser process
and the GPU process. They aren't yet fully wired.

The basic idea is that on the browser side DriWindow will have a
NativeWindowDelegateProxy and, on the GPU side the GbmSurface will have a
NativeWindowDelegateImpl. GbmSurface will then use the NativeWindowDelegate to
get its controller.

At the same time the ScreenManager logic will be updated to use bounds when
looking up a display controller.

[dnicoara, 2014-08-16 00:34:01]

https://codereview.chromium.org/479713002/diff/1/ui/ozone/platform/dri/gpu/gp...
File ui/ozone/platform/dri/gpu/gpu_messages.h (right):

https://codereview.chromium.org/479713002/diff/1/ui/ozone/platform/dri/gpu/gp...
ui/ozone/platform/dri/gpu/gpu_messages.h:17: #define IPC_MESSAGE_START
OzoneGpuMsgStart
spang@ pointed out that I can't do this in here since it would conflict with the
ones in ui/ozone/common/gpu. We reached a solution and I'll just make the
NativeWindowDelegate generic and shareable between platforms.

[alexst (slow to review), 2014-08-18 17:43:47]

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/gb...
File ui/ozone/platform/dri/gbm.gypi (right):

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/gb...
ui/ozone/platform/dri/gbm.gypi:51: 'native_window_delegate_proxy.cc',
Do you need to add these to .gn files as well? I think it's now a cq blocker.

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/gp...
File ui/ozone/platform/dri/gpu_platform_support_gbm.cc (right):

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/gp...
ui/ozone/platform/dri/gpu_platform_support_gbm.cc:71: new
NativeWindowDelegateImpl(widget, window_manager_, screen_manager_);
Can you make the registration with window manager explicit here instead of
explaining it in comments? That would also make ownership more clear as well, I
think.

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/gp...
ui/ozone/platform/dri/gpu_platform_support_gbm.cc:76:
scoped_ptr<NativeWindowDelegate> delegate(
DCHECK that it exists and remove it explicitly please.

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/gp...
ui/ozone/platform/dri/gpu_platform_support_gbm.cc:82:
window_manager_->GetNativeWindowDelegate(widget)->OnBoundsChanged(bounds);
DCHECK here please.

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/na...
File ui/ozone/platform/dri/native_window_delegate_proxy.cc (right):

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/na...
ui/ozone/platform/dri/native_window_delegate_proxy.cc:21: sender_->Send(new
OzoneGpuMsg_CreateNativeWindowDelegate(widget_));
Please don't use IPC in the constructor. Non-trivial operations here always lead
to pain down the road. I expect constructors to always succeed quickly and
anything else that has to happen should do so separately.

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/na...
ui/ozone/platform/dri/native_window_delegate_proxy.cc:28: sender_->Send(new
OzoneGpuMsg_DestoryNativeWindowDelegate(widget_));
As above, please keep the destructor as simple as possible, I prefer we didn't
IPC here.

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/na...
File ui/ozone/platform/dri/native_window_delegate_proxy.h (right):

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/na...
ui/ozone/platform/dri/native_window_delegate_proxy.h:15: class
NativeWindowDelegateProxy : public NativeWindowDelegate {
Please document this class a little bit, at least mention it's browser side
construct.

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/na...
File ui/ozone/platform/dri/native_window_manager.h (right):

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/na...
ui/ozone/platform/dri/native_window_manager.h:30:
std::map<gfx::AcceleratedWidget, NativeWindowDelegate*> delegate_map_;
Please typedef the map to something like WidgetToDelegateMap. This pattern is
very common throughout chrome, I prefer we follow it.

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/oz...
File ui/ozone/platform/dri/ozone_platform_gbm.cc (right):

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/oz...
ui/ozone/platform/dri/ozone_platform_gbm.cc:173: NativeWindowManager
gpu_window_manager_;
Who owns this?

[dnicoara, 2014-08-18 18:55:25]

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/gb...
File ui/ozone/platform/dri/gbm.gypi (right):

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/gb...
ui/ozone/platform/dri/gbm.gypi:51: 'native_window_delegate_proxy.cc',
On 2014/08/18 17:43:47, alexst wrote:
> Do you need to add these to .gn files as well? I think it's now a cq blocker.

Done.

Right, forgot about this. we should have an Ozone GN bot since none of the GN
bots would actually build our components.

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/gp...
File ui/ozone/platform/dri/gpu_platform_support_gbm.cc (right):

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/gp...
ui/ozone/platform/dri/gpu_platform_support_gbm.cc:71: new
NativeWindowDelegateImpl(widget, window_manager_, screen_manager_);
On 2014/08/18 17:43:47, alexst wrote:
> Can you make the registration with window manager explicit here instead of
> explaining it in comments? That would also make ownership more clear as well,
I
> think.

I think this is better since the object registers itself so there's less room
for error.

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/gp...
ui/ozone/platform/dri/gpu_platform_support_gbm.cc:76:
scoped_ptr<NativeWindowDelegate> delegate(
On 2014/08/18 17:43:47, alexst wrote:
> DCHECK that it exists and remove it explicitly please.

NativeWindowManager is doing all the DCHECKs, so I'd rather not duplicate them.

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/gp...
ui/ozone/platform/dri/gpu_platform_support_gbm.cc:82:
window_manager_->GetNativeWindowDelegate(widget)->OnBoundsChanged(bounds);
On 2014/08/18 17:43:47, alexst wrote:
> DCHECK here please.

See above comment.

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/na...
File ui/ozone/platform/dri/native_window_delegate_proxy.cc (right):

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/na...
ui/ozone/platform/dri/native_window_delegate_proxy.cc:21: sender_->Send(new
OzoneGpuMsg_CreateNativeWindowDelegate(widget_));
On 2014/08/18 17:43:47, alexst wrote:
> Please don't use IPC in the constructor. Non-trivial operations here always
lead
> to pain down the road. I expect constructors to always succeed quickly and
> anything else that has to happen should do so separately. 

Given that the IPCs are async, there's nothing slow in here. I'd rather not add
an Initialize/Shutdown pattern for this just for the sake of scheduling a
message (especially since they are strongly correlated with object
creation/destruction.

If we can't send this message, then we're pretty much screwed anyways (hence the
following DCHECK).

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/na...
File ui/ozone/platform/dri/native_window_delegate_proxy.h (right):

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/na...
ui/ozone/platform/dri/native_window_delegate_proxy.h:15: class
NativeWindowDelegateProxy : public NativeWindowDelegate {
On 2014/08/18 17:43:47, alexst wrote:
> Please document this class a little bit, at least mention it's browser side
> construct.

Done.

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/na...
File ui/ozone/platform/dri/native_window_manager.h (right):

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/na...
ui/ozone/platform/dri/native_window_manager.h:30:
std::map<gfx::AcceleratedWidget, NativeWindowDelegate*> delegate_map_;
On 2014/08/18 17:43:47, alexst wrote:
> Please typedef the map to something like WidgetToDelegateMap. This pattern is
> very common throughout chrome, I prefer we follow it.

Done.

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/oz...
File ui/ozone/platform/dri/ozone_platform_gbm.cc (right):

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/oz...
ui/ozone/platform/dri/ozone_platform_gbm.cc:173: NativeWindowManager
gpu_window_manager_;
On 2014/08/18 17:43:47, alexst wrote:
> Who owns this?

OzonePlatformGbm ... the one created on the GPU process.

[spang, 2014-08-18 19:31:30]

As I explained in person, there's already a type in the tree called
NativeWindow. A NativeWindowDelegate would be a delegate to a NativeWindow.
That's not what this is.

You said gpu_window_manager_ in one spot.

GpuWindow(gpu)
GpuWindowManager(gpu)

GpuWindowHost(browser)

are much better than this.

https://codereview.chromium.org/479713002/diff/40001/ui/ozone/platform/dri/na...
File ui/ozone/platform/dri/native_window_manager.cc (right):

https://codereview.chromium.org/479713002/diff/40001/ui/ozone/platform/dri/na...
ui/ozone/platform/dri/native_window_manager.cc:25: << "Surface delegate already
added.";
surface delegate?

https://codereview.chromium.org/479713002/diff/40001/ui/ozone/platform/dri/na...
File ui/ozone/platform/dri/native_window_manager.h (right):

https://codereview.chromium.org/479713002/diff/40001/ui/ozone/platform/dri/na...
ui/ozone/platform/dri/native_window_manager.h:21: gfx::AcceleratedWidget
AllocateAcceleratedWidget();
For consistency: NextAcceleratedWidget();

https://codereview.chromium.org/479713002/diff/40001/ui/ozone/platform/dri/na...
ui/ozone/platform/dri/native_window_manager.h:44: gfx::AcceleratedWidget
allocated_widgets_;
next/last_accelerated_widget_

https://codereview.chromium.org/479713002/diff/40001/ui/ozone/platform/dri/oz...
File ui/ozone/platform/dri/ozone_platform_gbm.cc (right):

https://codereview.chromium.org/479713002/diff/40001/ui/ozone/platform/dri/oz...
ui/ozone/platform/dri/ozone_platform_gbm.cc:173: NativeWindowManager
gpu_window_manager_;
the variable name is way better than the class name

[alexst (slow to review), 2014-08-18 20:30:34]

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/gp...
File ui/ozone/platform/dri/gpu_platform_support_gbm.cc (right):

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/gp...
ui/ozone/platform/dri/gpu_platform_support_gbm.cc:71: new
NativeWindowDelegateImpl(widget, window_manager_, screen_manager_);
On 2014/08/18 18:55:24, dnicoara wrote:
> On 2014/08/18 17:43:47, alexst wrote:
> > Can you make the registration with window manager explicit here instead of
> > explaining it in comments? That would also make ownership more clear as
well,
> I
> > think.
> 
> I think this is better since the object registers itself so there's less room
> for error.

I disagree. All I see is "new Stuff()" This requires me to read a long
explanation and go digging through the constructor to understand the
relationship between the two classes and their lifecycle.

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/na...
File ui/ozone/platform/dri/native_window_delegate_proxy.cc (right):

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/na...
ui/ozone/platform/dri/native_window_delegate_proxy.cc:21: sender_->Send(new
OzoneGpuMsg_CreateNativeWindowDelegate(widget_));
On 2014/08/18 18:55:24, dnicoara wrote:
> On 2014/08/18 17:43:47, alexst wrote:
> > Please don't use IPC in the constructor. Non-trivial operations here always
> lead
> > to pain down the road. I expect constructors to always succeed quickly and
> > anything else that has to happen should do so separately. 
> 
> Given that the IPCs are async, there's nothing slow in here. I'd rather not
add
> an Initialize/Shutdown pattern for this just for the sake of scheduling a
> message (especially since they are strongly correlated with object
> creation/destruction.
> 
> If we can't send this message, then we're pretty much screwed anyways (hence
the
> following DCHECK).

I know they are async, but my comment is about this usage pattern in general.
Also, it's in the code guide.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Doing_Work_in_...

[spang, 2014-08-18 23:48:46]

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/gp...
File ui/ozone/platform/dri/gpu_platform_support_gbm.cc (right):

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/gp...
ui/ozone/platform/dri/gpu_platform_support_gbm.cc:71: new
NativeWindowDelegateImpl(widget, window_manager_, screen_manager_);
On 2014/08/18 20:30:34, alexst wrote:
> On 2014/08/18 18:55:24, dnicoara wrote:
> > On 2014/08/18 17:43:47, alexst wrote:
> > > Can you make the registration with window manager explicit here instead of
> > > explaining it in comments? That would also make ownership more clear as
> well,
> > I
> > > think.
> > 
> > I think this is better since the object registers itself so there's less
room
> > for error.
> 
> I disagree. All I see is "new Stuff()" This requires me to read a long
> explanation and go digging through the constructor to understand the
> relationship between the two classes and their lifecycle. 

This does look like a memory leak to me as well. "new" should be returning
ownership to the caller. If you want other semantics then create a static
function and document the lifetime there.

[dnicoara, 2014-08-19 15:53:39]

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/gp...
File ui/ozone/platform/dri/gpu_platform_support_gbm.cc (right):

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/gp...
ui/ozone/platform/dri/gpu_platform_support_gbm.cc:71: new
NativeWindowDelegateImpl(widget, window_manager_, screen_manager_);
On 2014/08/18 23:48:46, spang wrote:
> On 2014/08/18 20:30:34, alexst wrote:
> > On 2014/08/18 18:55:24, dnicoara wrote:
> > > On 2014/08/18 17:43:47, alexst wrote:
> > > > Can you make the registration with window manager explicit here instead
of
> > > > explaining it in comments? That would also make ownership more clear as
> > well,
> > > I
> > > > think.
> > > 
> > > I think this is better since the object registers itself so there's less
> room
> > > for error.
> > 
> > I disagree. All I see is "new Stuff()" This requires me to read a long
> > explanation and go digging through the constructor to understand the
> > relationship between the two classes and their lifecycle. 
> 
> This does look like a memory leak to me as well. "new" should be returning
> ownership to the caller. If you want other semantics then create a static
> function and document the lifetime there.

Fine, updated.

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/na...
File ui/ozone/platform/dri/native_window_delegate_proxy.cc (right):

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/na...
ui/ozone/platform/dri/native_window_delegate_proxy.cc:21: sender_->Send(new
OzoneGpuMsg_CreateNativeWindowDelegate(widget_));
On 2014/08/18 20:30:34, alexst wrote:
> On 2014/08/18 18:55:24, dnicoara wrote:
> > On 2014/08/18 17:43:47, alexst wrote:
> > > Please don't use IPC in the constructor. Non-trivial operations here
always
> > lead
> > > to pain down the road. I expect constructors to always succeed quickly and
> > > anything else that has to happen should do so separately. 
> > 
> > Given that the IPCs are async, there's nothing slow in here. I'd rather not
> add
> > an Initialize/Shutdown pattern for this just for the sake of scheduling a
> > message (especially since they are strongly correlated with object
> > creation/destruction.
> > 
> > If we can't send this message, then we're pretty much screwed anyways (hence
> the
> > following DCHECK).
> 
> I know they are async, but my comment is about this usage pattern in general.
> Also, it's in the code guide.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Doing_Work_in_...

I've kept that in mind, but this doesn't break that rule since I'm not calling a
virtual function and I'm not failing initialization in any way. Having a "void
Initialize()" called right after the constructor is just extra complexity and
doesn't add any value.

https://codereview.chromium.org/479713002/diff/40001/ui/ozone/platform/dri/na...
File ui/ozone/platform/dri/native_window_manager.h (right):

https://codereview.chromium.org/479713002/diff/40001/ui/ozone/platform/dri/na...
ui/ozone/platform/dri/native_window_manager.h:21: gfx::AcceleratedWidget
AllocateAcceleratedWidget();
On 2014/08/18 19:31:30, spang wrote:
> For consistency: NextAcceleratedWidget();

Done.

https://codereview.chromium.org/479713002/diff/40001/ui/ozone/platform/dri/na...
ui/ozone/platform/dri/native_window_manager.h:44: gfx::AcceleratedWidget
allocated_widgets_;
On 2014/08/18 19:31:30, spang wrote:
> next/last_accelerated_widget_

Done.

https://codereview.chromium.org/479713002/diff/40001/ui/ozone/platform/dri/oz...
File ui/ozone/platform/dri/ozone_platform_gbm.cc (right):

https://codereview.chromium.org/479713002/diff/40001/ui/ozone/platform/dri/oz...
ui/ozone/platform/dri/ozone_platform_gbm.cc:173: NativeWindowManager
gpu_window_manager_;
On 2014/08/18 19:31:30, spang wrote:
> the variable name is way better than the class name

In this context, I wanted to show where this is used. In the next CL I'd have a
|ui_window_manager_| for the browser side one.

As for the class name, I'm not fond of GpuWindowManager, especially when it will
be instantiated on both the Browser and GPU process.

How about DriWindowManager? At least that's going to be in-sync with the
DriWindow naming.

[alexst (slow to review), 2014-08-19 16:02:09]

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/na...
File ui/ozone/platform/dri/native_window_delegate_proxy.cc (right):

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/na...
ui/ozone/platform/dri/native_window_delegate_proxy.cc:21: sender_->Send(new
OzoneGpuMsg_CreateNativeWindowDelegate(widget_));
You are arguing the specifics of the guide that says "Avoid doing complex
initialization in constructors"

If we are going to be specific about details, "in particular, initialization
that can fail"

sender_->Send() can very much fail.

[spang, 2014-08-19 17:26:25]

On 2014/08/19 15:53:39, dnicoara wrote:
>
https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/gp...
> File ui/ozone/platform/dri/gpu_platform_support_gbm.cc (right):
> 
>
https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/gp...
> ui/ozone/platform/dri/gpu_platform_support_gbm.cc:71: new
> NativeWindowDelegateImpl(widget, window_manager_, screen_manager_);
> On 2014/08/18 23:48:46, spang wrote:
> > On 2014/08/18 20:30:34, alexst wrote:
> > > On 2014/08/18 18:55:24, dnicoara wrote:
> > > > On 2014/08/18 17:43:47, alexst wrote:
> > > > > Can you make the registration with window manager explicit here
instead
> of
> > > > > explaining it in comments? That would also make ownership more clear
as
> > > well,
> > > > I
> > > > > think.
> > > > 
> > > > I think this is better since the object registers itself so there's less
> > room
> > > > for error.
> > > 
> > > I disagree. All I see is "new Stuff()" This requires me to read a long
> > > explanation and go digging through the constructor to understand the
> > > relationship between the two classes and their lifecycle. 
> > 
> > This does look like a memory leak to me as well. "new" should be returning
> > ownership to the caller. If you want other semantics then create a static
> > function and document the lifetime there.
> 
> Fine, updated.
> 
>
https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/na...
> File ui/ozone/platform/dri/native_window_delegate_proxy.cc (right):
> 
>
https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/na...
> ui/ozone/platform/dri/native_window_delegate_proxy.cc:21: sender_->Send(new
> OzoneGpuMsg_CreateNativeWindowDelegate(widget_));
> On 2014/08/18 20:30:34, alexst wrote:
> > On 2014/08/18 18:55:24, dnicoara wrote:
> > > On 2014/08/18 17:43:47, alexst wrote:
> > > > Please don't use IPC in the constructor. Non-trivial operations here
> always
> > > lead
> > > > to pain down the road. I expect constructors to always succeed quickly
and
> > > > anything else that has to happen should do so separately. 
> > > 
> > > Given that the IPCs are async, there's nothing slow in here. I'd rather
not
> > add
> > > an Initialize/Shutdown pattern for this just for the sake of scheduling a
> > > message (especially since they are strongly correlated with object
> > > creation/destruction.
> > > 
> > > If we can't send this message, then we're pretty much screwed anyways
(hence
> > the
> > > following DCHECK).
> > 
> > I know they are async, but my comment is about this usage pattern in
general.
> > Also, it's in the code guide.
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Doing_Work_in_...
> 
> I've kept that in mind, but this doesn't break that rule since I'm not calling
a
> virtual function and I'm not failing initialization in any way. Having a "void
> Initialize()" called right after the constructor is just extra complexity and
> doesn't add any value.
> 
>
https://codereview.chromium.org/479713002/diff/40001/ui/ozone/platform/dri/na...
> File ui/ozone/platform/dri/native_window_manager.h (right):
> 
>
https://codereview.chromium.org/479713002/diff/40001/ui/ozone/platform/dri/na...
> ui/ozone/platform/dri/native_window_manager.h:21: gfx::AcceleratedWidget
> AllocateAcceleratedWidget();
> On 2014/08/18 19:31:30, spang wrote:
> > For consistency: NextAcceleratedWidget();
> 
> Done.
> 
>
https://codereview.chromium.org/479713002/diff/40001/ui/ozone/platform/dri/na...
> ui/ozone/platform/dri/native_window_manager.h:44: gfx::AcceleratedWidget
> allocated_widgets_;
> On 2014/08/18 19:31:30, spang wrote:
> > next/last_accelerated_widget_
> 
> Done.
> 
>
https://codereview.chromium.org/479713002/diff/40001/ui/ozone/platform/dri/oz...
> File ui/ozone/platform/dri/ozone_platform_gbm.cc (right):
> 
>
https://codereview.chromium.org/479713002/diff/40001/ui/ozone/platform/dri/oz...
> ui/ozone/platform/dri/ozone_platform_gbm.cc:173: NativeWindowManager
> gpu_window_manager_;
> On 2014/08/18 19:31:30, spang wrote:
> > the variable name is way better than the class name
> 
> In this context, I wanted to show where this is used. In the next CL I'd have
a
> |ui_window_manager_| for the browser side one.
> 
> As for the class name, I'm not fond of GpuWindowManager, especially when it
will
> be instantiated on both the Browser and GPU process.

That's a fair point.

> 
> How about DriWindowManager? At least that's going to be in-sync with the
> DriWindow naming.

SGTM.

[dnicoara, 2014-08-19 17:54:02]

Renamed as well

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/na...
File ui/ozone/platform/dri/native_window_delegate_proxy.cc (right):

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/na...
ui/ozone/platform/dri/native_window_delegate_proxy.cc:21: sender_->Send(new
OzoneGpuMsg_CreateNativeWindowDelegate(widget_));
On 2014/08/19 16:02:09, alexst wrote:
> You are arguing the specifics of the guide that says "Avoid doing complex
> initialization in constructors"
> 
> If we are going to be specific about details, "in particular, initialization
> that can fail"
> 
> sender_->Send() can very much fail. 

Done.

https://codereview.chromium.org/479713002/diff/20001/ui/ozone/platform/dri/na...
ui/ozone/platform/dri/native_window_delegate_proxy.cc:28: sender_->Send(new
OzoneGpuMsg_DestoryNativeWindowDelegate(widget_));
On 2014/08/18 17:43:47, alexst wrote:
> As above, please keep the destructor as simple as possible, I prefer we didn't
> IPC here.

Done.

[spang, 2014-08-19 19:09:43]

lgtm

https://codereview.chromium.org/479713002/diff/120001/ui/ozone/platform/dri/d...
File ui/ozone/platform/dri/dri_window_delegate.h (right):

https://codereview.chromium.org/479713002/diff/120001/ui/ozone/platform/dri/d...
ui/ozone/platform/dri/dri_window_delegate.h:21: // hardware display controller.
I think this could be clarified. Please correct as necessary:

// Interface for display-server half of a DriWindow
//
// The main implementation of this lives in the process
// that owns the display (usually GPU process) and associates
// a platform window (DriWindow) with a display. A window
// is associated with the display whose bounds contains the
// window bounds. If there's no suitable display, the window
// is disconnected and its contents will not be visible. 
//
// In software mode, this is owned directly on DriWindow because
// the display controller object is in the same process.
//
// In accelerated mode, there's a proxy implementation and calls
// are forwarded to the real object in the GPU process via IPC.

https://codereview.chromium.org/479713002/diff/120001/ui/ozone/platform/dri/d...
ui/ozone/platform/dri/dri_window_delegate.h:31: virtual gfx::AcceleratedWidget
GetWidget() = 0;
For consistency: GetAcceleratedWidget()

https://codereview.chromium.org/479713002/diff/120001/ui/ozone/platform/dri/d...
File ui/ozone/platform/dri/dri_window_delegate_impl.cc (right):

https://codereview.chromium.org/479713002/diff/120001/ui/ozone/platform/dri/d...
ui/ozone/platform/dri/dri_window_delegate_impl.cc:34: controller_ =
screen_manager_->GetDisplayController(bounds);
What happens if the mapping is no longer 1:1?

[dnicoara, 2014-08-19 19:29:37]

https://codereview.chromium.org/479713002/diff/120001/ui/ozone/platform/dri/d...
File ui/ozone/platform/dri/dri_window_delegate.h (right):

https://codereview.chromium.org/479713002/diff/120001/ui/ozone/platform/dri/d...
ui/ozone/platform/dri/dri_window_delegate.h:21: // hardware display controller.
On 2014/08/19 19:09:43, spang wrote:
> I think this could be clarified. Please correct as necessary:
> 
> // Interface for display-server half of a DriWindow
> //
> // The main implementation of this lives in the process
> // that owns the display (usually GPU process) and associates
> // a platform window (DriWindow) with a display. A window
> // is associated with the display whose bounds contains the
> // window bounds. If there's no suitable display, the window
> // is disconnected and its contents will not be visible. 
> //
> // In software mode, this is owned directly on DriWindow because
> // the display controller object is in the same process.
> //
> // In accelerated mode, there's a proxy implementation and calls
> // are forwarded to the real object in the GPU process via IPC.

Done.

https://codereview.chromium.org/479713002/diff/120001/ui/ozone/platform/dri/d...
ui/ozone/platform/dri/dri_window_delegate.h:31: virtual gfx::AcceleratedWidget
GetWidget() = 0;
On 2014/08/19 19:09:43, spang wrote:
> For consistency: GetAcceleratedWidget()

Done.

https://codereview.chromium.org/479713002/diff/120001/ui/ozone/platform/dri/d...
File ui/ozone/platform/dri/dri_window_delegate_impl.cc (right):

https://codereview.chromium.org/479713002/diff/120001/ui/ozone/platform/dri/d...
ui/ozone/platform/dri/dri_window_delegate_impl.cc:34: controller_ =
screen_manager_->GetDisplayController(bounds);
On 2014/08/19 19:09:43, spang wrote:
> What happens if the mapping is no longer 1:1?

HDC deals with mirror mode configurations, so you can only have up to 1
controllers.

[alexst (slow to review), 2014-08-19 19:58:30]

https://codereview.chromium.org/479713002/diff/140001/ui/ozone/platform/dri/g...
File ui/ozone/platform/dri/gpu_platform_support_gbm.cc (right):

https://codereview.chromium.org/479713002/diff/140001/ui/ozone/platform/dri/g...
ui/ozone/platform/dri/gpu_platform_support_gbm.cc:66: DriWindowDelegateImpl*
delegate =
Can we do this with ScopedPtr instead?

[dnicoara, 2014-08-19 20:13:06]

https://codereview.chromium.org/479713002/diff/140001/ui/ozone/platform/dri/g...
File ui/ozone/platform/dri/gpu_platform_support_gbm.cc (right):

https://codereview.chromium.org/479713002/diff/140001/ui/ozone/platform/dri/g...
ui/ozone/platform/dri/gpu_platform_support_gbm.cc:66: DriWindowDelegateImpl*
delegate =
On 2014/08/19 19:58:30, alexst wrote:
> Can we do this with ScopedPtr instead?

Done.

[dnicoara, 2014-08-20 15:13:18]

As per our offline discussion, added an explicit owner for the delegates in the
GPU process.

[alexst (slow to review), 2014-08-20 15:31:02]

lgtm with a nit

https://codereview.chromium.org/479713002/diff/220001/ui/ozone/platform/dri/g...
File ui/ozone/platform/dri/gpu_platform_support_gbm.h (right):

https://codereview.chromium.org/479713002/diff/220001/ui/ozone/platform/dri/g...
ui/ozone/platform/dri/gpu_platform_support_gbm.h:65: typedef
std::map<gfx::AcceleratedWidget, DriWindowDelegate*>
nit: you can use scoped_ptr_hash_map.h

[dnicoara, 2014-08-20 16:03:00]

https://codereview.chromium.org/479713002/diff/220001/ui/ozone/platform/dri/g...
File ui/ozone/platform/dri/gpu_platform_support_gbm.h (right):

https://codereview.chromium.org/479713002/diff/220001/ui/ozone/platform/dri/g...
ui/ozone/platform/dri/gpu_platform_support_gbm.h:65: typedef
std::map<gfx::AcceleratedWidget, DriWindowDelegate*>
On 2014/08/20 15:31:01, alexst wrote:
> nit: you can use scoped_ptr_hash_map.h

Done & simplified.

Awesome, makes things easier.

[dnicoara, 2014-08-20 16:03:44]

+Ken for IPC changes

[kenrb, 2014-08-20 19:47:48]

ipc lgtm with a nit

https://codereview.chromium.org/479713002/diff/240001/ui/ozone/common/gpu/ozo...
File ui/ozone/common/gpu/ozone_gpu_messages.h (right):

https://codereview.chromium.org/479713002/diff/240001/ui/ozone/common/gpu/ozo...
ui/ozone/common/gpu/ozone_gpu_messages.h:68:
IPC_MESSAGE_CONTROL1(OzoneGpuMsg_DestoryWindowDelegate,
s/Destory/Destroy ?

[dnicoara, 2014-08-20 20:46:32]

https://codereview.chromium.org/479713002/diff/240001/ui/ozone/common/gpu/ozo...
File ui/ozone/common/gpu/ozone_gpu_messages.h (right):

https://codereview.chromium.org/479713002/diff/240001/ui/ozone/common/gpu/ozo...
ui/ozone/common/gpu/ozone_gpu_messages.h:68:
IPC_MESSAGE_CONTROL1(OzoneGpuMsg_DestoryWindowDelegate,
On 2014/08/20 19:47:48, kenrb wrote:
> s/Destory/Destroy ?

Done.

Arr, thanks for noticing!

[dnicoara, 2014-08-21 14:16:06]

The CQ bit was checked by dnicoara@chromium.org

[commit-bot: I haz the power, 2014-08-21 14:18:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dnicoara@chromium.org/479713002/260001

[commit-bot: I haz the power, 2014-08-21 15:02:15]

Committed patchset #14 (260001) as 291071