Remove external dependencies from ModuleActivityObserver

Implemented Strategy Pattern so that the class can be tested more easily.
Moved all external dependencies into an injectable class (SplitCompatStrategy).
Removed "spying" from unit tests.

Bug: 949729
Change-Id: If35af947f207a429e1d9badeb5b1b1fdb978e5ed
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1752504Reviewed-by: Tibor Goldschwendt <tiborg@chromium.org>
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Reviewed-by: Peter Wen <wnwen@chromium.org>
Commit-Queue: Fred Mello <fredmello@chromium.org>
Cr-Commit-Position: refs/heads/master@{#688191}

chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java

@@ -42,7 +42,7 @@ import org.chromium.chrome.browser.webapps.ChromeWebApkHost;
 import org.chromium.components.background_task_scheduler.BackgroundTaskSchedulerExternalUma;
 import org.chromium.components.crash.browser.ChildProcessCrashObserver;
 import org.chromium.components.minidump_uploader.CrashFileManager;
-import org.chromium.components.module_installer.ModuleActivityObserver;
+import org.chromium.components.module_installer.observers.ModuleActivityObserver;
 import org.chromium.content_public.browser.BrowserStartupController;
 import org.chromium.content_public.browser.DeviceUtils;
 import org.chromium.content_public.browser.SpeechRecognition;

components/module_installer/android/BUILD.gn

@@ -14,7 +14,9 @@ android_library("module_installer_java") {
     "java/src-common/org/chromium/components/module_installer/ModuleInstaller.java",
     "java/src-common/org/chromium/components/module_installer/OnModuleInstallFinishedListener.java",
     "java/src-common/org/chromium/components/module_installer/Module.java",
-    "java/src-common/org/chromium/components/module_installer/ModuleActivityObserver.java",
+    "java/src-common/org/chromium/components/module_installer/observers/ModuleActivityObserver.java",
+    "java/src-common/org/chromium/components/module_installer/observers/ObserverStrategy.java",
+    "java/src-common/org/chromium/components/module_installer/observers/ObserverStrategyImpl.java",
   ]
   jar_excluded_patterns = [ "*/ModuleInstallerImpl.class" ]
   deps = [
@@ -63,7 +65,7 @@ android_library("module_installer_test_java") {
 }
 
 junit_binary("module_installer_junit_tests") {
-  java_files = [ "junit/src/org/chromium/components/module_installer/ModuleActivityObserverTest.java" ]
+  java_files = [ "junit/src/org/chromium/components/module_installer/observers/ModuleActivityObserverTest.java" ]
   deps = [
     ":module_installer_impl_java",
     "//base:base_java",

components/module_installer/android/java/src-common/org/chromium/components/module_installer/ModuleActivityObserver.java → components/module_installer/android/java/src-common/org/chromium/components/module_installer/observers/ModuleActivityObserver.java

@@ -2,25 +2,37 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-package org.chromium.components.module_installer;
+package org.chromium.components.module_installer.observers;
 
 import android.app.Activity;
 
 import org.chromium.base.ActivityState;
 import org.chromium.base.ApplicationStatus;
 import org.chromium.base.ThreadUtils;
-import org.chromium.base.VisibleForTesting;
 
 import java.util.HashSet;
-import java.util.List;
 
-/** Observer for activities so that DFMs can be lazily installed on-demand. */
+/**
+ *  Observer for activities so that DFMs can be lazily installed on-demand.
+ *  Note that ActivityIds are managed globally and therefore any changes to it are to be made
+ *  using a single thread (in this case, the UI thread).
+ */
 public class ModuleActivityObserver implements ApplicationStatus.ActivityStateListener {
-    /** Tracks activities that have been splitcompatted. */
     private static HashSet<Integer> sActivityIds = new HashSet<Integer>();
+    private final ObserverStrategy mStrategy;
+
+    public ModuleActivityObserver() {
+        this(new ObserverStrategyImpl());
+    }
+
+    public ModuleActivityObserver(ObserverStrategy strategy) {
+        mStrategy = strategy;
+    }
 
     @Override
     public void onActivityStateChange(Activity activity, @ActivityState int newState) {
+        ThreadUtils.assertOnUiThread();
+
         if (newState == ActivityState.CREATED || newState == ActivityState.RESUMED) {
             splitCompatActivity(activity);
         } else if (newState == ActivityState.DESTROYED) {
@@ -34,8 +46,8 @@ public class ModuleActivityObserver implements ApplicationStatus.ActivityStateLi
 
         sActivityIds.clear();
 
-        for (Activity activity : getRunningActivities()) {
-            if (getStateForActivity(activity) == ActivityState.RESUMED) {
+        for (Activity activity : mStrategy.getRunningActivities()) {
+            if (mStrategy.getStateForActivity(activity) == ActivityState.RESUMED) {
                 splitCompatActivity(activity);
             }
         }
@@ -46,22 +58,7 @@ public class ModuleActivityObserver implements ApplicationStatus.ActivityStateLi
         Integer key = activity.hashCode();
         if (!sActivityIds.contains(key)) {
             sActivityIds.add(key);
-            getModuleInstaller().initActivity(activity);
+            mStrategy.getModuleInstaller().initActivity(activity);
         }
     }
-
-    @VisibleForTesting
-    public ModuleInstaller getModuleInstaller() {
-        return ModuleInstallerImpl.getInstance();
-    }
-
-    @VisibleForTesting
-    public List<Activity> getRunningActivities() {
-        return ApplicationStatus.getRunningActivities();
-    }
-
-    @VisibleForTesting
-    public int getStateForActivity(Activity activity) {
-        return ApplicationStatus.getStateForActivity(activity);
-    }
 }
\ No newline at end of file

components/module_installer/android/java/src-common/org/chromium/components/module_installer/observers/ObserverStrategy.java

+// Copyright 2019 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.
+
+package org.chromium.components.module_installer.observers;
+
+import android.app.Activity;
+
+import org.chromium.components.module_installer.ModuleInstaller;
+
+import java.util.List;
+
+/** Interface outlining the necessary strategy to load activities and install modules. */
+interface ObserverStrategy {
+    public ModuleInstaller getModuleInstaller();
+    public List<Activity> getRunningActivities();
+    public int getStateForActivity(Activity activity);
+}
\ No newline at end of file

components/module_installer/android/java/src-common/org/chromium/components/module_installer/observers/ObserverStrategyImpl.java

+// Copyright 2019 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.
+
+package org.chromium.components.module_installer.observers;
+
+import android.app.Activity;
+
+import org.chromium.base.ApplicationStatus;
+import org.chromium.components.module_installer.ModuleInstaller;
+
+import java.util.List;
+
+/** Strategy utilizing ModuleInstaller and ApplicationStatus. */
+class ObserverStrategyImpl implements ObserverStrategy {
+    @Override
+    public ModuleInstaller getModuleInstaller() {
+        return ModuleInstaller.getInstance();
+    }
+
+    @Override
+    public List<Activity> getRunningActivities() {
+        return ApplicationStatus.getRunningActivities();
+    }
+
+    @Override
+    public int getStateForActivity(Activity activity) {
+        return ApplicationStatus.getStateForActivity(activity);
+    }
+}
\ No newline at end of file

components/module_installer/android/java/src-impl/org/chromium/components/module_installer/ModuleInstallerImpl.java

@@ -21,6 +21,7 @@ import org.chromium.base.ThreadUtils;
 import org.chromium.base.annotations.MainDex;
 import org.chromium.components.crash.CrashKeyIndex;
 import org.chromium.components.crash.CrashKeys;
+import org.chromium.components.module_installer.observers.ModuleActivityObserver;
 
 import java.util.Arrays;
 import java.util.HashMap;

components/module_installer/android/junit/src/org/chromium/components/module_installer/ModuleActivityObserverTest.java → components/module_installer/android/junit/src/org/chromium/components/module_installer/observers/ModuleActivityObserverTest.java

@@ -2,13 +2,12 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-package org.chromium.components.module_installer;
+package org.chromium.components.module_installer.observers;
 
 import static org.mockito.Mockito.any;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
-import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 
@@ -22,6 +21,7 @@ import org.mockito.MockitoAnnotations;
 
 import org.chromium.base.ActivityState;
 import org.chromium.base.test.BaseRobolectricTestRunner;
+import org.chromium.components.module_installer.ModuleInstaller;
 
 import java.util.ArrayList;
 import java.util.List;
@@ -33,20 +33,24 @@ import java.util.List;
 public class ModuleActivityObserverTest {
     @Mock
     private ModuleInstaller mModuleInstallerMock;
+
     @Mock
     private Activity mActivityMock;
 
+    @Mock
+    private ObserverStrategy mStrategy;
+
     private ModuleActivityObserver mObserver;
 
     @Before
-    public void beforeTests() {
+    public void setUp() {
         MockitoAnnotations.initMocks(this);
 
-        mObserver = spy(new ModuleActivityObserver());
+        mObserver = new ModuleActivityObserver(mStrategy);
 
-        doReturn(mModuleInstallerMock).when(mObserver).getModuleInstaller();
-        doReturn(new ArrayList<>()).when(mObserver).getRunningActivities();
-        doReturn(ActivityState.CREATED).when(mObserver).getStateForActivity(any(Activity.class));
+        doReturn(mModuleInstallerMock).when(mStrategy).getModuleInstaller();
+        doReturn(new ArrayList<>()).when(mStrategy).getRunningActivities();
+        doReturn(ActivityState.CREATED).when(mStrategy).getStateForActivity(any(Activity.class));
     }
 
     @Test
@@ -121,10 +125,7 @@ public class ModuleActivityObserverTest {
         // Arrange.
         @ActivityState
         Integer newState = ActivityState.RESUMED;
-
-        ModuleInstaller newModuleInstallerMock = mock(ModuleInstaller.class);
-        ModuleActivityObserver newObserver = spy(new ModuleActivityObserver());
-        doReturn(newModuleInstallerMock).when(newObserver).getModuleInstaller();
+        ModuleActivityObserver newObserver = new ModuleActivityObserver(mStrategy);
 
         // Act.
         mObserver.onActivityStateChange(mActivityMock, newState);
@@ -132,7 +133,6 @@ public class ModuleActivityObserverTest {
 
         // Assert.
         verify(mModuleInstallerMock, times(1)).initActivity(mActivityMock);
-        verify(newModuleInstallerMock, never()).initActivity(mActivityMock);
     }
 
     @Test
@@ -147,11 +147,11 @@ public class ModuleActivityObserverTest {
         activitiesList.add(activityMock2);
         activitiesList.add(activityMock3);
 
-        doReturn(activitiesList).when(mObserver).getRunningActivities();
+        doReturn(activitiesList).when(mStrategy).getRunningActivities();
 
-        doReturn(ActivityState.RESUMED).when(mObserver).getStateForActivity(activityMock1);
-        doReturn(ActivityState.PAUSED).when(mObserver).getStateForActivity(activityMock2);
-        doReturn(ActivityState.DESTROYED).when(mObserver).getStateForActivity(activityMock3);
+        doReturn(ActivityState.RESUMED).when(mStrategy).getStateForActivity(activityMock1);
+        doReturn(ActivityState.PAUSED).when(mStrategy).getStateForActivity(activityMock2);
+        doReturn(ActivityState.DESTROYED).when(mStrategy).getStateForActivity(activityMock3);
 
         // Act.
         mObserver.onModuleInstalled();