PlatformScreenOzone using DisplayManager

mustash: ScreenManagerOzone uses DCO and DM.

ScreenManagerOzone will uses DisplayChangeObserver and DisplayManager
for display management. DCO listens for changes to displays and forwards
those to DM. These are the same components used in classic ash.

BUG=671651
Committed: https://crrev.com/f4cdc4429dc5e480b35e88bb781e78fdc1592478
Cr-Commit-Position: refs/heads/master@{#440014}

[kylechar, 2016-12-06 16:39:15]

Description was changed from

==========
PlatformScreenOzone using DisplayManager

WIP.

BUG=TBD.
==========

to

==========
mustash: ScreenManagerOzone uses DCO and DM.

ScreenManagerOzone will uses DisplayChangeObserver and DisplayManager
for display management. DCO listens for changes to displays and forwards
those to DM. These are the same components used in classic ash.

BUG=671651
==========

[kylechar, 2016-12-13 19:47:39]

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

[rjkroege, 2016-12-13 23:56:37]

lgtm with nits.

https://codereview.chromium.org/2476663003/diff/80001/services/ui/display/scr...
File services/ui/display/screen_manager_ozone.cc (right):

https://codereview.chromium.org/2476663003/diff/80001/services/ui/display/scr...
services/ui/display/screen_manager_ozone.cc:33: void SwapRecursive(const
std::map<int64_t, DisplayPlacement*>& id_to_placement,
Please write more expansive docs about what this does.

Should the map of id to DisplayPlacement be a class?

And: I'm not too enthused about raw pointers. As the other bug demonstrated, raw
pointers have their issues.

https://codereview.chromium.org/2476663003/diff/80001/services/ui/display/scr...
services/ui/display/screen_manager_ozone.cc:56:
ScreenManagerOzone::~ScreenManagerOzone() {
should we insert the configuration of the external console here?

https://codereview.chromium.org/2476663003/diff/80001/services/ui/display/scr...
services/ui/display/screen_manager_ozone.cc:214: if
(Display::HasInternalDisplay() &&
Observation not needing to be addressed: why is HasInternalDisplay a method on a
Display? Shouldn't this be on Screen?

https://codereview.chromium.org/2476663003/diff/80001/services/ui/display/scr...
services/ui/display/screen_manager_ozone.cc:220: const ManagedDisplayInfo& info
=
Managed* should (ideally) become an internal detail of DisplayManager. So this
code here is unfortunate. Can you add a TODO to move it to DisplayManager?

https://codereview.chromium.org/2476663003/diff/80001/services/ui/display/scr...
services/ui/display/screen_manager_ozone.cc:295: const ManagedDisplayInfo&
managed_info =
same comment about ManagedDisplayInfo as above.

https://codereview.chromium.org/2476663003/diff/80001/services/ui/display/scr...
services/ui/display/screen_manager_ozone.cc:323: void
ScreenManagerOzone::PostDisplayConfigurationChange(
I think we need some UML sequence diagrams for this code at some point. :-)

[kylechar, 2016-12-16 21:25:08]

https://codereview.chromium.org/2476663003/diff/80001/services/ui/display/scr...
File services/ui/display/screen_manager_ozone.cc (right):

https://codereview.chromium.org/2476663003/diff/80001/services/ui/display/scr...
services/ui/display/screen_manager_ozone.cc:33: void SwapRecursive(const
std::map<int64_t, DisplayPlacement*>& id_to_placement,
On 2016/12/13 23:56:36, rjkroege wrote:
> Please write more expansive docs about what this does.
> 
> Should the map of id to DisplayPlacement be a class?
> 
> And: I'm not too enthused about raw pointers. As the other bug demonstrated,
raw
> pointers have their issues.

It's swapping DisplayPlacements owned by the DisplayLayout, hence the raw
pointers. I added better comments and will remove this eventually. It should all
happen in the DisplayLayout object anyways I think.

https://codereview.chromium.org/2476663003/diff/80001/services/ui/display/scr...
services/ui/display/screen_manager_ozone.cc:56:
ScreenManagerOzone::~ScreenManagerOzone() {
On 2016/12/13 23:56:36, rjkroege wrote:
> should we insert the configuration of the external console here?

Maybe.. You mean shit+ctrl+f2 console right? Isn't that running separately from
Chrome?

https://codereview.chromium.org/2476663003/diff/80001/services/ui/display/scr...
services/ui/display/screen_manager_ozone.cc:214: if
(Display::HasInternalDisplay() &&
On 2016/12/13 23:56:36, rjkroege wrote:
> Observation not needing to be addressed: why is HasInternalDisplay a method on
a
> Display? Shouldn't this be on Screen?

That would make things too easy probably :)

https://codereview.chromium.org/2476663003/diff/80001/services/ui/display/scr...
services/ui/display/screen_manager_ozone.cc:220: const ManagedDisplayInfo& info
=
On 2016/12/13 23:56:36, rjkroege wrote:
> Managed* should (ideally) become an internal detail of DisplayManager. So this
> code here is unfortunate. Can you add a TODO to move it to DisplayManager?

I don't think we'll be able to get away from using ManagedDisplayInfo here
easily. Lots of things in DisplayManager work in terms of it, so I think
ScreenManager is the best point to convert data types to something more sane?

https://codereview.chromium.org/2476663003/diff/80001/services/ui/display/scr...
services/ui/display/screen_manager_ozone.cc:323: void
ScreenManagerOzone::PostDisplayConfigurationChange(
On 2016/12/13 23:56:36, rjkroege wrote:
> I think we need some UML sequence diagrams for this code at some point. :-)

Agreed :)

[kylechar, 2016-12-16 21:26:46]

kylechar@chromium.org changed reviewers:
+ derat@chromium.org, msw@chromium.org

[kylechar, 2016-12-16 21:26:47]

+msw for services/ui/BUILD.gn
+derat for ui/display/manager/*

[Daniel Erat, 2016-12-16 23:43:45]

lgtm for ui/display/

[msw, 2016-12-19 20:02:52]

services/ui/BUILD.gn lgtm; I glanced briefly beyond the BUILD.gn file and have
minor nits/tips.

https://codereview.chromium.org/2476663003/diff/100001/services/ui/display/sc...
File services/ui/display/screen_manager_ozone.cc (right):

https://codereview.chromium.org/2476663003/diff/100001/services/ui/display/sc...
services/ui/display/screen_manager_ozone.cc:35: // TODO(kylechar): This is
copied from WTHM. The concept of getting the same
nit: Try to avoid acronyms (ditto for NDD below, and elsewhere). They make it
harder to find/update instances of specific class names and isn't precise (I
don't know what WTHM is). Often, acronyms for proper nouns can often be replaced
with simpler common terms, like 'delegate', when the meaning is obvious and
you'd prefer some shorthand. For example:
  // Tests may inject a delegate, otherwise get it from OzonePlatform.
  if (!native_display_delegate_)...

https://codereview.chromium.org/2476663003/diff/100001/services/ui/display/sc...
File services/ui/display/screen_manager_ozone.h (right):

https://codereview.chromium.org/2476663003/diff/100001/services/ui/display/sc...
services/ui/display/screen_manager_ozone.h:82: // DisplayManager::Delegate:
optional nit: match the order of overrides with the order of subclasses listed
up top (ie DisplayObserver, DisplayManager::Delegate, then the InterfaceFactory
functions)

https://codereview.chromium.org/2476663003/diff/100001/ui/display/manager/dis...
File ui/display/manager/display_manager.h (right):

https://codereview.chromium.org/2476663003/diff/100001/ui/display/manager/dis...
ui/display/manager/display_manager.h:111: // When set to true, DisplayManager
will use DisplayConfigurator to configure
nit: It's nice to have one clear comment on the member and omit a comment
altogether (avoid duplication) on the simple accessor, and it's nice when the
accessor order matches the member ordering, but I see that's neither pattern is
really employed here, so I guess it's fine as-is.

[kylechar, 2016-12-20 14:58:38]

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

[commit-bot: I haz the power, 2016-12-20 14:58:54]

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, 2016-12-20 15:14:22]

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

[commit-bot: I haz the power, 2016-12-20 15:14:23]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kylechar, 2016-12-20 15:19:46]

https://codereview.chromium.org/2476663003/diff/100001/services/ui/display/sc...
File services/ui/display/screen_manager_ozone.cc (right):

https://codereview.chromium.org/2476663003/diff/100001/services/ui/display/sc...
services/ui/display/screen_manager_ozone.cc:35: // TODO(kylechar): This is
copied from WTHM. The concept of getting the same
On 2016/12/19 20:02:52, msw wrote:
> nit: Try to avoid acronyms (ditto for NDD below, and elsewhere). They make it
> harder to find/update instances of specific class names and isn't precise (I
> don't know what WTHM is). Often, acronyms for proper nouns can often be
replaced
> with simpler common terms, like 'delegate', when the meaning is obvious and
> you'd prefer some shorthand. For example:
>   // Tests may inject a delegate, otherwise get it from OzonePlatform.
>   if (!native_display_delegate_)... 

Done.

https://codereview.chromium.org/2476663003/diff/100001/services/ui/display/sc...
File services/ui/display/screen_manager_ozone.h (right):

https://codereview.chromium.org/2476663003/diff/100001/services/ui/display/sc...
services/ui/display/screen_manager_ozone.h:82: // DisplayManager::Delegate:
On 2016/12/19 20:02:52, msw wrote:
> optional nit: match the order of overrides with the order of subclasses listed
> up top (ie DisplayObserver, DisplayManager::Delegate, then the
InterfaceFactory
> functions)

Done.

https://codereview.chromium.org/2476663003/diff/100001/ui/display/manager/dis...
File ui/display/manager/display_manager.h (right):

https://codereview.chromium.org/2476663003/diff/100001/ui/display/manager/dis...
ui/display/manager/display_manager.h:111: // When set to true, DisplayManager
will use DisplayConfigurator to configure
On 2016/12/19 20:02:52, msw wrote:
> nit: It's nice to have one clear comment on the member and omit a comment
> altogether (avoid duplication) on the simple accessor, and it's nice when the
> accessor order matches the member ordering, but I see that's neither pattern
is
> really employed here, so I guess it's fine as-is.

Good point. Odds are both the setter/member variable comments won't get updated
in sync. Removed the setter comment.

[kylechar, 2016-12-20 15:19:55]

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

[commit-bot: I haz the power, 2016-12-20 15:20:22]

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, 2016-12-20 16:10:14]

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

[commit-bot: I haz the power, 2016-12-20 16:10:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kylechar, 2016-12-20 18:03:44]

The CQ bit was checked by kylechar@chromium.org

[kylechar, 2016-12-20 18:03:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
rjkroege@chromium.org, msw@chromium.org, derat@chromium.org
Link to the patchset: https://codereview.chromium.org/2476663003/#ps160001
(title: "Add missing data_dep.")

[commit-bot: I haz the power, 2016-12-20 18:04:05]

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, 2016-12-20 21:11:31]

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

[commit-bot: I haz the power, 2016-12-20 21:11:32]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kylechar, 2016-12-20 21:18:19]

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

[commit-bot: I haz the power, 2016-12-20 21:19:05]

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, 2016-12-20 22:55:05]

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

[commit-bot: I haz the power, 2016-12-20 22:55:06]

Dry run: This issue passed the CQ dry run.

[kylechar, 2016-12-21 04:24:57]

The CQ bit was checked by kylechar@chromium.org

[commit-bot: I haz the power, 2016-12-21 04:25:13]

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, 2016-12-21 04:29:22]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1482294297267920,
"parent_rev": "11c8088392716ac900d8354cc03b062c59feed45", "commit_rev":
"a45f21c67bcafcf916274ab718676add7a63d860"}

[commit-bot: I haz the power, 2016-12-21 04:29:52]

Description was changed from

==========
mustash: ScreenManagerOzone uses DCO and DM.

ScreenManagerOzone will uses DisplayChangeObserver and DisplayManager
for display management. DCO listens for changes to displays and forwards
those to DM. These are the same components used in classic ash.

BUG=671651
==========

to

==========
mustash: ScreenManagerOzone uses DCO and DM.

ScreenManagerOzone will uses DisplayChangeObserver and DisplayManager
for display management. DCO listens for changes to displays and forwards
those to DM. These are the same components used in classic ash.

BUG=671651

Review-Url: https://codereview.chromium.org/2476663003
==========

[commit-bot: I haz the power, 2016-12-21 04:29:53]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-12-21 04:31:25]

Description was changed from

==========
mustash: ScreenManagerOzone uses DCO and DM.

ScreenManagerOzone will uses DisplayChangeObserver and DisplayManager
for display management. DCO listens for changes to displays and forwards
those to DM. These are the same components used in classic ash.

BUG=671651

Review-Url: https://codereview.chromium.org/2476663003
==========

to

==========
mustash: ScreenManagerOzone uses DCO and DM.

ScreenManagerOzone will uses DisplayChangeObserver and DisplayManager
for display management. DCO listens for changes to displays and forwards
those to DM. These are the same components used in classic ash.

BUG=671651

Committed: https://crrev.com/f4cdc4429dc5e480b35e88bb781e78fdc1592478
Cr-Commit-Position: refs/heads/master@{#440014}
==========

[commit-bot: I haz the power, 2016-12-21 04:31:27]

Patchset 9 (id:??) landed as
https://crrev.com/f4cdc4429dc5e480b35e88bb781e78fdc1592478
Cr-Commit-Position: refs/heads/master@{#440014}