[Night Light] CL2: Ash and system tray work

[Night Light] CL2: Ash and system tray work

Add ash and system tray initial plumbing. Support only toggling
NightLight on / off and setting the temperature to a fixed 0.5f
value when it's turned on for now.

Follow-up CLs will add system settings support.
Demo: https://bugs.chromium.org/p/chromium/issues/detail?id=705816#c4

BUG=705816
TEST=System tray has a toggle button. NightLight turns on / off with
     a 2-second animation. When on, a tray icon shows next to the
     clock and battery.

Review-Url: https://codereview.chromium.org/2857103007
Cr-Commit-Position: refs/heads/master@{#470392}
Committed: https://chromium.googlesource.com/chromium/src/+/e4fae85260876af2c08495ccf33b6c3051db1730

[afakhry, 2017-05-04 02:12:54]

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

[commit-bot: I haz the power, 2017-05-04 02:13:12]

Dry run: CQ is trying da patch.

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

[afakhry, 2017-05-04 02:13:19]

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

[afakhry, 2017-05-04 02:13:20]

James, could you please take a look? Thank you!

[commit-bot: I haz the power, 2017-05-04 02:58:13]

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

[commit-bot: I haz the power, 2017-05-04 02:58:14]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[James Cook, 2017-05-04 14:29:30]

I pinged the bug for more input on what the final UX is supposed to be. (I don't
fully understand the final goal :-)

Also, what is your plan for tests for this code/feature?

[afakhry, 2017-05-04 20:56:52]

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

[commit-bot: I haz the power, 2017-05-04 20:57:17]

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, 2017-05-04 22:11:48]

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

[commit-bot: I haz the power, 2017-05-04 22:11:49]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[afakhry, 2017-05-05 01:50:00]

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

[commit-bot: I haz the power, 2017-05-05 01:50:23]

Dry run: CQ is trying da patch.

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

[afakhry, 2017-05-05 01:51:24]

On 2017/05/04 14:29:30, James Cook wrote:
> I pinged the bug for more input on what the final UX is supposed to be. (I
don't
> fully understand the final goal :-)
> 
> Also, what is your plan for tests for this code/feature?

I rebased on today's changed and added tests at
ash/system/night_light/night_light_unittest.cc. Please take a look.

[commit-bot: I haz the power, 2017-05-05 02:49:29]

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

[commit-bot: I haz the power, 2017-05-05 02:49:30]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[afakhry, 2017-05-05 04:05:42]

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

[commit-bot: I haz the power, 2017-05-05 04:06:03]

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, 2017-05-05 05:16:26]

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

[commit-bot: I haz the power, 2017-05-05 05:16:27]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-05-05 17:12:33]

https://codereview.chromium.org/2857103007/diff/60001/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/2857103007/diff/60001/ash/ash_strings.grd#new...
ash/ash_strings.grd:217: Toggle Night Light
nit: "Toggle night light" (sentence case)

https://codereview.chromium.org/2857103007/diff/60001/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/2857103007/diff/60001/ash/shell.h#newcode330
ash/shell.h:330: NightLightController* night_light_controller() {
optional: alphabetize

https://codereview.chromium.org/2857103007/diff/60001/ash/shell.h#newcode689
ash/shell.h:689: std::unique_ptr<NightLightController> night_light_controller_;
but you can't alphabetize here of course :-)

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
File ash/system/night_light/night_light_controller.cc (right):

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.cc:21: // the night light
settings.
super nit: You're inconsistent in comments about "night light" vs. "NightLight".
Either is fine, but be consistent.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.cc:36: bool
animations_disabled_for_tests = false;
optional: g_animations_disabled_for_test

not required by style guide, but g_ is commonly used in ash

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.cc:41: SessionController*
session_controller)
Just for my knowledge, any particular reason you're injecting the
SessionController? I'm fine with it (I kinda like dependency injection) but you
don't ever seem to change it.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.cc:87: for (aura::Window*
root_window : ash::Shell::GetAllRootWindows()) {
nit: no ash::

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.cc:92:
animations_disabled_for_tests ? 0 : kManualToggleAnimationDurationSec));
I'm a little surprised you have to have your own disabling for animations.
AshTestBase should be disabling animations here:
https://cs.chromium.org/chromium/src/ash/test/ash_test_helper.cc?type=cs&q=an...

Why doesn't that work for you? Is something wrong/different with setting layer
temperature down in the ui::Layer code?

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.cc:119: LOG(ERROR) << "Can't
initialize NightLight user settings. No "
Can this be a CHECK()? It seems unexpected to try to initialize when there is no
pref service.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.cc:140: if (!pref_service) {
ditto

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.cc:154: for (auto& obs :
observers_)
super nit: obs -> observer

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
File ash/system/night_light/night_light_controller.h (right):

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.h:18: class ASH_EXPORT
NightLightController : public SessionObserver {
nit: Please add a class comment with a one-line description of what this feature
is/does.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.h:22: Observer() {}
nit: We usually write observers as:

class Observer {
 public:
  virtual void OnFoo() = 0;

 protected:
  virtual ~Observer() {}
}

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.h:26: virtual void
OnStatusChanged(bool new_status) = 0;
nit: Maybe OnNightLightChanged(bool enabled)? That would avoid future function
name collisions if other classes want to be observers, and make it more clear
what "status" is.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.h:37: static void
DisableAnimationsForTests();
nit: ForTest or ForTesting

Also, document which animations (casual readers might expect the animation is
related to the tray icon)

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.h:47: void SetEnabled(bool
enabled);
Do you need both Toggle() and enabled()/SetEnabled() ?

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.h:49: void
SetColorTemperature(float temperature);
nit: document expected values for temperature, or refer to member variable
below.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.h:69: // value from 0.0f (least
warm -- NightLight is OFF) to 1.0f (most warm).
Is 0.0 equivalent to nightlight off? If so, do you need the enabled flag?

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
File ash/system/night_light/night_light_unittest.cc (right):

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_unittest.cc:1: // Copyright 2017 The Chromium
Authors. All rights reserved.
I would put this file in night_light_controller_unittest.cc, even though it's
testing the whole feature.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_unittest.cc:30: for (aura::Window*
root_window : ash::Shell::GetAllRootWindows()) {
By default AshTestBase only creates a single display and root window. Either use
GetPrimaryRootWindow() and don't iterate, or use
UpdateDisplays("800x600,800x600") and test the multi-display case.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_unittest.cc:57: NightLightTest() {}
nit: = default, and below

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_unittest.cc:83:
DISALLOW_COPY_AND_ASSIGN(NightLightTest);
#include base/macros.h

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_unittest.cc:86: // Tests toggling NightLight
on /off and makes sure the observer is updated and
super nit: "on/off" or "on / off"

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_unittest.cc:87: // the layer temperatures are
modified.
Thanks for documenting what you are testing.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_unittest.cc:90: const bool initial_enabled =
GetController()->enabled();
nit: I don't think you need this variable, just inline the call

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_unittest.cc:93: GetController()->Toggle();
optional: Just cache a NightLightController* controller at start of test

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_unittest.cc:107: // Doesn't work on mash.
Can you clarify why? (Feature doesn't work at all due to layer differences, or
test doesn't work due to session state issues, or mash doesn't handle user
switching properly...)

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_unittest.cc:133: }
Nice tests. Easy to read. Good balance between clarifying comments and just
letting the test expectations speak for themselves.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
File ash/system/night_light/tray_night_light.cc (right):

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/tray_night_light.cc:15:
ash::Shell::Get()->night_light_controller()->AddObserver(this);
nit: no ash::

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/tray_night_light.cc:29: }
optional: This class is very light weight, so I'm OK with it not having a test.
However, it would be pretty simple to add one similar to TrayUpdateTest or
TraySupervisedUserTest.CreateDefaultView. Up to you.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
File ash/system/night_light/tray_night_light.h (right):

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/tray_night_light.h:14: // and active.
Thanks for adding a class comment.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/tray_night_light.h:18: TrayNightLight(SystemTray*
system_tray);
nit: explicit

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/tray_night_light.h:28:
DISALLOW_COPY_AND_ASSIGN(TrayNightLight);
#include macros.h

https://codereview.chromium.org/2857103007/diff/60001/ash/system/tiles/tiles_...
File ash/system/tiles/tiles_default_view.cc (right):

https://codereview.chromium.org/2857103007/diff/60001/ash/system/tiles/tiles_...
ash/system/tiles/tiles_default_view.cc:92:
ash::Shell::Get()->night_light_controller()->enabled()
nit: no ash::

https://codereview.chromium.org/2857103007/diff/60001/ash/system/tiles/tiles_...
ash/system/tiles/tiles_default_view.cc:96:
night_light_button_->SetEnabled(!disable_buttons);
Is this button going to be a toggle, or will it open WebUI settings?

If the former, this should not use |disable_buttons|.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/tiles/tiles_...
ash/system/tiles/tiles_default_view.cc:98:
AddChildView(TrayPopupUtils::CreateVerticalSeparator());
Please add a test that the night light icon is created in the correct state. For
example, see TrayTilesTest.

https://codereview.chromium.org/2857103007/diff/60001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2857103007/diff/60001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:648:
ash::NightLightController::RegisterPrefs(registry);
Can this be done inside of ash somewhere, like maybe get called during Shell
init?

If not, can you make a one-line function ash::Shell::RegisterPrefs() and bounce
the call through to NightLightController? We'll need prefs registration in ash
for mustash and that would be a convenient place to call from.

[afakhry, 2017-05-05 20:03:52]

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

[afakhry, 2017-05-05 20:04:12]

Thanks for the detailed review!

https://codereview.chromium.org/2857103007/diff/60001/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/2857103007/diff/60001/ash/ash_strings.grd#new...
ash/ash_strings.grd:217: Toggle Night Light
On 2017/05/05 17:12:30, James Cook (OOO) wrote:
> nit: "Toggle night light" (sentence case)

Shouldn't it be capitalized since it's the name of the feature, just like
Android displays it here: https://screenshot.googleplex.com/w63nWCB3R1r.png?

https://codereview.chromium.org/2857103007/diff/60001/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/2857103007/diff/60001/ash/shell.h#newcode330
ash/shell.h:330: NightLightController* night_light_controller() {
On 2017/05/05 17:12:30, James Cook (OOO) wrote:
> optional: alphabetize

Done.

https://codereview.chromium.org/2857103007/diff/60001/ash/shell.h#newcode689
ash/shell.h:689: std::unique_ptr<NightLightController> night_light_controller_;
On 2017/05/05 17:12:30, James Cook (OOO) wrote:
> but you can't alphabetize here of course :-)

Yes, of course! :)

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
File ash/system/night_light/night_light_controller.cc (right):

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.cc:21: // the night light
settings.
On 2017/05/05 17:12:31, James Cook (OOO) wrote:
> super nit: You're inconsistent in comments about "night light" vs.
"NightLight".
> Either is fine, but be consistent.

Done.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.cc:36: bool
animations_disabled_for_tests = false;
On 2017/05/05 17:12:31, James Cook (OOO) wrote:
> optional: g_animations_disabled_for_test
> 
> not required by style guide, but g_ is commonly used in ash

A reviewer once told me that since such a variable is a file-private local, I
shouldn't call it g_.

But yes, I see this commonly done in many places. So, done.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.cc:41: SessionController*
session_controller)
On 2017/05/05 17:12:31, James Cook (OOO) wrote:
> Just for my knowledge, any particular reason you're injecting the
> SessionController? I'm fine with it (I kinda like dependency injection) but
you
> don't ever seem to change it.

I'm using it to observer user login and switch to start initializing the
NightLight parameters for the active user's pref_service.

I don't think it changes.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.cc:87: for (aura::Window*
root_window : ash::Shell::GetAllRootWindows()) {
On 2017/05/05 17:12:30, James Cook (OOO) wrote:
> nit: no ash::

Done.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.cc:92:
animations_disabled_for_tests ? 0 : kManualToggleAnimationDurationSec));
On 2017/05/05 17:12:31, James Cook (OOO) wrote:
> I'm a little surprised you have to have your own disabling for animations.
> AshTestBase should be disabling animations here:
>
https://cs.chromium.org/chromium/src/ash/test/ash_test_helper.cc?type=cs&q=an...
> 
> Why doesn't that work for you? Is something wrong/different with setting layer
> temperature down in the ui::Layer code?

Oh, I didn't know about that. I was just paranoid about flakiness so I added it
without testing. Interesting! Thanks for the pointer!
Now we can get rid of that flag entirely.

Done.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.cc:119: LOG(ERROR) << "Can't
initialize NightLight user settings. No "
On 2017/05/05 17:12:31, James Cook (OOO) wrote:
> Can this be a CHECK()? It seems unexpected to try to initialize when there is
no
> pref service.

It is expected in ash tests where there's no pref_service by default.

It is also needed until Xiyuan fixes this bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=718458

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.cc:140: if (!pref_service) {
On 2017/05/05 17:12:31, James Cook (OOO) wrote:
> ditto

Same reason as above.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.cc:154: for (auto& obs :
observers_)
On 2017/05/05 17:12:31, James Cook (OOO) wrote:
> super nit: obs -> observer

Done.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
File ash/system/night_light/night_light_controller.h (right):

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.h:18: class ASH_EXPORT
NightLightController : public SessionObserver {
On 2017/05/05 17:12:31, James Cook (OOO) wrote:
> nit: Please add a class comment with a one-line description of what this
feature
> is/does.

Done.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.h:22: Observer() {}
On 2017/05/05 17:12:31, James Cook (OOO) wrote:
> nit: We usually write observers as:
> 
> class Observer {
>  public:
>   virtual void OnFoo() = 0;
> 
>  protected:
>   virtual ~Observer() {}
> }

Done.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.h:26: virtual void
OnStatusChanged(bool new_status) = 0;
On 2017/05/05 17:12:31, James Cook (OOO) wrote:
> nit: Maybe OnNightLightChanged(bool enabled)? That would avoid future function
> name collisions if other classes want to be observers, and make it more clear
> what "status" is.

Done.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.h:37: static void
DisableAnimationsForTests();
On 2017/05/05 17:12:31, James Cook (OOO) wrote:
> nit: ForTest or ForTesting
> 
> Also, document which animations (casual readers might expect the animation is
> related to the tray icon)

Done.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.h:47: void SetEnabled(bool
enabled);
On 2017/05/05 17:12:31, James Cook (OOO) wrote:
> Do you need both Toggle() and enabled()/SetEnabled() ?

SetEnabled() is useful for initializing from prefs, to set it to a specific
value. Toggle() is just implemented in terms of SetEnabled() so it's not a
problem.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.h:49: void
SetColorTemperature(float temperature);
On 2017/05/05 17:12:31, James Cook (OOO) wrote:
> nit: document expected values for temperature, or refer to member variable
> below.

Done.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.h:69: // value from 0.0f (least
warm -- NightLight is OFF) to 1.0f (most warm).
On 2017/05/05 17:12:31, James Cook (OOO) wrote:
> Is 0.0 equivalent to nightlight off? If so, do you need the enabled flag?

Yes 0.0f === off. But I still need enabled_. The idea is color_temperature_
should hold the value that will be set when Night Light is ON. So I don't change
it unless the user changes it. This keeps the status separate.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
File ash/system/night_light/night_light_unittest.cc (right):

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_unittest.cc:1: // Copyright 2017 The Chromium
Authors. All rights reserved.
On 2017/05/05 17:12:32, James Cook (OOO) wrote:
> I would put this file in night_light_controller_unittest.cc, even though it's
> testing the whole feature.

Done.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_unittest.cc:30: for (aura::Window*
root_window : ash::Shell::GetAllRootWindows()) {
On 2017/05/05 17:12:32, James Cook (OOO) wrote:
> By default AshTestBase only creates a single display and root window. Either
use
> GetPrimaryRootWindow() and don't iterate, or use
> UpdateDisplays("800x600,800x600") and test the multi-display case.

Oh, I like UpdateDisplay(). That makes the tests more rigorous since the
expectation is that all displays should be updated.

Done.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_unittest.cc:57: NightLightTest() {}
On 2017/05/05 17:12:32, James Cook (OOO) wrote:
> nit: = default, and below

Done.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_unittest.cc:83:
DISALLOW_COPY_AND_ASSIGN(NightLightTest);
On 2017/05/05 17:12:31, James Cook (OOO) wrote:
> #include base/macros.h

Done.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_unittest.cc:86: // Tests toggling NightLight
on /off and makes sure the observer is updated and
On 2017/05/05 17:12:32, James Cook (OOO) wrote:
> super nit: "on/off" or "on / off"

My bad. Done.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_unittest.cc:87: // the layer temperatures are
modified.
On 2017/05/05 17:12:32, James Cook (OOO) wrote:
> Thanks for documenting what you are testing.

Acknowledged.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_unittest.cc:90: const bool initial_enabled =
GetController()->enabled();
On 2017/05/05 17:12:32, James Cook (OOO) wrote:
> nit: I don't think you need this variable, just inline the call

I was planning to use it, but I changed my mind and forgot to remove it. Thanks!

Done.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_unittest.cc:93: GetController()->Toggle();
On 2017/05/05 17:12:32, James Cook (OOO) wrote:
> optional: Just cache a NightLightController* controller at start of test

Done.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_unittest.cc:107: // Doesn't work on mash.
On 2017/05/05 17:12:32, James Cook (OOO) wrote:
> Can you clarify why? (Feature doesn't work at all due to layer differences, or
> test doesn't work due to session state issues, or mash doesn't handle user
> switching properly...)

I think it's because of user switching doesn't work on mash:
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.linux...

But I didn't write a comment here as I was waiting to see Xiyuan today to
confirm first, but he's not here.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_unittest.cc:133: }
On 2017/05/05 17:12:32, James Cook (OOO) wrote:
> Nice tests. Easy to read. Good balance between clarifying comments and just
> letting the test expectations speak for themselves.

Thank you very much!

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
File ash/system/night_light/tray_night_light.cc (right):

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/tray_night_light.cc:15:
ash::Shell::Get()->night_light_controller()->AddObserver(this);
On 2017/05/05 17:12:32, James Cook (OOO) wrote:
> nit: no ash::

Done.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/tray_night_light.cc:29: }
On 2017/05/05 17:12:32, James Cook (OOO) wrote:
> optional: This class is very light weight, so I'm OK with it not having a
test.
> However, it would be pretty simple to add one similar to TrayUpdateTest or
> TraySupervisedUserTest.CreateDefaultView. Up to you.

Thanks for the pointers.

I added a test. Please take a look.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
File ash/system/night_light/tray_night_light.h (right):

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/tray_night_light.h:14: // and active.
On 2017/05/05 17:12:32, James Cook (OOO) wrote:
> Thanks for adding a class comment.

Acknowledged.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/tray_night_light.h:18: TrayNightLight(SystemTray*
system_tray);
On 2017/05/05 17:12:32, James Cook (OOO) wrote:
> nit: explicit

I'm surprised the linter didn't catch that. Done.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/tray_night_light.h:28:
DISALLOW_COPY_AND_ASSIGN(TrayNightLight);
On 2017/05/05 17:12:32, James Cook (OOO) wrote:
> #include macros.h

Done.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/tiles/tiles_...
File ash/system/tiles/tiles_default_view.cc (right):

https://codereview.chromium.org/2857103007/diff/60001/ash/system/tiles/tiles_...
ash/system/tiles/tiles_default_view.cc:92:
ash::Shell::Get()->night_light_controller()->enabled()
On 2017/05/05 17:12:32, James Cook (OOO) wrote:
> nit: no ash::

Done.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/tiles/tiles_...
ash/system/tiles/tiles_default_view.cc:96:
night_light_button_->SetEnabled(!disable_buttons);
On 2017/05/05 17:12:32, James Cook (OOO) wrote:
> Is this button going to be a toggle, or will it open WebUI settings?
> 
> If the former, this should not use |disable_buttons|.

It's a toggle button, but this |disable_buttons| is still needed because before
login, it makes all these buttons disabled and grayed out.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/tiles/tiles_...
ash/system/tiles/tiles_default_view.cc:98:
AddChildView(TrayPopupUtils::CreateVerticalSeparator());
On 2017/05/05 17:12:32, James Cook (OOO) wrote:
> Please add a test that the night light icon is created in the correct state.
For
> example, see TrayTilesTest.

Done. I updated TrayTilesTest.

https://codereview.chromium.org/2857103007/diff/60001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2857103007/diff/60001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:648:
ash::NightLightController::RegisterPrefs(registry);
On 2017/05/05 17:12:33, James Cook (OOO) wrote:
> Can this be done inside of ash somewhere, like maybe get called during Shell
> init?
> 
> If not, can you make a one-line function ash::Shell::RegisterPrefs() and
bounce
> the call through to NightLightController? We'll need prefs registration in ash
> for mustash and that would be a convenient place to call from.

I don't think we can do it in Shell init, but I might be wrong. I believe this
has to be called from Chrome for the profile after login.  Shell init happens
much earlier than that.

But I like the idea of having a central place in ash that registers all ash
prefs. So, done!

[commit-bot: I haz the power, 2017-05-05 20:04:30]

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, 2017-05-05 20:09:29]

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

[commit-bot: I haz the power, 2017-05-05 20:09:31]

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

[James Cook, 2017-05-05 20:47:43]

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
File ash/system/night_light/night_light_controller.cc (right):

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.cc:119: LOG(ERROR) << "Can't
initialize NightLight user settings. No "
On 2017/05/05 20:04:10, afakhry wrote:
> On 2017/05/05 17:12:31, James Cook (OOO) wrote:
> > Can this be a CHECK()? It seems unexpected to try to initialize when there
is
> no
> > pref service.
> 
> It is expected in ash tests where there's no pref_service by default.
> 
> It is also needed until Xiyuan fixes this bug:
> https://bugs.chromium.org/p/chromium/issues/detail?id=718458

So does this log line print when you run ash_unittests? If so, that seems bad. I
would just return and not log and add a comment saying it happens in tests and
citing the bug.

If it doesn't print in tests then I would CHECK it.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
File ash/system/night_light/night_light_controller.h (right):

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.h:69: // value from 0.0f (least
warm -- NightLight is OFF) to 1.0f (most warm).
On 2017/05/05 20:04:11, afakhry wrote:
> On 2017/05/05 17:12:31, James Cook (OOO) wrote:
> > Is 0.0 equivalent to nightlight off? If so, do you need the enabled flag?
> 
> Yes 0.0f === off. But I still need enabled_. The idea is color_temperature_
> should hold the value that will be set when Night Light is ON. So I don't
change
> it unless the user changes it. This keeps the status separate.

So it's possible to turn Night Light on, but the color temperature doesn't
change from the screen's default? That seems odd.

Maybe I'm misreading the "NightLight is off" part of the comment?

https://codereview.chromium.org/2857103007/diff/60001/ash/system/tiles/tiles_...
File ash/system/tiles/tiles_default_view.cc (right):

https://codereview.chromium.org/2857103007/diff/60001/ash/system/tiles/tiles_...
ash/system/tiles/tiles_default_view.cc:96:
night_light_button_->SetEnabled(!disable_buttons);
On 2017/05/05 20:04:12, afakhry wrote:
> On 2017/05/05 17:12:32, James Cook (OOO) wrote:
> > Is this button going to be a toggle, or will it open WebUI settings?
> > 
> > If the former, this should not use |disable_buttons|.
> 
> It's a toggle button, but this |disable_buttons| is still needed because
before
> login, it makes all these buttons disabled and grayed out.

|disable_buttons| currently means "I can show webui settings". It only sort-of
means you're at the login screen - there are lots of other cases where it is
true.

I suggest renaming |disable_buttons| to |can_show_webui| and inversing the
logic, then adding the conditional you really mean here (maybe
SessionController::IsActiveUserSession, IsScreenLocked, or some combination
thereof).

If you really want CanOpenWebUISettings you need a comment explaining why you're
using that even though the button does not open webui, and/or rename the
CanOpenWebUISettings method.

https://codereview.chromium.org/2857103007/diff/80001/ash/system/night_light/...
File ash/system/night_light/night_light_controller_unittest.cc (right):

https://codereview.chromium.org/2857103007/diff/80001/ash/system/night_light/...
ash/system/night_light/night_light_controller_unittest.cc:137:
TEST_F(NightLightTest, TestNightLightTrayVisibility) {
Since this tests TrayNightLight, put it in a new file
tray_night_light_unittest.cc

It's pedantic, but otherwise I'm concerned that someone adding functionality to
TrayNightLight might not notice the existing test.

https://codereview.chromium.org/2857103007/diff/80001/ash/system/tiles/tray_t...
File ash/system/tiles/tray_tiles_unittest.cc (right):

https://codereview.chromium.org/2857103007/diff/80001/ash/system/tiles/tray_t...
ash/system/tiles/tray_tiles_unittest.cc:92: EXPECT_EQ(Button::STATE_DISABLED,
GetNightLightButton()->state());
Any particular reason this feature is disabled at the lock screen? Just so the
buttons look the same?

[afakhry, 2017-05-05 21:58:39]

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

[commit-bot: I haz the power, 2017-05-05 21:59:44]

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, 2017-05-05 23:54:00]

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

[commit-bot: I haz the power, 2017-05-05 23:54:01]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[afakhry, 2017-05-06 00:48:41]

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

[afakhry, 2017-05-06 00:49:35]

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
File ash/system/night_light/night_light_controller.cc (right):

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.cc:119: LOG(ERROR) << "Can't
initialize NightLight user settings. No "
On 2017/05/05 20:47:43, James Cook (OOO) wrote:
> On 2017/05/05 20:04:10, afakhry wrote:
> > On 2017/05/05 17:12:31, James Cook (OOO) wrote:
> > > Can this be a CHECK()? It seems unexpected to try to initialize when there
> is
> > no
> > > pref service.
> > 
> > It is expected in ash tests where there's no pref_service by default.
> > 
> > It is also needed until Xiyuan fixes this bug:
> > https://bugs.chromium.org/p/chromium/issues/detail?id=718458
> 
> So does this log line print when you run ash_unittests? If so, that seems bad.
I
> would just return and not log and add a comment saying it happens in tests and
> citing the bug.
> 
> If it doesn't print in tests then I would CHECK it.

Done.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
File ash/system/night_light/night_light_controller.h (right):

https://codereview.chromium.org/2857103007/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_controller.h:69: // value from 0.0f (least
warm -- NightLight is OFF) to 1.0f (most warm).
On 2017/05/05 20:47:43, James Cook (OOO) wrote:
> On 2017/05/05 20:04:11, afakhry wrote:
> > On 2017/05/05 17:12:31, James Cook (OOO) wrote:
> > > Is 0.0 equivalent to nightlight off? If so, do you need the enabled flag?
> > 
> > Yes 0.0f === off. But I still need enabled_. The idea is color_temperature_
> > should hold the value that will be set when Night Light is ON. So I don't
> change
> > it unless the user changes it. This keeps the status separate.
> 
> So it's possible to turn Night Light on, but the color temperature doesn't
> change from the screen's default? That seems odd.
> 
> Maybe I'm misreading the "NightLight is off" part of the comment?

Oh I think I might need to remove that "OFF" comment if it's confusing. What I
was trying to say is that if temperature is set to 0.0f, from the ui::Layer
perspective, it's as if it's completely off. 

https://cs.chromium.org/chromium/src/ui/compositor/layer.cc?type=cs&q=SetLaye...

What I also meant to say is that we can remove |enabled_| and keep changing
|color_temperature_|, set it to 0.0f when we want to turn it off, but then we'd
want to keep re-reading the set user-desired color temperature from the prefs
whenever we want to turn it back on.

https://codereview.chromium.org/2857103007/diff/60001/ash/system/tiles/tiles_...
File ash/system/tiles/tiles_default_view.cc (right):

https://codereview.chromium.org/2857103007/diff/60001/ash/system/tiles/tiles_...
ash/system/tiles/tiles_default_view.cc:96:
night_light_button_->SetEnabled(!disable_buttons);
On 2017/05/05 20:47:43, James Cook (OOO) wrote:
> On 2017/05/05 20:04:12, afakhry wrote:
> > On 2017/05/05 17:12:32, James Cook (OOO) wrote:
> > > Is this button going to be a toggle, or will it open WebUI settings?
> > > 
> > > If the former, this should not use |disable_buttons|.
> > 
> > It's a toggle button, but this |disable_buttons| is still needed because
> before
> > login, it makes all these buttons disabled and grayed out.
> 
> |disable_buttons| currently means "I can show webui settings". It only sort-of
> means you're at the login screen - there are lots of other cases where it is
> true.
> 
> I suggest renaming |disable_buttons| to |can_show_webui| and inversing the
> logic, then adding the conditional you really mean here (maybe
> SessionController::IsActiveUserSession, IsScreenLocked, or some combination
> thereof).
> 
> If you really want CanOpenWebUISettings you need a comment explaining why
you're
> using that even though the button does not open webui, and/or rename the
> CanOpenWebUISettings method.

According to this comment:
https://cs.chromium.org/chromium/src/ash/system/tiles/tiles_default_view.cc?t...
One can't open WebUI Settings in login screen, lock screen, or secondary account
flow. If that's true, it's also what we want for the Night Light toggle button;
we want it disabled in all the above situations. 

I modified |disabled_buttons| to |can_show_web_ui| as requested.

Do I still need to check IsActiveUserSession() or IsScreenLocked() etc? It seems
from the above comment that |can_show_web_ui| is enough.

https://codereview.chromium.org/2857103007/diff/80001/ash/system/night_light/...
File ash/system/night_light/night_light_controller_unittest.cc (right):

https://codereview.chromium.org/2857103007/diff/80001/ash/system/night_light/...
ash/system/night_light/night_light_controller_unittest.cc:137:
TEST_F(NightLightTest, TestNightLightTrayVisibility) {
On 2017/05/05 20:47:43, James Cook (OOO) wrote:
> Since this tests TrayNightLight, put it in a new file
> tray_night_light_unittest.cc
> 
> It's pedantic, but otherwise I'm concerned that someone adding functionality
to
> TrayNightLight might not notice the existing test.

Good point. Done.

https://codereview.chromium.org/2857103007/diff/80001/ash/system/tiles/tray_t...
File ash/system/tiles/tray_tiles_unittest.cc (right):

https://codereview.chromium.org/2857103007/diff/80001/ash/system/tiles/tray_t...
ash/system/tiles/tray_tiles_unittest.cc:92: EXPECT_EQ(Button::STATE_DISABLED,
GetNightLightButton()->state());
On 2017/05/05 20:47:43, James Cook (OOO) wrote:
> Any particular reason this feature is disabled at the lock screen? Just so the
> buttons look the same?

I guess so. I think I'll have to check again with Olga when she comes back next
week to see if we want users to be able to toggle it on or off at lock screen.

This is however is consistent with the other buttons, including the Settings
button, and the Night Light button is a shortcut to one of the settings. But I
can check and modify it later. There are several CLs coming up.

[commit-bot: I haz the power, 2017-05-06 00:49:36]

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, 2017-05-06 01:06:42]

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

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

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

[James Cook, 2017-05-08 16:09:08]

LGTM. I was confused about whether the feature should turn on/off when the
button is clicked (current behavior) vs. open settings (which I guess is the
final design).

https://codereview.chromium.org/2857103007/diff/80001/ash/system/tiles/tray_t...
File ash/system/tiles/tray_tiles_unittest.cc (right):

https://codereview.chromium.org/2857103007/diff/80001/ash/system/tiles/tray_t...
ash/system/tiles/tray_tiles_unittest.cc:92: EXPECT_EQ(Button::STATE_DISABLED,
GetNightLightButton()->state());
On 2017/05/06 00:49:35, afakhry wrote:
> On 2017/05/05 20:47:43, James Cook (OOO) wrote:
> > Any particular reason this feature is disabled at the lock screen? Just so
the
> > buttons look the same?
> 
> I guess so. I think I'll have to check again with Olga when she comes back
next
> week to see if we want users to be able to toggle it on or off at lock screen.
> 
> This is however is consistent with the other buttons, including the Settings
> button, and the Night Light button is a shortcut to one of the settings. But I
> can check and modify it later. There are several CLs coming up.

Ah. It wasn't clear to me from the videos / mocks that the icon opened the
settings. I thought it toggled the feature on and off when clicked.

https://codereview.chromium.org/2857103007/diff/120001/ash/system/night_light...
File ash/system/night_light/night_light_controller.h (right):

https://codereview.chromium.org/2857103007/diff/120001/ash/system/night_light...
ash/system/night_light/night_light_controller.h:68: // value from 0.0f (least
warm -- NightLight is OFF) to 1.0f (most warm).
nit: Instead of "NightLight is OFF" (which sounds to me like the feature is off)
I would say something like "least warm, default display color" or layer or
something like that.

https://codereview.chromium.org/2857103007/diff/120001/ash/system/night_light...
File ash/system/night_light/night_light_controller_unittest.cc (right):

https://codereview.chromium.org/2857103007/diff/120001/ash/system/night_light...
ash/system/night_light/night_light_controller_unittest.cc:90:
controller->Toggle();
optional: I think tests are clearer when they explicitly call SetEnabled()
instead of Toggle(), because then the reader doesn't have to think about the old
vs. new state.

https://codereview.chromium.org/2857103007/diff/120001/ash/system/night_light...
File ash/system/night_light/tray_night_light_unittest.cc (right):

https://codereview.chromium.org/2857103007/diff/120001/ash/system/night_light...
ash/system/night_light/tray_night_light_unittest.cc:5: #include
"ash/system/night_light/tray_night_light.h"
super nit: blank line below

https://codereview.chromium.org/2857103007/diff/120001/ash/system/night_light...
ash/system/night_light/tray_night_light_unittest.cc:11: using ash::Shell;
not needed, since you have namespace ash below

https://codereview.chromium.org/2857103007/diff/120001/ash/system/night_light...
ash/system/night_light/tray_night_light_unittest.cc:12: using
ash::test::AshTestBase;
ditto

https://codereview.chromium.org/2857103007/diff/120001/ash/system/night_light...
ash/system/night_light/tray_night_light_unittest.cc:28: controller->Toggle();
optional: ditto re: Toggle vs SetEnabled

[afakhry, 2017-05-08 17:34:54]

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

[commit-bot: I haz the power, 2017-05-08 17:35:27]

Dry run: CQ is trying da patch.

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

[afakhry, 2017-05-08 17:36:24]

afakhry@chromium.org changed reviewers:
+ mpearson@chromium.org, thestig@chromium.org

[afakhry, 2017-05-08 17:36:26]

+thestig@ for chrome/
+mpearson@ for tools/

https://codereview.chromium.org/2857103007/diff/80001/ash/system/tiles/tray_t...
File ash/system/tiles/tray_tiles_unittest.cc (right):

https://codereview.chromium.org/2857103007/diff/80001/ash/system/tiles/tray_t...
ash/system/tiles/tray_tiles_unittest.cc:92: EXPECT_EQ(Button::STATE_DISABLED,
GetNightLightButton()->state());
On 2017/05/08 16:09:07, James Cook (sheriff - slow) wrote:
> On 2017/05/06 00:49:35, afakhry wrote:
> > On 2017/05/05 20:47:43, James Cook (OOO) wrote:
> > > Any particular reason this feature is disabled at the lock screen? Just so
> the
> > > buttons look the same?
> > 
> > I guess so. I think I'll have to check again with Olga when she comes back
> next
> > week to see if we want users to be able to toggle it on or off at lock
screen.
> > 
> > This is however is consistent with the other buttons, including the Settings
> > button, and the Night Light button is a shortcut to one of the settings. But
I
> > can check and modify it later. There are several CLs coming up.
> 
> Ah. It wasn't clear to me from the videos / mocks that the icon opened the
> settings. I thought it toggled the feature on and off when clicked.

Oh, sorry, I think I confused even more! You are right. The button does NOT open
settings. It only toggles the feature on and off.

What I meant was, we don't want that button to be enabled when the user is not
logged in, similarly to the settings button, since the NightLight button
controls a user setting value, and for that we need the user to be logged in.

https://codereview.chromium.org/2857103007/diff/120001/ash/system/night_light...
File ash/system/night_light/night_light_controller.h (right):

https://codereview.chromium.org/2857103007/diff/120001/ash/system/night_light...
ash/system/night_light/night_light_controller.h:68: // value from 0.0f (least
warm -- NightLight is OFF) to 1.0f (most warm).
On 2017/05/08 16:09:07, James Cook (sheriff - slow) wrote:
> nit: Instead of "NightLight is OFF" (which sounds to me like the feature is
off)
> I would say something like "least warm, default display color" or layer or
> something like that.

Yes, much better! Thanks.
Done.

https://codereview.chromium.org/2857103007/diff/120001/ash/system/night_light...
File ash/system/night_light/night_light_controller_unittest.cc (right):

https://codereview.chromium.org/2857103007/diff/120001/ash/system/night_light...
ash/system/night_light/night_light_controller_unittest.cc:90:
controller->Toggle();
On 2017/05/08 16:09:07, James Cook (sheriff - slow) wrote:
> optional: I think tests are clearer when they explicitly call SetEnabled()
> instead of Toggle(), because then the reader doesn't have to think about the
old
> vs. new state.

Since I want to test Toggle() here, I added an explicit SetEnabled(false) at the
beginning of this test to explicitly specify the initial state.

https://codereview.chromium.org/2857103007/diff/120001/ash/system/night_light...
File ash/system/night_light/tray_night_light_unittest.cc (right):

https://codereview.chromium.org/2857103007/diff/120001/ash/system/night_light...
ash/system/night_light/tray_night_light_unittest.cc:5: #include
"ash/system/night_light/tray_night_light.h"
On 2017/05/08 16:09:07, James Cook (sheriff - slow) wrote:
> super nit: blank line below

Done.

https://codereview.chromium.org/2857103007/diff/120001/ash/system/night_light...
ash/system/night_light/tray_night_light_unittest.cc:11: using ash::Shell;
On 2017/05/08 16:09:07, James Cook (sheriff - slow) wrote:
> not needed, since you have namespace ash below

Yes, I know. Eclipse adds these automatically without me noticing. I hate that!
Done.

https://codereview.chromium.org/2857103007/diff/120001/ash/system/night_light...
ash/system/night_light/tray_night_light_unittest.cc:12: using
ash::test::AshTestBase;
On 2017/05/08 16:09:07, James Cook (sheriff - slow) wrote:
> ditto

Done.

https://codereview.chromium.org/2857103007/diff/120001/ash/system/night_light...
ash/system/night_light/tray_night_light_unittest.cc:28: controller->Toggle();
On 2017/05/08 16:09:07, James Cook (sheriff - slow) wrote:
> optional: ditto re: Toggle vs SetEnabled

Done.

[commit-bot: I haz the power, 2017-05-08 19:41:55]

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

[commit-bot: I haz the power, 2017-05-08 19:41:56]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2017-05-08 22:02:12]

chrome/ lgtm

[Mark P, 2017-05-08 23:18:09]

actions.xml lgtm

https://codereview.chromium.org/2857103007/diff/140001/tools/metrics/actions/...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2857103007/diff/140001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:16328: User action to toggle the Night Light
feature on/off.
optional nit: Everything here is a user action.  Perhaps drop that part of the
phrase, "Toggled the ..."

[afakhry, 2017-05-09 00:06:30]

James, I rebased on Xiyuan's change that landed today. We no longer need to
listen to OnSessionStateChanged() thanks to that fix.

https://codereview.chromium.org/2857103007/diff/140001/tools/metrics/actions/...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2857103007/diff/140001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:16328: User action to toggle the Night Light
feature on/off.
On 2017/05/08 23:18:09, Mark P wrote:
> optional nit: Everything here is a user action.  Perhaps drop that part of the
> phrase, "Toggled the ..."
> 

I updated the comment.

[afakhry, 2017-05-09 00:45:26]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2017-05-09 00:45:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org, thestig@chromium.org, mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/2857103007/#ps160001
(title: "Rebase on Xiyuan's change, and mpearson's nit.")

[commit-bot: I haz the power, 2017-05-09 00:49:56]

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, 2017-05-09 01:41:47]

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

[commit-bot: I haz the power, 2017-05-09 01:41:48]

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_...)

[James Cook, 2017-05-09 02:45:35]

https://codereview.chromium.org/2857103007/diff/160001/ash/system/night_light...
File ash/system/night_light/night_light_controller.cc (right):

https://codereview.chromium.org/2857103007/diff/160001/ash/system/night_light...
ash/system/night_light/night_light_controller.cc:94: // When user is switched in
multi profiles.
Is this comment still true? Is this code path also used for initial login?

[afakhry, 2017-05-09 16:32:17]

https://codereview.chromium.org/2857103007/diff/160001/ash/system/night_light...
File ash/system/night_light/night_light_controller.cc (right):

https://codereview.chromium.org/2857103007/diff/160001/ash/system/night_light...
ash/system/night_light/night_light_controller.cc:94: // When user is switched in
multi profiles.
On 2017/05/09 02:45:35, James Cook (sheriff - slow) wrote:
> Is this comment still true? Is this code path also used for initial login?

Yes, it's still true, and is also used for initial login.

I updated the comment.

[afakhry, 2017-05-09 16:41:18]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2017-05-09 16:41:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, jamescook@chromium.org, mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/2857103007/#ps180001
(title: "Update comment")

[commit-bot: I haz the power, 2017-05-09 16:42:03]

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, 2017-05-09 18:49:10]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1494348078673020,
"parent_rev": "13c178302b8a4ce77dc493127b1a9eff97c4e7c2", "commit_rev":
"e4fae85260876af2c08495ccf33b6c3051db1730"}

[commit-bot: I haz the power, 2017-05-09 18:49:21]

Description was changed from

==========
[Night Light] CL2: Ash and system tray work

Add ash and system tray initial plumbing. Support only toggling
NightLight on / off and setting the temperature to a fixed 0.5f
value when it's turned on for now.

Follow-up CLs will add system settings support.
Demo: https://bugs.chromium.org/p/chromium/issues/detail?id=705816#c4

BUG=705816
TEST=System tray has a toggle button. NightLight turns on / off with
     a 2-second animation. When on, a tray icon shows next to the
     clock and battery.
==========

to

==========
[Night Light] CL2: Ash and system tray work

Add ash and system tray initial plumbing. Support only toggling
NightLight on / off and setting the temperature to a fixed 0.5f
value when it's turned on for now.

Follow-up CLs will add system settings support.
Demo: https://bugs.chromium.org/p/chromium/issues/detail?id=705816#c4

BUG=705816
TEST=System tray has a toggle button. NightLight turns on / off with
     a 2-second animation. When on, a tray icon shows next to the
     clock and battery.

Review-Url: https://codereview.chromium.org/2857103007
Cr-Commit-Position: refs/heads/master@{#470392}
Committed:
https://chromium.googlesource.com/chromium/src/+/e4fae85260876af2c08495ccf33b...
==========

[commit-bot: I haz the power, 2017-05-09 18:49:22]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/e4fae85260876af2c08495ccf33b...