GAIA ID migration for Android

chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java

Index: chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java b/chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java
new file mode 100644
index 0000000000000000000000000000000000000000..dabf97140213c84e5ec010334567ded8a81fcfee
--- /dev/null
+++ b/chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java
@@ -0,0 +1,148 @@
+// Copyright 2015 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.chrome.browser.signin;
+
+import android.content.Context;
+import android.os.AsyncTask;
+import android.util.Log;

[nyquist, 2015/08/28 19:18:26]

Use org.chromium.base.Log

[gogerald1, 2015/08/28 21:38:46]

Done.

+
+import org.chromium.base.ObserverList;
+import org.chromium.base.ThreadUtils;
+import org.chromium.sync.signin.AccountManagerHelper;
+
+/**
+* Android wrapper of AccountTrackerService which provides access from the java layer.
+* It offers the capability of fetching and seeding system accounts into AccountTrackerService in C++
+* layer, and notifies observers when it is complete.
+*/
+public class AccountTrackerService {
+    private static final String TAG = "AccountTrackerService";

[nyquist, 2015/08/28 19:18:26]

cr.AccountTrackerService or just cr.signin or something along those lines?

[gogerald1, 2015/08/28 21:38:46]

Done.

+    private static AccountTrackerService sAccountTrackerService;
+    private static SystemAccountsSeedingStatus sSystemAccountsSeedingStatus =
+            SystemAccountsSeedingStatus.SEEDING_NOT_STARTED;
+    private static boolean sForceRefresh = false;
+
+    private final Context mContext;
+    private final long mNativeAccountTrackerService;
+
+    private enum SystemAccountsSeedingStatus {
+        SEEDING_NOT_STARTED,
+        SEEDING_IN_PROGRESS,
+        SEEDING_DONE
+    }
+
+    /**
+    * Classes that want to listen for system accounts fetching and seeding should implement
+    * this interface and register with {@link #addSystemAccountsSeededListener}.
+    */
+    public interface OnSystemAccountsSeededListener {
+        void onSystemAccountsSeedingComplete();
+        void onSystemAccountsForceRefreshed();

[nyquist, 2015/08/28 19:18:26]

It's a little bit unclear from this whether the accounts have already been force
refreshed or the force refresh is just now starting from this. Could you add an
explanatory comment?

[gogerald1, 2015/08/28 21:38:46]

Done.

+    }
+
+    private static final ObserverList<OnSystemAccountsSeededListener>

[nyquist, 2015/08/28 19:18:26]

Could this be non-static?

[gogerald1, 2015/08/28 21:38:46]

Done. Yes, we could, the original thinking is to make observer could be added
without instance and we only have one instance.

+            sSystemAccountsSeedingObservers = new ObserverList<OnSystemAccountsSeededListener>();
+
+    public static AccountTrackerService get(Context context) {
+        ThreadUtils.assertOnUiThread();
+        if (sAccountTrackerService == null) {
+            sAccountTrackerService = new AccountTrackerService(context);
+        }
+        return sAccountTrackerService;
+    }
+
+    private AccountTrackerService(Context context) {
+        mContext = context;
+        mNativeAccountTrackerService = nativeInit();
+    }
+
+    /**
+    * Check whether systems accounts have been seeded into AccountTrackerService in C++ layer.
+    */
+    public boolean isSystemAccountsSeeded() {
+        ThreadUtils.assertOnUiThread();
+        if (sSystemAccountsSeedingStatus == SystemAccountsSeedingStatus.SEEDING_DONE) {
+            return true;
+        }
+
+        if (sSystemAccountsSeedingStatus == SystemAccountsSeedingStatus.SEEDING_NOT_STARTED) {
+            seedSystemAccounts();
+        }
+
+        return false;
+    }
+
+    /**
+    * Register an |observer| to observe system accounts seeding status.
+    */
+    public static void addSystemAccountsSeededListener(OnSystemAccountsSeededListener observer) {

[nyquist, 2015/08/28 19:18:26]

Why is this method static? Could this not be an instance method?

[gogerald1, 2015/08/28 21:38:46]

Done.

+        ThreadUtils.assertOnUiThread();
+        sSystemAccountsSeedingObservers.addObserver(observer);
+        if (sSystemAccountsSeedingStatus == SystemAccountsSeedingStatus.SEEDING_DONE) {
+            observer.onSystemAccountsSeedingComplete();
+        }
+    }
+
+    private void seedSystemAccounts() {
+        ThreadUtils.assertOnUiThread();
+        sSystemAccountsSeedingStatus = SystemAccountsSeedingStatus.SEEDING_IN_PROGRESS;
+        sForceRefresh = false;
+        AccountManagerHelper accountManagerHelper = AccountManagerHelper.get(mContext);
+        java.util.List<String> accountNamesList = accountManagerHelper.getGoogleAccountNames();

[nyquist, 2015/08/28 19:18:26]

Just import this class. We don't usually fully specify package name unless it is
strictly necessary (for example if class names overlap)

[gogerald1, 2015/08/28 21:38:46]

Done.

+        final String[] accountNames = accountNamesList.toArray(new String[accountNamesList.size()]);
+        new AsyncTask<Void, Void, String[]>() {
+            @Override
+            public String[] doInBackground(Void... params) {
+                Log.d(TAG, "Getting id/email mapping");
+                String[] accountIds = new String[accountNames.length];
+                AccountIdProvider provider = AccountIdProvider.getInstance();
+                for (int i = 0; i < accountIds.length; ++i) {
+                    accountIds[i] = provider.getAccountId(mContext, accountNames[i]);
+                }
+                return accountIds;
+            }
+            @Override
+            public void onPostExecute(String[] accountIds) {
+                if (sForceRefresh) {
+                    seedSystemAccounts();
+                } else {
+                    nativeSeedAccountsInfo(mNativeAccountTrackerService, accountIds, accountNames);
+                    sSystemAccountsSeedingStatus = SystemAccountsSeedingStatus.SEEDING_DONE;

[nyquist, 2015/08/28 19:18:26]

This is assigning to a static from an instance of this class. Does the status
field need to be static?

[gogerald1, 2015/08/28 21:38:46]

Acknowledged.

+                    notifyObserversOnSeedingComplete();
+                }
+            }
+
+        }.execute();

[nyquist, 2015/08/28 19:18:26]

Could you specify which executor you want? AsyncTask.THREAD_POOL_EXECUTOR or
something along those lines?

[gogerald1, 2015/08/28 21:38:46]

Done.

+    }
+
+    private void notifyObserversOnSeedingComplete() {

[nyquist, 2015/08/28 19:18:26]

Either make static, or (preferrably) make the observer non-static. Same below
for the other publish-method.

[gogerald1, 2015/08/28 21:38:46]

Acknowledged.

+        for (OnSystemAccountsSeededListener observer : sSystemAccountsSeedingObservers) {
+            observer.onSystemAccountsSeedingComplete();
+        }
+    }
+
+    /**
+    * Force AccountTrackerService refresh/update systems accounts.
+    */
+    public void forceRefresh() {
+        ThreadUtils.assertOnUiThread();
+        sForceRefresh = true;
+        notifyObserversOnForceRefreshed();
+        if (sSystemAccountsSeedingStatus != SystemAccountsSeedingStatus.SEEDING_IN_PROGRESS) {
+            seedSystemAccounts();

[nyquist, 2015/08/28 19:18:26]

Are we guaranteed that this call will not lead to notifications to the observer
that will again lead to those classes calling forceRefresh() as a result of the
notification, triggering an endless loop? I was thinking about both the
onSystemAccountsForceRefreshed and onSystemAccountsSeedingComplete.

[gogerald1, 2015/08/28 21:38:46]

Unlikely. First, force refresh is only happened when system accounts changed or
user manually signed out. Both of them are not triggered by observer. Secondly,
all these observers are called on UI thread and should clean their local status
after processing OnSystemAccountsForceRefreshed/notifyObserversOnForceRefreshed
notification. If the endless loop happened, then there is definitely a usage
error.

+        }
+    }
+
+    private void notifyObserversOnForceRefreshed() {
+        for (OnSystemAccountsSeededListener observer : sSystemAccountsSeedingObservers) {
+            observer.onSystemAccountsForceRefreshed();
+        }
+    }
+
+    // Native methods.
+    private native long nativeInit();
+    private native void nativeSeedAccountsInfo(
+            long nativeAccountTrackerServiceAndroid, String[] gaiaIds, String[] accountNames);
+}