Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2772203004:
Add progress and timeout dialogs for getting account management policy (Closed)
DescriptionAdd progress and timeout dialogs for getting account management policy
This CL adds two dialogs that are shown by ConfirmSyncDataStateMachine.
Dialogs implementation is in ConfirmSyncDataStateMachineDelegate.
The first dialog is a progress dialog that is shown while Chrome requests
server for account management policy.
The second dialog is a timeout dialog that is shown if Chrome doesn't receive
account management policy within 30 seconds.
BUG=650121
Review-Url: https://codereview.chromium.org/2772203004
Cr-Commit-Position: refs/heads/master@{#460471}
Committed: https://chromium.googlesource.com/chromium/src/+/33295fdb5fa26e409b197b81b2069c435184d6e3
Patch Set 1 #
Total comments: 32
Patch Set 2 : Addressed comments #
Total comments: 9
Patch Set 3 : Updated strings and addressed comments #
Messages
Total messages: 25 (13 generated)
The CQ bit was checked by bsazonov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bsazonov@chromium.org changed reviewers: + bauerb@chromium.org, gogerald@chromium.org, msarda@chromium.org
Please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachine.java (right): https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachine.java:231: ThreadUtils.postOnUiThreadDelayed(new Runnable() { I do not know how this works, but it would be better to use a timer that can be cancelled if it is no longer needed. https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachine.java:272: // ConfirmImportSyncDataDialog.Listener & ConfirmManagedSyncDataDialog.Listener implementation. This is also the implementation for the new dialogs, right? https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java (right): https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java:20: * Listener to receive events from progress dialog. If dialog is not dismissed by calling Nit: I would add a *the* in: "If *the* dialog ..." https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java:62: public void showProgressDialog(final ProgressDialogListener listener) { My understand is that this delegate is the generic delegate that will present all the dialogs for the SyncConfirmationDataStateMachine. If so, the I would suggest we name these dialogs to cater for the case when we need to use other progress dialog. Maybe call it showFetchManagedPolicyProgressDialog. https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java:63: dismissProgressDialog(); Why does the previous dialog need to be dismissed here? Would it be possible to change this to an assert that the progress dialog is null? https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java:64: AlertDialog.Builder builder = new AlertDialog.Builder(mContext, R.style.AlertDialogTheme); For consistency with the code below (and if it is possible), let's get rid of the builder and use the same pattern: mProgressDialog = new AlertDialog.Builder(mContext, R.style.AlertDialogTheme).setView().setNegativeButton()...create() https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java:65: LayoutInflater inflater = LayoutInflater.from(builder.getContext()); Stupid question: Is builder.getContext() the same as mContext? If so, use mContext here. https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java:69: public void onClick(DialogInterface dialogInterface, int i) { For consistency, use dialog instead of dialogInterface here. https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java:70: listener.onCancel(); For consistency, please call dialogInterface.cancel() here https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java:96: public void showTimeoutDialog(final TimeoutDialogListener listener) { Same here: showFetchManagedPolicyTimeoutDialog https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java (right): https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java:647: * otherwise. Can be called directly from this function. Optional nit: s/Can be called directly from this function./May be called synchronously from this function. https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/strings... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/strings... chrome/android/java/strings/android_chrome_strings.grd:1084: Getting account management policy… Is this the string that you suggested. Or was it provided by PM/UX? I would suggest to change it to "Fetching account management policy…" https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/strings... chrome/android/java/strings/android_chrome_strings.grd:1090: Google took too long to respond Same here about the string. If this is your proposal, then I suggest changing it to "Fetching account management policy timed out"
Mihai, thank you for thorough review. I've tried to address all of your comments. https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachine.java (right): https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachine.java:231: ThreadUtils.postOnUiThreadDelayed(new Runnable() { On 2017/03/28 11:09:02, msarda wrote: > I do not know how this works, but it would be better to use a timer that can be > cancelled if it is no longer needed. I've found only https://developer.android.com/reference/android/os/CountDownTimer.html and it doesn't seem like a good fit for this task. https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachine.java:272: // ConfirmImportSyncDataDialog.Listener & ConfirmManagedSyncDataDialog.Listener implementation. On 2017/03/28 11:09:02, msarda wrote: > This is also the implementation for the new dialogs, right? Nope. This method is called from new dialogs listeners, but it is not a part implementation in extends/implements sense. https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java (right): https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java:20: * Listener to receive events from progress dialog. If dialog is not dismissed by calling On 2017/03/28 11:09:02, msarda wrote: > Nit: I would add a *the* in: "If *the* dialog ..." Done. I've also changed the same sentence in TimeoutDialogListener. https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java:62: public void showProgressDialog(final ProgressDialogListener listener) { On 2017/03/28 11:09:02, msarda wrote: > My understand is that this delegate is the generic delegate that will present > all the dialogs for the SyncConfirmationDataStateMachine. If so, the I would > suggest we name these dialogs to cater for the case when we need to use other > progress dialog. Maybe call it showFetchManagedPolicyProgressDialog. Done. I think that "Management" fits better than "Managed" here. WDYT? https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java:63: dismissProgressDialog(); On 2017/03/28 11:09:02, msarda wrote: > Why does the previous dialog need to be dismissed here? Would it be possible to > change this to an assert that the progress dialog is null? Thanks for bringing this up. I've made this functions behave this way because there's a lot of asynchronous stuff happening in ConfirmSyncDataStateMachine: network requests, timeouts, user actions, state changes, etc. It would be very hard to keep track of dialogs shown inside ConfirmSyncDataStateMachine. Handling this in ConfirmSyncDataStateMachineDelegate seems like better approach without any real drawback. Ideally, I would prefer interface like public interface ConfirmSyncDataStateMachineDelegate { void showSignInToManagedAccountDialog(SignInToManagedAccountDialogListener callback, String domain); void showSignOutFromManagedAccountDialog(SignOutFromManagedAccountDialogListener callback, String domain); void showSwitchFromManagedAccountDialog(SwitchFromManagedAccountDialogListener callback, String domain, String oldAccount, String newAccount); void showConfirmImportDataSyncDialog(ConfirmImportSyncDataDialogListener listener); void showFetchManagementPolicyProgressDialog(ProgressDialogListener listener); void showFetchManagementPolicyTimeoutDialog(TimeoutDialogListener listener); void dismissAllDialogs(); } WDYT? I will merge dismissProgressDialog() and dismissTimeoutDialog() if you like this approach. https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java:64: AlertDialog.Builder builder = new AlertDialog.Builder(mContext, R.style.AlertDialogTheme); On 2017/03/28 11:09:02, msarda wrote: > For consistency with the code below (and if it is possible), let's get rid of > the builder and use the same pattern: > mProgressDialog = new AlertDialog.Builder(mContext, > R.style.AlertDialogTheme).setView().setNegativeButton()...create() See next comment. https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java:65: LayoutInflater inflater = LayoutInflater.from(builder.getContext()); On 2017/03/28 11:09:02, msarda wrote: > Stupid question: Is builder.getContext() the same as mContext? If so, use > mContext here. AFAIK, these context can be different (at least in terms of UI theme). It is mentioned it getContext() documentation: https://developer.android.com/reference/android/support/v7/app/AlertDialog.Bu... However, your comment made me reread the docs and I've found that AlertDialog.Builder actually has an overload for setView that takes an layout id. Using this method simplifies the code and makes it more consistent with timeout dialog. https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java:69: public void onClick(DialogInterface dialogInterface, int i) { On 2017/03/28 11:09:02, msarda wrote: > For consistency, use dialog instead of dialogInterface here. Done. https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java:70: listener.onCancel(); On 2017/03/28 11:09:02, msarda wrote: > For consistency, please call dialogInterface.cancel() here Done. https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java:96: public void showTimeoutDialog(final TimeoutDialogListener listener) { On 2017/03/28 11:09:02, msarda wrote: > Same here: showFetchManagedPolicyTimeoutDialog Done. https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java (right): https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java:647: * otherwise. Can be called directly from this function. On 2017/03/28 11:09:02, msarda wrote: > Optional nit: s/Can be called directly from this function./May be called > synchronously from this function. Done. https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/strings... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/strings... chrome/android/java/strings/android_chrome_strings.grd:1084: Getting account management policy… On 2017/03/28 11:09:02, msarda wrote: > Is this the string that you suggested. Or was it provided by PM/UX? > > I would suggest to change it to "Fetching account management policy…" The wording is not final yet. Eli sent request to UX team, so I will change it accordingly to their response. https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/strings... chrome/android/java/strings/android_chrome_strings.grd:1090: Google took too long to respond On 2017/03/28 11:09:02, msarda wrote: > Same here about the string. If this is your proposal, then I suggest changing it > to > "Fetching account management policy timed out" This one was already approved by UX team (see discussion in the bug).
LGTM with a comment about dismissing the dialogs. https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java (right): https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java:20: * Listener to receive events from progress dialog. If dialog is not dismissed by calling On 2017/03/28 12:59:58, bsazonov wrote: > On 2017/03/28 11:09:02, msarda wrote: > > Nit: I would add a *the* in: "If *the* dialog ..." > > Done. I've also changed the same sentence in TimeoutDialogListener. Acknowledged. https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java:63: dismissProgressDialog(); On 2017/03/28 12:59:58, bsazonov wrote: > On 2017/03/28 11:09:02, msarda wrote: > > Why does the previous dialog need to be dismissed here? Would it be possible > to > > change this to an assert that the progress dialog is null? > > Thanks for bringing this up. I've made this functions behave this way because > there's a lot of asynchronous stuff happening in ConfirmSyncDataStateMachine: > network requests, timeouts, user actions, state changes, etc. It would be very > hard to keep track of dialogs shown inside ConfirmSyncDataStateMachine. Handling > this in ConfirmSyncDataStateMachineDelegate seems like better approach without > any real drawback. Ideally, I would prefer interface like > > public interface ConfirmSyncDataStateMachineDelegate { > void showSignInToManagedAccountDialog(SignInToManagedAccountDialogListener > callback, String domain); > void > showSignOutFromManagedAccountDialog(SignOutFromManagedAccountDialogListener > callback, String domain); > void > showSwitchFromManagedAccountDialog(SwitchFromManagedAccountDialogListener > callback, String domain, String oldAccount, String newAccount); > void showConfirmImportDataSyncDialog(ConfirmImportSyncDataDialogListener > listener); > void showFetchManagementPolicyProgressDialog(ProgressDialogListener > listener); > void showFetchManagementPolicyTimeoutDialog(TimeoutDialogListener listener); > void dismissAllDialogs(); > } > > WDYT? I will merge dismissProgressDialog() and dismissTimeoutDialog() if you > like this approach. I think we should not have this call be done in the delegate - I would expect the ConfirmSyncDataStateMachine to know when it needs to dismiss the dialogs it has presented. I think having a dismissAllDialogs makes sense, so please add that method. https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/strings... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/strings... chrome/android/java/strings/android_chrome_strings.grd:1090: Google took too long to respond On 2017/03/28 12:59:58, bsazonov wrote: > On 2017/03/28 11:09:02, msarda wrote: > > Same here about the string. If this is your proposal, then I suggest changing > it > > to > > "Fetching account management policy timed out" > > This one was already approved by UX team (see discussion in the bug). Acknowledged.
https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachine.java (right): https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachine.java:231: ThreadUtils.postOnUiThreadDelayed(new Runnable() { On 2017/03/28 12:59:58, bsazonov wrote: > On 2017/03/28 11:09:02, msarda wrote: > > I do not know how this works, but it would be better to use a timer that can > be > > cancelled if it is no longer needed. > > I've found only > https://developer.android.com/reference/android/os/CountDownTimer.html and it > doesn't seem like a good fit for this task. If you keep a reference to the Runnable around, you can cancel it on the UI thread handler with https://developer.android.com/reference/android/os/Handler.html#removeCallbac.... https://codereview.chromium.org/2772203004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java (right): https://codereview.chromium.org/2772203004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java:17: public class ConfirmSyncDataStateMachineDelegate { Nit: I'm not sure delegate is the best name for this -- a class that creates its own delegate doesn't really delegate any logic to it. Maybe ...DialogHandler?
lgtm with comments + bauerb@'s https://codereview.chromium.org/2772203004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachine.java (right): https://codereview.chromium.org/2772203004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachine.java:128: // This check isn't needed right now, but can take a few seconds, so we kick it off early. nit: might change this comment accordingly for the change of the function name below or rename function to "checkNewAccountManagementStatus"? https://codereview.chromium.org/2772203004/diff/20001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2772203004/diff/20001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1084: Getting account management policy… Use the unicode ellipsis instead of three periods (\u2026). http://www.fileformat.info/info/unicode/char/2026/index.htm
The CQ bit was checked by bsazonov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachine.java (right): https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachine.java:231: ThreadUtils.postOnUiThreadDelayed(new Runnable() { On 2017/03/28 14:19:21, Bernhard Bauer wrote: > On 2017/03/28 12:59:58, bsazonov wrote: > > On 2017/03/28 11:09:02, msarda wrote: > > > I do not know how this works, but it would be better to use a timer that can > > be > > > cancelled if it is no longer needed. > > > > I've found only > > https://developer.android.com/reference/android/os/CountDownTimer.html and it > > doesn't seem like a good fit for this task. > > If you keep a reference to the Runnable around, you can cancel it on the UI > thread handler with > https://developer.android.com/reference/android/os/Handler.html#removeCallbac.... Thanks for the tip! Done. https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java (right): https://codereview.chromium.org/2772203004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java:63: dismissProgressDialog(); On 2017/03/28 13:41:05, msarda wrote: > On 2017/03/28 12:59:58, bsazonov wrote: > > On 2017/03/28 11:09:02, msarda wrote: > > > Why does the previous dialog need to be dismissed here? Would it be possible > > to > > > change this to an assert that the progress dialog is null? > > > > Thanks for bringing this up. I've made this functions behave this way because > > there's a lot of asynchronous stuff happening in ConfirmSyncDataStateMachine: > > network requests, timeouts, user actions, state changes, etc. It would be very > > hard to keep track of dialogs shown inside ConfirmSyncDataStateMachine. > Handling > > this in ConfirmSyncDataStateMachineDelegate seems like better approach without > > any real drawback. Ideally, I would prefer interface like > > > > public interface ConfirmSyncDataStateMachineDelegate { > > void showSignInToManagedAccountDialog(SignInToManagedAccountDialogListener > > callback, String domain); > > void > > showSignOutFromManagedAccountDialog(SignOutFromManagedAccountDialogListener > > callback, String domain); > > void > > showSwitchFromManagedAccountDialog(SwitchFromManagedAccountDialogListener > > callback, String domain, String oldAccount, String newAccount); > > void showConfirmImportDataSyncDialog(ConfirmImportSyncDataDialogListener > > listener); > > void showFetchManagementPolicyProgressDialog(ProgressDialogListener > > listener); > > void showFetchManagementPolicyTimeoutDialog(TimeoutDialogListener > listener); > > void dismissAllDialogs(); > > } > > > > WDYT? I will merge dismissProgressDialog() and dismissTimeoutDialog() if you > > like this approach. > > I think we should not have this call be done in the delegate - I would expect > the ConfirmSyncDataStateMachine to know when it needs to dismiss the dialogs it > has presented. > > I think having a dismissAllDialogs makes sense, so please add that method. Done. I've added dismissAllDialogs instead of separate dismisses for every dialog. https://codereview.chromium.org/2772203004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachine.java (right): https://codereview.chromium.org/2772203004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachine.java:128: // This check isn't needed right now, but can take a few seconds, so we kick it off early. On 2017/03/28 20:22:52, gogerald1 wrote: > nit: might change this comment accordingly for the change of the function name > below or rename function to "checkNewAccountManagementStatus"? Fixed the comment. Thanks for pointing it out. https://codereview.chromium.org/2772203004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java (right): https://codereview.chromium.org/2772203004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java:17: public class ConfirmSyncDataStateMachineDelegate { On 2017/03/28 14:19:21, Bernhard Bauer wrote: > Nit: I'm not sure delegate is the best name for this -- a class that creates its > own delegate doesn't really delegate any logic to it. Maybe ...DialogHandler? Actually, I was thinking of adding mock delegate to test ConfirmSyncDataStateMachine in the future. WDYT? https://codereview.chromium.org/2772203004/diff/20001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2772203004/diff/20001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1084: Getting account management policy… On 2017/03/28 20:22:52, gogerald1 wrote: > Use the unicode ellipsis instead of three periods (\u2026). > > http://www.fileformat.info/info/unicode/char/2026/index.htm It is already an unicode ellipsis. I've updated the stings accordingly to the UX team reply (in the bug).
LGTM -- I'd like to sort out the name of the dialog class, but if we're going to make more changes there, we can do that in a followup. https://codereview.chromium.org/2772203004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java (right): https://codereview.chromium.org/2772203004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java:17: public class ConfirmSyncDataStateMachineDelegate { On 2017/03/29 13:05:05, bsazonov wrote: > On 2017/03/28 14:19:21, Bernhard Bauer wrote: > > Nit: I'm not sure delegate is the best name for this -- a class that creates > its > > own delegate doesn't really delegate any logic to it. Maybe ...DialogHandler? > > Actually, I was thinking of adding mock delegate to test > ConfirmSyncDataStateMachine in the future. WDYT? Okay, then we could start to really decouple the two classes -- maybe create an interface inside of ConfirmSyncDataStateMachine and have the dialog class implement it. Right now the two classes are pretty closely coupled, if only because of the name :)
https://codereview.chromium.org/2772203004/diff/20001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2772203004/diff/20001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1084: Getting account management policy… On 2017/03/29 13:05:06, bsazonov wrote: > On 2017/03/28 20:22:52, gogerald1 wrote: > > Use the unicode ellipsis instead of three periods (\u2026). > > > > http://www.fileformat.info/info/unicode/char/2026/index.htm > > It is already an unicode ellipsis. I've updated the stings accordingly to the UX > team reply (in the bug). I mean use \u2026 instead of ... in grd might be better for translation,
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2772203004/diff/20001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2772203004/diff/20001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1084: Getting account management policy… On 2017/03/29 15:14:02, gogerald1 wrote: > On 2017/03/29 13:05:06, bsazonov wrote: > > On 2017/03/28 20:22:52, gogerald1 wrote: > > > Use the unicode ellipsis instead of three periods (\u2026). > > > > > > http://www.fileformat.info/info/unicode/char/2026/index.htm > > > > It is already an unicode ellipsis. I've updated the stings accordingly to the > UX > > team reply (in the bug). > > I mean use \u2026 instead of ... in grd might be better for translation, FWIW, the other instances of an ellipsis character in this file also use the Unicode character.
The CQ bit was checked by bsazonov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gogerald@chromium.org, msarda@chromium.org Link to the patchset: https://codereview.chromium.org/2772203004/#ps40001 (title: "Updated strings and addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490811945541260,
"parent_rev": "4e53ee167df2f069c380ad218e56d34772ec83e2", "commit_rev":
"33295fdb5fa26e409b197b81b2069c435184d6e3"}
Message was sent while issue was closed.
Description was changed from ========== Add progress and timeout dialogs for getting account management policy This CL adds two dialogs that are shown by ConfirmSyncDataStateMachine. Dialogs implementation is in ConfirmSyncDataStateMachineDelegate. The first dialog is a progress dialog that is shown while Chrome requests server for account management policy. The second dialog is a timeout dialog that is shown if Chrome doesn't receive account management policy within 30 seconds. BUG=650121 ========== to ========== Add progress and timeout dialogs for getting account management policy This CL adds two dialogs that are shown by ConfirmSyncDataStateMachine. Dialogs implementation is in ConfirmSyncDataStateMachineDelegate. The first dialog is a progress dialog that is shown while Chrome requests server for account management policy. The second dialog is a timeout dialog that is shown if Chrome doesn't receive account management policy within 30 seconds. BUG=650121 Review-Url: https://codereview.chromium.org/2772203004 Cr-Commit-Position: refs/heads/master@{#460471} Committed: https://chromium.googlesource.com/chromium/src/+/33295fdb5fa26e409b197b81b206... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/33295fdb5fa26e409b197b81b206... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
