Primary display change notifications.

Handle primary display changes in mustash.

This CL adds code to propagate the primary display id from
PlatformScreen to the WS, then from the WS to all mus clients.

1. Rename display.mojom to display_manager.mojom to reflect the main
   interface name.
2. Add primary and internal display id to the OnDisplays() message.
3. Add OnPrimaryDisplayChanged() message for when the primary display
   changes.
4. Improve platform_screen_ozone_unittests. Track the ordering of events
   to ensure the expected events happen in the expected order.
5. Fix some ordering problems with PlatformScreenOzone.

BUG=657125
Committed: https://crrev.com/100dccedb99065973e21ecf0661d123e719784e1
Cr-Commit-Position: refs/heads/master@{#428517}

[kylechar, 2016-10-27 20:43:50]

Description was changed from

==========
Primary display change notifications.

WIP.

BUG=657125
==========

to

==========
Handle primary display changes in mustash.

This CL adds code to propagate the primary display id from
PlatformScreen to the WS, then from the WS to all mus clients.

1. Rename display.mojom to display_manager.mojom to reflect the main
   interface name.
2. Add primary and internal display id to the OnDisplays() message.
3. Add OnPrimaryDisplayChanged() message for when the primary display
   changes.
4. Change the keyboard shortcut to print the WS window tree hierarchy to
   Ctrl-Shift-Alt-b instead of Ctrl-Shift-Alt-s. The current shortcut
   conflicts with an existing ash shortcut.
5. Improve platform_screen_ozone_unittests. Track the ordering of events
   to ensure the expected events happen in the expected order.
6. Fix some ordering problems with PlatformScreenOzone.

BUG=657125
==========

[kylechar, 2016-10-28 13:57:56]

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

[kylechar, 2016-10-28 13:57:56]

sky: services/ui/*
tsepez: *.mojom

[Tom Sepez, 2016-10-28 16:26:10]

lgtm

[sky, 2016-10-28 17:08:47]

https://codereview.chromium.org/2461513002/diff/20001/services/ui/display/pla...
File services/ui/display/platform_screen_delegate.h (right):

https://codereview.chromium.org/2461513002/diff/20001/services/ui/display/pla...
services/ui/display/platform_screen_delegate.h:38: // to be removed then
|primary_display_id| will be set to kInvalidDisplayID
Is it necessary that OnPrimaryDisplayChanged() is sent in the case of a removal
of the last display? That seems mildly confusing and would think the
OnDisplayRemoved of the last display implies there is no primary display.

https://codereview.chromium.org/2461513002/diff/20001/services/ui/display/pla...
File services/ui/display/platform_screen_ozone.cc (right):

https://codereview.chromium.org/2461513002/diff/20001/services/ui/display/pla...
services/ui/display/platform_screen_ozone.cc:333: for (const DisplayInfo&
display : cached_displays_) {
Do you need to handle the case of no more displays here? That is, should this
set primary_display_id_ to invalid before the for loop?

https://codereview.chromium.org/2461513002/diff/20001/services/ui/display/pla...
File services/ui/display/platform_screen_ozone_unittests.cc (right):

https://codereview.chromium.org/2461513002/diff/20001/services/ui/display/pla...
services/ui/display/platform_screen_ozone_unittests.cc:73: std::string changes()
{ return changes_; }
const std::string& changes() const
Please also document what the format of this is.

https://codereview.chromium.org/2461513002/diff/20001/services/ui/public/inte...
File services/ui/public/interfaces/display_manager.mojom (right):

https://codereview.chromium.org/2461513002/diff/20001/services/ui/public/inte...
services/ui/public/interfaces/display_manager.mojom:11: // |internal_display_id|
will be the display id of an existing display or
It's not clear what you mean by internal in this context?

https://codereview.chromium.org/2461513002/diff/20001/services/ui/public/inte...
services/ui/public/interfaces/display_manager.mojom:28:
OnPrimaryDisplayChanged(int64 primary_display_id);
Similar comment to PlatformScreenDelegate.

https://codereview.chromium.org/2461513002/diff/20001/services/ui/ws/user_dis...
File services/ui/ws/user_display_manager_unittest.cc (right):

https://codereview.chromium.org/2461513002/diff/20001/services/ui/ws/user_dis...
services/ui/ws/user_display_manager_unittest.cc:55: PlatformScreenTest*
platform_screen = new PlatformScreenTest();
If you create PlatformScreenTest outside of the test environment it means it
lives across tests, and can easily lead to flake in the future. Make it owned by
each test and destroyed at the end of the test, probably as a member of
UserDisplayManagerTest. Also, I would prefix the name with Test rather than
using a suffix of Test. Having of a suffix makes it look like it's an actual
test (e.g. UserDisplayManagerTest), which isn't the case.

https://codereview.chromium.org/2461513002/diff/20001/services/ui/ws/window_m...
File services/ui/ws/window_manager_state.cc (right):

https://codereview.chromium.org/2461513002/diff/20001/services/ui/ws/window_m...
services/ui/ws/window_manager_state.cc:439: DebugAcceleratorType::PRINT_WINDOWS,
ui::VKEY_B,
Is there a reason you're changing this?

https://codereview.chromium.org/2461513002/diff/20001/ui/views/mus/screen_mus.cc
File ui/views/mus/screen_mus.cc (right):

https://codereview.chromium.org/2461513002/diff/20001/ui/views/mus/screen_mus...
ui/views/mus/screen_mus.cc:116: 
DCHECK found primary display?

[kylechar, 2016-10-28 19:40:37]

https://codereview.chromium.org/2461513002/diff/20001/services/ui/display/pla...
File services/ui/display/platform_screen_delegate.h (right):

https://codereview.chromium.org/2461513002/diff/20001/services/ui/display/pla...
services/ui/display/platform_screen_delegate.h:38: // to be removed then
|primary_display_id| will be set to kInvalidDisplayID
On 2016/10/28 17:08:47, sky wrote:
> Is it necessary that OnPrimaryDisplayChanged() is sent in the case of a
removal
> of the last display? That seems mildly confusing and would think the
> OnDisplayRemoved of the last display implies there is no primary display.

Not necessary to do it one way or the other. Changed.

https://codereview.chromium.org/2461513002/diff/20001/services/ui/display/pla...
File services/ui/display/platform_screen_ozone.cc (right):

https://codereview.chromium.org/2461513002/diff/20001/services/ui/display/pla...
services/ui/display/platform_screen_ozone.cc:333: for (const DisplayInfo&
display : cached_displays_) {
On 2016/10/28 17:08:47, sky wrote:
> Do you need to handle the case of no more displays here? That is, should this
> set primary_display_id_ to invalid before the for loop?

That's handled when the last primary display is marked removed,
primary_display_id_ will have been set to kInvalidDisplayID. If there is no
other displays it will stay as kInvalidDisplayID. 

I've changed this not to call OnPrimaryDisplayChanged() for kInvalidDisplayID
though as per your other comments.

https://codereview.chromium.org/2461513002/diff/20001/services/ui/display/pla...
File services/ui/display/platform_screen_ozone_unittests.cc (right):

https://codereview.chromium.org/2461513002/diff/20001/services/ui/display/pla...
services/ui/display/platform_screen_ozone_unittests.cc:73: std::string changes()
{ return changes_; }
On 2016/10/28 17:08:47, sky wrote:
> const std::string& changes() const
> Please also document what the format of this is.

Done.

https://codereview.chromium.org/2461513002/diff/20001/services/ui/public/inte...
File services/ui/public/interfaces/display_manager.mojom (right):

https://codereview.chromium.org/2461513002/diff/20001/services/ui/public/inte...
services/ui/public/interfaces/display_manager.mojom:11: // |internal_display_id|
will be the display id of an existing display or
On 2016/10/28 17:08:47, sky wrote:
> It's not clear what you mean by internal in this context?

Done.

https://codereview.chromium.org/2461513002/diff/20001/services/ui/public/inte...
services/ui/public/interfaces/display_manager.mojom:28:
OnPrimaryDisplayChanged(int64 primary_display_id);
On 2016/10/28 17:08:47, sky wrote:
> Similar comment to PlatformScreenDelegate.

Done.

https://codereview.chromium.org/2461513002/diff/20001/services/ui/ws/user_dis...
File services/ui/ws/user_display_manager_unittest.cc (right):

https://codereview.chromium.org/2461513002/diff/20001/services/ui/ws/user_dis...
services/ui/ws/user_display_manager_unittest.cc:55: PlatformScreenTest*
platform_screen = new PlatformScreenTest();
On 2016/10/28 17:08:47, sky wrote:
> If you create PlatformScreenTest outside of the test environment it means it
> lives across tests, and can easily lead to flake in the future. Make it owned
by
> each test and destroyed at the end of the test, probably as a member of
> UserDisplayManagerTest. Also, I would prefix the name with Test rather than
> using a suffix of Test. Having of a suffix makes it look like it's an actual
> test (e.g. UserDisplayManagerTest), which isn't the case.

Done and done.

https://codereview.chromium.org/2461513002/diff/20001/services/ui/ws/window_m...
File services/ui/ws/window_manager_state.cc (right):

https://codereview.chromium.org/2461513002/diff/20001/services/ui/ws/window_m...
services/ui/ws/window_manager_state.cc:439: DebugAcceleratorType::PRINT_WINDOWS,
ui::VKEY_B,
On 2016/10/28 17:08:47, sky wrote:
> Is there a reason you're changing this?

There is an existing ash shortcut that's mapped to Ctrl+Shift+Alt+S. I'm not
sure which is better to change?

https://codereview.chromium.org/2461513002/diff/20001/ui/views/mus/screen_mus.cc
File ui/views/mus/screen_mus.cc (right):

https://codereview.chromium.org/2461513002/diff/20001/ui/views/mus/screen_mus...
ui/views/mus/screen_mus.cc:116: 
On 2016/10/28 17:08:47, sky wrote:
> DCHECK found primary display?

Done.

[sky, 2016-10-28 19:59:37]

Only one question about accelerator.

https://codereview.chromium.org/2461513002/diff/20001/services/ui/ws/window_m...
File services/ui/ws/window_manager_state.cc (right):

https://codereview.chromium.org/2461513002/diff/20001/services/ui/ws/window_m...
services/ui/ws/window_manager_state.cc:439: DebugAcceleratorType::PRINT_WINDOWS,
ui::VKEY_B,
On 2016/10/28 19:40:37, kylechar wrote:
> On 2016/10/28 17:08:47, sky wrote:
> > Is there a reason you're changing this?
> 
> There is an existing ash shortcut that's mapped to Ctrl+Shift+Alt+S. I'm not
> sure which is better to change?

I tend to think we should make it the same as that in ash for 
PRINT_UI_HIERARCHIES as if you're executing that accelerator you likely want to
see all the windows too. WDYT?

https://codereview.chromium.org/2461513002/diff/40001/services/ui/display/pla...
File services/ui/display/platform_screen_ozone_unittests.cc (right):

https://codereview.chromium.org/2461513002/diff/40001/services/ui/display/pla...
services/ui/display/platform_screen_ozone_unittests.cc:71: const
std::vector<DisplayState>& added() { return added_; }
const functions too?

https://codereview.chromium.org/2461513002/diff/40001/services/ui/ws/user_dis...
File services/ui/ws/user_display_manager_unittest.cc (right):

https://codereview.chromium.org/2461513002/diff/40001/services/ui/ws/user_dis...
services/ui/ws/user_display_manager_unittest.cc:126:
std::unique_ptr<display::PlatformScreen> platform_screen_;
optional: nuke unique_ptr and use value here.

[kylechar, 2016-10-28 20:43:53]

https://codereview.chromium.org/2461513002/diff/40001/services/ui/display/pla...
File services/ui/display/platform_screen_ozone_unittests.cc (right):

https://codereview.chromium.org/2461513002/diff/40001/services/ui/display/pla...
services/ui/display/platform_screen_ozone_unittests.cc:71: const
std::vector<DisplayState>& added() { return added_; }
On 2016/10/28 19:59:36, sky wrote:
> const functions too?

Done.

https://codereview.chromium.org/2461513002/diff/40001/services/ui/ws/user_dis...
File services/ui/ws/user_display_manager_unittest.cc (right):

https://codereview.chromium.org/2461513002/diff/40001/services/ui/ws/user_dis...
services/ui/ws/user_display_manager_unittest.cc:126:
std::unique_ptr<display::PlatformScreen> platform_screen_;
On 2016/10/28 19:59:36, sky wrote:
> optional: nuke unique_ptr and use value here.

Done.

[kylechar, 2016-10-28 20:48:00]

On 2016/10/28 19:59:37, sky wrote:
> I tend to think we should make it the same as that in ash for 
> PRINT_UI_HIERARCHIES as if you're executing that accelerator you likely want
to
> see all the windows too. WDYT?

That seems reasonable although maybe the log statements could clarify what they
are printing out? Both trees look pretty similar and it could be confusing. I
removed the change from this CL, can do that separately to make it clear why
it's happening, either way.

[kylechar, 2016-10-28 20:49:05]

Description was changed from

==========
Handle primary display changes in mustash.

This CL adds code to propagate the primary display id from
PlatformScreen to the WS, then from the WS to all mus clients.

1. Rename display.mojom to display_manager.mojom to reflect the main
   interface name.
2. Add primary and internal display id to the OnDisplays() message.
3. Add OnPrimaryDisplayChanged() message for when the primary display
   changes.
4. Change the keyboard shortcut to print the WS window tree hierarchy to
   Ctrl-Shift-Alt-b instead of Ctrl-Shift-Alt-s. The current shortcut
   conflicts with an existing ash shortcut.
5. Improve platform_screen_ozone_unittests. Track the ordering of events
   to ensure the expected events happen in the expected order.
6. Fix some ordering problems with PlatformScreenOzone.

BUG=657125
==========

to

==========
Handle primary display changes in mustash.

This CL adds code to propagate the primary display id from
PlatformScreen to the WS, then from the WS to all mus clients.

1. Rename display.mojom to display_manager.mojom to reflect the main
   interface name.
2. Add primary and internal display id to the OnDisplays() message.
3. Add OnPrimaryDisplayChanged() message for when the primary display
   changes.
4. Improve platform_screen_ozone_unittests. Track the ordering of events
   to ensure the expected events happen in the expected order.
5. Fix some ordering problems with PlatformScreenOzone.

BUG=657125
==========

[sky, 2016-10-28 20:55:02]

LGTM

[kylechar, 2016-10-28 20:56:59]

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

[commit-bot: I haz the power, 2016-10-28 20:57:28]

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

[kylechar, 2016-10-28 21:51:25]

The CQ bit was unchecked by kylechar@chromium.org

[kylechar, 2016-10-28 21:51:26]

The CQ bit was checked by kylechar@chromium.org

[kylechar, 2016-10-28 21:51:26]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2461513002/#ps60001
(title: "More fixes.")

[commit-bot: I haz the power, 2016-10-28 21:51:41]

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-10-28 22:19:38]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-28 22:22:55]

Description was changed from

==========
Handle primary display changes in mustash.

This CL adds code to propagate the primary display id from
PlatformScreen to the WS, then from the WS to all mus clients.

1. Rename display.mojom to display_manager.mojom to reflect the main
   interface name.
2. Add primary and internal display id to the OnDisplays() message.
3. Add OnPrimaryDisplayChanged() message for when the primary display
   changes.
4. Improve platform_screen_ozone_unittests. Track the ordering of events
   to ensure the expected events happen in the expected order.
5. Fix some ordering problems with PlatformScreenOzone.

BUG=657125
==========

to

==========
Handle primary display changes in mustash.

This CL adds code to propagate the primary display id from
PlatformScreen to the WS, then from the WS to all mus clients.

1. Rename display.mojom to display_manager.mojom to reflect the main
   interface name.
2. Add primary and internal display id to the OnDisplays() message.
3. Add OnPrimaryDisplayChanged() message for when the primary display
   changes.
4. Improve platform_screen_ozone_unittests. Track the ordering of events
   to ensure the expected events happen in the expected order.
5. Fix some ordering problems with PlatformScreenOzone.

BUG=657125

Committed: https://crrev.com/100dccedb99065973e21ecf0661d123e719784e1
Cr-Commit-Position: refs/heads/master@{#428517}
==========

[commit-bot: I haz the power, 2016-10-28 22:22:57]

Patchset 4 (id:??) landed as
https://crrev.com/100dccedb99065973e21ecf0661d123e719784e1
Cr-Commit-Position: refs/heads/master@{#428517}