mash: Create system tray display and rotation lock items via delegate

mash: Create system tray display and rotation lock items via delegate

This will allow SystemTray to be moved to //ash/common, and will allow mus
to have its own implementations. (Display management under mus will be quite
different compared to classic ash.)

BUG=619636, 548429
TEST=ash_unittests, run chrome and rotate screen and verify tray item appears

Committed: https://crrev.com/399a6f9bb3fd934d8fd97d4d805ef576ce83c4c5
Cr-Commit-Position: refs/heads/master@{#405340}

[James Cook, 2016-07-13 18:34:35]

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

[commit-bot: I haz the power, 2016-07-13 18:36:04]

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

[James Cook, 2016-07-13 18:37:43]

jamescook@chromium.org changed reviewers:
+ msw@chromium.org

[James Cook, 2016-07-13 18:37:44]

msw, please take a look.

https://codereview.chromium.org/2148943002/diff/1/ash/system/tray/system_tray.h
File ash/system/tray/system_tray.h (right):

https://codereview.chromium.org/2148943002/diff/1/ash/system/tray/system_tray...
ash/system/tray/system_tray.h:55: void AddTrayItem(SystemTrayItem* item);
I did not refactor this to take a std::unique_ptr because:
A) The function immediately sticks the |item| into ScopedVector |items_| and it
seemed wasteful to immediately release the pointer, and
B) Converting |items_| from ScopedVector to std::vector<std::unique_ptr> is not
a trivial conversion given how this class uses items_.get().  Probably
straightforward, but not trivial.

[msw, 2016-07-13 18:51:49]

Perhaps file a new mash display bug or cite some open one?

https://codereview.chromium.org/2148943002/diff/1/ash/common/system/tray/syst...
File ash/common/system/tray/system_tray_delegate.cc (right):

https://codereview.chromium.org/2148943002/diff/1/ash/common/system/tray/syst...
ash/common/system/tray/system_tray_delegate.cc:109: return nullptr;
nit: NOTIMPLEMENTED? (seems like others omit, so maybe okay as-is)

https://codereview.chromium.org/2148943002/diff/1/ash/common/system/tray/syst...
File ash/common/system/tray/system_tray_delegate.h (right):

https://codereview.chromium.org/2148943002/diff/1/ash/common/system/tray/syst...
ash/common/system/tray/system_tray_delegate.h:188: // Creates the system tray
item for display settings.
nit: Move far down (not amid Show* functions). Despite a minor relation to
ShowDisplaySettings(), it's rather distinct and a short-term measure that should
be removed later (right?). Maybe add a comment about the long-term plan here?

https://codereview.chromium.org/2148943002/diff/1/ash/system/tray/system_tray.cc
File ash/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2148943002/diff/1/ash/system/tray/system_tray...
ash/system/tray/system_tray.cc:200: std::unique_ptr<SystemTrayItem> tray_display
=
optional nit: add a comment?

https://codereview.chromium.org/2148943002/diff/1/ash/system/tray/system_tray.h
File ash/system/tray/system_tray.h (right):

https://codereview.chromium.org/2148943002/diff/1/ash/system/tray/system_tray...
ash/system/tray/system_tray.h:55: void AddTrayItem(SystemTrayItem* item);
On 2016/07/13 18:37:44, James Cook wrote:
> I did not refactor this to take a std::unique_ptr because:
> A) The function immediately sticks the |item| into ScopedVector |items_| and
it
> seemed wasteful to immediately release the pointer, and
> B) Converting |items_| from ScopedVector to std::vector<std::unique_ptr> is
not
> a trivial conversion given how this class uses items_.get().  Probably
> straightforward, but not trivial.

Acknowledged.

https://codereview.chromium.org/2148943002/diff/1/ash/test/test_system_tray_d...
File ash/test/test_system_tray_delegate.cc (right):

https://codereview.chromium.org/2148943002/diff/1/ash/test/test_system_tray_d...
ash/test/test_system_tray_delegate.cc:95: return base::WrapUnique(new
TrayDisplay(tray));
nit: MakeUnique

https://codereview.chromium.org/2148943002/diff/1/chrome/browser/ui/ash/syste...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/2148943002/diff/1/chrome/browser/ui/ash/syste...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:459: return
base::WrapUnique(new ash::TrayDisplay(tray));
nit: MakeUnique

[commit-bot: I haz the power, 2016-07-13 18:54:55]

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

[commit-bot: I haz the power, 2016-07-13 18:54:56]

Dry run: 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_...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[James Cook, 2016-07-13 21:25:30]

Description was changed from

==========
mash: Create system tray display item via delegate

This will allow SystemTray to be moved to //ash/common, and will allow mus
to have its own implementation. (Display management under mus will be quite
different compared to classic ash.)

BUG=619636
TEST=ash_unittests, run chrome and rotate screen and verify tray item appears
==========

to

==========
mash: Create system tray display and rotation lock items via delegate

This will allow SystemTray to be moved to //ash/common, and will allow mus
to have its own implementations. (Display management under mus will be quite
different compared to classic ash.)

BUG=619636
TEST=ash_unittests, run chrome and rotate screen and verify tray item appears
==========

[James Cook, 2016-07-13 21:30:10]

Description was changed from

==========
mash: Create system tray display and rotation lock items via delegate

This will allow SystemTray to be moved to //ash/common, and will allow mus
to have its own implementations. (Display management under mus will be quite
different compared to classic ash.)

BUG=619636
TEST=ash_unittests, run chrome and rotate screen and verify tray item appears
==========

to

==========
mash: Create system tray display and rotation lock items via delegate

This will allow SystemTray to be moved to //ash/common, and will allow mus
to have its own implementations. (Display management under mus will be quite
different compared to classic ash.)

BUG=619636,548429
TEST=ash_unittests, run chrome and rotate screen and verify tray item appears
==========

[James Cook, 2016-07-13 21:30:33]

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

[James Cook, 2016-07-13 21:30:46]

msw, please take another look.

https://codereview.chromium.org/2148943002/diff/1/ash/common/system/tray/syst...
File ash/common/system/tray/system_tray_delegate.cc (right):

https://codereview.chromium.org/2148943002/diff/1/ash/common/system/tray/syst...
ash/common/system/tray/system_tray_delegate.cc:109: return nullptr;
On 2016/07/13 18:51:49, msw wrote:
> nit: NOTIMPLEMENTED? (seems like others omit, so maybe okay as-is)

I kept it the same since none of the other ones use NOTIMPLEMENTED.

https://codereview.chromium.org/2148943002/diff/1/ash/common/system/tray/syst...
File ash/common/system/tray/system_tray_delegate.h (right):

https://codereview.chromium.org/2148943002/diff/1/ash/common/system/tray/syst...
ash/common/system/tray/system_tray_delegate.h:188: // Creates the system tray
item for display settings.
On 2016/07/13 18:51:49, msw wrote:
> nit: Move far down (not amid Show* functions). Despite a minor relation to
> ShowDisplaySettings(), it's rather distinct and a short-term measure that
should
> be removed later (right?). Maybe add a comment about the long-term plan here?

Done.

https://codereview.chromium.org/2148943002/diff/1/ash/system/tray/system_tray.cc
File ash/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2148943002/diff/1/ash/system/tray/system_tray...
ash/system/tray/system_tray.cc:200: std::unique_ptr<SystemTrayItem> tray_display
=
On 2016/07/13 18:51:49, msw wrote:
> optional nit: add a comment?

Done.

https://codereview.chromium.org/2148943002/diff/1/ash/test/test_system_tray_d...
File ash/test/test_system_tray_delegate.cc (right):

https://codereview.chromium.org/2148943002/diff/1/ash/test/test_system_tray_d...
ash/test/test_system_tray_delegate.cc:95: return base::WrapUnique(new
TrayDisplay(tray));
On 2016/07/13 18:51:49, msw wrote:
> nit: MakeUnique

Done.

https://codereview.chromium.org/2148943002/diff/1/chrome/browser/ui/ash/syste...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/2148943002/diff/1/chrome/browser/ui/ash/syste...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:459: return
base::WrapUnique(new ash::TrayDisplay(tray));
On 2016/07/13 18:51:49, msw wrote:
> nit: MakeUnique

Done.

[James Cook, 2016-07-13 21:31:09]

Description was changed from

==========
mash: Create system tray display and rotation lock items via delegate

This will allow SystemTray to be moved to //ash/common, and will allow mus
to have its own implementations. (Display management under mus will be quite
different compared to classic ash.)

BUG=619636,548429
TEST=ash_unittests, run chrome and rotate screen and verify tray item appears
==========

to

==========
mash: Create system tray display and rotation lock items via delegate

This will allow SystemTray to be moved to //ash/common, and will allow mus
to have its own implementations. (Display management under mus will be quite
different compared to classic ash.)

BUG=619636,548429
TEST=ash_unittests, run chrome and rotate screen and verify tray item appears
==========

[commit-bot: I haz the power, 2016-07-13 21:33: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, 2016-07-13 22:35:25]

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

[commit-bot: I haz the power, 2016-07-13 22:35:27]

Dry run: This issue passed the CQ dry run.

[msw, 2016-07-13 22:36:01]

lgtm

[James Cook, 2016-07-13 22:39:38]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2016-07-13 22:40:43]

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-07-13 23:08:24]

Description was changed from

==========
mash: Create system tray display and rotation lock items via delegate

This will allow SystemTray to be moved to //ash/common, and will allow mus
to have its own implementations. (Display management under mus will be quite
different compared to classic ash.)

BUG=619636,548429
TEST=ash_unittests, run chrome and rotate screen and verify tray item appears
==========

to

==========
mash: Create system tray display and rotation lock items via delegate

This will allow SystemTray to be moved to //ash/common, and will allow mus
to have its own implementations. (Display management under mus will be quite
different compared to classic ash.)

BUG=619636,548429
TEST=ash_unittests, run chrome and rotate screen and verify tray item appears
==========

[commit-bot: I haz the power, 2016-07-13 23:08:25]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-13 23:08:27]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 23:10:04]

Description was changed from

==========
mash: Create system tray display and rotation lock items via delegate

This will allow SystemTray to be moved to //ash/common, and will allow mus
to have its own implementations. (Display management under mus will be quite
different compared to classic ash.)

BUG=619636,548429
TEST=ash_unittests, run chrome and rotate screen and verify tray item appears
==========

to

==========
mash: Create system tray display and rotation lock items via delegate

This will allow SystemTray to be moved to //ash/common, and will allow mus
to have its own implementations. (Display management under mus will be quite
different compared to classic ash.)

BUG=619636,548429
TEST=ash_unittests, run chrome and rotate screen and verify tray item appears

Committed: https://crrev.com/399a6f9bb3fd934d8fd97d4d805ef576ce83c4c5
Cr-Commit-Position: refs/heads/master@{#405340}
==========

[commit-bot: I haz the power, 2016-07-13 23:10:05]

Patchset 2 (id:??) landed as
https://crrev.com/399a6f9bb3fd934d8fd97d4d805ef576ce83c4c5
Cr-Commit-Position: refs/heads/master@{#405340}