Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1698043006:
Created the dialog offering the user to merge their account data or keep it (Closed)
|
Created:
4 years, 10 months ago by PEConn Modified:
4 years, 9 months ago CC:
chromium-reviews, tim+watch_chromium.org, tfarina, maxbogue+watch_chromium.org, plaree+watch_chromium.org, zea+watch_chromium.org Base URL:
maybelle.lon.corp.google.com:/usr/local/google/code/clankium/src@sync_settings Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description(formerly: Created the dialog offering the user to merge their account data or keep it)
This CL contains two changes to the Switching Sync Account flows.
- It adds the ability to switch sync accounts, presenting a dialog offering the user to merge their account data or keep it separate.
- It upgrades the account signin from preferences from using the ChooseAccountFragment to using the AccountSigninView (formerly the AccountFirstRunView).
In addition it:
- Renames AccountFirstRunView to AccountSigninView (since it is not used only in the first run).
- Renames functions in the AccountSigninView from using SignIn to Signin.
The following user actions were modified:
- Signin_AddAccountToDevice
- Signin_ImportDataPrompt_Cancel
- Signin_ImportDataPrompt_DontImport
- Signin_ImportDataPrompt_ImportData
- Signin_Signin_FromSettings
- Signin_Undo_Signin
BUG=557786
Committed: https://crrev.com/bd705aeab0594a1790478e335edee728f32fb026
Cr-Commit-Position: refs/heads/master@{#377769}
Patch Set 1 #
Total comments: 14
Patch Set 2 : #
Total comments: 23
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #
Total comments: 35
Patch Set 5 : #Patch Set 6 : Rebased onto Master #Patch Set 7 : #Patch Set 8 : #
Total comments: 63
Patch Set 9 : #
Total comments: 26
Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #
Total comments: 7
Messages
Total messages: 79 (31 generated)
peconn@chromium.org changed reviewers: + bauerb@chromium.org, maybelle@chromium.org, newt@chromium.org
https://codereview.chromium.org/1698043006/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataFragment.java (right): https://codereview.chromium.org/1698043006/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataFragment.java:97: assert listener == null; Should this be |mListener|?
https://codereview.chromium.org/1698043006/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/sync/SyncAccountSwitcher.java (right): https://codereview.chromium.org/1698043006/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/sync/SyncAccountSwitcher.java:85: PrefServiceBridge.getInstance().clearBrowsingData(this, SYNC_DATA_TYPES); Don't you still need to sign in the new account at this point? https://codereview.chromium.org/1698043006/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java (right): https://codereview.chromium.org/1698043006/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java:282: * Update the state of all settings from sync. This sets the state of the sync switch from I've had to revert a bunch of auto-formatted whitespace changes, when my CL lands please revert these as well here. https://codereview.chromium.org/1698043006/diff/1/chrome/android/java/strings... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1698043006/diff/1/chrome/android/java/strings... chrome/android/java/strings/android_chrome_strings.grd:1072: Replace this device's data with <ph name="TO_ACCOUNT">%1$s<ex>johndoe@new.com</ex></ph>'s data. You can retrieve your existing data by switching back to <ph name="FROM_ACCOUNT">%2$s<ex>johndoe@old.com</ex></ph>, but data created since you signed out will be lost. Keep the curly apostrophes https://codereview.chromium.org/1698043006/diff/1/ui/android/java/res/layout/... File ui/android/java/res/layout/radio_button_with_description.xml (right): https://codereview.chromium.org/1698043006/diff/1/ui/android/java/res/layout/... ui/android/java/res/layout/radio_button_with_description.xml:14: android:id="@+id/radio_button" android:id should be the first attribute https://codereview.chromium.org/1698043006/diff/1/ui/android/java/src/org/chr... File ui/android/java/src/org/chromium/ui/widget/RadioButtonWithDescription.java (right): https://codereview.chromium.org/1698043006/diff/1/ui/android/java/src/org/chr... ui/android/java/src/org/chromium/ui/widget/RadioButtonWithDescription.java:24: public class RadioButtonWithDescription extends LinearLayout implements OnClickListener{ Space before { https://codereview.chromium.org/1698043006/diff/1/ui/android/java/src/org/chr... ui/android/java/src/org/chromium/ui/widget/RadioButtonWithDescription.java:77: public void onClick(View v) { Is there a reason you can't surround the custom RadioButtons in an android RadioGroup in the layout so that you don't have to do all this? I find it unintuitive to have to call setRadioButtonGroup() on each RadioButtonWithDescription that I create.
https://codereview.chromium.org/1698043006/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/sync/SyncAccountSwitcher.java (right): https://codereview.chromium.org/1698043006/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/sync/SyncAccountSwitcher.java:85: PrefServiceBridge.getInstance().clearBrowsingData(this, SYNC_DATA_TYPES); On 2016/02/17 14:22:11, May wrote: > Don't you still need to sign in the new account at this point? Yes, but not before the browsing data has been cleared. So we have to go through yet another callback to 'onBrowsingDataCleared' below. https://codereview.chromium.org/1698043006/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataFragment.java (right): https://codereview.chromium.org/1698043006/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataFragment.java:97: assert listener == null; On 2016/02/17 13:24:56, Bernhard Bauer wrote: > Should this be |mListener|? Done. https://codereview.chromium.org/1698043006/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java (right): https://codereview.chromium.org/1698043006/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java:282: * Update the state of all settings from sync. This sets the state of the sync switch from On 2016/02/17 14:22:11, May wrote: > I've had to revert a bunch of auto-formatted whitespace changes, when my CL > lands please revert these as well here. Acknowledged. https://codereview.chromium.org/1698043006/diff/1/ui/android/java/src/org/chr... File ui/android/java/src/org/chromium/ui/widget/RadioButtonWithDescription.java (right): https://codereview.chromium.org/1698043006/diff/1/ui/android/java/src/org/chr... ui/android/java/src/org/chromium/ui/widget/RadioButtonWithDescription.java:77: public void onClick(View v) { On 2016/02/17 14:22:12, May wrote: > Is there a reason you can't surround the custom RadioButtons in an android > RadioGroup in the layout so that you don't have to do all this? I find it > unintuitive to have to call setRadioButtonGroup() on each > RadioButtonWithDescription that I create. This class extends FrameLayout, not RadioButton so the RadioGroup won't work. One option would be to create a RadioButtonWithDescription*Group*, but that seemed a bit like over engineering.
newt@chromium.org changed reviewers: + yusufo@chromium.org
sending this to yusufo@ to take a look
Description was changed from ========== Created the dialog offering the user to merge their account data or keep it separate when changing sync accounts. BUG=557786 ========== to ========== (formerly: Created the dialog offering the user to merge their account data or keep it) This CL contains two changes to the Switching Sync Account flows. - It adds the ability to switch sync accounts, presenting a dialog offering the user to merge their account data or keep it separate. - It upgrades the account signin from preferences from using the ChooseAccountFragment to using the AccountSigninView (formerly the AccountFirstRunView) In addition it: - Renames AccountFirstRunView to AccountSigninView (since it is not used only in the first run). - Renames functions in the AccountSigninView from using SignIn to Signin. BUG=557786 ==========
The description for this CL has been updated. https://codereview.chromium.org/1698043006/diff/1/chrome/android/java/strings... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1698043006/diff/1/chrome/android/java/strings... chrome/android/java/strings/android_chrome_strings.grd:1072: Replace this device's data with <ph name="TO_ACCOUNT">%1$s<ex>johndoe@new.com</ex></ph>'s data. You can retrieve your existing data by switching back to <ph name="FROM_ACCOUNT">%2$s<ex>johndoe@old.com</ex></ph>, but data created since you signed out will be lost. On 2016/02/17 14:22:11, May wrote: > Keep the curly apostrophes Done. https://codereview.chromium.org/1698043006/diff/1/ui/android/java/res/layout/... File ui/android/java/res/layout/radio_button_with_description.xml (right): https://codereview.chromium.org/1698043006/diff/1/ui/android/java/res/layout/... ui/android/java/res/layout/radio_button_with_description.xml:14: android:id="@+id/radio_button" On 2016/02/17 14:22:11, May wrote: > android:id should be the first attribute Done. https://codereview.chromium.org/1698043006/diff/1/ui/android/java/src/org/chr... File ui/android/java/src/org/chromium/ui/widget/RadioButtonWithDescription.java (right): https://codereview.chromium.org/1698043006/diff/1/ui/android/java/src/org/chr... ui/android/java/src/org/chromium/ui/widget/RadioButtonWithDescription.java:24: public class RadioButtonWithDescription extends LinearLayout implements OnClickListener{ On 2016/02/17 14:22:12, May wrote: > Space before { Done.
Description was changed from ========== (formerly: Created the dialog offering the user to merge their account data or keep it) This CL contains two changes to the Switching Sync Account flows. - It adds the ability to switch sync accounts, presenting a dialog offering the user to merge their account data or keep it separate. - It upgrades the account signin from preferences from using the ChooseAccountFragment to using the AccountSigninView (formerly the AccountFirstRunView) In addition it: - Renames AccountFirstRunView to AccountSigninView (since it is not used only in the first run). - Renames functions in the AccountSigninView from using SignIn to Signin. BUG=557786 ========== to ========== (formerly: Created the dialog offering the user to merge their account data or keep it) This CL contains two changes to the Switching Sync Account flows. - It adds the ability to switch sync accounts, presenting a dialog offering the user to merge their account data or keep it separate. - It upgrades the account signin from preferences from using the ChooseAccountFragment to using the AccountSigninView (formerly the AccountFirstRunView). In addition it: - Renames AccountFirstRunView to AccountSigninView (since it is not used only in the first run). - Renames functions in the AccountSigninView from using SignIn to Signin. BUG=557786 ==========
https://codereview.chromium.org/1698043006/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/account_signin_view.xml (left): https://codereview.chromium.org/1698043006/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/account_signin_view.xml:10: android:id="@+id/fre_account_layout" let's get rid of the fre mentions in other places as well if we are going to rename this. https://codereview.chromium.org/1698043006/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/confirm_import_sync_data.xml (right): https://codereview.chromium.org/1698043006/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/confirm_import_sync_data.xml:16: android:layout_width="match_parent" android:layout_x tags should come first. if there is an id it should be android:id android:layout_width android:layout_height https://codereview.chromium.org/1698043006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncedAccountPreference.java (right): https://codereview.chromium.org/1698043006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncedAccountPreference.java:34: public void update() { javadoc https://codereview.chromium.org/1698043006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncedAccountPreference.java:69: public String valueToEntry(String value) { Can we add this as a TODO somewhere in the class description(or a better place). Without a current implementation the added call and the comments seem superfluous. https://codereview.chromium.org/1698043006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java (right): https://codereview.chromium.org/1698043006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java:23: public class AccountSigninActivity extends Activity extends AppcompatActivity for getting themes to work properly. https://codereview.chromium.org/1698043006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java (right): https://codereview.chromium.org/1698043006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:541: ((Activity) getContext()).getFragmentManager(); (Activity) getContext() is a fairly dangerous assumption to make (even if it is probably true here). where do we use this FragmentManager? And what happened to mFragmentManager? https://codereview.chromium.org/1698043006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:561: == InvestigatedScenario.DIFFERENT_ACCOUNT 8 spaces https://codereview.chromium.org/1698043006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:576: confirmSync.show(mFragmentManager, CONFIRM_IMPORT_SYNC_DATA_DIALOG_TAG); Instead of setting a FragmentManager for this class, can we not set a onClickListener or another listener with an onConfirm() to be called onClick? Having all this logic with the FragmentManager inside a private class inside a View is a bit too long winded. Or all this above can be a static call inside the Fragment class itself that takes in a ConfirmImportSyncDataFragment.Listener. Fragment itself has a getFragmentManager() that is public and we should be using that and not getting the activity's fragment(or I am missing something here.) https://codereview.chromium.org/1698043006/diff/20001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/widget/RadioButtonWithDescription.java (right): https://codereview.chromium.org/1698043006/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/widget/RadioButtonWithDescription.java:33: init(null); instead of using init and extending all constructors here use the class name itself org.chromium.ui.widget.RadioButtonWithDescription inside the xml file (instead of the <merge> you have there). https://codereview.chromium.org/1698043006/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/widget/RadioButtonWithDescription.java:87: public void setTitleText(CharSequence text) { javadoc for all below public calls
https://codereview.chromium.org/1698043006/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/account_signin_view.xml (left): https://codereview.chromium.org/1698043006/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/account_signin_view.xml:10: android:id="@+id/fre_account_layout" On 2016/02/18 19:16:16, Yusuf wrote: > let's get rid of the fre mentions in other places as well if we are going to > rename this. Done. https://codereview.chromium.org/1698043006/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/confirm_import_sync_data.xml (right): https://codereview.chromium.org/1698043006/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/confirm_import_sync_data.xml:16: android:layout_width="match_parent" On 2016/02/18 19:16:16, Yusuf wrote: > android:layout_x tags should come first. if there is an id it should be > android:id > android:layout_width > android:layout_height Done. https://codereview.chromium.org/1698043006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncedAccountPreference.java (right): https://codereview.chromium.org/1698043006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncedAccountPreference.java:34: public void update() { On 2016/02/18 19:16:16, Yusuf wrote: > javadoc Done. https://codereview.chromium.org/1698043006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncedAccountPreference.java:69: public String valueToEntry(String value) { On 2016/02/18 19:16:16, Yusuf wrote: > Can we add this as a TODO somewhere in the class description(or a better place). > Without a current implementation the added call and the comments seem > superfluous. I simplified the code and got rid of this. https://codereview.chromium.org/1698043006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java (right): https://codereview.chromium.org/1698043006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java:23: public class AccountSigninActivity extends Activity On 2016/02/18 19:16:16, Yusuf wrote: > extends AppcompatActivity for getting themes to work properly. Done. https://codereview.chromium.org/1698043006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java (right): https://codereview.chromium.org/1698043006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:541: ((Activity) getContext()).getFragmentManager(); On 2016/02/18 19:16:16, Yusuf wrote: > (Activity) getContext() is a fairly dangerous assumption to make (even if it is > probably true here). where do we use this FragmentManager? And what happened to > mFragmentManager? Sorry, that line should have been removed. https://codereview.chromium.org/1698043006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:561: == InvestigatedScenario.DIFFERENT_ACCOUNT On 2016/02/18 19:16:16, Yusuf wrote: > 8 spaces Done. https://codereview.chromium.org/1698043006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:576: confirmSync.show(mFragmentManager, CONFIRM_IMPORT_SYNC_DATA_DIALOG_TAG); On 2016/02/18 19:16:16, Yusuf wrote: > Instead of setting a FragmentManager for this class, can we not set a > onClickListener or another listener > with an onConfirm() to be called onClick? Having all this logic with the > FragmentManager inside a private > class inside a View is a bit too long winded. > > Or all this above can be a static call inside the Fragment > class itself that takes in a ConfirmImportSyncDataFragment.Listener. Fragment > itself has a getFragmentManager() that is > public and we should be using that and not getting the activity's > fragment(or I am missing something here.) Before a Fragment is shown, it's getFragment returns null. I decided to extend the Listener class so the owner of AccountSigninAccount can deal with creating the Dialog. https://codereview.chromium.org/1698043006/diff/20001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/widget/RadioButtonWithDescription.java (right): https://codereview.chromium.org/1698043006/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/widget/RadioButtonWithDescription.java:33: init(null); On 2016/02/18 19:16:16, Yusuf wrote: > instead of using init and extending all constructors here > > use the class name itself org.chromium.ui.widget.RadioButtonWithDescription > inside the xml file (instead of the <merge> you have there). I could not get this to work. I changed the xml to use org...RadioButtonWithDescription and the constructor to take (Context, AttributeSet) (and not inflate the view). This gave fatal errors inflating the xml. Likewise when I changed confirm_import_sync_data.xml to use 'include layout="@layout/radio_button_with_description"' instead of 'rg.chromium.ui.widget.RadioButtonWithDescription'. What did work was changing the 'merge' to a LinearLayout, then inflating it in the constructor of RadioButtonWithDescription, but I think this creates a superfluous layout. https://codereview.chromium.org/1698043006/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/widget/RadioButtonWithDescription.java:87: public void setTitleText(CharSequence text) { On 2016/02/18 19:16:16, Yusuf wrote: > javadoc for all below public calls Done.
https://codereview.chromium.org/1698043006/diff/20001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/widget/RadioButtonWithDescription.java (right): https://codereview.chromium.org/1698043006/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/widget/RadioButtonWithDescription.java:33: init(null); On 2016/02/19 18:10:11, PEConn1 wrote: > On 2016/02/18 19:16:16, Yusuf wrote: > > instead of using init and extending all constructors here > > > > use the class name itself org.chromium.ui.widget.RadioButtonWithDescription > > inside the xml file (instead of the <merge> you have there). > > I could not get this to work. > > I changed the xml to use org...RadioButtonWithDescription and the constructor to > take (Context, AttributeSet) (and not inflate the view). This gave fatal errors > inflating the xml. Likewise when I changed confirm_import_sync_data.xml to use > 'include layout="@layout/radio_button_with_description"' instead of > 'rg.chromium.ui.widget.RadioButtonWithDescription'. > > What did work was changing the 'merge' to a LinearLayout, then inflating it in > the constructor of RadioButtonWithDescription, but I think this creates a > superfluous layout. Do you mind sending over or pasting the error that you get here? When you use the class name inside the xml file, at what point do you inflate it and get the errors?
Just wanted to weigh in on the RadioButtonWithDescription discussion. https://codereview.chromium.org/1698043006/diff/20001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/widget/RadioButtonWithDescription.java (right): https://codereview.chromium.org/1698043006/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/widget/RadioButtonWithDescription.java:33: init(null); In this situation, because we want RadioButtonWithDescription to be a reusable component, there are two options: 1. The current approach. Use a <merge> tag in radio_button_with_description.xml. Inflate the xml file inside RadioButtonWithDescription's constructor. Then, to use this component in another xml layout, add a <...RadioButtonWithDescription> element. 2. Yusuf's suggestion. Use <...RadioButtonWithDescription> in radio_button_with_description.xml. Skip the inflation step in the constructor. Then, to use this component in another xml layout, use an <include> tag to include radio_button_with_description.xml. Both approaches are used in our codebase. The former option is perhaps slightly better: it encapsulates the RadioButtonWithDescription implementation logic, and doesn't bloat the layouts (whereas each <include> tag actually causes a copy of the included layout to be inlined at compile time). https://codereview.chromium.org/1698043006/diff/40001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/widget/RadioButtonWithDescription.java (right): https://codereview.chromium.org/1698043006/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/widget/RadioButtonWithDescription.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. ui/android is for UI components that will be used by the webpage, e.g. date pickers. Since this is only used in chrome/android, I'd move this into chrome/android/java. One reason for this: webview depends on the ui/ layer, so any code in ui/android will be included in webview. https://codereview.chromium.org/1698043006/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/widget/RadioButtonWithDescription.java:36: public RadioButtonWithDescription(Context context, AttributeSet attrs) { Typically, we only define the constructor that's actually used. This follows the spirit of never writing code "just in case" it's needed in the future: it probably doesn't work and/or will break, and it increases maintenance effort. (If we were providing a standard library of widgets to third-party developers, this would be a totally different story.)
peconn@chromium.org changed reviewers: + gogerald@chromium.org
I added gogerald to find places to add user actions. I decided to stick with the merge approach for RadioButtonWithDescription (and moved it into chrome/android/java). https://codereview.chromium.org/1698043006/diff/20001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/widget/RadioButtonWithDescription.java (right): https://codereview.chromium.org/1698043006/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/widget/RadioButtonWithDescription.java:33: init(null); On 2016/02/19 19:47:13, newt wrote: > In this situation, because we want RadioButtonWithDescription to be a reusable > component, there are two options: > > 1. The current approach. Use a <merge> tag in > radio_button_with_description.xml. Inflate the xml file inside > RadioButtonWithDescription's constructor. Then, to use this component in another > xml layout, add a <...RadioButtonWithDescription> element. > > 2. Yusuf's suggestion. Use <...RadioButtonWithDescription> in > radio_button_with_description.xml. Skip the inflation step in the constructor. > Then, to use this component in another xml layout, use an <include> tag to > include radio_button_with_description.xml. > > Both approaches are used in our codebase. The former option is perhaps slightly > better: it encapsulates the RadioButtonWithDescription implementation logic, and > doesn't bloat the layouts (whereas each <include> tag actually causes a copy of > the included layout to be inlined at compile time). Acknowledged. https://codereview.chromium.org/1698043006/diff/40001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/widget/RadioButtonWithDescription.java (right): https://codereview.chromium.org/1698043006/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/widget/RadioButtonWithDescription.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/02/19 19:47:13, newt wrote: > ui/android is for UI components that will be used by the webpage, e.g. date > pickers. > > Since this is only used in chrome/android, I'd move this into > chrome/android/java. One reason for this: webview depends on the ui/ layer, so > any code in ui/android will be included in webview. Done. https://codereview.chromium.org/1698043006/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/widget/RadioButtonWithDescription.java:36: public RadioButtonWithDescription(Context context, AttributeSet attrs) { On 2016/02/19 19:47:13, newt wrote: > Typically, we only define the constructor that's actually used. This follows the > spirit of never writing code "just in case" it's needed in the future: it > probably doesn't work and/or will break, and it increases maintenance effort. > > (If we were providing a standard library of widgets to third-party developers, > this would be a totally different story.) Done.
https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/res... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/res... chrome/android/java/res/values/dimens.xml:177: <dimen name="signin_button_padding">12dp</dimen> just curious, are the fre_ dimensions still used? If not, we should remove them as well. https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataFragment.java (right): https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataFragment.java:61: public enum ImportSyncType { SWITCHING_SYNC_ACCOUNTS, PREVIOUS_DATA_FOUND } lets push these public fields out of all the privates, so that they are visible. https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataFragment.java:151: javadoc https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/RadioButtonWithDescription.java (right): https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/RadioButtonWithDescription.java:36: private void init(AttributeSet attrs) { I think this can be a part of the above constructor now. https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/RadioButtonWithDescription.java:79: * @param text lets either enter (admittedly mundane) @param descriptions here or leave @param out completely. Similarly for the ones below.
Please also help add "Signin_Declined_From*" as you start in CL 1693383002 for all onAccountSelectionCancelled. The access points can be find in this doc https://docs.google.com/a/google.com/document/d/1-gXYAMXXgsJhk6jxO55RuYJ00JBG... https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunFragment.java (right): https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunFragment.java:79: public void onPreviousAccountFound( This looks should not happen in first run experience, may be DCHECK here. https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunSignInProcessor.java (right): https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunSignInProcessor.java:87: signinManager.signIn(accountName, null, new SignInCallback() { SigninManager depends on |activity| to distinguish interactive and forced sign in, you may need to figure out how to do it right now. https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java (right): https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java:66: public void onDoneClicked() { RecordUserAction.record("Signin_Signin_FromSettings"); https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java:72: mShowSyncSettings = true; RecordUserAction.record("Signin_Signin_FromSettings"); is it by design to sign user in when click setting? what if user abort setting? https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java (right): https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:98: public void onPreviousAccountFound(String oldAccountName, String newAccountName, Showing ConfirmImportSyncDataFragment seems the only way we deal with this conflict, how about show it directly from here? https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:112: mListener.onNewAccount(); RecordUserAction.record("Signin_AddAccountToDevice"); remove this user action in other java files. https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:567: mListener.onNewAccount(); RecordUserAction.record("Signin_AddAccountToDevice"); remove this user action in other java files. https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:580: public void onClick(View v) { RecordUserAction.record("Signin_Undo_Signin"); https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataFragment.java (right): https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataFragment.java:159: if (which != AlertDialog.BUTTON_POSITIVE) return; RecordUserAction.record("Signin_ImportDataPrompt_Cancel"); https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataFragment.java:164: if (mConfirmImportOption.isChecked()) { RecordUserAction.record("Signin_ImportDataPrompt_ImportData"); https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataFragment.java:166: } else { RecordUserAction.record("Signin_ImportDataPrompt_DontImport");
Description was changed from ========== (formerly: Created the dialog offering the user to merge their account data or keep it) This CL contains two changes to the Switching Sync Account flows. - It adds the ability to switch sync accounts, presenting a dialog offering the user to merge their account data or keep it separate. - It upgrades the account signin from preferences from using the ChooseAccountFragment to using the AccountSigninView (formerly the AccountFirstRunView). In addition it: - Renames AccountFirstRunView to AccountSigninView (since it is not used only in the first run). - Renames functions in the AccountSigninView from using SignIn to Signin. BUG=557786 ========== to ========== (formerly: Created the dialog offering the user to merge their account data or keep it) This CL contains two changes to the Switching Sync Account flows. - It adds the ability to switch sync accounts, presenting a dialog offering the user to merge their account data or keep it separate. - It upgrades the account signin from preferences from using the ChooseAccountFragment to using the AccountSigninView (formerly the AccountFirstRunView). In addition it: - Renames AccountFirstRunView to AccountSigninView (since it is not used only in the first run). - Renames functions in the AccountSigninView from using SignIn to Signin. The following user actions were modified: - Signin_AddAccountToDevice - Signin_ImportDataPrompt_Cancel - Signin_ImportDataPrompt_DontImport - Signin_ImportDataPrompt_ImportData - Signin_Signin_FromSettings - Signin_Undo_Signin BUG=557786 ==========
https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/res... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/res... chrome/android/java/res/values/dimens.xml:177: <dimen name="signin_button_padding">12dp</dimen> On 2016/02/22 19:40:09, Yusuf wrote: > just curious, are the fre_ dimensions still used? If not, we should remove them > as well. Yeah, they are all used. I should have a CL in hopefully tomorrow with a redo of the fre screens (because at the moment we have three screens (terms of service, data saver, account sign in) that look similar but actually share very little code so getting them to be laid out the same is difficult. Would you like me to cc you on it? https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunFragment.java (right): https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunFragment.java:79: public void onPreviousAccountFound( On 2016/02/22 23:32:19, gogerald1 wrote: > This looks should not happen in first run experience, may be DCHECK here. Done. https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunSignInProcessor.java (right): https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunSignInProcessor.java:87: signinManager.signIn(accountName, null, new SignInCallback() { On 2016/02/22 23:32:19, gogerald1 wrote: > SigninManager depends on |activity| to distinguish interactive and forced sign > in, you may need to figure out how to do it right now. For the code to get to this point, the user has already gone through the AccountSigninView, so has already been presented with the dialog asking if they want to merge their data or keep it separate (and if they chose to keep it separate, their old data has already been cleared). https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java (right): https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java:66: public void onDoneClicked() { On 2016/02/22 23:32:19, gogerald1 wrote: > RecordUserAction.record("Signin_Signin_FromSettings"); Done. https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java:72: mShowSyncSettings = true; On 2016/02/22 23:32:19, gogerald1 wrote: > RecordUserAction.record("Signin_Signin_FromSettings"); > > is it by design to sign user in when click setting? what if user abort setting? The current design has 'sign in' and 'confirm sign in' screens. The settings option only appears in the 'confirm sign in' screen (as does the done option). The 'confirm sign in' screen makes it appear to the user that they are signed in, so if someone clicks settings they believe they are already signed in. (also, the UI makes it apparent Settings doesn't take you to Chrome Settings, but to Sync Settings, which don't exist unless the user is signed in) https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java (right): https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:98: public void onPreviousAccountFound(String oldAccountName, String newAccountName, On 2016/02/22 23:32:19, gogerald1 wrote: > Showing ConfirmImportSyncDataFragment seems the only way we deal with this > conflict, how about show it directly from here? Unfortunately we need a fragment manager to create the ConfirmImportSyncDataFragment which needs to be provided by whatever contains the AccountSigninView. My original approach (you can see it in Patch Set #2) was to give the AccountSigninView a 'setFragmentManager' function, but it seemed this approach was nicer (less long winded and reduced the number of states the AccountSigninView could be in). https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:112: mListener.onNewAccount(); On 2016/02/22 23:32:19, gogerald1 wrote: > RecordUserAction.record("Signin_AddAccountToDevice"); remove this user action in > other java files. Done. https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:567: mListener.onNewAccount(); On 2016/02/22 23:32:19, gogerald1 wrote: > RecordUserAction.record("Signin_AddAccountToDevice"); remove this user action in > other java files. Done. https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:580: public void onClick(View v) { On 2016/02/22 23:32:19, gogerald1 wrote: > RecordUserAction.record("Signin_Undo_Signin"); Done. https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataFragment.java (right): https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataFragment.java:61: public enum ImportSyncType { SWITCHING_SYNC_ACCOUNTS, PREVIOUS_DATA_FOUND } On 2016/02/22 19:40:09, Yusuf wrote: > lets push these public fields out of all the privates, so that they are visible. Done. https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataFragment.java:151: On 2016/02/22 19:40:09, Yusuf wrote: > javadoc Done. https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataFragment.java:159: if (which != AlertDialog.BUTTON_POSITIVE) return; On 2016/02/22 23:32:19, gogerald1 wrote: > RecordUserAction.record("Signin_ImportDataPrompt_Cancel"); Done. https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataFragment.java:164: if (mConfirmImportOption.isChecked()) { On 2016/02/22 23:32:19, gogerald1 wrote: > RecordUserAction.record("Signin_ImportDataPrompt_ImportData"); Done. https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataFragment.java:166: } else { On 2016/02/22 23:32:19, gogerald1 wrote: > RecordUserAction.record("Signin_ImportDataPrompt_DontImport"); Done. https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/RadioButtonWithDescription.java (right): https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/RadioButtonWithDescription.java:36: private void init(AttributeSet attrs) { On 2016/02/22 19:40:09, Yusuf wrote: > I think this can be a part of the above constructor now. Done. https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/RadioButtonWithDescription.java:79: * @param text On 2016/02/22 19:40:09, Yusuf wrote: > lets either enter (admittedly mundane) @param descriptions here or leave @param > out completely. Similarly for the ones below. Done.
The CQ bit was checked by peconn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1698043006/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1698043006/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: CLs for remote refs other than refs/pending/heads/master must contain NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them
https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunSignInProcessor.java (right): https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunSignInProcessor.java:87: signinManager.signIn(accountName, null, new SignInCallback() { On 2016/02/23 15:01:53, PEConn1 wrote: > On 2016/02/22 23:32:19, gogerald1 wrote: > > SigninManager depends on |activity| to distinguish interactive and forced sign > > in, you may need to figure out how to do it right now. > > For the code to get to this point, the user has already gone through the > AccountSigninView, so has already been presented with the dialog asking if they > want to merge their data or keep it separate (and if they chose to keep it > separate, their old data has already been cleared). Yes, but there are some other things need to be done for interactive sign in only in SigninManager, like managed account confirmation (policy check), histograms and user actions recording. You probably need to move them out from SigninManager to somewhere else. In addition, remove |activity| if it is always supposed to be null and related code in SigninManager. Clean ConfirmAccountChangeFragment related code in SigninManager as well
The CQ bit was checked by peconn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1698043006/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1698043006/100001
Ping - I'm hoping to get this committed by Friday (to get in 50). https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunSignInProcessor.java (right): https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunSignInProcessor.java:87: signinManager.signIn(accountName, null, new SignInCallback() { On 2016/02/23 16:57:19, gogerald1 wrote: > On 2016/02/23 15:01:53, PEConn1 wrote: > > On 2016/02/22 23:32:19, gogerald1 wrote: > > > SigninManager depends on |activity| to distinguish interactive and forced > sign > > > in, you may need to figure out how to do it right now. > > > > For the code to get to this point, the user has already gone through the > > AccountSigninView, so has already been presented with the dialog asking if > they > > want to merge their data or keep it separate (and if they chose to keep it > > separate, their old data has already been cleared). > > Yes, but there are some other things need to be done for interactive sign in > only in SigninManager, like managed account confirmation (policy check), > histograms and user actions recording. You probably need to move them out from > SigninManager to somewhere else. In addition, remove |activity| if it is always > supposed to be null and related code in SigninManager. Clean > ConfirmAccountChangeFragment related code in SigninManager as well Good point. I'll change this back to |activity|. It may be worth splitting out the ConfirmAccountChangeFragment, but it's not in the scope of this CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by peconn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1698043006/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1698043006/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by peconn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1698043006/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1698043006/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with a single nit. Thanks a lot! https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/RadioButtonWithDescription.java (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/RadioButtonWithDescription.java:88: * @return If checked. only @return is sufficient here, no need for the description.
Mostly small comments, and a few suggestions for future changes. The main important comment is about SigninManager.signIn() and "interactive sign-in" -- which happens when you pass a non-null Activity to signIn(), and which pops up dialogs warning about both merging of sync data and signing in to a managed account. It seems like you've removed most (perhaps all?) instances of interactive sign-in, and you've effectively recreated interactive sign-in at a higher level by popping up the ConfirmImportSyncDataDialog when needed. However, the warning about managed accounts seems to have been lost, and IIUC dead interactive sign-in code remains in SigninManager. Why not replace the existing interactive sign-in logic in SigninManager -- which uses the now mostly-obsolete ConfirmAccountChangeFragment -- with the new ConfirmImportSyncDataDialog? In any case, you should be sure to fully migrate to the new solution and delete ConfirmAccountChangeFragment and ClearSyncDataPreferences. Also: fix the first line of the CL description. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:267: android:launchMode = "singleInstance"> Is singleInstance really what we want? This means that the activity will appear in a new task (i.e. in recents it will appear as a separate item from the main Chrome activity) The docs for <activity> also warn: "singleTask and singleInstance — are not appropriate for most applications, since they result in an interaction model that is likely to be unfamiliar to users and is very different from most other applications." nit: remove spaces around = https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/re... File chrome/android/java/res/layout/confirm_import_sync_data.xml (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/re... chrome/android/java/res/layout/confirm_import_sync_data.xml:24: android:fontFamily="sans-serif" not needed https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/re... chrome/android/java/res/layout/confirm_import_sync_data.xml:25: android:layout_gravity="center_horizontal" not needed? https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/re... File chrome/android/java/res/layout/radio_button_with_description.xml (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/re... chrome/android/java/res/layout/radio_button_with_description.xml:7: <merge xmlns:android="http://schemas.android.com/apk/res/android" A RelativeLayout would be better here to reduce layout nesting (then you can remove the inner LinearLayout) https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/re... chrome/android/java/res/layout/radio_button_with_description.xml:15: android:layout_gravity="center_horizontal" does this do anything? https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/re... chrome/android/java/res/layout/radio_button_with_description.xml:27: android:fontFamily="sans-serif" not needed. same below. sans-serif is the default font value. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/re... File chrome/android/java/res/values/attrs.xml (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/re... chrome/android/java/res/values/attrs.xml:61: <attr name="descriptionText" format="string" /> is this used? https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/re... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/re... chrome/android/java/res/values/dimens.xml:170: <dimen name="fre_image_carousel_width">260dp</dimen> Delete this and other now-unused "fre" dimens. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkSigninActivity.java (left): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkSigninActivity.java:130: RecordUserAction.record("Signin_AddAccountToDevice"); why remove this? https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunFragment.java (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunFragment.java:79: public void onPreviousAccountFound( Every time we override onPreviousAccountFound(), the implementation is the same AFAICT, modulo the ImportSyncType value. Perhaps you could replace this method with a simple getter that returns the desired ImportSyncType, and then call ConfirmImportSyncDataFragment.showNewInstance() from shared code. Likewise, the implementations of onNewAccount() and a few other methods could be deduplicated. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsRowAdapter.java (left): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsRowAdapter.java:604: RecordUserAction.record("Signin_AddAccountToDevice"); why remove this? https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java (left): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java:214: SigninManager.logSigninStartAccessPoint(SigninAccessPoint.SETTINGS); Do we still call this somewhere? https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java:92: if (SigninManager.get(getActivity()).isSigninDisabledByPolicy()) { this if block should be nested inside the isSignInAllowed() if block above https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java:39: protected void onCreate(Bundle savedInstanceState) { We need to make sure the native library is loaded here to handle this scenario: 1. User opens Chrome, and launches this activity 2. User goes to home screen and uses other apps for a while 3. User goes back to Chrome via Android's recent apps list In that case, in step 3, Android will recreate this activity (but not the activities "underneath" it), and the native library be loaded... which would lead to a crash. See Preferences.onCreate() for an example of how to load the native library synchronously. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java:69: SigninManager.get(this).signIn(mAccountName, null, this); We're using "non-interactive" sign-in here, which means that the managed account warning will be bypassed (see SigninManager.signIn()). That doesn't seem right. Is that intentional? https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:29: import org.chromium.chrome.browser.firstrun.ImageCarousel; ImageCarousel and perhaps ProfileDataCache should also be moved to the signin package. Could be a separate CL since this one is already large. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:524: if (SigninInvestigator.investigate(mAccountName) == InvestigatedScenario.DIFFERENT_ACCOUNT Looks like we're duplicating some logic from SigninManager.progressSignInFlowInvestigateScenario() here. Does this logic need to live in both places now? Why move the logic up to this higher level, instead of keeping it inside of SigninManager? https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:531: new ConfirmImportSyncDataFragment.Listener(){ nit: space before { https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/SigninPromoScreen.java (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/SigninPromoScreen.java:124: SigninManager.get(activity).signIn(mAccountName, null, new SignInCallback() { Why pass null now? Do we ever need "interactive" sign-in via SigninManager.signIn() now, or can SigninManager be updated? https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/sync/SyncAccountSwitcher.java (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/sync/SyncAccountSwitcher.java:39: public SyncAccountSwitcher(Context c, FragmentManager fm, SyncedAccountPreference pref) { javadoc https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/sync/SyncAccountSwitcher.java:41: mContext = c; avoid abbreviations, like "c" and "fm" https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataFragment.java (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataFragment.java:32: public class ConfirmImportSyncDataFragment extends DialogFragment suggestion: I think "ConfirmImportSyncDataDialog" is a more helpful name than "ConfirmImportSyncDataFragment" (although both are accurate in some sense). https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataFragment.java:151: .setTitle(getActivity().getString(R.string.sync_import_data_title)) setTitle(), etc have overrides that take a string resource ID: .setTitle(R.string.sync_import_data_title) .setPositiveButton(R.string.cont, this) etc https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataFragment.java:178: ChromeBrowserProviderClient.removeAllUserBookmarks(getActivity()); Does calling this on the UI thread work? I'm just wondering because of all the complexity in ClearSyncDataPreferences.java when calling this same method: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... (In the nearish future, ChromeBrowserProviderClient.removeAllUserBookmarks() will be replaced with a method that definitely does work on the UI thread, via BookmarkBridge.) https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java:285: * Update the state of all settings from sync. This sets the state of the sync switch from Revert the javadoc reformatting changes in this file. They don't look intentional anyway. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/RadioButtonWithDescription.java (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/RadioButtonWithDescription.java:31: public RadioButtonWithDescription(Context context, AttributeSet attrs) { javadoc https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/RadioButtonWithDescription.java:50: try { Where did this try/finally pattern come from for recycling the TypedArray? We don't typically use this pattern, since an exception here will likely cause Chrome to halt, and since it's not mandatory to recycle the TypedArray, just recommended to reduce garbage collection. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:173: <message name="IDS_CONT" desc="Generic label for a continue prompt. Used in multiple contexts. [CHAR-LIMIT=20]"> "... for a button to continue to the next screen" (It's important to mention "button" so translators know this is an action, not a description or label) https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:173: <message name="IDS_CONT" desc="Generic label for a continue prompt. Used in multiple contexts. [CHAR-LIMIT=20]"> s/CONT/CONTINUE/ As a rule, avoid abbreviations (except for a few very common unambiguous abbreviations for long words, e.g. pref instead of preferences) https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:1091: import_existing_data Store this constant in Java. (Unless this needs to be used from a resource file) It's better not to store string constants in grd files unless it's necessary. Loading resources is slower than accessing a string constant, resources are global while constants can have limited scope, and it's just more indirection and code.
The reason that ConfirmImportSyncDataFragment was called separately from the 4 different classes wasn't to do with the ImportSyncType, but because it needed a FragmentManager (which the View did not have access to). I decided to add a Delegate for each caller to use to provide the FragmentManager. I think that moving the managed account strings to the AccountSigninView would be a good idea, but beyond the scope of this CL. I modified SigninManager so that it doesn't check for previous accounts (and deleted ConfirmAccountChangeFragment), since that check are now done by the AccountSigninView, but I have left the managed account logic in. In the SigninManager, I removed progressSignInFlowInvestigateScenario, fast forwarding straight to progressSignInFlowCheckPolicy. I moved the clearing bookmarks to the background thread - I originally had tried to extract similar code between ClearSyncDataPreferences and ConfirmImportSyncDataFragment, but there was more different. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:267: android:launchMode = "singleInstance"> On 2016/02/25 06:25:52, newt wrote: > Is singleInstance really what we want? This means that the activity will appear > in a new task (i.e. in recents it will appear as a separate item from the main > Chrome activity) > > The docs for <activity> also warn: "singleTask and singleInstance — are not > appropriate for most applications, since they result in an interaction model > that is likely to be unfamiliar to users and is very different from most other > applications." > > nit: remove spaces around = Done. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/re... File chrome/android/java/res/layout/confirm_import_sync_data.xml (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/re... chrome/android/java/res/layout/confirm_import_sync_data.xml:24: android:fontFamily="sans-serif" On 2016/02/25 06:25:52, newt wrote: > not needed Done. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/re... chrome/android/java/res/layout/confirm_import_sync_data.xml:25: android:layout_gravity="center_horizontal" On 2016/02/25 06:25:52, newt wrote: > not needed? Done. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/re... File chrome/android/java/res/layout/radio_button_with_description.xml (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/re... chrome/android/java/res/layout/radio_button_with_description.xml:7: <merge xmlns:android="http://schemas.android.com/apk/res/android" On 2016/02/25 06:25:52, newt wrote: > A RelativeLayout would be better here to reduce layout nesting (then you can > remove the inner LinearLayout) Done. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/re... chrome/android/java/res/layout/radio_button_with_description.xml:15: android:layout_gravity="center_horizontal" On 2016/02/25 06:25:52, newt wrote: > does this do anything? Done. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/re... chrome/android/java/res/layout/radio_button_with_description.xml:27: android:fontFamily="sans-serif" On 2016/02/25 06:25:52, newt wrote: > not needed. same below. sans-serif is the default font value. Done. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/re... File chrome/android/java/res/values/attrs.xml (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/re... chrome/android/java/res/values/attrs.xml:61: <attr name="descriptionText" format="string" /> On 2016/02/25 06:25:52, newt wrote: > is this used? Acknowledged. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/re... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/re... chrome/android/java/res/values/dimens.xml:170: <dimen name="fre_image_carousel_width">260dp</dimen> On 2016/02/25 06:25:53, newt wrote: > Delete this and other now-unused "fre" dimens. Done. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkSigninActivity.java (left): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkSigninActivity.java:130: RecordUserAction.record("Signin_AddAccountToDevice"); On 2016/02/25 06:25:53, newt wrote: > why remove this? At the request of gogerald@ . https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunFragment.java (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunFragment.java:79: public void onPreviousAccountFound( On 2016/02/25 06:25:53, newt wrote: > Every time we override onPreviousAccountFound(), the implementation is the same > AFAICT, modulo the ImportSyncType value. Perhaps you could replace this method > with a simple getter that returns the desired ImportSyncType, and then call > ConfirmImportSyncDataFragment.showNewInstance() from shared code. > > Likewise, the implementations of onNewAccount() and a few other methods could be > deduplicated. Done. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsRowAdapter.java (left): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsRowAdapter.java:604: RecordUserAction.record("Signin_AddAccountToDevice"); On 2016/02/25 06:25:53, newt wrote: > why remove this? At the request of @gogerald . https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java (left): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java:214: SigninManager.logSigninStartAccessPoint(SigninAccessPoint.SETTINGS); On 2016/02/25 06:25:53, newt wrote: > Do we still call this somewhere? It's called when signing in through the Recent Tabs, Signin Promo and the first run activity. I put this call back in the OnPreferenceClickListener above. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java:92: if (SigninManager.get(getActivity()).isSigninDisabledByPolicy()) { On 2016/02/25 06:25:53, newt wrote: > this if block should be nested inside the isSignInAllowed() if block above Done. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java:39: protected void onCreate(Bundle savedInstanceState) { On 2016/02/25 06:25:53, newt wrote: > We need to make sure the native library is loaded here to handle this scenario: > 1. User opens Chrome, and launches this activity > 2. User goes to home screen and uses other apps for a while > 3. User goes back to Chrome via Android's recent apps list > > In that case, in step 3, Android will recreate this activity (but not the > activities "underneath" it), and the native library be loaded... which would > lead to a crash. > > See Preferences.onCreate() for an example of how to load the native library > synchronously. Done. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java:69: SigninManager.get(this).signIn(mAccountName, null, this); On 2016/02/25 06:25:53, newt wrote: > We're using "non-interactive" sign-in here, which means that the managed account > warning will be bypassed (see SigninManager.signIn()). That doesn't seem right. > Is that intentional? I changed all of these back. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:29: import org.chromium.chrome.browser.firstrun.ImageCarousel; On 2016/02/25 06:25:53, newt wrote: > ImageCarousel and perhaps ProfileDataCache should also be moved to the signin > package. Could be a separate CL since this one is already large. Acknowledged. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:524: if (SigninInvestigator.investigate(mAccountName) == InvestigatedScenario.DIFFERENT_ACCOUNT On 2016/02/25 06:25:53, newt wrote: > Looks like we're duplicating some logic from > SigninManager.progressSignInFlowInvestigateScenario() here. Does this logic need > to live in both places now? Why move the logic up to this higher level, instead > of keeping it inside of SigninManager? Done. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:531: new ConfirmImportSyncDataFragment.Listener(){ On 2016/02/25 06:25:53, newt wrote: > nit: space before { Done. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/SigninPromoScreen.java (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/SigninPromoScreen.java:124: SigninManager.get(activity).signIn(mAccountName, null, new SignInCallback() { On 2016/02/25 06:25:53, newt wrote: > Why pass null now? Do we ever need "interactive" sign-in via > SigninManager.signIn() now, or can SigninManager be updated? Acknowledged. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/sync/SyncAccountSwitcher.java (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/sync/SyncAccountSwitcher.java:39: public SyncAccountSwitcher(Context c, FragmentManager fm, SyncedAccountPreference pref) { On 2016/02/25 06:25:53, newt wrote: > javadoc Done. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/sync/SyncAccountSwitcher.java:41: mContext = c; On 2016/02/25 06:25:53, newt wrote: > avoid abbreviations, like "c" and "fm" Done. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataFragment.java (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataFragment.java:32: public class ConfirmImportSyncDataFragment extends DialogFragment On 2016/02/25 06:25:53, newt wrote: > suggestion: I think "ConfirmImportSyncDataDialog" is a more helpful name than > "ConfirmImportSyncDataFragment" (although both are accurate in some sense). Done. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataFragment.java:151: .setTitle(getActivity().getString(R.string.sync_import_data_title)) On 2016/02/25 06:25:53, newt wrote: > setTitle(), etc have overrides that take a string resource ID: > > .setTitle(R.string.sync_import_data_title) > .setPositiveButton(R.string.cont, this) > etc Done. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataFragment.java:178: ChromeBrowserProviderClient.removeAllUserBookmarks(getActivity()); On 2016/02/25 06:25:53, newt wrote: > Does calling this on the UI thread work? > > I'm just wondering because of all the complexity in > ClearSyncDataPreferences.java when calling this same method: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... > > (In the nearish future, ChromeBrowserProviderClient.removeAllUserBookmarks() > will be replaced with a method that definitely does work on the UI thread, via > BookmarkBridge.) Done. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java:285: * Update the state of all settings from sync. This sets the state of the sync switch from On 2016/02/25 06:25:53, newt wrote: > Revert the javadoc reformatting changes in this file. They don't look > intentional anyway. Done. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/RadioButtonWithDescription.java (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/RadioButtonWithDescription.java:31: public RadioButtonWithDescription(Context context, AttributeSet attrs) { On 2016/02/25 06:25:53, newt wrote: > javadoc Done. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/RadioButtonWithDescription.java:50: try { On 2016/02/25 06:25:53, newt wrote: > Where did this try/finally pattern come from for recycling the TypedArray? We > don't typically use this pattern, since an exception here will likely cause > Chrome to halt, and since it's not mandatory to recycle the TypedArray, just > recommended to reduce garbage collection. Done. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/RadioButtonWithDescription.java:88: * @return If checked. On 2016/02/24 16:41:40, Yusuf wrote: > only @return is sufficient here, no need for the description. Done. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:173: <message name="IDS_CONT" desc="Generic label for a continue prompt. Used in multiple contexts. [CHAR-LIMIT=20]"> On 2016/02/25 06:25:53, newt wrote: > s/CONT/CONTINUE/ As a rule, avoid abbreviations (except for a few very common > unambiguous abbreviations for long words, e.g. pref instead of preferences) So this is used in the java code as R.string.cont and since continue is a keyword, R.string.continue doesn't work. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:173: <message name="IDS_CONT" desc="Generic label for a continue prompt. Used in multiple contexts. [CHAR-LIMIT=20]"> On 2016/02/25 06:25:53, newt wrote: > "... for a button to continue to the next screen" > > (It's important to mention "button" so translators know this is an action, not a > description or label) Done. https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:1091: import_existing_data On 2016/02/25 06:25:53, newt wrote: > Store this constant in Java. (Unless this needs to be used from a resource file) > > It's better not to store string constants in grd files unless it's necessary. > Loading resources is slower than accessing a string constant, resources are > global while constants can have limited scope, and it's just more indirection > and code. I'm removing this, as well as IDS_SYNC_KEEP_EXISTING_DATA_SEPARATE_VALUE below (which I can't find mentioned anywhere else in the code).
The CQ bit was checked by peconn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1698043006/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1698043006/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:266: android:theme="@style/MainTheme"> Nit: Indent two more spaces. https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsPromoView.java (right): https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsPromoView.java:297: signInPromoView.setDelegate(new AccountSigninView.Delegate(){ Nit: space before opening brace. https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java (right): https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:568: mPositiveButton.setOnClickListener(new OnClickListener(){ Space before opening brace. https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataDialog.java (right): https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataDialog.java:102: ConfirmImportSyncDataDialog confirmSync = ConfirmImportSyncDataDialog.newInstance( Nit: You don't need the class name to call newInstance(). https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:1082: You are signing in to <ph name="TO_ACCOUNT">%1$s<ex>johndoe@new.com</ex></ph>. Some of your existing bookmarks, history, passwords, and other settings were previously synced to <ph name="FROM_ACCOUNT">%2$s<ex>johndoe@old.com</ex></ph>. What do you want to do with your existing data? This still uses "you are", but the one above uses "you're". Should we unify these?
x-a-x@hotmail.co.th changed reviewers: + x-a-x@hotmail.co.th
peconn@chromium.org changed reviewers: + ewald@chromium.org
lgtm after comments. As a next step, I think we should unify the "interactive" sign-in flow so that it's all handled in the same place (rather than handling the sync data dialog in AccountFirstRunView and the managed account dialog in SigninManager). I'm not convinced that AccountFirstRunView is the right place for these dialogs, since that tightly couples the UI for signing in with the dialogs that are shown once the sign-in flow begins. But that can be decided later. https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/re... File chrome/android/java/res/layout/confirm_import_sync_data.xml (right): https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/confirm_import_sync_data.xml:31: android:layout_gravity="center_horizontal" center_horizontal isn't needed here or below https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/re... File chrome/android/java/res/layout/radio_button_with_description.xml (right): https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/radio_button_with_description.xml:8: android:orientation="horizontal" remove this https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/re... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/re... chrome/android/java/res/values/dimens.xml:175: <dimen name="signin_button_padding">12dp</dimen> I don't think it's worth duplicating these dimens. If they're duplicated, they can get out of sync with the fre dimens. I'd just use the fre dimens with the understanding that the sign in view can appear outside of FRE, but needs to match the FRE style. https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java (right): https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:535: if (SigninManager.shouldShowDataSyncDialog(mAccountName)) { s/DataSync/SyncData/ ? https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java (right): https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java:227: public static boolean shouldShowDataSyncDialog(String newAccountName) { Now that this the SigninInvestigator logic has been otherwise removed from SigninManager, you could just inline this code in AccountFirstRunView. https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/sync/SyncAccountSwitcher.java (right): https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/sync/SyncAccountSwitcher.java:72: SigninManager.get(mContext).signIn(mNewAccountName, null, this); we want to pass non-null here to get interactive sign-in, right? https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataDialog.java (right): https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataDialog.java:195: PrefServiceBridge.getInstance().clearBrowsingData(listener, SYNC_DATA_TYPES); Do we need to call SigninManager.get(mApplicationContext).clearLastSignedInUser(), like we used to do in ClearSyncDataPreferences.java? If not, let's delete that method. https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:173: <message name="IDS_CONT" desc="Generic label for a button to continue to the next screen. Used in multiple contexts. [CHAR-LIMIT=20]"> Ah, since we can't we "IDS_CONTINUE", how about "IDS_CONTINUE_BUTTON"?
The CQ bit was checked by peconn@chromium.org to run a CQ dry run
Yeah, it would be nice to get everything separated out logically. I think the issue is that though we want to keep the UI and the logic separate, they'll need to progress in the same order. https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:266: android:theme="@style/MainTheme"> On 2016/02/25 14:52:43, Bernhard Bauer wrote: > Nit: Indent two more spaces. Done. https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/re... File chrome/android/java/res/layout/confirm_import_sync_data.xml (right): https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/confirm_import_sync_data.xml:31: android:layout_gravity="center_horizontal" On 2016/02/25 18:01:32, newt wrote: > center_horizontal isn't needed here or below Done. https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/re... File chrome/android/java/res/layout/radio_button_with_description.xml (right): https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/radio_button_with_description.xml:8: android:orientation="horizontal" On 2016/02/25 18:01:32, newt wrote: > remove this Done. https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/re... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/re... chrome/android/java/res/values/dimens.xml:175: <dimen name="signin_button_padding">12dp</dimen> On 2016/02/25 18:01:32, newt wrote: > I don't think it's worth duplicating these dimens. If they're duplicated, they > can get out of sync with the fre dimens. I'd just use the fre dimens with the > understanding that the sign in view can appear outside of FRE, but needs to > match the FRE style. Done. https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsPromoView.java (right): https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsPromoView.java:297: signInPromoView.setDelegate(new AccountSigninView.Delegate(){ On 2016/02/25 14:52:43, Bernhard Bauer wrote: > Nit: space before opening brace. Done. https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java (right): https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:535: if (SigninManager.shouldShowDataSyncDialog(mAccountName)) { On 2016/02/25 18:01:32, newt wrote: > s/DataSync/SyncData/ ? Acknowledged. https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:568: mPositiveButton.setOnClickListener(new OnClickListener(){ On 2016/02/25 14:52:43, Bernhard Bauer wrote: > Space before opening brace. Done. https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java (right): https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java:227: public static boolean shouldShowDataSyncDialog(String newAccountName) { On 2016/02/25 18:01:32, newt wrote: > Now that this the SigninInvestigator logic has been otherwise removed from > SigninManager, you could just inline this code in AccountFirstRunView. Done. https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/sync/SyncAccountSwitcher.java (right): https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/sync/SyncAccountSwitcher.java:72: SigninManager.get(mContext).signIn(mNewAccountName, null, this); On 2016/02/25 18:01:32, newt wrote: > we want to pass non-null here to get interactive sign-in, right? Done. https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataDialog.java (right): https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataDialog.java:102: ConfirmImportSyncDataDialog confirmSync = ConfirmImportSyncDataDialog.newInstance( On 2016/02/25 14:52:43, Bernhard Bauer wrote: > Nit: You don't need the class name to call newInstance(). Done. https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataDialog.java:195: PrefServiceBridge.getInstance().clearBrowsingData(listener, SYNC_DATA_TYPES); On 2016/02/25 18:01:32, newt wrote: > Do we need to call > SigninManager.get(mApplicationContext).clearLastSignedInUser(), like we used to > do in ClearSyncDataPreferences.java? If not, let's delete that method. I had omitted it assuming that it would get overwritten when the next account is signed in, but there are ways to go through this dialog and then not sign in. https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:173: <message name="IDS_CONT" desc="Generic label for a button to continue to the next screen. Used in multiple contexts. [CHAR-LIMIT=20]"> On 2016/02/25 18:01:32, newt wrote: > Ah, since we can't we "IDS_CONTINUE", how about "IDS_CONTINUE_BUTTON"? Done. https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:1082: You are signing in to <ph name="TO_ACCOUNT">%1$s<ex>johndoe@new.com</ex></ph>. Some of your existing bookmarks, history, passwords, and other settings were previously synced to <ph name="FROM_ACCOUNT">%2$s<ex>johndoe@old.com</ex></ph>. What do you want to do with your existing data? On 2016/02/25 14:52:43, Bernhard Bauer wrote: > This still uses "you are", but the one above uses "you're". Should we unify > these? I passed this on to Eli.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1698043006/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1698043006/180001
The CQ bit was checked by peconn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1698043006/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1698043006/200001
On 2016/02/25 20:45:12, PEConn1 wrote: > Yeah, it would be nice to get everything separated out logically. I think the > issue is that though we want to keep the UI and the logic separate, they'll need > to progress in the same order. Perhaps SigninManager could have a method call "signInInteractively()" that takes a delegate with methods to show the confirm import data dialog and the managed account dialog. That could be a nice way to separate out responsibilities.
The CQ bit was unchecked by peconn@chromium.org
The CQ bit was checked by peconn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org, newt@chromium.org Link to the patchset: https://codereview.chromium.org/1698043006/#ps200001 (title: " ")
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by peconn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org, yusufo@chromium.org Link to the patchset: https://codereview.chromium.org/1698043006/#ps220001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1698043006/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1698043006/220001
On 2016/02/25 20:45:12, PEConn1 wrote: > Yeah, it would be nice to get everything separated out logically. I think the > issue is that though we want to keep the UI and the logic separate, they'll need > to progress in the same order. > > https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/An... > File chrome/android/java/AndroidManifest.xml (right): > > https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/An... > chrome/android/java/AndroidManifest.xml:266: android:theme="@style/MainTheme"> > On 2016/02/25 14:52:43, Bernhard Bauer wrote: > > Nit: Indent two more spaces. > > Done. > > https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/re... > File chrome/android/java/res/layout/confirm_import_sync_data.xml (right): > > https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/re... > chrome/android/java/res/layout/confirm_import_sync_data.xml:31: > android:layout_gravity="center_horizontal" > On 2016/02/25 18:01:32, newt wrote: > > center_horizontal isn't needed here or below > > Done. > > https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/re... > File chrome/android/java/res/layout/radio_button_with_description.xml (right): > > https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/re... > chrome/android/java/res/layout/radio_button_with_description.xml:8: > android:orientation="horizontal" > On 2016/02/25 18:01:32, newt wrote: > > remove this > > Done. > > https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/re... > File chrome/android/java/res/values/dimens.xml (right): > > https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/re... > chrome/android/java/res/values/dimens.xml:175: <dimen > name="signin_button_padding">12dp</dimen> > On 2016/02/25 18:01:32, newt wrote: > > I don't think it's worth duplicating these dimens. If they're duplicated, they > > can get out of sync with the fre dimens. I'd just use the fre dimens with the > > understanding that the sign in view can appear outside of FRE, but needs to > > match the FRE style. > > Done. > > https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsPromoView.java > (right): > > https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsPromoView.java:297: > signInPromoView.setDelegate(new AccountSigninView.Delegate(){ > On 2016/02/25 14:52:43, Bernhard Bauer wrote: > > Nit: space before opening brace. > > Done. > > https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java > (right): > > https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:535: > if (SigninManager.shouldShowDataSyncDialog(mAccountName)) { > On 2016/02/25 18:01:32, newt wrote: > > s/DataSync/SyncData/ ? > > Acknowledged. > > https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:568: > mPositiveButton.setOnClickListener(new OnClickListener(){ > On 2016/02/25 14:52:43, Bernhard Bauer wrote: > > Space before opening brace. > > Done. > > https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java > (right): > > https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java:227: > public static boolean shouldShowDataSyncDialog(String newAccountName) { > On 2016/02/25 18:01:32, newt wrote: > > Now that this the SigninInvestigator logic has been otherwise removed from > > SigninManager, you could just inline this code in AccountFirstRunView. > > Done. > > https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/sync/SyncAccountSwitcher.java > (right): > > https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/sync/SyncAccountSwitcher.java:72: > SigninManager.get(mContext).signIn(mNewAccountName, null, this); > On 2016/02/25 18:01:32, newt wrote: > > we want to pass non-null here to get interactive sign-in, right? > > Done. > > https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataDialog.java > (right): > > https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataDialog.java:102: > ConfirmImportSyncDataDialog confirmSync = > ConfirmImportSyncDataDialog.newInstance( > On 2016/02/25 14:52:43, Bernhard Bauer wrote: > > Nit: You don't need the class name to call newInstance(). > > Done. > > https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataDialog.java:195: > PrefServiceBridge.getInstance().clearBrowsingData(listener, SYNC_DATA_TYPES); > On 2016/02/25 18:01:32, newt wrote: > > Do we need to call > > SigninManager.get(mApplicationContext).clearLastSignedInUser(), like we used > to > > do in ClearSyncDataPreferences.java? If not, let's delete that method. > > I had omitted it assuming that it would get overwritten when the next account is > signed in, but there are ways to go through this dialog and then not sign in. > > https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/st... > File chrome/android/java/strings/android_chrome_strings.grd (right): > > https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/st... > chrome/android/java/strings/android_chrome_strings.grd:173: <message > name="IDS_CONT" desc="Generic label for a button to continue to the next screen. > Used in multiple contexts. [CHAR-LIMIT=20]"> > On 2016/02/25 18:01:32, newt wrote: > > Ah, since we can't we "IDS_CONTINUE", how about "IDS_CONTINUE_BUTTON"? > > Done. > > https://codereview.chromium.org/1698043006/diff/160001/chrome/android/java/st... > chrome/android/java/strings/android_chrome_strings.grd:1082: You are signing in > to <ph mailto:name="TO_ACCOUNT">%1$s<ex>johndoe@new.com</ex></ph>. Some of your > existing bookmarks, history, passwords, and other settings were previously > synced to <ph mailto:name="FROM_ACCOUNT">%2$s<ex>johndoe@old.com</ex></ph>. What do you > want to do with your existing data? > On 2016/02/25 14:52:43, Bernhard Bauer wrote: > > This still uses "you are", but the one above uses "you're". Should we unify > > these? > > I passed this on to Eli. Let's use "you're" in both places.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by peconn@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1698043006/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1698043006/220001
https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunSignInProcessor.java (right): https://codereview.chromium.org/1698043006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunSignInProcessor.java:87: signinManager.signIn(accountName, null, new SignInCallback() { On 2016/02/24 11:37:51, PEConn1 wrote: > On 2016/02/23 16:57:19, gogerald1 wrote: > > On 2016/02/23 15:01:53, PEConn1 wrote: > > > On 2016/02/22 23:32:19, gogerald1 wrote: > > > > SigninManager depends on |activity| to distinguish interactive and forced > > sign > > > > in, you may need to figure out how to do it right now. > > > > > > For the code to get to this point, the user has already gone through the > > > AccountSigninView, so has already been presented with the dialog asking if > > they > > > want to merge their data or keep it separate (and if they chose to keep it > > > separate, their old data has already been cleared). > > > > Yes, but there are some other things need to be done for interactive sign in > > only in SigninManager, like managed account confirmation (policy check), > > histograms and user actions recording. You probably need to move them out from > > SigninManager to somewhere else. In addition, remove |activity| if it is > always > > supposed to be null and related code in SigninManager. Clean > > ConfirmAccountChangeFragment related code in SigninManager as well > > Good point. I'll change this back to |activity|. > > It may be worth splitting out the ConfirmAccountChangeFragment, but it's not in > the scope of this CL. This is not the only place needs to be taken care of (all interactive sign in). https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java (right): https://codereview.chromium.org/1698043006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:524: if (SigninInvestigator.investigate(mAccountName) == InvestigatedScenario.DIFFERENT_ACCOUNT Do not check it again in SigninManager https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... https://codereview.chromium.org/1698043006/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunFragment.java (right): https://codereview.chromium.org/1698043006/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunFragment.java:32: R.layout.account_signin_view, container, false); Please RecordUserAction.record("Signin_Impression_FromStartPage"); https://codereview.chromium.org/1698043006/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunFragment.java:43: getPageDelegate().refuseSignIn(); Please RecordUserAction.record("Signin_Declined_FromStartPage"); https://codereview.chromium.org/1698043006/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java (right): https://codereview.chromium.org/1698043006/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java:66: public void onAccountSelectionCanceled() { Please RecordUserAction.record("Signin_Declined_FromSettings");
Message was sent while issue was closed.
Description was changed from ========== (formerly: Created the dialog offering the user to merge their account data or keep it) This CL contains two changes to the Switching Sync Account flows. - It adds the ability to switch sync accounts, presenting a dialog offering the user to merge their account data or keep it separate. - It upgrades the account signin from preferences from using the ChooseAccountFragment to using the AccountSigninView (formerly the AccountFirstRunView). In addition it: - Renames AccountFirstRunView to AccountSigninView (since it is not used only in the first run). - Renames functions in the AccountSigninView from using SignIn to Signin. The following user actions were modified: - Signin_AddAccountToDevice - Signin_ImportDataPrompt_Cancel - Signin_ImportDataPrompt_DontImport - Signin_ImportDataPrompt_ImportData - Signin_Signin_FromSettings - Signin_Undo_Signin BUG=557786 ========== to ========== (formerly: Created the dialog offering the user to merge their account data or keep it) This CL contains two changes to the Switching Sync Account flows. - It adds the ability to switch sync accounts, presenting a dialog offering the user to merge their account data or keep it separate. - It upgrades the account signin from preferences from using the ChooseAccountFragment to using the AccountSigninView (formerly the AccountFirstRunView). In addition it: - Renames AccountFirstRunView to AccountSigninView (since it is not used only in the first run). - Renames functions in the AccountSigninView from using SignIn to Signin. The following user actions were modified: - Signin_AddAccountToDevice - Signin_ImportDataPrompt_Cancel - Signin_ImportDataPrompt_DontImport - Signin_ImportDataPrompt_ImportData - Signin_Signin_FromSettings - Signin_Undo_Signin BUG=557786 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== (formerly: Created the dialog offering the user to merge their account data or keep it) This CL contains two changes to the Switching Sync Account flows. - It adds the ability to switch sync accounts, presenting a dialog offering the user to merge their account data or keep it separate. - It upgrades the account signin from preferences from using the ChooseAccountFragment to using the AccountSigninView (formerly the AccountFirstRunView). In addition it: - Renames AccountFirstRunView to AccountSigninView (since it is not used only in the first run). - Renames functions in the AccountSigninView from using SignIn to Signin. The following user actions were modified: - Signin_AddAccountToDevice - Signin_ImportDataPrompt_Cancel - Signin_ImportDataPrompt_DontImport - Signin_ImportDataPrompt_ImportData - Signin_Signin_FromSettings - Signin_Undo_Signin BUG=557786 ========== to ========== (formerly: Created the dialog offering the user to merge their account data or keep it) This CL contains two changes to the Switching Sync Account flows. - It adds the ability to switch sync accounts, presenting a dialog offering the user to merge their account data or keep it separate. - It upgrades the account signin from preferences from using the ChooseAccountFragment to using the AccountSigninView (formerly the AccountFirstRunView). In addition it: - Renames AccountFirstRunView to AccountSigninView (since it is not used only in the first run). - Renames functions in the AccountSigninView from using SignIn to Signin. The following user actions were modified: - Signin_AddAccountToDevice - Signin_ImportDataPrompt_Cancel - Signin_ImportDataPrompt_DontImport - Signin_ImportDataPrompt_ImportData - Signin_Signin_FromSettings - Signin_Undo_Signin BUG=557786 Committed: https://crrev.com/bd705aeab0594a1790478e335edee728f32fb026 Cr-Commit-Position: refs/heads/master@{#377769} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/bd705aeab0594a1790478e335edee728f32fb026 Cr-Commit-Position: refs/heads/master@{#377769}
Message was sent while issue was closed.
skym@chromium.org changed reviewers: + skym@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1698043006/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java (left): https://codereview.chromium.org/1698043006/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java:398: if (SigninInvestigator.investigate(mSignInState.account.name) I'm concerned that by changing the ordering of the signin flow, you've introduced an error. The signin investigator logic uses the account id/account name mapping that is updated by the account seeding step. If you don't wait for account seeding to complete, then your data is potentially invalid/stale. https://codereview.chromium.org/1698043006/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataDialog.java (right): https://codereview.chromium.org/1698043006/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/ConfirmImportSyncDataDialog.java:205: mListener.onConfirm(); Shouldn't you have a null check before you do this?
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:220001) has been created in https://codereview.chromium.org/1745563002/ by aelias@chromium.org. The reason for reverting is: Causes the downstream test failure http://crbug.com/590238 in "testConsumerSignin", blocking the Clank roll. junit.framework.AssertionFailedError: Could not locate the fragment with tag 'account_picker_dialog_tag' BUG=590238.
Message was sent while issue was closed.
https://codereview.chromium.org/1698043006/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java (left): https://codereview.chromium.org/1698043006/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java:398: if (SigninInvestigator.investigate(mSignInState.account.name) On 2016/02/26 20:22:57, skym wrote: > I'm concerned that by changing the ordering of the signin flow, you've > introduced an error. The signin investigator logic uses the account id/account > name mapping that is updated by the account seeding step. If you don't wait for > account seeding to complete, then your data is potentially invalid/stale. Can you expand on this? I can see that the signin investigator relies on prefs::kGoogleServicesLastUsername and prefs::kGoogleServicesLastAccountId, but I had assumed they wouldn't need updating during this flow.
Message was sent while issue was closed.
https://codereview.chromium.org/1698043006/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java (left): https://codereview.chromium.org/1698043006/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java:398: if (SigninInvestigator.investigate(mSignInState.account.name) On 2016/03/03 18:23:25, PEConn1 wrote: > On 2016/02/26 20:22:57, skym wrote: > > I'm concerned that by changing the ordering of the signin flow, you've > > introduced an error. The signin investigator logic uses the account id/account > > name mapping that is updated by the account seeding step. If you don't wait > for > > account seeding to complete, then your data is potentially invalid/stale. > > Can you expand on this? I can see that the signin investigator relies on > prefs::kGoogleServicesLastUsername and prefs::kGoogleServicesLastAccountId, but > I had assumed they wouldn't need updating during this flow. Sorry for not being very clear. The problem isn't the prefs, they don't involve lookups. The problem is that SigninInvestigatorAndroid uses the AccountTrackerService to get the GAIA id from the email for the account that's currently being used to sign in. If we haven't ever seeded for this account, then we fallback to email comparisons which could be wrong. If we haven't seeded since this account's email address was changed, we potentially display the wrong email in the warning message. If we haven't seeded since this account's email address was change AND we don't have a last-signed-in-obfuscated-gaia-id (legacy), our fallback to email comparison will use the wrong email. |
