WebView: Refactor PlatformServiceBridge.getInstance

android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java

Index: android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java
diff --git a/android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java b/android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java
index 59a1a2dce46ee9a04ce3c9d62bd0b598b5c780c0..fcc82015030061df44f22e2091680fa33a83d00a 100644
--- a/android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java
+++ b/android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java
@@ -55,45 +55,49 @@ public class MinidumpUploaderTest extends CrashTestCase {
     }
 
     private static class TestPlatformServiceBridge extends PlatformServiceBridge {
-        boolean mCanUseGms;
-        boolean mUserPermitted;
-
-        public TestPlatformServiceBridge(boolean canUseGms, boolean userPermitted) {
-            mCanUseGms = canUseGms;
-            mUserPermitted = userPermitted;
-        }
-
         @Override
         public boolean canUseGms() {
-            return mCanUseGms;
+            return true;
+        }
+    }
+
+    private static class BooleanTestPlatformServiceBridge extends TestPlatformServiceBridge {

[Tobias Sargeant, 2017/04/03 10:40:06]

Can't this just be replaced with a PermissionTestPlatformServiceBridge created
with

MockCrashReportingPermissionManager() {{ mIsUserPermitted = enabled; }};?

And then you could fold it all back into TestPlatformServiceBridge.

Although, to be honest, this seems backwards. The PSB should be the source of
truth regarding whether usage and crash reporting is permitted by the user.

Otherwise: the name should mention what boolean this is controlling.

[paulmiller, 2017/04/03 23:34:16]

Done.
I don't follow. Do you mean, injecting a mock PlatformServiceBridge is
backwards?

[Tobias Sargeant, 2017/04/04 12:30:47]

I mean the fact that the PermissionTestPlatformServiceBridge is asking the
(mocked) permission manager for the value of
isUsageAndCrashReportingPermittedByUser(). It's the other way around in non-test
code.

+        private final boolean mEnabled;
+
+        public BooleanTestPlatformServiceBridge(boolean enabled) {
+            mEnabled = enabled;
         }
 
         @Override
         public void queryMetricsSetting(ValueCallback<Boolean> callback) {
             ThreadUtils.assertOnUiThread();
-            callback.onReceiveValue(mUserPermitted);
+            callback.onReceiveValue(mEnabled);
         }
     }
 
-    private static class TestMinidumpUploaderDelegate extends AwMinidumpUploaderDelegate {
+    private static class PermissionTestPlatformServiceBridge extends TestPlatformServiceBridge {
         private final CrashReportingPermissionManager mPermissionManager;
 
-        TestMinidumpUploaderDelegate(
-                Context context, CrashReportingPermissionManager permissionManager) {
-            super(context);
+        public PermissionTestPlatformServiceBridge(
+                CrashReportingPermissionManager permissionManager) {
             mPermissionManager = permissionManager;
         }
 
         @Override
-        public PlatformServiceBridge createPlatformServiceBridge() {
-            return new TestPlatformServiceBridge(true /* canUseGms */,
-                    mPermissionManager.isUsageAndCrashReportingPermittedByUser());
+        public void queryMetricsSetting(ValueCallback<Boolean> callback) {
+            ThreadUtils.assertOnUiThread();
+            callback.onReceiveValue(mPermissionManager.isUsageAndCrashReportingPermittedByUser());
         }
     }
 
-    private static class TestMinidumpUploaderImpl extends MinidumpUploaderImpl {
+    private class TestMinidumpUploaderImpl extends MinidumpUploaderImpl {
         private final CrashReportingPermissionManager mPermissionManager;
 
+        TestMinidumpUploaderImpl(CrashReportingPermissionManager permissionManager) {
+            super(new AwMinidumpUploaderDelegate(getInstrumentation().getTargetContext()));
+            mPermissionManager = permissionManager;
+        }
+
         TestMinidumpUploaderImpl(MinidumpUploaderDelegate delegate,
                 CrashReportingPermissionManager permissionManager) {
             super(delegate);
@@ -132,9 +136,8 @@ public class MinidumpUploaderTest extends CrashTestCase {
                         mIsEnabledForTests = false;
                     }
                 };
-        MinidumpUploaderDelegate delegate = new TestMinidumpUploaderDelegate(
-                getInstrumentation().getTargetContext(), permManager);
-        MinidumpUploader minidumpUploader = new TestMinidumpUploaderImpl(delegate, permManager);
+        PlatformServiceBridge.injectInstance(new PermissionTestPlatformServiceBridge(permManager));
+        MinidumpUploader minidumpUploader = new TestMinidumpUploaderImpl(permManager);
 
         File firstFile = createMinidumpFileInCrashDir("1_abc.dmp0");
         File secondFile = createMinidumpFileInCrashDir("12_abc.dmp0");
@@ -224,9 +227,8 @@ public class MinidumpUploaderTest extends CrashTestCase {
                 new MockCrashReportingPermissionManager() {
                     { mIsEnabledForTests = true; }
                 };
-        MinidumpUploaderDelegate delegate = new TestMinidumpUploaderDelegate(
-                getInstrumentation().getTargetContext(), permManager);
-        MinidumpUploader minidumpUploader = new TestMinidumpUploaderImpl(delegate, permManager);
+        PlatformServiceBridge.injectInstance(new PermissionTestPlatformServiceBridge(permManager));
+        MinidumpUploader minidumpUploader = new TestMinidumpUploaderImpl(permManager);
 
         File firstFile = createMinidumpFileInCrashDir("1_abc.dmp0");
         File secondFile = createMinidumpFileInCrashDir("12_abcd.dmp0");
@@ -247,15 +249,10 @@ public class MinidumpUploaderTest extends CrashTestCase {
         MinidumpUploadCallable createCallable(File minidumpFile, File logfile);
     }
 
-    private static MinidumpUploaderImpl createCallableListMinidumpUploader(Context context,
+    private MinidumpUploaderImpl createCallableListMinidumpUploader(Context context,
             final List<MinidumpUploadCallableCreator> callables, final boolean userPermitted) {
-        MinidumpUploaderDelegate delegate = new AwMinidumpUploaderDelegate(context) {
-            @Override
-            public PlatformServiceBridge createPlatformServiceBridge() {
-                return new TestPlatformServiceBridge(true /* canUseGms*/, userPermitted);
-            }
-        };
-        return new TestMinidumpUploaderImpl(delegate, null) {
+        PlatformServiceBridge.injectInstance(new BooleanTestPlatformServiceBridge(userPermitted));
+        return new TestMinidumpUploaderImpl(null) {
             private int mIndex = 0;
 
             @Override
@@ -379,16 +376,8 @@ public class MinidumpUploaderTest extends CrashTestCase {
                     { mIsEnabledForTests = true; }
                 };
         final CountDownLatch stopStallingLatch = new CountDownLatch(1);
-        MinidumpUploaderDelegate delegate = new TestMinidumpUploaderDelegate(
-                getInstrumentation().getTargetContext(), permManager) {
-            @Override
-            public PlatformServiceBridge createPlatformServiceBridge() {
-                return new TestPlatformServiceBridge(
-                        true /* canUseGms*/, permManager.isUsageAndCrashReportingPermittedByUser());
-            }
-        };
-        MinidumpUploaderImpl minidumpUploader = new TestMinidumpUploaderImpl(
-                delegate, permManager) {
+        PlatformServiceBridge.injectInstance(new PermissionTestPlatformServiceBridge(permManager));
+        MinidumpUploaderImpl minidumpUploader = new TestMinidumpUploaderImpl(permManager) {
             @Override
             public MinidumpUploadCallable createMinidumpUploadCallable(
                     File minidumpFile, File logfile) {
@@ -499,10 +488,6 @@ public class MinidumpUploaderTest extends CrashTestCase {
             mUserConsent = userConsent;
         }
         @Override
-        public PlatformServiceBridge createPlatformServiceBridge() {
-            return new TestPlatformServiceBridge(true /* canUseGms */, mUserConsent);
-        }
-        @Override
         public CrashReportingPermissionManager createCrashReportingPermissionManager() {
             final CrashReportingPermissionManager realPermissionManager =
                     super.createCrashReportingPermissionManager();
@@ -528,6 +513,7 @@ public class MinidumpUploaderTest extends CrashTestCase {
     }
 
     private void testPlatformServicesBridgeIsUsed(final boolean userConsent) throws IOException {
+        PlatformServiceBridge.injectInstance(new BooleanTestPlatformServiceBridge(userConsent));
         MinidumpUploaderDelegate delegate = new WebViewUserConsentMinidumpUploaderDelegate(
                 getInstrumentation().getTargetContext(), userConsent);
         MinidumpUploader minidumpUploader = new TestMinidumpUploaderImpl(
@@ -672,9 +658,8 @@ public class MinidumpUploaderTest extends CrashTestCase {
                 new MockCrashReportingPermissionManager() {
                     { mIsEnabledForTests = true; }
                 };
-        MinidumpUploaderDelegate delegate = new TestMinidumpUploaderDelegate(
-                getInstrumentation().getTargetContext(), permManager);
-        MinidumpUploader minidumpUploader = new TestMinidumpUploaderImpl(delegate, permManager);
+        PlatformServiceBridge.injectInstance(new PermissionTestPlatformServiceBridge(permManager));
+        MinidumpUploader minidumpUploader = new TestMinidumpUploaderImpl(permManager);
 
         uploadMinidumpsSync(minidumpUploader, false /* expectReschedule */);
         // Ensure there are no minidumps left to upload.