Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1660353002:
Update account and sync management UX (Closed)
|
Created:
4 years, 10 months ago by May Modified:
4 years, 10 months ago CC:
chromium-reviews, tim+watch_chromium.org, maxbogue+watch_chromium.org, plaree+watch_chromium.org, zea+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate account and sync management UX
These changes encompass all the UX changes to the accounts list screen, and the sync settings screen. This does not include signing in from the settings screen or switching sync accounts.
BUG=557784
Committed: https://crrev.com/f3ff6dac2fba2a55ed76e3c0ae958d5350140f97
Cr-Commit-Position: refs/heads/master@{#376729}
Patch Set 1 #Patch Set 2 : Minor formatting changes #
Total comments: 98
Patch Set 3 : #
Total comments: 33
Patch Set 4 : #Patch Set 5 : #
Total comments: 9
Patch Set 6 : CR comments, changed opening links to use Custom Tabs #
Total comments: 4
Patch Set 7 : CR comments #Patch Set 8 : Fixing lint errors #Patch Set 9 : Add TODO in comments for an additional task for the bug #Messages
Total messages: 33 (10 generated)
maybelle@chromium.org changed reviewers: + bauerb@chromium.org, newt@chromium.org
Part 1 of updates to account settings UX. Some TODOs still left, as marked in the code.
Is it just me, or does the hdpi image look a bit blurry? Where did you get the images from? https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:389: Intent intent = new Intent(Settings.ACTION_SYNC_SETTINGS); Extract this into a shared method?
On 2016/02/03 14:37:39, Bernhard Bauer wrote: > Is it just me, or does the hdpi image look a bit blurry? Where did you get the > images from? > > https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java > (right): > > https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:389: > Intent intent = new Intent(Settings.ACTION_SYNC_SETTINGS); > Extract this into a shared method? Hmm, it does look a little blurry. I got the images from cleer@. Let me double check that I have the correct ones.
@newt - friendly ping, now that you're back. I wanted to make sure this doesn't get lost in your mountain of todos.
I'm taking a look now :) Could you enumerate which changes are this CL in the CL description? The mocks and spec encompass a huge number of changes, and I'm not sure which one are included in this CL. Also, could you attach screenshots to the bug showing the changes due to this CL? Thanks!
*Could you enumerate which changes are included in this CL in the CL description?
Description was changed from ========== Update account and sync management UX BUG=557784 ========== to ========== Update account and sync management UX These changes encompass all the UX changes to the accounts list screen, and the sync settings screen. This does not include signing in from the settings screen or switching sync accounts. BUG=557784 ==========
https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/sync_custom_passphrase.xml (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/sync_custom_passphrase.xml:34: android:layout_marginBottom="25dp" Is this still the right margin size? https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/res... File chrome/android/java/res/values-v17/styles.xml (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/res... chrome/android/java/res/values-v17/styles.xml:76: <item name="android:divider">@null</item> Does this remove the dividers all throughout preferences? If so, we should do this as a separate change since other preference screens will need updating. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/res... File chrome/android/java/res/values/arrays.xml (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/res... chrome/android/java/res/values/arrays.xml:36: <!-- Sync preferences--> Do you need to use an array resource? I'd avoid it unless it's necessary, i.e. you're referencing this from XML. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/res... File chrome/android/java/res/xml/account_management_preferences.xml (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/res... chrome/android/java/res/xml/account_management_preferences.xml:19: android:icon="@drawable/add_circle_blue" Can you delete the plus.png assets, or are they used elsewhere? Also, be sure to fix the issue with the blurry hdpi add_circle_blue.png https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/res... File chrome/android/java/res/xml/sync_customization_preferences.xml (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/res... chrome/android/java/res/xml/sync_customization_preferences.xml:15: android:title="@string/sync_to_account_header" I'd suggest setting the title and summary in Java, so that all the logic related to this preference lives in SyncAccountListPreference.java, rather than having a small bit of the logic live in the xml file. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncAccountListPreference.java (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncAccountListPreference.java:4: package org.chromium.chrome.browser.preferences; newline before package https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncAccountListPreference.java:17: * A custom preference that opens a dialog picker to choose an account to use for syncing. maybe: "A preference that displays the account that's currently being synced and allows the user to choose a new account for syncing" https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncAccountListPreference.java:19: public class SyncAccountListPreference extends ListPreference { How about calling this "SyncedAccountPreference"? https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncAccountListPreference.java:20: public SyncAccountListPreference(Context context, AttributeSet attrs) { javadoc for all public methods https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncAccountListPreference.java:33: String[] accountDisplayEntries = new String[accounts.length]; How about calling this "accountNames", and the variable below "accountValues" (to be consistent with the ListPreference terminology)? https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncAccountListPreference.java:43: accountSyncSettingsKeys[i] = "sync_setting_" + account.name; Do you really need to prefix with "sync_setting"? or could you just use account.name directly? https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncAccountListPreference.java:53: setSummary(signedInAccountName); 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. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncAccountListPreference.java:56: public void updateState(boolean enabled) { 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... https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncPreference.java (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncPreference.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. where is this class used? https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncPreference.java:20: * A custom preference that displays information about the currently synced account. "displays the current sync account and status (enabled, error, needs passphrase, etc)" https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncPreference.java:25: setShouldDisableView(false); why? https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncPreference.java:29: public void updateSyncSummary() { javadoc for all public methods https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncPreference.java:68: Account account = ChromeSigninController.get(context).getSignedInUser(); Could this ever be null? It shouldn't be (since ChromeSigninController.isSignedIn() is true above), but it's worth checking the contract of ChromeSigninController to determine whether we should be defensive here with a null check. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:61: * The settings screen with information and settings related to the user's accounts. This shows Revert rewrapping of this comment. It's less readable now. (Did you run an automatic formatter on this file? There seem to be lots of minor formatting changes unrelated to this CL, which make it harder to focus on the real changes. If you want to fix a bunch of minor formatting issues in a file, it's best to do it as a separate trivial CL.) https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:66: public class AccountManagementFragment undo indentation changes https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:252: final Preferences activity = (Preferences) getActivity(); It doesn't look like you need this cast. It's much better to avoid casting if possible. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:268: intent.putExtra("account_types", new String[] {"com.google"}); is there not a constant you can use instead of hard-coding "account_types"? https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:384: pref.setOnPreferenceClickListener(new OnPreferenceClickListener() { Why do we launch Android sync settings when the user taps on an account? Is that in the spec? https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:616: * Gets the user picture for the account from the cache, or returns the default picture if I'd put this longer more-helpful JavaDoc on the public method above, and change this JavaDoc to simply @see getBadgedUserPicture(String, Resources) https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/SignOutDialogFragment.java (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/SignOutDialogFragment.java:41: private int mGaiaServiceType; This needs a comment explaining what the valid values are. Pointing the reader to AccountManagementFragment.GAIA_SERVICE_TYPE_NONE would be sufficient. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/SignOutDialogFragment.java:48: AccountManagementFragment.SHOW_GAIA_SERVICE_TYPE_EXTRA, mGaiaServiceType); Rather than reusing the value of AccountManagementFragment.SHOW_GAIA_SERVICE_TYPE_EXTRA, I'd define a constant in this file similar to SHOW_GAIA_SERVICE_TYPE_EXTRA. That'll make it clearer to anyone who reads this file than SHOW_GAIA_SERVICE_TYPE_EXTRA is a valid extra that can be passed to this fragment. In general, I'm a bit wary of adding dependencies on AccountManagementFragment, which is really supposed to be a UI class that consumes model data, but isn't depended on by other UI classes. If you could move GAIA_SERVICE_TYPE_NONE to a non-UI class, say, AccountManagementScreenHelper, that would make more sense. In fact, AccountManagementScreenHelper already contains GAIA_SERVICE_TYPE_SIGNUP. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseCreationDialogFragment.java (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseCreationDialogFragment.java:67: .setView(view) the indentation was correct before. Indent by 8 not 28. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseCreationDialogFragment.java:83: Uri syncPassphraseHC = Uri.parse(SYNC_PASSPHRASE_SUPPORT_URL); s/HC/Url/ https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseCreationDialogFragment.java:84: Intent intent = new Intent(Intent.ACTION_VIEW, syncPassphraseHC); We should be using either: - custom tabs, or - HelpAndFeedback.show() (see LearnMorePreference.java for an example) Ask yusufo@ about custom tabs. Or see the list of help and feedback topics in values.xml (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...) and add this new topic at go/mobilehelprecs Both of these solutions will show the help article *on top of* the current activity, so there's no need to call activity().finish(), and when the user presses back from the custom tabs or help and feedback activity, they'll be able to continue the passphrase creation flow. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java:109: final AlertDialog d = Revert the wrapping to how it was. In general, try to avoid excessive indentation. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java:213: verifying.setTextColor(Color.LTGRAY); Use color resources from colors.xml instead of constants from Colors.java. The colors in Colors.java don't necessarily match the Chrome color palette. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java:238: TextView verifying = (TextView) getDialog().findViewById(R.id.verifying); Do you need to set the error state on the text view so the underline becomes red? https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java:240: verifying.setTextColor(Color.RED); likewise https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java:82: public static final String PREFERENCE_SYNC_IMPORT_DATA = "sync_import_data"; unused? https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java:151: mAllTypes = new CheckBoxPreference[] {mSyncAutofill, mSyncBookmarks, mSyncOmnibox, revert rewrapping https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java:277: * Update the state of all settings from sync. This sets the state of the sync switch from revert rewrapping https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java:305: * Update the encryption state. If sync's backend is initialized, the button is enabled and the revert rewrapping https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java:510: * Update the state of the sync everything switch. If sync is on, load the pref from native. revert rewrapping, here and below https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:173: <message name="IDS_SUBMIT" desc="Generic label for a submit button. Used in multiple contexts. [CHAR-LIMIT=20]"> Maybe "Generic label for a button to submit data." https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:954: <message name="IDS_SIGNOUT_DIALOG_POSITIVE_BUTTON" desc="Positive button for signing out of Chrome dialog"> or just "Button to sign out of Chrome" https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:962: <message name="IDS_SYNC_DESCRIPTION" desc="Describes the purpose of syncing. This will be displayed on the sync on/off toggle [CHAR-LIMIT=64]"> Don't use CHAR-LIMITS unless longer text would break the UI or get cut off. Rule of thumb: if a CHAR-LIMIT is longer than 32 characters, it's probably not needed. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1020: Encrypting your data with a passphrase lets you sync your data without letting Google read it. To change this setting, you will need to reset sync. <ph name="BEGIN_LINK"><link></ph>Learn more<ph name="END_LINK"></link></ph> Looks like this string needs updating to match the new mocks. Also, for readability where this string is used, I'd use <learnmore> instead of <link> https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1053: You're switching sync accounts from <ph name="FROM_ACCOUNT">%1$s<ex>johndoe@gmail.com</ex></ph> to <ph name="TO_ACCOUNT">%2$s<ex>john@gmail.com</ex></ph>. What do you want to do with your existing bookmarks, history, passwords, and other settings? use curly quotes (you can copy paste from elsewhere in this file) https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1061: <message name="IDS_SYNC_IMPORT_EXISTING_DATA_VALUE" desc="Internal value for import data preference" translateable="false"> Unless you need to reference this string from both xml resources and from Java code, I'd move this from the grd file into a Java file. There's much less overhead to read a string constant in Java and it's simpler that way. (Likewise for keep_existing_data below). https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1068: Replace this device's data with <ph name="ACCOUNT">%1$s<ex>john@doe.com</ex></ph>'s data. You can retrieve your existing data by switching back to <ph name="ACCOUNT">%1$s<ex>johndoe@gmail.com</ex></ph> curly quotes https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1615: Select an account to get your bookmarks, history, passwords, and other settings on all your devices add back the period? It's in the mocks. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1636: Add an account to get your bookmarks, history, passwords, and other settings on all your devices add back the period? https://codereview.chromium.org/1660353002/diff/20001/components/sync_ui_stri... File components/sync_ui_strings.grdp (right): https://codereview.chromium.org/1660353002/diff/20001/components/sync_ui_stri... components/sync_ui_strings.grdp:57: Your data is encrypted with your sync passphrase. Enter it to start sync. s/your sync passphrase/a sync passphrase/ ?
On 2016/02/12 19:49:15, newt wrote: > *Could you enumerate which changes are included in this CL in the CL > description? Listed in the description. Here's a video demo of what it looks like: https://drive.google.com/a/google.com/file/d/0B1flmW1uZoFBT1dBRXFDUDQ3OEU/vie...
Also updated with comments from cleer@ and ewald@ after they saw the video demo. Updated encryption strings to the latest ones. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/sync_custom_passphrase.xml (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/sync_custom_passphrase.xml:34: android:layout_marginBottom="25dp" On 2016/02/12 22:03:49, newt wrote: > Is this still the right margin size? Changed to margin top instead. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/res... File chrome/android/java/res/values-v17/styles.xml (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/res... chrome/android/java/res/values-v17/styles.xml:76: <item name="android:divider">@null</item> On 2016/02/12 22:03:49, newt wrote: > Does this remove the dividers all throughout preferences? If so, we should do > this as a separate change since other preference screens will need updating. It was supposed to, but when I check on it now, the divider is still there. Not sure why (I swear I had a build where there were no dividers...) but I'm removing this bit for now. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/res... File chrome/android/java/res/values/arrays.xml (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/res... chrome/android/java/res/values/arrays.xml:36: <!-- Sync preferences--> On 2016/02/12 22:03:49, newt wrote: > Do you need to use an array resource? I'd avoid it unless it's necessary, i.e. > you're referencing this from XML. Not entirely necessary. Removed. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/res... File chrome/android/java/res/xml/account_management_preferences.xml (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/res... chrome/android/java/res/xml/account_management_preferences.xml:19: android:icon="@drawable/add_circle_blue" On 2016/02/12 22:03:49, newt wrote: > Can you delete the plus.png assets, or are they used elsewhere? Also, be sure to > fix the issue with the blurry hdpi add_circle_blue.png It's still used in a couple of places. I've added a new less blurry asset. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/res... File chrome/android/java/res/xml/sync_customization_preferences.xml (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/res... chrome/android/java/res/xml/sync_customization_preferences.xml:15: android:title="@string/sync_to_account_header" On 2016/02/12 22:03:50, newt wrote: > I'd suggest setting the title and summary in Java, so that all the logic related > to this preference lives in SyncAccountListPreference.java, rather than having a > small bit of the logic live in the xml file. Done. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncAccountListPreference.java (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncAccountListPreference.java:17: * A custom preference that opens a dialog picker to choose an account to use for syncing. On 2016/02/12 22:03:50, newt wrote: > maybe: "A preference that displays the account that's currently being synced and > allows the user to choose a new account for syncing" Done. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncAccountListPreference.java:19: public class SyncAccountListPreference extends ListPreference { On 2016/02/12 22:03:50, newt wrote: > How about calling this "SyncedAccountPreference"? Done. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncAccountListPreference.java:20: public SyncAccountListPreference(Context context, AttributeSet attrs) { On 2016/02/12 22:03:50, newt wrote: > javadoc for all public methods Done. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncAccountListPreference.java:33: String[] accountDisplayEntries = new String[accounts.length]; On 2016/02/12 22:03:50, newt wrote: > How about calling this "accountNames", and the variable below "accountValues" > (to be consistent with the ListPreference terminology)? Done. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncAccountListPreference.java:43: accountSyncSettingsKeys[i] = "sync_setting_" + account.name; On 2016/02/12 22:03:50, newt wrote: > Do you really need to prefix with "sync_setting"? or could you just use > account.name directly? No, I suppose not at the moment. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncAccountListPreference.java:53: setSummary(signedInAccountName); On 2016/02/12 22:03:50, newt wrote: > 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. Removed the android:summary entry in the xml https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncAccountListPreference.java:56: public void updateState(boolean enabled) { On 2016/02/12 22:03:50, newt wrote: > 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... 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. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncPreference.java (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncPreference.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/02/12 22:03:50, newt wrote: > where is this class used? in account_management_preferences.xml https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncPreference.java:25: setShouldDisableView(false); On 2016/02/12 22:03:50, newt wrote: > why? This shouldn't be there. Removed. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncPreference.java:29: public void updateSyncSummary() { On 2016/02/12 22:03:50, newt wrote: > javadoc for all public methods Done. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncPreference.java:68: Account account = ChromeSigninController.get(context).getSignedInUser(); On 2016/02/12 22:03:50, newt wrote: > Could this ever be null? It shouldn't be (since > ChromeSigninController.isSignedIn() is true above), but it's worth checking the > contract of ChromeSigninController to determine whether we should be defensive > here with a null check. Technically it can be (as in, there's a path to return null if getSignedInAccountName() is null), but isSignedIn() above, literally is: public boolean isSignedIn() { return getSignedInAccountName() != null; } so it shouldn't ever be null, unless there's a race condition where the signed in name preference is in the process of being unset while this is being called. Anyway, if it is null, the worst that happens is that it just says "Syncing to null" which is bad, but not horrible. At least it's obvious what's going on, for bug-filing purposes. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:61: * The settings screen with information and settings related to the user's accounts. This shows On 2016/02/12 22:03:50, newt wrote: > Revert rewrapping of this comment. It's less readable now. > > (Did you run an automatic formatter on this file? There seem to be lots of minor > formatting changes unrelated to this CL, which make it harder to focus on the > real changes. If you want to fix a bunch of minor formatting issues in a file, > it's best to do it as a separate trivial CL.) I ran git cl format, and may have accidentally done Ctrl+Shift+F on the file at some point (not sure which one of those two cause this). I'll restore the formatting best as I can. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:252: final Preferences activity = (Preferences) getActivity(); On 2016/02/12 22:03:50, newt wrote: > It doesn't look like you need this cast. It's much better to avoid casting if > possible. I do need the cast (because I need a Preferences object instead of an Activity to do startFragment() in line 265). Do you see a way to do this differently? https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:268: intent.putExtra("account_types", new String[] {"com.google"}); On 2016/02/12 22:03:50, newt wrote: > is there not a constant you can use instead of hard-coding "account_types"? Moved this code from a different file, but I'll change it to a constant. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:384: pref.setOnPreferenceClickListener(new OnPreferenceClickListener() { On 2016/02/12 22:03:50, newt wrote: > Why do we launch Android sync settings when the user taps on an account? Is that > in the spec? Yes, spec says, "Clicking on an individual account brings up the SysUI (either Android or iOS) for managing an individual account (which will also have a control to remove the account)" https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:389: Intent intent = new Intent(Settings.ACTION_SYNC_SETTINGS); On 2016/02/03 14:37:39, Bernhard Bauer wrote: > Extract this into a shared method? Done. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:616: * Gets the user picture for the account from the cache, or returns the default picture if On 2016/02/12 22:03:50, newt wrote: > I'd put this longer more-helpful JavaDoc on the public method above, and change > this JavaDoc to simply @see getBadgedUserPicture(String, Resources) Done. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/SignOutDialogFragment.java (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/SignOutDialogFragment.java:41: private int mGaiaServiceType; On 2016/02/12 22:03:50, newt wrote: > This needs a comment explaining what the valid values are. Pointing the reader > to AccountManagementFragment.GAIA_SERVICE_TYPE_NONE would be sufficient. Done. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/SignOutDialogFragment.java:48: AccountManagementFragment.SHOW_GAIA_SERVICE_TYPE_EXTRA, mGaiaServiceType); On 2016/02/12 22:03:50, newt wrote: > Rather than reusing the value of > AccountManagementFragment.SHOW_GAIA_SERVICE_TYPE_EXTRA, I'd define a constant in > this file similar to SHOW_GAIA_SERVICE_TYPE_EXTRA. That'll make it clearer to > anyone who reads this file than SHOW_GAIA_SERVICE_TYPE_EXTRA is a valid extra > that can be passed to this fragment. > > In general, I'm a bit wary of adding dependencies on AccountManagementFragment, > which is really supposed to be a UI class that consumes model data, but isn't > depended on by other UI classes. If you could move GAIA_SERVICE_TYPE_NONE to a > non-UI class, say, AccountManagementScreenHelper, that would make more sense. In > fact, AccountManagementScreenHelper already contains GAIA_SERVICE_TYPE_SIGNUP. Done. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseCreationDialogFragment.java (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseCreationDialogFragment.java:67: .setView(view) On 2016/02/12 22:03:51, newt wrote: > the indentation was correct before. Indent by 8 not 28. Done. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseCreationDialogFragment.java:83: Uri syncPassphraseHC = Uri.parse(SYNC_PASSPHRASE_SUPPORT_URL); On 2016/02/12 22:03:51, newt wrote: > s/HC/Url/ Done. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseCreationDialogFragment.java:84: Intent intent = new Intent(Intent.ACTION_VIEW, syncPassphraseHC); On 2016/02/12 22:03:51, newt wrote: > We should be using either: > - custom tabs, or > - HelpAndFeedback.show() (see LearnMorePreference.java for an example) > > Ask yusufo@ about custom tabs. Or see the list of help and feedback topics in > values.xml > (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...) > and add this new topic at go/mobilehelprecs > > Both of these solutions will show the help article *on top of* the current > activity, so there's no need to call activity().finish(), and when the user > presses back from the custom tabs or help and feedback activity, they'll be able > to continue the passphrase creation flow. Thanks for the tip. Will my addition to go/mobilehelprecs be propagated so that tapping on this will eventually go to the right link? I tried to follow the instructions, but the contact person listed doesn't work at Google anymore. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java:109: final AlertDialog d = On 2016/02/12 22:03:51, newt wrote: > Revert the wrapping to how it was. In general, try to avoid excessive > indentation. Another auto-formatting... https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java:213: verifying.setTextColor(Color.LTGRAY); On 2016/02/12 22:03:51, newt wrote: > Use color resources from colors.xml instead of constants from Colors.java. The > colors in Colors.java don't necessarily match the Chrome color palette. Actually, I can't find anymore where the spec said to change this color. I'm removing it for now. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java:238: TextView verifying = (TextView) getDialog().findViewById(R.id.verifying); On 2016/02/12 22:03:51, newt wrote: > Do you need to set the error state on the text view so the underline becomes > red? I'm not sure I understand. What underline? https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java:240: verifying.setTextColor(Color.RED); On 2016/02/12 22:03:51, newt wrote: > likewise Changed to colors.xml. Also changed the error color in SyncCustomizationFragment (which was already using Color.RED). https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java:82: public static final String PREFERENCE_SYNC_IMPORT_DATA = "sync_import_data"; On 2016/02/12 22:03:51, newt wrote: > unused? Removed https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java:151: mAllTypes = new CheckBoxPreference[] {mSyncAutofill, mSyncBookmarks, mSyncOmnibox, On 2016/02/12 22:03:51, newt wrote: > revert rewrapping Removed all auto-formatted changes in this file https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java:277: * Update the state of all settings from sync. This sets the state of the sync switch from On 2016/02/12 22:03:51, newt wrote: > revert rewrapping Done. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java:305: * Update the encryption state. If sync's backend is initialized, the button is enabled and the On 2016/02/12 22:03:51, newt wrote: > revert rewrapping Done. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java:510: * Update the state of the sync everything switch. If sync is on, load the pref from native. On 2016/02/12 22:03:51, newt wrote: > revert rewrapping, here and below Done. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:962: <message name="IDS_SYNC_DESCRIPTION" desc="Describes the purpose of syncing. This will be displayed on the sync on/off toggle [CHAR-LIMIT=64]"> On 2016/02/12 22:03:51, newt wrote: > Don't use CHAR-LIMITS unless longer text would break the UI or get cut off. Rule > of thumb: if a CHAR-LIMIT is longer than 32 characters, it's probably not > needed. Done. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1020: Encrypting your data with a passphrase lets you sync your data without letting Google read it. To change this setting, you will need to reset sync. <ph name="BEGIN_LINK"><link></ph>Learn more<ph name="END_LINK"></link></ph> On 2016/02/12 22:03:51, newt wrote: > Looks like this string needs updating to match the new mocks. Also, for > readability where this string is used, I'd use <learnmore> instead of <link> Done. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1053: You're switching sync accounts from <ph name="FROM_ACCOUNT">%1$s<ex>johndoe@gmail.com</ex></ph> to <ph name="TO_ACCOUNT">%2$s<ex>john@gmail.com</ex></ph>. What do you want to do with your existing bookmarks, history, passwords, and other settings? On 2016/02/12 22:03:51, newt wrote: > use curly quotes (you can copy paste from elsewhere in this file) I don't need all these strings yet (import data piece is still being worked on), so removed from this CL. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1061: <message name="IDS_SYNC_IMPORT_EXISTING_DATA_VALUE" desc="Internal value for import data preference" translateable="false"> On 2016/02/12 22:03:51, newt wrote: > Unless you need to reference this string from both xml resources and from Java > code, I'd move this from the grd file into a Java file. There's much less > overhead to read a string constant in Java and it's simpler that way. (Likewise > for keep_existing_data below). Ditto https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1068: Replace this device's data with <ph name="ACCOUNT">%1$s<ex>john@doe.com</ex></ph>'s data. You can retrieve your existing data by switching back to <ph name="ACCOUNT">%1$s<ex>johndoe@gmail.com</ex></ph> On 2016/02/12 22:03:51, newt wrote: > curly quotes Ditto. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1615: Select an account to get your bookmarks, history, passwords, and other settings on all your devices On 2016/02/12 22:03:51, newt wrote: > add back the period? It's in the mocks. Done. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1636: Add an account to get your bookmarks, history, passwords, and other settings on all your devices On 2016/02/12 22:03:51, newt wrote: > add back the period? Done. https://codereview.chromium.org/1660353002/diff/20001/components/sync_ui_stri... File components/sync_ui_strings.grdp (right): https://codereview.chromium.org/1660353002/diff/20001/components/sync_ui_stri... components/sync_ui_strings.grdp:57: Your data is encrypted with your sync passphrase. Enter it to start sync. On 2016/02/12 22:03:51, newt wrote: > s/your sync passphrase/a sync passphrase/ ? Mocks say "your sync passphrase": https://folio.googleplex.com/chrome-ux/mocks/332-mobile-accounts/accounts/1#%...
https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/res... File chrome/android/java/res/values-v17/styles.xml (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/res... chrome/android/java/res/values-v17/styles.xml:76: <item name="android:divider">@null</item> On 2016/02/16 17:32:06, May wrote: > On 2016/02/12 22:03:49, newt wrote: > > Does this remove the dividers all throughout preferences? If so, we should do > > this as a separate change since other preference screens will need updating. > > It was supposed to, but when I check on it now, the divider is still there. Not > sure why (I swear I had a build where there were no dividers...) but I'm > removing this bit for now. Strange. Once we switch to using the support library preferences (crbug.com/585814), it may be easier to control the dividers in settings. I'd like to update our dividers on all settings screens, since the dividers (on all screens) have never matched the mocks. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncPreference.java (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncPreference.java:68: Account account = ChromeSigninController.get(context).getSignedInUser(); On 2016/02/16 17:32:07, May wrote: > On 2016/02/12 22:03:50, newt wrote: > > Could this ever be null? It shouldn't be (since > > ChromeSigninController.isSignedIn() is true above), but it's worth checking > the > > contract of ChromeSigninController to determine whether we should be defensive > > here with a null check. > > Technically it can be (as in, there's a path to return null if > getSignedInAccountName() is null), but isSignedIn() above, literally is: > public boolean isSignedIn() { > return getSignedInAccountName() != null; > } > so it shouldn't ever be null, unless there's a race condition where the signed > in name preference is in the process of being unset while this is being called. > Anyway, if it is null, the worst that happens is that it just says "Syncing to > null" which is bad, but not horrible. At least it's obvious what's going on, for > bug-filing purposes. Sounds good. Thanks for checking. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:61: * The settings screen with information and settings related to the user's accounts. This shows On 2016/02/16 17:32:07, May wrote: > On 2016/02/12 22:03:50, newt wrote: > > Revert rewrapping of this comment. It's less readable now. > > > > (Did you run an automatic formatter on this file? There seem to be lots of > minor > > formatting changes unrelated to this CL, which make it harder to focus on the > > real changes. If you want to fix a bunch of minor formatting issues in a file, > > it's best to do it as a separate trivial CL.) > > I ran git cl format, and may have accidentally done Ctrl+Shift+F on the file at > some point (not sure which one of those two cause this). I'll restore the > formatting best as I can. Thanks. Ctrl+Shift+F on a whole file is pretty dangerous, as Eclipse formatting is far from perfect. I'd only use it on a highlighted section of already-modified code, and verify the results. I haven't used "git cl format" on Java code, but AIUI git cl format uses the C++-based clang formatter, which only sort of understands Java. https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:252: final Preferences activity = (Preferences) getActivity(); On 2016/02/16 17:32:07, May wrote: > On 2016/02/12 22:03:50, newt wrote: > > It doesn't look like you need this cast. It's much better to avoid casting if > > possible. > > I do need the cast (because I need a Preferences object instead of an Activity > to do startFragment() in line 265). Do you see a way to do this differently? Oh, I see. In that case, maybe call this "preferences" instead of "activity". https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java (right): https://codereview.chromium.org/1660353002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java:109: final AlertDialog d = On 2016/02/16 17:32:07, May wrote: > On 2016/02/12 22:03:51, newt wrote: > > Revert the wrapping to how it was. In general, try to avoid excessive > > indentation. > > Another auto-formatting... By the way, "git difftool --tool=meld" is super handy for reverting parts of changes. https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/res... File chrome/android/java/res/layout/sync_custom_passphrase.xml (right): https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/sync_custom_passphrase.xml:35: android:layout_marginBottom="25dp" Still seems like a lot of extra padding/margin. Does this match the mocks? https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/sync_custom_passphrase.xml:38: chrome:helpContext="@string/help_context_clear_browsing_data" /> Huh? chrome:helpContext is used for the LearnMorePreference. It doesn't apply to TextViews. https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/res... File chrome/android/java/res/xml/sync_customization_preferences.xml (right): https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/res... chrome/android/java/res/xml/sync_customization_preferences.xml:14: android:key="sync_account_list" /> call this "synced_account"? https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncPreference.java (right): https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncPreference.java:20: * A custom preference that displays information about the currently synced account. "Preference that displays the current sync account and status (enabled, error, needs passphrase, etc)." https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java (right): https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:61: * The settings screen with information and settings related to the user's accounts. add back the removed newlines in this class comment https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:85: public static final int GAIA_SERVICE_TYPE_NONE = 0; remove this, now that it's in AccountManagementScreenHelper https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:627: private Bitmap getBadgedUserPicture(String accountId) { actually, since this convenience method is only used once, I'd just delete/inline it https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/SignOutDialogFragment.java (right): https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/SignOutDialogFragment.java:46: /** newline before this https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java (right): https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java:234: TextView verifying = (TextView) getDialog().findViewById(R.id.verifying); On 2016/02/16 17:32:07, May wrote: > On 2016/02/12 22:03:51, newt wrote: > > Do you need to set the error state on the text view so the underline becomes > > red? > > I'm not sure I understand. What underline? The EditText has a horizontal line below the cursor. In the mocks, this horizontal line is red when the passphrase is incorrect (in addition to the text being red). https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseTypeDialogFragment.java (right): https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseTypeDialogFragment.java:194: Uri syncDashboardUrl = Uri.parse( It's probably better to use a custom tab here, so that the user doesn't lose their place in Settings. ask yusufo@ for details about how to do that. https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseTypeDialogFragment.java:202: if (activity != null) activity.finish(); activity.finish() will only finish the topmost Settings activity. Any activities below that (e.g. the activity showing the main settings screen) will still be showing. Is that what we want? https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseTypeDialogFragment.java:206: @Override newline before this https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java (right): https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java:81: public static final String PREFERENCE_SYNC_IMPORT_DATA = "sync_import_data"; still unused https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java:94: public static final String[] PREFS_TO_SAVE = { why remove PREFERENCE_SYNC_RECENT_TABS? https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1054: Replace this device’s data with <ph name="ACCOUNT">%1$s<ex>john@doe.com</ex></ph>'s data. You can retrieve your existing data by switching back to <ph name="ACCOUNT">%1$s<ex>johndoe@gmail.com</ex></ph> also need curly quotes in "john@doe.com's data" https://codereview.chromium.org/1660353002/diff/40001/sync/android/java/src/o... File sync/android/java/src/org/chromium/sync/PassphraseType.java (right): https://codereview.chromium.org/1660353002/diff/40001/sync/android/java/src/o... sync/android/java/src/org/chromium/sync/PassphraseType.java:59: visibleTypes.add(CUSTOM_PASSPHRASE); why reorder this?
https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/res... File chrome/android/java/res/layout/sync_custom_passphrase.xml (right): https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/sync_custom_passphrase.xml:35: android:layout_marginBottom="25dp" On 2016/02/16 23:05:01, newt wrote: > Still seems like a lot of extra padding/margin. Does this match the mocks? It does visually. I don't have any vizd for actual numbers. https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/sync_custom_passphrase.xml:38: chrome:helpContext="@string/help_context_clear_browsing_data" /> On 2016/02/16 23:05:01, newt wrote: > Huh? chrome:helpContext is used for the LearnMorePreference. It doesn't apply to > TextViews. Ugh, missed this before uploading. Removed. https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/res... File chrome/android/java/res/xml/sync_customization_preferences.xml (right): https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/res... chrome/android/java/res/xml/sync_customization_preferences.xml:14: android:key="sync_account_list" /> On 2016/02/16 23:05:01, newt wrote: > call this "synced_account"? Done. https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncPreference.java (right): https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncPreference.java:20: * A custom preference that displays information about the currently synced account. On 2016/02/16 23:05:01, newt wrote: > "Preference that displays the current sync account and status (enabled, error, > needs passphrase, etc)." Done. https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java (right): https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:61: * The settings screen with information and settings related to the user's accounts. On 2016/02/16 23:05:01, newt wrote: > add back the removed newlines in this class comment Done. https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:85: public static final int GAIA_SERVICE_TYPE_NONE = 0; On 2016/02/16 23:05:01, newt wrote: > remove this, now that it's in AccountManagementScreenHelper Done. https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:627: private Bitmap getBadgedUserPicture(String accountId) { On 2016/02/16 23:05:01, newt wrote: > actually, since this convenience method is only used once, I'd just > delete/inline it Done. https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/SignOutDialogFragment.java (right): https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/SignOutDialogFragment.java:46: /** On 2016/02/16 23:05:01, newt wrote: > newline before this Done. https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java (right): https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java:234: TextView verifying = (TextView) getDialog().findViewById(R.id.verifying); On 2016/02/16 23:05:01, newt wrote: > On 2016/02/16 17:32:07, May wrote: > > On 2016/02/12 22:03:51, newt wrote: > > > Do you need to set the error state on the text view so the underline becomes > > > red? > > > > I'm not sure I understand. What underline? > > The EditText has a horizontal line below the cursor. In the mocks, this > horizontal line is red when the passphrase is incorrect (in addition to the text > being red). Oh that underline. Yes, you're right. I added that in. https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseTypeDialogFragment.java (right): https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseTypeDialogFragment.java:194: Uri syncDashboardUrl = Uri.parse( On 2016/02/16 23:05:01, newt wrote: > It's probably better to use a custom tab here, so that the user doesn't lose > their place in Settings. ask yusufo@ for details about how to do that. I've sent an email to yusufo@. I've added a TODO for that (in case it takes a while to figure out what to do). https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseTypeDialogFragment.java:202: if (activity != null) activity.finish(); On 2016/02/16 23:05:01, newt wrote: > activity.finish() will only finish the topmost Settings activity. Any activities > below that (e.g. the activity showing the main settings screen) will still be > showing. Is that what we want? Yes, because they may not be done doing other config yet. https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseTypeDialogFragment.java:206: @Override On 2016/02/16 23:05:01, newt wrote: > newline before this Done. https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java (right): https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java:81: public static final String PREFERENCE_SYNC_IMPORT_DATA = "sync_import_data"; On 2016/02/16 23:05:01, newt wrote: > still unused Done. https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java:94: public static final String[] PREFS_TO_SAVE = { On 2016/02/16 23:05:01, newt wrote: > why remove PREFERENCE_SYNC_RECENT_TABS? ACK. I don't know how that happened. Added back. https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1054: Replace this device’s data with <ph name="ACCOUNT">%1$s<ex>john@doe.com</ex></ph>'s data. You can retrieve your existing data by switching back to <ph name="ACCOUNT">%1$s<ex>johndoe@gmail.com</ex></ph> On 2016/02/16 23:05:01, newt wrote: > also need curly quotes in mailto:"john@doe.com data" Done. https://codereview.chromium.org/1660353002/diff/40001/sync/android/java/src/o... File sync/android/java/src/org/chromium/sync/PassphraseType.java (right): https://codereview.chromium.org/1660353002/diff/40001/sync/android/java/src/o... sync/android/java/src/org/chromium/sync/PassphraseType.java:59: visibleTypes.add(CUSTOM_PASSPHRASE); On 2016/02/16 23:05:01, newt wrote: > why reorder this? Spec says that's the order of the list presented to the user.
last few comments, then lgtm. The new UI looks good! Thanks for putting this together :) https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/res... File chrome/android/java/res/layout/sync_custom_passphrase.xml (right): https://codereview.chromium.org/1660353002/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/sync_custom_passphrase.xml:35: android:layout_marginBottom="25dp" On 2016/02/17 13:24:31, May wrote: > On 2016/02/16 23:05:01, newt wrote: > > Still seems like a lot of extra padding/margin. Does this match the mocks? > > It does visually. I don't have any vizd for actual numbers. Ok. Sometimes counting pixels in GIMP is the best option :) https://codereview.chromium.org/1660353002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java (right): https://codereview.chromium.org/1660353002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:623: private Bitmap getBadgedUserPicture(String accountId) { Likewise, I'd suggest removing this method and inlining its one use. https://codereview.chromium.org/1660353002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java (right): https://codereview.chromium.org/1660353002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java:243: verifying.setTextColor(ApiCompatibilityUtils.getColor( I'd store the result of getColor() and use it again below. Getting resources has non-trivial cost, and I think it'll improve readability. https://codereview.chromium.org/1660353002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java:246: passphraseEditText.getBackground().setColorFilter(ApiCompatibilityUtils.getColor( You must call getBackground().mutate() before changing the getBackground() drawable. This is a general rule in Android: always call mutate() on a Drawable before changing it. Otherwise, since Android shares state across Drawables, you may affect other Drawables. E.g. this could cause all EditTexts to change to red. https://codereview.chromium.org/1660353002/diff/80001/sync/android/java/src/o... File sync/android/java/src/org/chromium/sync/PassphraseType.java (right): https://codereview.chromium.org/1660353002/diff/80001/sync/android/java/src/o... sync/android/java/src/org/chromium/sync/PassphraseType.java:59: visibleTypes.add(CUSTOM_PASSPHRASE); visibleTypes is a Set, so it doesn't matter in what order you add items to it. I'd revert this change.
https://codereview.chromium.org/1660353002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java (right): https://codereview.chromium.org/1660353002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:623: private Bitmap getBadgedUserPicture(String accountId) { On 2016/02/17 19:34:41, newt wrote: > Likewise, I'd suggest removing this method and inlining its one use. Done. https://codereview.chromium.org/1660353002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java (right): https://codereview.chromium.org/1660353002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java:243: verifying.setTextColor(ApiCompatibilityUtils.getColor( On 2016/02/17 19:34:41, newt wrote: > I'd store the result of getColor() and use it again below. Getting resources has > non-trivial cost, and I think it'll improve readability. Done. https://codereview.chromium.org/1660353002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java:246: passphraseEditText.getBackground().setColorFilter(ApiCompatibilityUtils.getColor( On 2016/02/17 19:34:41, newt wrote: > You must call getBackground().mutate() before changing the getBackground() > drawable. This is a general rule in Android: always call mutate() on a Drawable > before changing it. Otherwise, since Android shares state across Drawables, you > may affect other Drawables. E.g. this could cause all EditTexts to change to > red. Ah, good to know. Done https://codereview.chromium.org/1660353002/diff/80001/sync/android/java/src/o... File sync/android/java/src/org/chromium/sync/PassphraseType.java (right): https://codereview.chromium.org/1660353002/diff/80001/sync/android/java/src/o... sync/android/java/src/org/chromium/sync/PassphraseType.java:59: visibleTypes.add(CUSTOM_PASSPHRASE); On 2016/02/17 19:34:41, newt wrote: > visibleTypes is a Set, so it doesn't matter in what order you add items to it. > I'd revert this change. Ah, didn't see that it was a set. Done.
maybelle@chromium.org changed reviewers: + blundell@chromium.org
blundell@chromium.org: Please review changes in components/sync_ui_strings.grdp Thanks!
https://codereview.chromium.org/1660353002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java (right): https://codereview.chromium.org/1660353002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java:246: passphraseEditText.getBackground().setColorFilter(ApiCompatibilityUtils.getColor( On 2016/02/19 18:10:04, May wrote: > On 2016/02/17 19:34:41, newt wrote: > > You must call getBackground().mutate() before changing the getBackground() > > drawable. This is a general rule in Android: always call mutate() on a > Drawable > > before changing it. Otherwise, since Android shares state across Drawables, > you > > may affect other Drawables. E.g. this could cause all EditTexts to change to > > red. > > Ah, good to know. Done Store that tidbit away :) This is definitely a rough edge in Android, and this issues comes up fairly frequently. https://codereview.chromium.org/1660353002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java (right): https://codereview.chromium.org/1660353002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java:185: intent.putExtra(CustomTabsIntent.EXTRA_SESSION, true); should this be "null" instead of "true"? https://codereview.chromium.org/1660353002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java:217: passphraseEditText.getBackground().setColorFilter(mPasswordEditTextOriginalColorFilter); for safety (or to prevent breakage in the future), I'd add "mutate()" here too.
lgtm after comments
//components/sync_ui_strings.grdp lgtm
https://codereview.chromium.org/1660353002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java (right): https://codereview.chromium.org/1660353002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java:185: intent.putExtra(CustomTabsIntent.EXTRA_SESSION, true); On 2016/02/20 00:25:17, newt wrote: > should this be "null" instead of "true"? I ended up replacing this with the CustomTabsIntent#getViewIntentWithNoSession convenience method. https://codereview.chromium.org/1660353002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java:217: passphraseEditText.getBackground().setColorFilter(mPasswordEditTextOriginalColorFilter); On 2016/02/20 00:25:17, newt wrote: > for safety (or to prevent breakage in the future), I'd add "mutate()" here too. Done.
The CQ bit was checked by maybelle@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org, blundell@chromium.org Link to the patchset: https://codereview.chromium.org/1660353002/#ps120001 (title: "CR comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1660353002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1660353002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by maybelle@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org, blundell@chromium.org Link to the patchset: https://codereview.chromium.org/1660353002/#ps160001 (title: "Add TODO in comments for an additional task for the bug")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1660353002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1660353002/160001
Message was sent while issue was closed.
Description was changed from ========== Update account and sync management UX These changes encompass all the UX changes to the accounts list screen, and the sync settings screen. This does not include signing in from the settings screen or switching sync accounts. BUG=557784 ========== to ========== Update account and sync management UX These changes encompass all the UX changes to the accounts list screen, and the sync settings screen. This does not include signing in from the settings screen or switching sync accounts. BUG=557784 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Update account and sync management UX These changes encompass all the UX changes to the accounts list screen, and the sync settings screen. This does not include signing in from the settings screen or switching sync accounts. BUG=557784 ========== to ========== Update account and sync management UX These changes encompass all the UX changes to the accounts list screen, and the sync settings screen. This does not include signing in from the settings screen or switching sync accounts. BUG=557784 Committed: https://crrev.com/f3ff6dac2fba2a55ed76e3c0ae958d5350140f97 Cr-Commit-Position: refs/heads/master@{#376729} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/f3ff6dac2fba2a55ed76e3c0ae958d5350140f97 Cr-Commit-Position: refs/heads/master@{#376729} |
