Disable idle delay doubling when smart dim is enabled.

In order to check if an experiment is enabled from //chromeos, this CL
also moves the feature flag from //chrome/common/chrome_features to
//chromeos/chromeos_features.

Bug: 888392
Change-Id: I05c6f7f096c8f1ed98e867ac147da3607c043776
Reviewed-on: https://chromium-review.googlesource.com/1242253Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Commit-Queue: Jia Meng <jiameng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594157}

ash/system/power/power_prefs.cc

@@ -256,6 +256,7 @@ void PowerPrefs::UpdatePowerPolicyFromPrefs() {
       prefs->GetBoolean(prefs::kPowerWaitForInitialUserActivity);
   values.force_nonzero_brightness_for_user_activity =
       prefs->GetBoolean(prefs::kPowerForceNonzeroBrightnessForUserActivity);
+  values.smart_dim_enabled = prefs->GetBoolean(prefs::kPowerSmartDimEnabled);
 
   power_policy_controller_->ApplyPrefs(values);
 }

chrome/browser/chromeos/power/ml/smart_dim/BUILD.gn

@@ -21,6 +21,7 @@ source_set("smart_dim") {
     "//chrome/browser/chromeos:user_activity_event_proto",
     "//chrome/browser/chromeos/power/ml:user_activity_ukm_logger_helpers",
     "//chrome/common:constants",
+    "//chromeos:chromeos",
     "//components/assist_ranker",
     "//components/assist_ranker/proto",
     "//components/sessions",

chrome/browser/chromeos/power/ml/smart_dim/model_impl.cc

@@ -17,8 +17,8 @@
 #include "base/strings/string_number_conversions.h"
 #include "chrome/browser/chromeos/power/ml/smart_dim/tf_native_inference.h"
 #include "chrome/browser/chromeos/power/ml/user_activity_ukm_logger_helpers.h"
-#include "chrome/common/chrome_features.h"
 #include "chrome/grit/browser_resources.h"
+#include "chromeos/chromeos_features.h"
 #include "components/assist_ranker/example_preprocessing.h"
 #include "components/assist_ranker/proto/example_preprocessor.pb.h"
 #include "components/assist_ranker/proto/ranker_example.pb.h"

chrome/browser/chromeos/power/ml/smart_dim/model_unittest.cc

@@ -11,7 +11,7 @@
 #include "base/test/scoped_feature_list.h"
 #include "base/time/time.h"
 #include "chrome/browser/chromeos/power/ml/user_activity_event.pb.h"
-#include "chrome/common/chrome_features.h"
+#include "chromeos/chromeos_features.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/page_transition_types.h"
 

chrome/browser/chromeos/power/ml/user_activity_manager.cc

@@ -18,7 +18,7 @@
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
-#include "chrome/common/chrome_features.h"
+#include "chromeos/chromeos_features.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/power_manager/power_supply_properties.pb.h"
 #include "chromeos/system/devicetype.h"

chrome/browser/chromeos/power/ml/user_activity_manager_unittest.cc

@@ -24,10 +24,10 @@
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/tabs/tab_activity_simulator.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
-#include "chrome/common/chrome_features.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/test_browser_window_aura.h"
 #include "chrome/test/base/testing_profile.h"
+#include "chromeos/chromeos_features.h"
 #include "chromeos/dbus/fake_power_manager_client.h"
 #include "chromeos/dbus/power_manager/idle.pb.h"
 #include "chromeos/dbus/power_manager/power_supply_properties.pb.h"

chrome/common/chrome_features.cc

@@ -587,12 +587,6 @@ const base::Feature kAdaptiveScreenBrightnessLogging{
 // Chrome OS.
 const base::Feature kUserActivityEventLogging{"UserActivityEventLogging",
                                               base::FEATURE_ENABLED_BY_DEFAULT};
-
-// Enables or disables user activity prediction for power management on
-// Chrome OS.
-const base::Feature kUserActivityPrediction{"UserActivityPrediction",
-                                            base::FEATURE_DISABLED_BY_DEFAULT};
-
 #endif
 
 // Enables using the main HTTP cache for media files as well.

chrome/common/chrome_features.h

@@ -393,8 +393,6 @@ extern const base::Feature kAdaptiveScreenBrightnessLogging;
 COMPONENT_EXPORT(CHROME_FEATURES)
 extern const base::Feature kUserActivityEventLogging;
 
-COMPONENT_EXPORT(CHROME_FEATURES)
-extern const base::Feature kUserActivityPrediction;
 #endif
 
 COMPONENT_EXPORT(CHROME_FEATURES)

chromeos/chromeos_features.cc

@@ -45,6 +45,13 @@ const base::Feature kInstantTethering{"InstantTethering",
 const base::Feature kMultiDeviceApi{"MultiDeviceApi",
                                     base::FEATURE_DISABLED_BY_DEFAULT};
 
+// Enables or disables user activity prediction for power management on
+// Chrome OS.
+// Defined here rather than in //chrome alongside other related features so that
+// PowerPolicyController can check it.
+const base::Feature kUserActivityPrediction{"UserActivityPrediction",
+                                            base::FEATURE_DISABLED_BY_DEFAULT};
+
 }  // namespace features
 
 }  // namespace chromeos

chromeos/chromeos_features.h

@@ -23,8 +23,8 @@ CHROMEOS_EXPORT extern const base::Feature kEnableUnifiedMultiDeviceSettings;
 CHROMEOS_EXPORT extern const base::Feature kEnableUnifiedMultiDeviceSetup;
 CHROMEOS_EXPORT extern const base::Feature kImeServiceConnectable;
 CHROMEOS_EXPORT extern const base::Feature kInstantTethering;
-
 CHROMEOS_EXPORT extern const base::Feature kMultiDeviceApi;
+CHROMEOS_EXPORT extern const base::Feature kUserActivityPrediction;
 
 }  // namespace features
 

chromeos/dbus/DEPS

@@ -3,6 +3,7 @@ noparent = True
 include_rules = [
   "+base",
   "+chromeos/chromeos_export.h",
+  "+chromeos/chromeos_features.h",
   "+components/account_id/account_id.h",
   "+components/device_event_log",
   "+components/policy/proto",

chromeos/dbus/power_policy_controller.cc

@@ -8,10 +8,12 @@
 
 #include <utility>
 
+#include "base/feature_list.h"
 #include "base/format_macros.h"
 #include "base/logging.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "chromeos/chromeos_features.h"
 
 // Avoid some ugly line-wrapping later.
 using base::StringAppendF;
@@ -111,7 +113,8 @@ PowerPolicyController::PrefValues::PrefValues()
       presentation_screen_dim_delay_factor(1.0),
       user_activity_screen_dim_delay_factor(1.0),
       wait_for_initial_user_activity(false),
-      force_nonzero_brightness_for_user_activity(true) {}
+      force_nonzero_brightness_for_user_activity(true),
+      smart_dim_enabled(true) {}
 
 // static
 std::string PowerPolicyController::GetPolicyDebugString(
@@ -236,10 +239,22 @@ void PowerPolicyController::ApplyPrefs(const PrefValues& values) {
     prefs_policy_.set_battery_brightness_percent(
         values.battery_brightness_percent);
   }
-  prefs_policy_.set_presentation_screen_dim_delay_factor(
-      values.presentation_screen_dim_delay_factor);
-  prefs_policy_.set_user_activity_screen_dim_delay_factor(
-      values.user_activity_screen_dim_delay_factor);
+
+  // Screen-dim deferral in response to user activity predictions can
+  // interact poorly with delay scaling, resulting in the system staying
+  // awake for a long time if a prediction is wrong. See
+  // https://crbug.com/888392.
+  if (values.smart_dim_enabled &&
+      base::FeatureList::IsEnabled(features::kUserActivityPrediction)) {
+    prefs_policy_.set_presentation_screen_dim_delay_factor(1.0);
+    prefs_policy_.set_user_activity_screen_dim_delay_factor(1.0);
+  } else {
+    prefs_policy_.set_presentation_screen_dim_delay_factor(
+        values.presentation_screen_dim_delay_factor);
+    prefs_policy_.set_user_activity_screen_dim_delay_factor(
+        values.user_activity_screen_dim_delay_factor);
+  }
+
   prefs_policy_.set_wait_for_initial_user_activity(
       values.wait_for_initial_user_activity);
   prefs_policy_.set_force_nonzero_brightness_for_user_activity(

chromeos/dbus/power_policy_controller.h

@@ -77,6 +77,7 @@ class CHROMEOS_EXPORT PowerPolicyController
     double user_activity_screen_dim_delay_factor;
     bool wait_for_initial_user_activity;
     bool force_nonzero_brightness_for_user_activity;
+    bool smart_dim_enabled;
   };
 
   // Returns a string describing |policy|.  Useful for tests.

chromeos/dbus/power_policy_controller_unittest.cc

@@ -7,6 +7,8 @@
 #include <memory>
 
 #include "base/message_loop/message_loop.h"
+#include "base/test/scoped_feature_list.h"
+#include "chromeos/chromeos_features.h"
 #include "chromeos/dbus/fake_power_manager_client.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -325,4 +327,40 @@ TEST_F(PowerPolicyControllerTest, SuspendOnLidClosedWhileSignedOut) {
                 fake_power_client_->policy()));
 }
 
+TEST_F(PowerPolicyControllerTest, SmartDimEnabledExperimentEnabled) {
+  const std::map<std::string, std::string> params = {
+      {"dim_threshold", "0.651"}};
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeatureWithParameters(
+      chromeos::features::kUserActivityPrediction, params);
+
+  PowerPolicyController::PrefValues prefs;
+  policy_controller_->ApplyPrefs(prefs);
+  const power_manager::PowerManagementPolicy kDefaultPolicy =
+      fake_power_client_->policy();
+
+  // First disable smart dim model.
+  prefs.smart_dim_enabled = false;
+  prefs.presentation_screen_dim_delay_factor = 3.0;
+  prefs.user_activity_screen_dim_delay_factor = 2.0;
+  policy_controller_->ApplyPrefs(prefs);
+
+  power_manager::PowerManagementPolicy expected_policy = kDefaultPolicy;
+  expected_policy.set_presentation_screen_dim_delay_factor(3.0);
+  expected_policy.set_user_activity_screen_dim_delay_factor(2.0);
+  EXPECT_EQ(PowerPolicyController::GetPolicyDebugString(expected_policy),
+            PowerPolicyController::GetPolicyDebugString(
+                fake_power_client_->policy()));
+
+  // Then enable smart dim model.
+  prefs.smart_dim_enabled = true;
+  policy_controller_->ApplyPrefs(prefs);
+
+  expected_policy.set_presentation_screen_dim_delay_factor(1.0);
+  expected_policy.set_user_activity_screen_dim_delay_factor(1.0);
+  EXPECT_EQ(PowerPolicyController::GetPolicyDebugString(expected_policy),
+            PowerPolicyController::GetPolicyDebugString(
+                fake_power_client_->policy()));
+}
+
 }  // namespace chromeos