cros: Remove OS_CHROMEOS ifdefs in ash/common

cros: Remove OS_CHROMEOS ifdefs in ash/common

Ash only builds on Chrome OS these days, so it doesn't need platform
ifdefs.

The only thing remaining in ash/common is accelerators, which is a bigger
reshuffle.

BUG=666773
TEST=ash_unittests

Review-Url: https://codereview.chromium.org/2616023005
Cr-Commit-Position: refs/heads/master@{#442050}
Committed: https://chromium.googlesource.com/chromium/src/+/599dea53e8b6318344fd9d7375c9021d2bde0adb

[James Cook, 2017-01-06 05:19:46]

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

[commit-bot: I haz the power, 2017-01-06 05:20: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, 2017-01-06 05:21:25]

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

[James Cook, 2017-01-06 05:21:26]

msw, please take a look.

The only thing non-trivial is the reordering of member variables in WmShell,
which changes their construction/destruction order. I checked them and this
looks safe to me.

[commit-bot: I haz the power, 2017-01-06 05:49:31]

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

[commit-bot: I haz the power, 2017-01-06 05:49:32]

Dry run: This issue passed the CQ dry run.

[msw, 2017-01-06 18:22:51]

lgtm with a q, nit, and aside.

https://codereview.chromium.org/2616023005/diff/1/ash/common/shutdown_control...
File ash/common/shutdown_controller.cc (right):

https://codereview.chromium.org/2616023005/diff/1/ash/common/shutdown_control...
ash/common/shutdown_controller.cc:20: if (base::SysInfo::IsRunningOnChromeOS())
{
Weird, what is this? (It's not very well documented...) If it's just a way of
checking that it's not a desktop linux dev build of chrome os, can you modify
the comment below to explicitly mention dev builds or similar? (optionally
invert the check to move the comment up top)

https://codereview.chromium.org/2616023005/diff/1/ash/common/wm_shell.cc
File ash/common/wm_shell.cc (right):

https://codereview.chromium.org/2616023005/diff/1/ash/common/wm_shell.cc#newc...
ash/common/wm_shell.cc:374: logout_confirmation_controller_.reset(new
LogoutConfirmationController(
aside: logout_confirmation_controller_ lifetime doesn't seem like it belongs in
[Set|Delete]SystemTrayDelegate.

https://codereview.chromium.org/2616023005/diff/1/ash/common/wm_shell.h
File ash/common/wm_shell.h (right):

https://codereview.chromium.org/2616023005/diff/1/ash/common/wm_shell.h#newco...
ash/common/wm_shell.h:533: scoped_refptr<base::SequencedWorkerPool>
blocking_pool_;
nit: maybe sort this alphabetically too, or maybe unsort it's accessor (to keep
the member/accessor ordering consistent without mixing unique_ptr and refptr)?

[James Cook, 2017-01-06 20:33:05]

Thanks for the review.

https://codereview.chromium.org/2616023005/diff/1/ash/common/shutdown_control...
File ash/common/shutdown_controller.cc (right):

https://codereview.chromium.org/2616023005/diff/1/ash/common/shutdown_control...
ash/common/shutdown_controller.cc:20: if (base::SysInfo::IsRunningOnChromeOS())
{
On 2017/01/06 18:22:51, msw wrote:
> Weird, what is this? (It's not very well documented...) If it's just a way of
> checking that it's not a desktop linux dev build of chrome os, can you modify
> the comment below to explicitly mention dev builds or similar? (optionally
> invert the check to move the comment up top)

It's how we check if we're running on real chromeos hardware. Inverted the logic
and updated the comments.

https://codereview.chromium.org/2616023005/diff/1/ash/common/wm_shell.cc
File ash/common/wm_shell.cc (right):

https://codereview.chromium.org/2616023005/diff/1/ash/common/wm_shell.cc#newc...
ash/common/wm_shell.cc:374: logout_confirmation_controller_.reset(new
LogoutConfirmationController(
On 2017/01/06 18:22:51, msw wrote:
> aside: logout_confirmation_controller_ lifetime doesn't seem like it belongs
in
> [Set|Delete]SystemTrayDelegate.

Yeah, I think this is a side effect of how it used to exist in ash::Shell.

https://codereview.chromium.org/2616023005/diff/1/ash/common/wm_shell.h
File ash/common/wm_shell.h (right):

https://codereview.chromium.org/2616023005/diff/1/ash/common/wm_shell.h#newco...
ash/common/wm_shell.h:533: scoped_refptr<base::SequencedWorkerPool>
blocking_pool_;
On 2017/01/06 18:22:51, msw wrote:
> nit: maybe sort this alphabetically too, or maybe unsort it's accessor (to
keep
> the member/accessor ordering consistent without mixing unique_ptr and refptr)?

Unsorted accessor.

[James Cook, 2017-01-06 20:33:11]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2017-01-06 20:33:11]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2616023005/#ps20001
(title: "review comments")

[commit-bot: I haz the power, 2017-01-06 20:34:09]

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

[msw, 2017-01-06 20:34:20]

lgtm

[commit-bot: I haz the power, 2017-01-06 21:08:27]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1483734791630460,
"parent_rev": "901cf165f79e89e6e041cc413e1b47e20a8fe898", "commit_rev":
"599dea53e8b6318344fd9d7375c9021d2bde0adb"}

[commit-bot: I haz the power, 2017-01-06 21:08:56]

Description was changed from

==========
cros: Remove OS_CHROMEOS ifdefs in ash/common

Ash only builds on Chrome OS these days, so it doesn't need platform
ifdefs.

The only thing remaining in ash/common is accelerators, which is a bigger
reshuffle.

BUG=666773
TEST=ash_unittests
==========

to

==========
cros: Remove OS_CHROMEOS ifdefs in ash/common

Ash only builds on Chrome OS these days, so it doesn't need platform
ifdefs.

The only thing remaining in ash/common is accelerators, which is a bigger
reshuffle.

BUG=666773
TEST=ash_unittests

Review-Url: https://codereview.chromium.org/2616023005
Cr-Commit-Position: refs/heads/master@{#442050}
Committed:
https://chromium.googlesource.com/chromium/src/+/599dea53e8b6318344fd9d7375c9...
==========

[commit-bot: I haz the power, 2017-01-06 21:08:57]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/599dea53e8b6318344fd9d7375c9...