Update account and sync management UX

chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncAccountListPreference.java

Index: chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncAccountListPreference.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncAccountListPreference.java b/chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncAccountListPreference.java
new file mode 100644
index 0000000000000000000000000000000000000000..4e9d8af642c5e35be13106c68d767781cafcdc00
--- /dev/null
+++ b/chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncAccountListPreference.java
@@ -0,0 +1,65 @@
+// Copyright 2016 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.preferences;

[newt (away), 2016/02/12 22:03:50]

newline before package

+
+import android.accounts.Account;
+import android.content.Context;
+import android.preference.ListPreference;
+import android.text.TextUtils;
+import android.util.AttributeSet;
+
+import org.chromium.sync.AndroidSyncSettings;
+import org.chromium.sync.signin.AccountManagerHelper;
+import org.chromium.sync.signin.ChromeSigninController;
+
+/**
+ * A custom preference that opens a dialog picker to choose an account to use for syncing.

[newt (away), 2016/02/12 22:03:50]

maybe: "A preference that displays the account that's currently being synced and
allows the user to choose a new account for syncing"

[May, 2016/02/16 17:32:07]

Done.

+ */
+public class SyncAccountListPreference extends ListPreference {

[newt (away), 2016/02/12 22:03:50]

How about calling this "SyncedAccountPreference"?

[May, 2016/02/16 17:32:07]

Done.

+    public SyncAccountListPreference(Context context, AttributeSet attrs) {

[newt (away), 2016/02/12 22:03:50]

javadoc for all public methods

[May, 2016/02/16 17:32:07]

Done.

+        super(context, attrs);
+        updateAccountsList();
+    }
+
+    private void updateAccountsList() {
+        boolean syncEnabled = AndroidSyncSettings.isSyncEnabled(getContext());
+        if (!syncEnabled) {
+            setEnabled(false);
+            return;
+        }
+
+        Account[] accounts = AccountManagerHelper.get(getContext()).getGoogleAccounts();
+        String[] accountDisplayEntries = new String[accounts.length];

[newt (away), 2016/02/12 22:03:50]

How about calling this "accountNames", and the variable below "accountValues"
(to be consistent with the ListPreference terminology)?

[May, 2016/02/16 17:32:07]

Done.

+        String[] accountSyncSettingsKeys = new String[accounts.length];
+
+        String signedInAccountName =
+                ChromeSigninController.get(getContext()).getSignedInAccountName();
+        String signedInSettingsKey = "";
+
+        for (int i = 0; i < accounts.length; ++i) {
+            Account account = accounts[i];
+            accountDisplayEntries[i] = account.name;
+            accountSyncSettingsKeys[i] = "sync_setting_" + account.name;

[newt (away), 2016/02/12 22:03:50]

Do you really need to prefix with "sync_setting"? or could you just use
account.name directly?

[May, 2016/02/16 17:32:07]

No, I suppose not at the moment.

+            boolean isPrimaryAccount = TextUtils.equals(account.name, signedInAccountName);
+            if (isPrimaryAccount) {
+                signedInSettingsKey = accountSyncSettingsKeys[i];
+            }
+        }
+
+        setEntries(accountDisplayEntries);
+        setEntryValues(accountSyncSettingsKeys);
+        setDefaultValue(signedInSettingsKey);
+        setSummary(signedInAccountName);

[newt (away), 2016/02/12 22:03:50]

Do you need this? I thought the summary was already set to "%s" in the xml file.
If you do need this, then remove android:summary="%s" from the xml file.

[May, 2016/02/16 17:32:07]

Removed the android:summary entry in the xml

+    }
+
+    public void updateState(boolean enabled) {

[newt (away), 2016/02/12 22:03:50]

Since this doesn't actually update which account is being shown, but rather just
enables or disables the preference, I'd probably remove this method and just
call setEnabled() directly when needed. I think that'll make for clearer code.

OTOH, when sync is disabled, shouldn't the title and summary be updated to
reflect that? I couldn't find any mocks showing what this preference should look
like when sync is disabled...

[May, 2016/02/16 17:32:07]

When sync is disabled, this becomes disabled as well. Which come to think of it,
is not ideal, as that means I'd have to turn on sync for the account that I
explicitly didn't want synced, in order to then switch to a different sync
account. Which... defeats the purpose. Let me check first with UX.

+        setEnabled(enabled);
+    }
+
+    @Override
+    protected void onDialogClosed(boolean positiveResult) {
+        super.onDialogClosed(positiveResult);
+        // TODO(crbug/557786): Add switching sync accounts
+    }
+}