[Night Light] CL2: Ash and system tray work

ash/system/night_light/night_light_controller.cc

Index: ash/system/night_light/night_light_controller.cc
diff --git a/ash/system/night_light/night_light_controller.cc b/ash/system/night_light/night_light_controller.cc
new file mode 100644
index 0000000000000000000000000000000000000000..250710775e401a850cee191903b44460c4c1632a
--- /dev/null
+++ b/ash/system/night_light/night_light_controller.cc
@@ -0,0 +1,158 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ash/system/night_light/night_light_controller.h"
+
+#include "ash/session/session_controller.h"
+#include "ash/shell.h"
+#include "base/time/time.h"
+#include "base/values.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/scoped_user_pref_update.h"
+#include "ui/compositor/layer.h"
+#include "ui/compositor/scoped_layer_animation_settings.h"
+
+namespace ash {
+
+namespace {
+
+// The key of the dictionary value in the user's pref service that contains all
+// the night light settings.

[James Cook, 2017/05/05 17:12:31]

super nit: You're inconsistent in comments about "night light" vs. "NightLight".
Either is fine, but be consistent.

[afakhry, 2017/05/05 20:04:10]

Done.

+constexpr char kNightLightPrefsKey[] = "prefs.night_light_prefs";
+
+// Keys to the various NightLight settings inside its dictionary value.
+constexpr char kStatusKey[] = "night_light_status";
+constexpr char kColorTemperatureKey[] = "night_light_color_temperature";
+
+// The duration of the temperature change animation when the change is a result
+// of a manual user setting.
+// TODO(afakhry): Add automatic schedule animation duration when you implement
+// that part. It should be large enough (20 seconds as agreed) to give the user
+// a nice smooth transition.
+constexpr int kManualToggleAnimationDurationSec = 2;
+
+// True if animations are disabled for tests.
+bool animations_disabled_for_tests = false;

[James Cook, 2017/05/05 17:12:31]

optional: g_animations_disabled_for_test

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

[afakhry, 2017/05/05 20:04:10]

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.

+
+}  // namespace
+
+NightLightController::NightLightController(
+    SessionController* session_controller)

[James Cook, 2017/05/05 17:12:31]

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.

[afakhry, 2017/05/05 20:04:10]

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.

+    : session_controller_(session_controller) {
+  session_controller_->AddObserver(this);
+}
+
+NightLightController::~NightLightController() {
+  session_controller_->RemoveObserver(this);
+}
+
+// static
+void NightLightController::RegisterPrefs(PrefRegistrySimple* registry) {
+  registry->RegisterDictionaryPref(kNightLightPrefsKey);
+}
+
+// static
+void NightLightController::DisableAnimationsForTests() {
+  animations_disabled_for_tests = true;
+}
+
+void NightLightController::AddObserver(Observer* observer) {
+  observers_.AddObserver(observer);
+}
+
+void NightLightController::RemoveObserver(Observer* observer) {
+  observers_.RemoveObserver(observer);
+}
+
+void NightLightController::Toggle() {
+  SetEnabled(!enabled_);
+}
+
+void NightLightController::SetEnabled(bool enabled) {
+  if (enabled_ == enabled)
+    return;
+
+  enabled_ = enabled;
+  Refresh();
+  NotifyStatusChanged();
+  PersistUserPrefs();
+}
+
+void NightLightController::SetColorTemperature(float temperature) {
+  // TODO(afakhry): Spport changing the temperature when you implement the
+  // settings part of this feature. Right now we'll keep it fixed at the value
+  // |color_temperature_| whenever NightLight is turned on.
+
+  for (aura::Window* root_window : ash::Shell::GetAllRootWindows()) {

[James Cook, 2017/05/05 17:12:30]

nit: no ash::

[afakhry, 2017/05/05 20:04:10]

Done.

+    ui::Layer* layer = root_window->layer();
+
+    ui::ScopedLayerAnimationSettings settings(layer->GetAnimator());
+    settings.SetTransitionDuration(base::TimeDelta::FromSeconds(
+        animations_disabled_for_tests ? 0 : kManualToggleAnimationDurationSec));

[James Cook, 2017/05/05 17:12:31]

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?

[afakhry, 2017/05/05 20:04:10]

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.

+    settings.SetPreemptionStrategy(
+        ui::LayerAnimator::IMMEDIATELY_ANIMATE_TO_NEW_TARGET);
+
+    layer->SetLayerTemperature(temperature);
+  }
+}
+
+void NightLightController::OnActiveUserSessionChanged(
+    const AccountId& account_id) {
+  // When user is switched in multi profiles.
+  InitFromUserPrefs();
+}
+
+void NightLightController::Refresh() {
+  SetColorTemperature(enabled_ ? color_temperature_ : 0.0f);
+}
+
+void NightLightController::OnSessionStateChanged(
+    session_manager::SessionState state) {
+  if (state == session_manager::SessionState::ACTIVE)
+    InitFromUserPrefs();
+}
+
+void NightLightController::InitFromUserPrefs() {
+  auto* pref_service = Shell::Get()->GetActiveUserPrefService();
+  if (!pref_service) {
+    LOG(ERROR) << "Can't initialize NightLight user settings. No "

[James Cook, 2017/05/05 17:12:31]

Can this be a CHECK()? It seems unexpected to try to initialize when there is no
pref service.

[afakhry, 2017/05/05 20:04:10]

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

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

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.

[afakhry, 2017/05/06 00:49:34]

Done.

+               << "PrefService available.";
+    return;
+  }
+
+  const base::DictionaryValue* night_light_prefs =
+      pref_service->GetDictionary(kNightLightPrefsKey);
+  bool enabled = false;
+  night_light_prefs->GetBoolean(kStatusKey, &enabled);
+  enabled_ = enabled;
+
+  double color_temperature = 0.5;
+  night_light_prefs->GetDouble(kColorTemperatureKey, &color_temperature);
+  color_temperature_ = static_cast<float>(color_temperature);
+
+  Refresh();
+  NotifyStatusChanged();
+}
+
+void NightLightController::PersistUserPrefs() {
+  auto* pref_service = ash::Shell::Get()->GetActiveUserPrefService();
+  if (!pref_service) {

[James Cook, 2017/05/05 17:12:31]

ditto

[afakhry, 2017/05/05 20:04:10]

Same reason as above.

+    LOG(ERROR) << "Can't persist NightLight user settings. No PrefService "
+               << "Available.";
+    return;
+  }
+  DictionaryPrefUpdate pref_updater(pref_service, kNightLightPrefsKey);
+
+  base::DictionaryValue* dictionary = pref_updater.Get();
+  dictionary->SetBoolean(kStatusKey, enabled_);
+  dictionary->SetDouble(kColorTemperatureKey,
+                        static_cast<double>(color_temperature_));
+}
+
+void NightLightController::NotifyStatusChanged() {
+  for (auto& obs : observers_)

[James Cook, 2017/05/05 17:12:31]

super nit: obs -> observer

[afakhry, 2017/05/05 20:04:10]

Done.

+    obs.OnStatusChanged(enabled_);
+}
+
+}  // namespace ash