mash: Migrate [Keyboard]Brightness code to ash/common.

mash: Migrate [Keyboard]Brightness code to ash/common.

Move BrightnessControlDelegate ownership to WmShell.
(it is not closely related to AcceleratorController)

tools/git/move_source_file.py ash/system/brightness_control_delegate.h ash/common/system
tools/git/move_source_file.py ash/system/keyboard_brightness/keyboard_brightness_control_delegate.h ash/common/system
tools/git/move_source_file.py ash/system/chromeos/keyboard_brightness_controller* ash/common/system/chromeos
tools/git/move_source_file.py ash/system/chromeos/brightness/* ash/common/system/chromeos/brightness

BUG=619636
TEST=compiles; unit tests; no behavior changes.
R=jamescook@chromium.org

Committed: https://crrev.com/308af3930bd5a7f2d85ce76fb76b1c1a8eec9e11
Cr-Commit-Position: refs/heads/master@{#404439}

[msw, 2016-07-07 22:39:09]

Description was changed from

==========
mash: Migrate [Keyboard]Brightness code to ash/common.

BUG=619636
TEST=compiles; unit tests; no behavior changes.
R=jamescook@chromium.org
==========

to

==========
mash: Migrate [Keyboard]Brightness code to ash/common.

Move BrightnessControlDelegate ownership to WmShell.
(it is not closely related to AcceleratorController)

tools/git/move_source_file.py ash/system/brightness_control_delegate.h
ash/common/system
tools/git/move_source_file.py
ash/system/keyboard_brightness/keyboard_brightness_control_delegate.h
ash/common/system
tools/git/move_source_file.py
ash/system/chromeos/keyboard_brightness_controller* ash/common/system/chromeos
tools/git/move_source_file.py ash/system/chromeos/brightness/*
ash/common/system/chromeos/brightness

BUG=619636
TEST=compiles; unit tests; no behavior changes.
R=jamescook@chromium.org
==========

[msw, 2016-07-07 22:41:59]

Hey James, please take a look; thanks!

https://codereview.chromium.org/2131733002/diff/40001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller.h (right):

https://codereview.chromium.org/2131733002/diff/40001/ash/accelerators/accele...
ash/accelerators/accelerator_controller.h:174:
keyboard_brightness_control_delegate_;
I could also move this to WmShell for consistency, but it's not necessary nor
clearly better; wdyt?

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

https://codereview.chromium.org/2131733002/diff/40001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:250: friend class test::AshTestBase;
I could instead make SetBrightnessControlDelegate public, add "friend class
AcceleratorControllerTest" with a forward decl, or something else(?); wdyt?

https://codereview.chromium.org/2131733002/diff/40001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2131733002/diff/40001/ash/shell.cc#newcode1076
ash/shell.cc:1076: WmShell::Get()->SetBrightnessControlDelegate(
I could move this to WmShell's ctor or similar; wdyt?

[msw, 2016-07-07 22:42:46]

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

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

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

https://codereview.chromium.org/2131733002/diff/40001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller.h (right):

https://codereview.chromium.org/2131733002/diff/40001/ash/accelerators/accele...
ash/accelerators/accelerator_controller.h:174:
keyboard_brightness_control_delegate_;
On 2016/07/07 22:41:59, msw wrote:
> I could also move this to WmShell for consistency, but it's not necessary nor
> clearly better; wdyt?

Yeah, I would move it. It exists for tests and to have someone own
KeyboardBrightnessController. I think it makes sense to keep it with the other
one.

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

https://codereview.chromium.org/2131733002/diff/40001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:250: friend class test::AshTestBase;
On 2016/07/07 22:41:59, msw wrote:
> I could instead make SetBrightnessControlDelegate public, add "friend class
> AcceleratorControllerTest" with a forward decl, or something else(?); wdyt?

I would friend AcceleratorControllerTest.

https://codereview.chromium.org/2131733002/diff/40001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2131733002/diff/40001/ash/shell.cc#newcode1076
ash/shell.cc:1076: WmShell::Get()->SetBrightnessControlDelegate(
On 2016/07/07 22:41:59, msw wrote:
> I could move this to WmShell's ctor or similar; wdyt?

I would try moving it to WmShell ctor. If that works, it's what I would prefer.

https://codereview.chromium.org/2131733002/diff/40001/ash/system/tray/system_...
File ash/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2131733002/diff/40001/ash/system/tray/system_...
ash/system/tray/system_tray.cc:49: #include
"ash/common/system/chromeos/brightness/tray_brightness.h"
Hooray!

https://codereview.chromium.org/2131733002/diff/40001/ash/test/ash_test_base.h
File ash/test/ash_test_base.h (right):

https://codereview.chromium.org/2131733002/diff/40001/ash/test/ash_test_base....
ash/test/ash_test_base.h:157: void SetBrightnessControlDelegate(
Yeah, I'd prefer to friend the test or tests if there are only going to be a
couple places that need it.

[commit-bot: I haz the power, 2016-07-08 00:45:30]

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

[commit-bot: I haz the power, 2016-07-08 00:45:32]

Dry run: Try jobs failed on following builders:
  mac_chromium_gyp_rel on master.tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)

[msw, 2016-07-08 01:40:49]

Please take another look (and consider my new comments); thanks!

https://codereview.chromium.org/2131733002/diff/40001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller.h (right):

https://codereview.chromium.org/2131733002/diff/40001/ash/accelerators/accele...
ash/accelerators/accelerator_controller.h:174:
keyboard_brightness_control_delegate_;
On 2016/07/07 23:08:49, James Cook wrote:
> On 2016/07/07 22:41:59, msw wrote:
> > I could also move this to WmShell for consistency, but it's not necessary
nor
> > clearly better; wdyt?
> 
> Yeah, I would move it. It exists for tests and to have someone own
> KeyboardBrightnessController. I think it makes sense to keep it with the other
> one.

Done.

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

https://codereview.chromium.org/2131733002/diff/40001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:250: friend class test::AshTestBase;
On 2016/07/07 23:08:49, James Cook wrote:
> On 2016/07/07 22:41:59, msw wrote:
> > I could instead make SetBrightnessControlDelegate public, add "friend class
> > AcceleratorControllerTest" with a forward decl, or something else(?); wdyt?
> 
> I would friend AcceleratorControllerTest.

Done.

https://codereview.chromium.org/2131733002/diff/40001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2131733002/diff/40001/ash/shell.cc#newcode1076
ash/shell.cc:1076: WmShell::Get()->SetBrightnessControlDelegate(
On 2016/07/07 23:08:49, James Cook wrote:
> On 2016/07/07 22:41:59, msw wrote:
> > I could move this to WmShell's ctor or similar; wdyt?
> 
> I would try moving it to WmShell ctor. If that works, it's what I would
prefer.

Done.

https://codereview.chromium.org/2131733002/diff/40001/ash/system/tray/system_...
File ash/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2131733002/diff/40001/ash/system/tray/system_...
ash/system/tray/system_tray.cc:49: #include
"ash/common/system/chromeos/brightness/tray_brightness.h"
On 2016/07/07 23:08:49, James Cook wrote:
> Hooray!

Acknowledged.

https://codereview.chromium.org/2131733002/diff/40001/ash/test/ash_test_base.h
File ash/test/ash_test_base.h (right):

https://codereview.chromium.org/2131733002/diff/40001/ash/test/ash_test_base....
ash/test/ash_test_base.h:157: void SetBrightnessControlDelegate(
On 2016/07/07 23:08:50, James Cook wrote:
> Yeah, I'd prefer to friend the test or tests if there are only going to be a
> couple places that need it.

Done.

https://codereview.chromium.org/2131733002/diff/80001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller_unittest.cc (right):

https://codereview.chromium.org/2131733002/diff/80001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_unittest.cc:859:
WmShell::Get()->SetBrightnessControlDelegate(
If we want to get really minimal, I could remove the setters and do: (wdyt?)
    WmShell::Get()->brightness_control_delegate_.reset(delegate);

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

https://codereview.chromium.org/2131733002/diff/80001/ash/common/wm_shell.cc#...
ash/common/wm_shell.cc:70: brightness_control_delegate_.reset(new
system::BrightnessControllerChromeos);
This seems cleaner than using separate preprocessor blocks in the initializer
list, or reordering members with cros-specific impls together in the decl. wdyt?

[msw, 2016-07-08 01:40:57]

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

[commit-bot: I haz the power, 2016-07-08 01:41:15]

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-08 04:11:01]

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

[commit-bot: I haz the power, 2016-07-08 04:11:01]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-07-08 16:29:35]

LGTM with some comments. I like this patch, it's a good cleanup in addition to
the move.

https://codereview.chromium.org/2131733002/diff/80001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller_unittest.cc (right):

https://codereview.chromium.org/2131733002/diff/80001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_unittest.cc:859:
WmShell::Get()->SetBrightnessControlDelegate(
On 2016/07/08 01:40:48, msw wrote:
> If we want to get really minimal, I could remove the setters and do: (wdyt?)
>     WmShell::Get()->brightness_control_delegate_.reset(delegate);

Yeah, I like that idea.

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

https://codereview.chromium.org/2131733002/diff/80001/ash/common/wm_shell.cc#...
ash/common/wm_shell.cc:70: brightness_control_delegate_.reset(new
system::BrightnessControllerChromeos);
On 2016/07/08 01:40:48, msw wrote:
> This seems cleaner than using separate preprocessor blocks in the initializer
> list, or reordering members with cros-specific impls together in the decl.
wdyt?

Yeah, I like this better. I hate ifdefs in initializer lists.

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

https://codereview.chromium.org/2131733002/diff/80001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:65: BrightnessControlDelegate*
brightness_control_delegate() const {
see below

https://codereview.chromium.org/2131733002/diff/80001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:72: const {
I don't think we generally make these sorts of accessors const (ones that return
a non-const pointer). I think sky@ makes the argument that since the returned
object is mutable it's not possible to guarantee the caller doesn't change
internal state.

https://codereview.chromium.org/2131733002/diff/80001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:251: FRIEND_TEST_ALL_PREFIXES(AcceleratorControllerTest,
GlobalAccelerators);
Could you "friend AcceleratorControllerTest" then add setter methods like
AcceleratorControllerTest::SetKeyboardBrightnessControllerDelegate() that does
the kbc_delegate_.reset()?

I don't really like gtest_prod_util.h, especially in a widely included header
like this one.

[msw, 2016-07-08 17:18:58]

https://codereview.chromium.org/2131733002/diff/80001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller_unittest.cc (right):

https://codereview.chromium.org/2131733002/diff/80001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_unittest.cc:859:
WmShell::Get()->SetBrightnessControlDelegate(
On 2016/07/08 16:29:34, James Cook wrote:
> On 2016/07/08 01:40:48, msw wrote:
> > If we want to get really minimal, I could remove the setters and do: (wdyt?)
> >     WmShell::Get()->brightness_control_delegate_.reset(delegate);
> 
> Yeah, I like that idea.

I made AcceleratorControllerTest a friend class with setters that do this.

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

https://codereview.chromium.org/2131733002/diff/80001/ash/common/wm_shell.cc#...
ash/common/wm_shell.cc:70: brightness_control_delegate_.reset(new
system::BrightnessControllerChromeos);
On 2016/07/08 16:29:34, James Cook wrote:
> On 2016/07/08 01:40:48, msw wrote:
> > This seems cleaner than using separate preprocessor blocks in the
initializer
> > list, or reordering members with cros-specific impls together in the decl.
> wdyt?
> 
> Yeah, I like this better. I hate ifdefs in initializer lists.

Acknowledged.

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

https://codereview.chromium.org/2131733002/diff/80001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:65: BrightnessControlDelegate*
brightness_control_delegate() const {
On 2016/07/08 16:29:34, James Cook wrote:
> see below

Done.

https://codereview.chromium.org/2131733002/diff/80001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:72: const {
On 2016/07/08 16:29:34, James Cook wrote:
> I don't think we generally make these sorts of accessors const (ones that
return
> a non-const pointer). I think sky@ makes the argument that since the returned
> object is mutable it's not possible to guarantee the caller doesn't change
> internal state.

Done.

https://codereview.chromium.org/2131733002/diff/80001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:251: FRIEND_TEST_ALL_PREFIXES(AcceleratorControllerTest,
GlobalAccelerators);
On 2016/07/08 16:29:35, James Cook wrote:
> Could you "friend AcceleratorControllerTest" then add setter methods like
> AcceleratorControllerTest::SetKeyboardBrightnessControllerDelegate() that does
> the kbc_delegate_.reset()?
> 
> I don't really like gtest_prod_util.h, especially in a widely included header
> like this one.

Done.

[msw, 2016-07-08 17:21:19]

The CQ bit was checked by msw@chromium.org

[msw, 2016-07-08 17:21:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2131733002/#ps100001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-07-08 17:21:38]

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-08 18:11:27]

Description was changed from

==========
mash: Migrate [Keyboard]Brightness code to ash/common.

Move BrightnessControlDelegate ownership to WmShell.
(it is not closely related to AcceleratorController)

tools/git/move_source_file.py ash/system/brightness_control_delegate.h
ash/common/system
tools/git/move_source_file.py
ash/system/keyboard_brightness/keyboard_brightness_control_delegate.h
ash/common/system
tools/git/move_source_file.py
ash/system/chromeos/keyboard_brightness_controller* ash/common/system/chromeos
tools/git/move_source_file.py ash/system/chromeos/brightness/*
ash/common/system/chromeos/brightness

BUG=619636
TEST=compiles; unit tests; no behavior changes.
R=jamescook@chromium.org
==========

to

==========
mash: Migrate [Keyboard]Brightness code to ash/common.

Move BrightnessControlDelegate ownership to WmShell.
(it is not closely related to AcceleratorController)

tools/git/move_source_file.py ash/system/brightness_control_delegate.h
ash/common/system
tools/git/move_source_file.py
ash/system/keyboard_brightness/keyboard_brightness_control_delegate.h
ash/common/system
tools/git/move_source_file.py
ash/system/chromeos/keyboard_brightness_controller* ash/common/system/chromeos
tools/git/move_source_file.py ash/system/chromeos/brightness/*
ash/common/system/chromeos/brightness

BUG=619636
TEST=compiles; unit tests; no behavior changes.
R=jamescook@chromium.org
==========

[commit-bot: I haz the power, 2016-07-08 18:11:29]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-08 18:12:57]

Description was changed from

==========
mash: Migrate [Keyboard]Brightness code to ash/common.

Move BrightnessControlDelegate ownership to WmShell.
(it is not closely related to AcceleratorController)

tools/git/move_source_file.py ash/system/brightness_control_delegate.h
ash/common/system
tools/git/move_source_file.py
ash/system/keyboard_brightness/keyboard_brightness_control_delegate.h
ash/common/system
tools/git/move_source_file.py
ash/system/chromeos/keyboard_brightness_controller* ash/common/system/chromeos
tools/git/move_source_file.py ash/system/chromeos/brightness/*
ash/common/system/chromeos/brightness

BUG=619636
TEST=compiles; unit tests; no behavior changes.
R=jamescook@chromium.org
==========

to

==========
mash: Migrate [Keyboard]Brightness code to ash/common.

Move BrightnessControlDelegate ownership to WmShell.
(it is not closely related to AcceleratorController)

tools/git/move_source_file.py ash/system/brightness_control_delegate.h
ash/common/system
tools/git/move_source_file.py
ash/system/keyboard_brightness/keyboard_brightness_control_delegate.h
ash/common/system
tools/git/move_source_file.py
ash/system/chromeos/keyboard_brightness_controller* ash/common/system/chromeos
tools/git/move_source_file.py ash/system/chromeos/brightness/*
ash/common/system/chromeos/brightness

BUG=619636
TEST=compiles; unit tests; no behavior changes.
R=jamescook@chromium.org

Committed: https://crrev.com/308af3930bd5a7f2d85ce76fb76b1c1a8eec9e11
Cr-Commit-Position: refs/heads/master@{#404439}
==========

[commit-bot: I haz the power, 2016-07-08 18:12:59]

Patchset 6 (id:??) landed as
https://crrev.com/308af3930bd5a7f2d85ce76fb76b1c1a8eec9e11
Cr-Commit-Position: refs/heads/master@{#404439}