Add Mojo NativeDisplayDelegate / NativeDisplayObserver.

Add Mojo NativeDisplayDelegate / NativeDisplayObserver.

For mushrome, in order to have screen management in one process and
Ozone in another process we need a mojom::NativeDisplayDelegate and
implementation. The ForwardingDisplayDelegate will be created in ash and
will connect to mus-ws. In mus-ws ScreenManagerForwarding will own the
real NativeDisplayDelegate. A mojom::NativeDisplayObserver works in the
other direction.

This only implements the Ozone DRM parts of NativeDisplayDelegate. Most
of the other methods exist for the X11 implementation which isn't used.

BUG=706589
Review-Url: https://codereview.chromium.org/2805633004
Cr-Commit-Position: refs/heads/master@{#463321}
Committed: https://chromium.googlesource.com/chromium/src/+/f47442541a1b4292f7c428717a90a15712f9c62b

[kylechar, 2017-04-06 17:44:14]

kylechar@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2017-04-06 20:30:21]

https://codereview.chromium.org/2805633004/diff/1/services/ui/display/screen_...
File services/ui/display/screen_manager_forwarding.cc (right):

https://codereview.chromium.org/2805633004/diff/1/services/ui/display/screen_...
services/ui/display/screen_manager_forwarding.cc:98:
weak_ptr_factory_.GetWeakPtr(), callback));
Similar comment here about not needing a weak_ptr_factory_.

https://codereview.chromium.org/2805633004/diff/1/services/ui/display/screen_...
File services/ui/display/screen_manager_forwarding.h (right):

https://codereview.chromium.org/2805633004/diff/1/services/ui/display/screen_...
services/ui/display/screen_manager_forwarding.h:25: // This will own a  real
NativeDisplayDelegate and forwards calls to and
'  ' -> ' '

https://codereview.chromium.org/2805633004/diff/1/services/ui/display/screen_...
services/ui/display/screen_manager_forwarding.h:91: std::unordered_map<int64_t,
DisplaySnapshot*> snapshot_map_;
Who owns the values in this map? It seems like they are leaked.

https://codereview.chromium.org/2805633004/diff/1/ui/display/manager/forwardi...
File ui/display/manager/forwarding_display_delegate.cc (right):

https://codereview.chromium.org/2805633004/diff/1/ui/display/manager/forwardi...
ui/display/manager/forwarding_display_delegate.cc:53:
weak_ptr_factory_.GetWeakPtr(), callback));
As delegate_ is owned by this class you shouldn't need the weak_ptr_factory_
here. Instead use base::Unretained.

https://codereview.chromium.org/2805633004/diff/1/ui/display/manager/forwardi...
ui/display/manager/forwarding_display_delegate.cc:129: snapshots_ =
std::move(snapshots);
AFACIT you never use these. Is it worth caching them? Should GetDisplays() use
the cached version under some conditions?

https://codereview.chromium.org/2805633004/diff/1/ui/display/mojo/native_disp...
File ui/display/mojo/native_display_delegate.mojom (right):

https://codereview.chromium.org/2805633004/diff/1/ui/display/mojo/native_disp...
ui/display/mojo/native_display_delegate.mojom:23: Initialize();
WDYT Of removing RegisterObserver and passing the observer here?

[kylechar, 2017-04-07 13:52:16]

https://codereview.chromium.org/2805633004/diff/1/services/ui/display/screen_...
File services/ui/display/screen_manager_forwarding.cc (right):

https://codereview.chromium.org/2805633004/diff/1/services/ui/display/screen_...
services/ui/display/screen_manager_forwarding.cc:98:
weak_ptr_factory_.GetWeakPtr(), callback));
On 2017/04/06 20:30:20, sky wrote:
> Similar comment here about not needing a weak_ptr_factory_.

Done.

https://codereview.chromium.org/2805633004/diff/1/services/ui/display/screen_...
File services/ui/display/screen_manager_forwarding.h (right):

https://codereview.chromium.org/2805633004/diff/1/services/ui/display/screen_...
services/ui/display/screen_manager_forwarding.h:25: // This will own a  real
NativeDisplayDelegate and forwards calls to and
On 2017/04/06 20:30:20, sky wrote:
> '  ' -> ' '

Done.

https://codereview.chromium.org/2805633004/diff/1/services/ui/display/screen_...
services/ui/display/screen_manager_forwarding.h:91: std::unordered_map<int64_t,
DisplaySnapshot*> snapshot_map_;
On 2017/04/06 20:30:20, sky wrote:
> Who owns the values in this map? It seems like they are leaked.

DisplaySnapshot* are passed out of NDD and it's assumed that.. something owns
them until they get updated the next time. It was totally asking for a use after
free, which is what I found a few months ago and had to fix in crbug.com/669226.

I added NativeDisplayObserver::OnDisplaySnapshotsInvalidated() so that observers
know to clear cached DisplaySnapshot*s. It was far from the ideal solution, but
the control flow in this area of the display code is complicated and we needed a
solution that could be merged back to M56 without breaking things.

A lot of the display configuration code needs to be rewritten/refactored soon.
Lots of the logic is for X11 Chromebooks, which no longer exist, so it could be
simplified a bunch.

https://codereview.chromium.org/2805633004/diff/1/ui/display/manager/forwardi...
File ui/display/manager/forwarding_display_delegate.cc (right):

https://codereview.chromium.org/2805633004/diff/1/ui/display/manager/forwardi...
ui/display/manager/forwarding_display_delegate.cc:53:
weak_ptr_factory_.GetWeakPtr(), callback));
On 2017/04/06 20:30:20, sky wrote:
> As delegate_ is owned by this class you shouldn't need the weak_ptr_factory_
> here. Instead use base::Unretained.

Done.

https://codereview.chromium.org/2805633004/diff/1/ui/display/manager/forwardi...
ui/display/manager/forwarding_display_delegate.cc:129: snapshots_ =
std::move(snapshots);
On 2017/04/06 20:30:21, sky wrote:
> AFACIT you never use these. Is it worth caching them? Should GetDisplays() use
> the cached version under some conditions?

Since this is the NDD implementation it needs to own the snapshots.
DisplaySnapshot*s eventually make their way back to DisplayConfigurator via a
series of callbacks. I think that UpdateDisplaysTask, DisplayChangeObserver and
DisplayConfigurator all use those pointers.

Also, the GetDisplays() method is kind of badly named. The results are cached in
DisplayConfigurator already. GetDisplays() is mostly called when we actually
want to refresh the set of displays because something has changed.

I updated the comment on |snapshots_|.

https://codereview.chromium.org/2805633004/diff/1/ui/display/mojo/native_disp...
File ui/display/mojo/native_display_delegate.mojom (right):

https://codereview.chromium.org/2805633004/diff/1/ui/display/mojo/native_disp...
ui/display/mojo/native_display_delegate.mojom:23: Initialize();
On 2017/04/06 20:30:21, sky wrote:
> WDYT Of removing RegisterObserver and passing the observer here?

I like that better. Done.

[sky, 2017-04-07 14:45:55]

LGTM

https://codereview.chromium.org/2805633004/diff/40001/services/ui/display/scr...
File services/ui/display/screen_manager_forwarding.h (right):

https://codereview.chromium.org/2805633004/diff/40001/services/ui/display/scr...
services/ui/display/screen_manager_forwarding.h:88: // Cached pointers to
snapshots owned by the |native_display_delegate_|.
Thanks for the comment!

https://codereview.chromium.org/2805633004/diff/40001/ui/display/manager/forw...
File ui/display/manager/forwarding_display_delegate.h (right):

https://codereview.chromium.org/2805633004/diff/40001/ui/display/manager/forw...
ui/display/manager/forwarding_display_delegate.h:30:
ForwardingDisplayDelegate(mojom::NativeDisplayDelegatePtr delegate);
explicit

[kylechar, 2017-04-07 14:49:51]

kylechar@chromium.org changed reviewers:
+ tsepez@chromium.org

[kylechar, 2017-04-07 14:49:51]

+tsepez for security review

https://codereview.chromium.org/2805633004/diff/40001/ui/display/manager/forw...
File ui/display/manager/forwarding_display_delegate.h (right):

https://codereview.chromium.org/2805633004/diff/40001/ui/display/manager/forw...
ui/display/manager/forwarding_display_delegate.h:30:
ForwardingDisplayDelegate(mojom::NativeDisplayDelegatePtr delegate);
On 2017/04/07 14:45:54, sky wrote:
> explicit

Done.

[Tom Sepez, 2017-04-07 17:30:11]

Are both of these processes equally privileged / trusted?

[kylechar, 2017-04-07 18:31:20]

On 2017/04/07 17:30:11, Tom Sepez wrote:
> Are both of these processes equally privileged / trusted?

Basically. Both mus+chrome process and mus-ws process are fully trusted in
mushrome. For mus+ash then I think there was talk that one day ash could be less
privileged than mus-ws but  I'm not sure on the specifics.

[Tom Sepez, 2017-04-07 20:30:43]

On 2017/04/07 18:31:20, kylechar wrote:
> On 2017/04/07 17:30:11, Tom Sepez wrote:
> > Are both of these processes equally privileged / trusted?
> 
> Basically. Both mus+chrome process and mus-ws process are fully trusted in
> mushrome. For mus+ash then I think there was talk that one day ash could be
less
> privileged than mus-ws but  I'm not sure on the specifics.

That's what I thought.  LGTM.

[kylechar, 2017-04-07 20:49:42]

The CQ bit was checked by kylechar@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 20:50:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 22:00:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-07 22:00:52]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[kylechar, 2017-04-10 13:09:35]

The CQ bit was checked by kylechar@chromium.org

[kylechar, 2017-04-10 13:09:36]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2805633004/#ps70001
(title: "NON_EXPORT_BASE.")

[commit-bot: I haz the power, 2017-04-10 13:09:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 13:14:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-10 13:14:11]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[kylechar, 2017-04-10 16:30:46]

The CQ bit was checked by kylechar@chromium.org

[kylechar, 2017-04-10 16:30:47]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2805633004/#ps90001
(title: "s/InterfaceRegister/BinderRegistry")

[commit-bot: I haz the power, 2017-04-10 16:31:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 18:01:19]

CQ is committing da patch.

Bot data: {"patchset_id": 90001, "attempt_start_ts": 1491841846600810,
"parent_rev": "903ebf3235f4057351f7734c0e148e7938009dbd", "commit_rev":
"f47442541a1b4292f7c428717a90a15712f9c62b"}

[commit-bot: I haz the power, 2017-04-10 18:02:05]

Description was changed from

==========
Add Mojo NativeDisplayDelegate / NativeDisplayObserver.

For mushrome, in order to have screen management in one process and
Ozone in another process we need a mojom::NativeDisplayDelegate and
implementation. The ForwardingDisplayDelegate will be created in ash and
will connect to mus-ws. In mus-ws ScreenManagerForwarding will own the
real NativeDisplayDelegate. A mojom::NativeDisplayObserver works in the
other direction.

This only implements the Ozone DRM parts of NativeDisplayDelegate. Most
of the other methods exist for the X11 implementation which isn't used.

BUG=706589
==========

to

==========
Add Mojo NativeDisplayDelegate / NativeDisplayObserver.

For mushrome, in order to have screen management in one process and
Ozone in another process we need a mojom::NativeDisplayDelegate and
implementation. The ForwardingDisplayDelegate will be created in ash and
will connect to mus-ws. In mus-ws ScreenManagerForwarding will own the
real NativeDisplayDelegate. A mojom::NativeDisplayObserver works in the
other direction.

This only implements the Ozone DRM parts of NativeDisplayDelegate. Most
of the other methods exist for the X11 implementation which isn't used.

BUG=706589

Review-Url: https://codereview.chromium.org/2805633004
Cr-Commit-Position: refs/heads/master@{#463321}
Committed:
https://chromium.googlesource.com/chromium/src/+/f47442541a1b4292f7c428717a90...
==========

[commit-bot: I haz the power, 2017-04-10 18:02:07]

Committed patchset #6 (id:90001) as
https://chromium.googlesource.com/chromium/src/+/f47442541a1b4292f7c428717a90...