[ChildAccount][Android] Refactor and test ChildAccountService#reauthenticateChildAccount

This CL refactors ChildAccountService#reauthenticateChildAccount by
removing the redundant boolean parameter in the method and adds JUnit
tests for the method.

Bug: 1161702
Change-Id: I83172f25bc55783b7736c4c3465a1415ea966e6a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2620261Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Alice Wang <aliceywang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842521}

chrome/android/java/src/org/chromium/chrome/browser/childaccounts/ChildAccountService.java

@@ -8,6 +8,7 @@ import android.accounts.Account;
 import android.app.Activity;
 
 import androidx.annotation.MainThread;
+import androidx.annotation.VisibleForTesting;
 
 import org.chromium.base.ThreadUtils;
 import org.chromium.base.annotations.CalledByNative;
@@ -68,28 +69,28 @@ public class ChildAccountService {
         });
     }
 
+    @VisibleForTesting
     @CalledByNative
-    private static void reauthenticateChildAccount(
-            WindowAndroid windowAndroid, String accountName, final long nativeCallback) {
+    static void reauthenticateChildAccount(
+            WindowAndroid windowAndroid, String accountName, final long nativeOnFailureCallback) {
         ThreadUtils.assertOnUiThread();
-
-        Activity activity = windowAndroid.getActivity().get();
+        final Activity activity = windowAndroid.getActivity().get();
         if (activity == null) {
-            PostTask.postTask(UiThreadTaskTraits.DEFAULT,
-                    ()
-                            -> ChildAccountServiceJni.get().onReauthenticationResult(
-                                    nativeCallback, false));
+            PostTask.postTask(UiThreadTaskTraits.DEFAULT, () -> {
+                ChildAccountServiceJni.get().onReauthenticationFailed(nativeOnFailureCallback);
+            });
             return;
         }
-
         Account account = AccountUtils.createAccountFromName(accountName);
-        AccountManagerFacadeProvider.getInstance().updateCredentials(account, activity,
-                result
-                -> ChildAccountServiceJni.get().onReauthenticationResult(nativeCallback, result));
+        AccountManagerFacadeProvider.getInstance().updateCredentials(account, activity, success -> {
+            if (!success) {
+                ChildAccountServiceJni.get().onReauthenticationFailed(nativeOnFailureCallback);
+            }
+        });
     }
 
     @NativeMethods
     interface Natives {
-        void onReauthenticationResult(long callbackPtr, boolean reauthSuccessful);
+        void onReauthenticationFailed(long onFailureCallbackPtr);
     }
 }

chrome/android/junit/src/org/chromium/chrome/browser/childaccounts/ChildAccountServiceTest.java

@@ -4,10 +4,21 @@
 
 package org.chromium.chrome.browser.childaccounts;
 
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
 
 import android.accounts.Account;
+import android.app.Activity;
 
+import org.junit.Assert;
+import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -15,12 +26,17 @@ import org.mockito.Mock;
 import org.mockito.junit.MockitoJUnit;
 import org.mockito.junit.MockitoRule;
 
+import org.chromium.base.Callback;
 import org.chromium.base.test.BaseRobolectricTestRunner;
+import org.chromium.base.test.util.JniMocker;
 import org.chromium.chrome.test.util.browser.signin.AccountManagerTestRule;
 import org.chromium.components.signin.AccountManagerFacade.ChildAccountStatusListener;
 import org.chromium.components.signin.AccountUtils;
 import org.chromium.components.signin.ChildAccountStatus;
 import org.chromium.components.signin.test.util.FakeAccountManagerFacade;
+import org.chromium.ui.base.WindowAndroid;
+
+import java.lang.ref.WeakReference;
 
 /**
  * Unit tests for {@link ChildAccountService}.
@@ -29,19 +45,23 @@ import org.chromium.components.signin.test.util.FakeAccountManagerFacade;
 public class ChildAccountServiceTest {
     private static final Account CHILD_ACCOUNT =
             AccountUtils.createAccountFromName("child.account@gmail.com");
+    private static final long FAKE_NATIVE_CALLBACK = 1000L;
 
-    private final FakeAccountManagerFacade mFakeFacade = new FakeAccountManagerFacade(null) {
+    private final FakeAccountManagerFacade mFakeFacade = spy(new FakeAccountManagerFacade(null) {
         @Override
         public void checkChildAccountStatus(Account account, ChildAccountStatusListener listener) {
             listener.onStatusReady(account.name.startsWith("child")
                             ? ChildAccountStatus.REGULAR_CHILD
                             : ChildAccountStatus.NOT_CHILD);
         }
-    };
+    });
 
     @Rule
     public final MockitoRule mMockitoRule = MockitoJUnit.rule();
 
+    @Rule
+    public final JniMocker mocker = new JniMocker();
+
     @Rule
     public final AccountManagerTestRule mAccountManagerTestRule =
             new AccountManagerTestRule(mFakeFacade);
@@ -49,6 +69,17 @@ public class ChildAccountServiceTest {
     @Mock
     private ChildAccountStatusListener mListenerMock;
 
+    @Mock
+    private ChildAccountService.Natives mNativeMock;
+
+    @Mock
+    private WindowAndroid mWindowAndroidMock;
+
+    @Before
+    public void setUp() {
+        mocker.mock(ChildAccountServiceJni.TEST_HOOKS, mNativeMock);
+    }
+
     @Test
     public void testChildAccountStatusWhenNoAccountsOnDevice() {
         ChildAccountService.checkChildAccountStatus(mListenerMock);
@@ -95,4 +126,50 @@ public class ChildAccountServiceTest {
         ChildAccountService.checkChildAccountStatus(mListenerMock);
         verify(mListenerMock).onStatusReady(ChildAccountStatus.REGULAR_CHILD);
     }
+
+    @Test
+    public void testReauthenticateChildAccountWhenActivityIsNull() {
+        when(mWindowAndroidMock.getActivity()).thenReturn(new WeakReference<>(null));
+        ChildAccountService.reauthenticateChildAccount(
+                mWindowAndroidMock, CHILD_ACCOUNT.name, FAKE_NATIVE_CALLBACK);
+        verify(mNativeMock).onReauthenticationFailed(FAKE_NATIVE_CALLBACK);
+    }
+
+    @Test
+    public void testReauthenticateChildAccountWhenReauthenticationSucceeded() {
+        final Activity activity = mock(Activity.class);
+        when(mWindowAndroidMock.getActivity()).thenReturn(new WeakReference<>(activity));
+        doAnswer(invocation -> {
+            Account account = invocation.getArgument(0);
+            Assert.assertEquals(CHILD_ACCOUNT.name, account.name);
+            Callback<Boolean> callback = invocation.getArgument(2);
+            callback.onResult(true);
+            return null;
+        })
+                .when(mFakeFacade)
+                .updateCredentials(any(Account.class), eq(activity), any());
+
+        ChildAccountService.reauthenticateChildAccount(
+                mWindowAndroidMock, CHILD_ACCOUNT.name, FAKE_NATIVE_CALLBACK);
+        verify(mNativeMock, never()).onReauthenticationFailed(anyLong());
+    }
+
+    @Test
+    public void testReauthenticateChildAccountWhenReauthenticationFailed() {
+        final Activity activity = mock(Activity.class);
+        when(mWindowAndroidMock.getActivity()).thenReturn(new WeakReference<>(activity));
+        doAnswer(invocation -> {
+            Account account = invocation.getArgument(0);
+            Assert.assertEquals(CHILD_ACCOUNT.name, account.name);
+            Callback<Boolean> callback = invocation.getArgument(2);
+            callback.onResult(false);
+            return null;
+        })
+                .when(mFakeFacade)
+                .updateCredentials(any(Account.class), eq(activity), any());
+
+        ChildAccountService.reauthenticateChildAccount(
+                mWindowAndroidMock, CHILD_ACCOUNT.name, FAKE_NATIVE_CALLBACK);
+        verify(mNativeMock).onReauthenticationFailed(FAKE_NATIVE_CALLBACK);
+    }
 }
\ No newline at end of file

chrome/browser/supervised_user/child_accounts/child_account_service_android.cc

@@ -25,14 +25,14 @@ using base::android::ScopedJavaGlobalRef;
 void ReauthenticateChildAccount(
     content::WebContents* web_contents,
     const std::string& email,
-    const base::RepeatingCallback<void(bool)>& callback) {
+    const base::RepeatingCallback<void()>& on_failure_callback) {
   ui::WindowAndroid* window_android =
       web_contents->GetNativeView()->GetWindowAndroid();
 
   // Make a copy of the callback which can be passed as a pointer through
   // to Java.
   auto callback_copy =
-      std::make_unique<base::RepeatingCallback<void(bool)>>(callback);
+      std::make_unique<base::RepeatingCallback<void()>>(on_failure_callback);
 
   JNIEnv* env = AttachCurrentThread();
   Java_ChildAccountService_reauthenticateChildAccount(
@@ -40,13 +40,11 @@ void ReauthenticateChildAccount(
       reinterpret_cast<jlong>(callback_copy.release()));
 }
 
-void JNI_ChildAccountService_OnReauthenticationResult(
-    JNIEnv* env,
-    jlong jcallbackPtr,
-    jboolean result) {
+void JNI_ChildAccountService_OnReauthenticationFailed(JNIEnv* env,
+                                                      jlong jcallbackPtr) {
   // Cast the pointer value back to a Callback and take ownership of it.
-  std::unique_ptr<base::RepeatingCallback<void(bool)>> callback(
-      reinterpret_cast<base::RepeatingCallback<void(bool)>*>(jcallbackPtr));
+  std::unique_ptr<base::RepeatingCallback<void()>> callback(
+      reinterpret_cast<base::RepeatingCallback<void()>*>(jcallbackPtr));
 
-  callback->Run(result);
+  callback->Run();
 }

chrome/browser/supervised_user/child_accounts/child_account_service_android.h

@@ -16,6 +16,6 @@ class WebContents;
 void ReauthenticateChildAccount(
     content::WebContents* web_contents,
     const std::string& email,
-    const base::RepeatingCallback<void(bool)>& callback);
+    const base::RepeatingCallback<void()>& on_failure_callback);
 
 #endif  // CHROME_BROWSER_SUPERVISED_USER_CHILD_ACCOUNTS_CHILD_ACCOUNT_SERVICE_ANDROID_H_

chrome/browser/supervised_user/supervised_user_google_auth_navigation_throttle.cc

@@ -148,7 +148,7 @@ SupervisedUserGoogleAuthNavigationThrottle::ShouldProceed() {
     ReauthenticateChildAccount(
         web_contents, account_info.email,
         base::BindRepeating(&SupervisedUserGoogleAuthNavigationThrottle::
-                                OnReauthenticationResult,
+                                OnReauthenticationFailed,
                             weak_ptr_factory_.GetWeakPtr()));
   }
   return content::NavigationThrottle::DEFER;
@@ -161,14 +161,7 @@ SupervisedUserGoogleAuthNavigationThrottle::ShouldProceed() {
 #endif
 }
 
-void SupervisedUserGoogleAuthNavigationThrottle::OnReauthenticationResult(
-    bool reauth_successful) {
-  if (reauth_successful) {
-    // If reauthentication was not successful, wait until the cookies are
-    // refreshed, which will call us back separately.
-    return;
-  }
-
-  // Otherwise cancel immediately.
+void SupervisedUserGoogleAuthNavigationThrottle::OnReauthenticationFailed() {
+  // Cancel the navifation if reauthentication failed.
   CancelDeferredNavigation(content::NavigationThrottle::CANCEL_AND_IGNORE);
 }

chrome/browser/supervised_user/supervised_user_google_auth_navigation_throttle.h

@@ -42,7 +42,7 @@ class SupervisedUserGoogleAuthNavigationThrottle
 
   ThrottleCheckResult ShouldProceed();
 
-  void OnReauthenticationResult(bool reauth_successful);
+  void OnReauthenticationFailed();
 
   ChildAccountService* child_account_service_;
   base::CallbackListSubscription google_auth_state_subscription_;