Follow-up for Ife38c2cd94ac9902911b005dbbca8b0d0a62e6d7

Address review comments on the previous CL.
(Plus a couple bug fixes.)

Test: atest BatterySaverPolicyTest
Test: manual test
Bug: 68769804
Change-Id: If2de9148d1b8175a9f0d66bc3a7ecd02ce7a620b

5 files changed, 87 insertions, 55 deletions

diff --git a/core/java/com/android/internal/util/ArrayUtils.java b/core/java/com/android/internal/util/ArrayUtils.java
index 91bc6813c5fc..22bfcc347c3a 100644
--- a/core/java/com/android/internal/util/ArrayUtils.java
+++ b/core/java/com/android/internal/util/ArrayUtils.java
@@ -30,6 +30,7 @@ import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
+import java.util.Map;
 import java.util.Objects;
 
 /**
@@ -134,6 +135,13 @@ public class ArrayUtils {
     }
 
     /**
+     * Checks if given map is null or has zero elements.
+     */
+    public static boolean isEmpty(@Nullable Map<?, ?> map) {
+        return map == null || map.isEmpty();
+    }
+
+    /**
      * Checks if given array is null or has zero elements.
      */
     public static <T> boolean isEmpty(@Nullable T[] array) {
diff --git a/services/core/java/com/android/server/power/BatterySaverPolicy.java b/services/core/java/com/android/server/power/BatterySaverPolicy.java
index 15121b809f95..afa4eb078b9a 100644
--- a/services/core/java/com/android/server/power/BatterySaverPolicy.java
+++ b/services/core/java/com/android/server/power/BatterySaverPolicy.java
@@ -35,10 +35,15 @@ import com.android.internal.R;
 
 import java.io.PrintWriter;
 import java.util.ArrayList;
+import java.util.List;
 
 /**
  * Class to decide whether to turn on battery saver mode for specific service
  *
+ * TODO: We should probably make {@link #mFilesForInteractive} and {@link #mFilesForNoninteractive}
+ * less flexible and just take a list of "CPU number - frequency" pairs. Being able to write
+ * anything under /sys/ and /proc/ is too loose.
+ *
  * Test: atest BatterySaverPolicyTest
  */
 public class BatterySaverPolicy extends ContentObserver {
@@ -66,8 +71,8 @@ public class BatterySaverPolicy extends ContentObserver {
     private static final String KEY_FORCE_ALL_APPS_STANDBY_ALARMS = "force_all_apps_standby_alarms";
     private static final String KEY_OPTIONAL_SENSORS_DISABLED = "optional_sensors_disabled";
 
-    private static final String KEY_SCREEN_ON_FILE_PREFIX = "file-on:";
-    private static final String KEY_SCREEN_OFF_FILE_PREFIX = "file-off:";
+    private static final String KEY_FILE_FOR_INTERACTIVE_PREFIX = "file-on:";
+    private static final String KEY_FILE_FOR_NONINTERACTIVE_PREFIX = "file-off:";
 
     private static String mSettings;
     private static String mDeviceSpecificSettings;
@@ -179,25 +184,25 @@ public class BatterySaverPolicy extends ContentObserver {
     private ContentResolver mContentResolver;
 
     @GuardedBy("mLock")
-    private final ArrayList<BatterySaverPolicyListener> mListeners = new ArrayList<>();
+    private final List<BatterySaverPolicyListener> mListeners = new ArrayList<>();
 
     /**
      * List of [Filename -> content] that should be written when battery saver is activated
-     * and the screen is on.
+     * and the device is interactive.
      *
      * We use this to change the max CPU frequencies.
      */
     @GuardedBy("mLock")
-    private ArrayMap<String, String> mScreenOnFiles;
+    private ArrayMap<String, String> mFilesForInteractive;
 
     /**
      * List of [Filename -> content] that should be written when battery saver is activated
-     * and the screen is off.
+     * and the device is non-interactive.
      *
      * We use this to change the max CPU frequencies.
      */
     @GuardedBy("mLock")
-    private ArrayMap<String, String> mScreenOffFiles;
+    private ArrayMap<String, String> mFilesForNoninteractive;
 
     public interface BatterySaverPolicyListener {
         void onBatterySaverPolicyChanged(BatterySaverPolicy policy);
@@ -236,11 +241,6 @@ public class BatterySaverPolicy extends ContentObserver {
         return R.string.config_batterySaverDeviceSpecificConfig;
     }
 
-    @VisibleForTesting
-    void onChangeForTest() {
-        onChange(true, null);
-    }
-
     @Override
     public void onChange(boolean selfChange, Uri uri) {
         final BatterySaverPolicyListener[] listeners;
@@ -315,8 +315,8 @@ public class BatterySaverPolicy extends ContentObserver {
                     + deviceSpecificSetting);
         }
 
-        mScreenOnFiles = collectParams(parser, KEY_SCREEN_ON_FILE_PREFIX);
-        mScreenOffFiles = collectParams(parser, KEY_SCREEN_OFF_FILE_PREFIX);
+        mFilesForInteractive = collectParams(parser, KEY_FILE_FOR_INTERACTIVE_PREFIX);
+        mFilesForNoninteractive = collectParams(parser, KEY_FILE_FOR_NONINTERACTIVE_PREFIX);
     }
 
     private static ArrayMap<String, String> collectParams(
@@ -330,7 +330,7 @@ public class BatterySaverPolicy extends ContentObserver {
             }
             final String path = key.substring(prefix.length());
 
-            if (!(path.startsWith("/sys/") || path.startsWith("/proc"))) {
+            if (!(path.startsWith("/sys/") || path.startsWith("/proc/"))) {
                 Slog.wtf(TAG, "Invalid path: " + path);
                 continue;
             }
@@ -403,9 +403,9 @@ public class BatterySaverPolicy extends ContentObserver {
         }
     }
 
-    public ArrayMap<String, String> getFileValues(boolean screenOn) {
+    public ArrayMap<String, String> getFileValues(boolean interactive) {
         synchronized (mLock) {
-            return screenOn ? mScreenOnFiles : mScreenOffFiles;
+            return interactive ? mFilesForInteractive : mFilesForNoninteractive;
         }
     }
 
@@ -433,12 +433,12 @@ public class BatterySaverPolicy extends ContentObserver {
             pw.println("  " + KEY_OPTIONAL_SENSORS_DISABLED + "=" + mOptionalSensorsDisabled);
             pw.println();
 
-            pw.print("  Screen On Files:\n");
-            dumpMap(pw, "    ", mScreenOnFiles);
+            pw.print("  Interactive File values:\n");
+            dumpMap(pw, "    ", mFilesForInteractive);
             pw.println();
 
-            pw.print("  Screen Off Files:\n");
-            dumpMap(pw, "    ", mScreenOffFiles);
+            pw.print("  Noninteractive File values:\n");
+            dumpMap(pw, "    ", mFilesForNoninteractive);
             pw.println();
         }
     }
diff --git a/services/core/java/com/android/server/power/PowerManagerService.java b/services/core/java/com/android/server/power/PowerManagerService.java
index a47b8095dae7..584761c3c0ef 100644
--- a/services/core/java/com/android/server/power/PowerManagerService.java
+++ b/services/core/java/com/android/server/power/PowerManagerService.java
@@ -3105,7 +3105,7 @@ public final class PowerManagerService extends SystemService
         mIsVrModeEnabled = enabled;
     }
 
-    public static void powerHintInternal(int hintId, int data) {
+    private static void powerHintInternal(int hintId, int data) {
         nativeSendPowerHint(hintId, data);
     }
 
diff --git a/services/core/java/com/android/server/power/batterysaver/BatterySaverController.java b/services/core/java/com/android/server/power/batterysaver/BatterySaverController.java
index b3e853838069..3db6a25f5413 100644
--- a/services/core/java/com/android/server/power/batterysaver/BatterySaverController.java
+++ b/services/core/java/com/android/server/power/batterysaver/BatterySaverController.java
@@ -25,6 +25,7 @@ import android.os.Handler;
 import android.os.Looper;
 import android.os.Message;
 import android.os.PowerManager;
+import android.os.PowerManagerInternal;
 import android.os.PowerManagerInternal.LowPowerModeListener;
 import android.os.PowerSaveState;
 import android.os.UserHandle;
@@ -33,7 +34,9 @@ import android.util.Slog;
 import android.widget.Toast;
 
 import com.android.internal.annotations.GuardedBy;
+import com.android.internal.util.ArrayUtils;
 import com.android.internal.util.Preconditions;
+import com.android.server.LocalServices;
 import com.android.server.power.BatterySaverPolicy;
 import com.android.server.power.BatterySaverPolicy.BatterySaverPolicyListener;
 import com.android.server.power.PowerManagerService;
@@ -63,20 +66,17 @@ public class BatterySaverController implements BatterySaverPolicyListener {
     @GuardedBy("mLock")
     private boolean mEnabled;
 
-    /**
-     * Keep track of the previous enabled state, which we use to decide when to send broadcasts,
-     * which we don't want to send only when the screen state changes.
-     */
-    @GuardedBy("mLock")
-    private boolean mWasEnabled;
-
     private final BroadcastReceiver mReceiver = new BroadcastReceiver() {
         @Override
         public void onReceive(Context context, Intent intent) {
             switch (intent.getAction()) {
                 case Intent.ACTION_SCREEN_ON:
                 case Intent.ACTION_SCREEN_OFF:
-                    mHandler.postStateChanged();
+                    if (!isEnabled()) {
+                        return; // No need to send it if not enabled.
+                    }
+                    // Don't send the broadcast, because we never did so in this case.
+                    mHandler.postStateChanged(/*sendBroadcast=*/ false);
                     break;
             }
         }
@@ -121,25 +121,32 @@ public class BatterySaverController implements BatterySaverPolicyListener {
 
     @Override
     public void onBatterySaverPolicyChanged(BatterySaverPolicy policy) {
-        mHandler.postStateChanged();
+        if (!isEnabled()) {
+            return; // No need to send it if not enabled.
+        }
+        mHandler.postStateChanged(/*sendBroadcast=*/ true);
     }
 
     private class MyHandler extends Handler {
-        private final int MSG_STATE_CHANGED = 1;
+        private static final int MSG_STATE_CHANGED = 1;
+
+        private static final int ARG_DONT_SEND_BROADCAST = 0;
+        private static final int ARG_SEND_BROADCAST = 1;
 
         public MyHandler(Looper looper) {
             super(looper);
         }
 
-        public void postStateChanged() {
-            obtainMessage(MSG_STATE_CHANGED).sendToTarget();
+        public void postStateChanged(boolean sendBroadcast) {
+            obtainMessage(MSG_STATE_CHANGED, sendBroadcast ?
+                    ARG_SEND_BROADCAST : ARG_DONT_SEND_BROADCAST, 0).sendToTarget();
         }
 
         @Override
         public void dispatchMessage(Message msg) {
             switch (msg.what) {
                 case MSG_STATE_CHANGED:
-                    handleBatterySaverStateChanged();
+                    handleBatterySaverStateChanged(msg.arg1 == ARG_SEND_BROADCAST);
                     break;
             }
         }
@@ -155,38 +162,53 @@ public class BatterySaverController implements BatterySaverPolicyListener {
             }
             mEnabled = enable;
 
-            mHandler.postStateChanged();
+            mHandler.postStateChanged(/*sendBroadcast=*/ true);
+        }
+    }
+
+    /** @return whether battery saver is enabled or not. */
+    boolean isEnabled() {
+        synchronized (mLock) {
+            return mEnabled;
         }
     }
 
     /**
      * Dispatch power save events to the listeners.
      *
-     * This is always called on the handler thread.
+     * This method is always called on the handler thread.
+     *
+     * This method is called only in the following cases:
+     * - When battery saver becomes activated.
+     * - When battery saver becomes deactivated.
+     * - When battery saver is on the interactive state changes.
+     * - When battery saver is on the battery saver policy changes.
      */
-    void handleBatterySaverStateChanged() {
+    void handleBatterySaverStateChanged(boolean sendBroadcast) {
         final LowPowerModeListener[] listeners;
 
-        final boolean wasEnabled;
         final boolean enabled;
-        final boolean isScreenOn = getPowerManager().isInteractive();
+        final boolean isInteractive = getPowerManager().isInteractive();
         final ArrayMap<String, String> fileValues;
 
         synchronized (mLock) {
-            Slog.i(TAG, "Battery saver enabled: screen on=" + isScreenOn);
+            Slog.i(TAG, "Battery saver " + (mEnabled ? "enabled" : "disabled")
+                    + ": isInteractive=" + isInteractive);
 
             listeners = mListeners.toArray(new LowPowerModeListener[mListeners.size()]);
-            wasEnabled = mWasEnabled;
             enabled = mEnabled;
 
             if (enabled) {
-                fileValues = mBatterySaverPolicy.getFileValues(isScreenOn);
+                fileValues = mBatterySaverPolicy.getFileValues(isInteractive);
             } else {
                 fileValues = null;
             }
         }
 
-        PowerManagerService.powerHintInternal(PowerHint.LOW_POWER, enabled ? 1 : 0);
+        final PowerManagerInternal pmi = LocalServices.getService(PowerManagerInternal.class);
+        if (pmi != null) {
+            pmi.powerHint(PowerHint.LOW_POWER, enabled ? 1 : 0);
+        }
 
         if (enabled) {
             // STOPSHIP Remove the toast.
@@ -195,13 +217,13 @@ public class BatterySaverController implements BatterySaverPolicyListener {
                     Toast.LENGTH_LONG).show();
         }
 
-        if (fileValues == null || fileValues.size() == 0) {
+        if (ArrayUtils.isEmpty(fileValues)) {
             mFileUpdater.restoreDefault();
         } else {
             mFileUpdater.writeFiles(fileValues);
         }
 
-        if (enabled != wasEnabled) {
+        if (sendBroadcast) {
             if (DEBUG) {
                 Slog.i(TAG, "Sending broadcasts for mode: " + enabled);
             }
@@ -231,9 +253,5 @@ public class BatterySaverController implements BatterySaverPolicyListener {
                 listener.onLowPowerModeChanged(result);
             }
         }
-
-        synchronized (mLock) {
-            mWasEnabled = enabled;
-        }
     }
 }
diff --git a/services/tests/servicestests/src/com/android/server/power/BatterySaverPolicyTest.java b/services/tests/servicestests/src/com/android/server/power/BatterySaverPolicyTest.java
index 5b6225e79f8e..0db19e452650 100644
--- a/services/tests/servicestests/src/com/android/server/power/BatterySaverPolicyTest.java
+++ b/services/tests/servicestests/src/com/android/server/power/BatterySaverPolicyTest.java
@@ -18,13 +18,13 @@ package com.android.server.power;
 import android.os.PowerManager.ServiceType;
 import android.os.PowerSaveState;
 import android.os.Handler;
-import android.provider.Settings;
 import android.provider.Settings.Global;
 import android.test.AndroidTestCase;
 import android.test.suitebuilder.annotation.SmallTest;
 import android.util.ArrayMap;
 
 import com.android.frameworks.servicestests.R;
+import com.android.internal.annotations.VisibleForTesting;
 
 import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
@@ -68,6 +68,12 @@ public class BatterySaverPolicyTest extends AndroidTestCase {
         int getDeviceSpecificConfigResId() {
             return mDeviceSpecificConfigResId;
         }
+
+
+        @VisibleForTesting
+        void onChange() {
+            onChange(true, null);
+        }
     }
 
     @Mock
@@ -221,14 +227,14 @@ public class BatterySaverPolicyTest extends AndroidTestCase {
         mMockGlobalSettings.put(Global.BATTERY_SAVER_CONSTANTS, "");
         mMockGlobalSettings.put(Global.BATTERY_SAVER_DEVICE_SPECIFIC_CONSTANTS, "");
 
-        mBatterySaverPolicy.onChangeForTest();
+        mBatterySaverPolicy.onChange();
         assertThat(mBatterySaverPolicy.getFileValues(true).toString()).isEqualTo("{}");
         assertThat(mBatterySaverPolicy.getFileValues(false).toString()).isEqualTo("{}");
 
 
         mDeviceSpecificConfigResId = R.string.config_batterySaverDeviceSpecificConfig_2;
 
-        mBatterySaverPolicy.onChangeForTest();
+        mBatterySaverPolicy.onChange();
         assertThat(mBatterySaverPolicy.getFileValues(true).toString()).isEqualTo("{}");
         assertThat(mBatterySaverPolicy.getFileValues(false).toString())
                 .isEqualTo("{/sys/a=1, /sys/b=2}");
@@ -236,7 +242,7 @@ public class BatterySaverPolicyTest extends AndroidTestCase {
 
         mDeviceSpecificConfigResId = R.string.config_batterySaverDeviceSpecificConfig_3;
 
-        mBatterySaverPolicy.onChangeForTest();
+        mBatterySaverPolicy.onChange();
         assertThat(mBatterySaverPolicy.getFileValues(true).toString()).isEqualTo("{/proc/c=4}");
         assertThat(mBatterySaverPolicy.getFileValues(false).toString()).isEqualTo("{/sys/a=3}");
 
@@ -244,7 +250,7 @@ public class BatterySaverPolicyTest extends AndroidTestCase {
         mMockGlobalSettings.put(Global.BATTERY_SAVER_DEVICE_SPECIFIC_CONSTANTS,
                 "file-on:/proc/z=4");
 
-        mBatterySaverPolicy.onChangeForTest();
+        mBatterySaverPolicy.onChange();
         assertThat(mBatterySaverPolicy.getFileValues(true).toString()).isEqualTo("{/proc/z=4}");
         assertThat(mBatterySaverPolicy.getFileValues(false).toString()).isEqualTo("{}");
     }