Add progress and timeout dialogs for getting account management policy

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

Index: chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java b/chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java
new file mode 100644
index 0000000000000000000000000000000000000000..90ba01ce503d86f5cb57dafc88b7355923de4229
--- /dev/null
+++ b/chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java
@@ -0,0 +1,133 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.signin;
+
+import android.app.Dialog;
+import android.content.Context;
+import android.content.DialogInterface;
+import android.support.v7.app.AlertDialog;
+import android.view.LayoutInflater;
+
+import org.chromium.chrome.R;
+
+/**
+ * Class to decouple ConfirmSyncDataStateMachine from UI code and dialog management.
+ */
+public class ConfirmSyncDataStateMachineDelegate {
+    /**
+     * Listener to receive events from progress dialog. If dialog is not dismissed by calling

[msarda, 2017/03/28 11:09:02]

Nit: I would add a *the* in: "If *the* dialog ..."

[bsazonov, 2017/03/28 12:59:58]

Done. I've also changed the same sentence in TimeoutDialogListener.

[msarda, 2017/03/28 13:41:05]

Acknowledged.

+     * {@link ConfirmSyncDataStateMachineDelegate#dismissProgressDialog}, then
+     * {@link #onCancel} will be called once.
+     */
+    public interface ProgressDialogListener {
+        /**
+         * This method is called when user cancels the dialog in any way.
+         */
+        void onCancel();
+    }
+
+    /**
+     * Listener to receive events from timeout dialog. If dialog is not dismissed by calling
+     * {@link ConfirmSyncDataStateMachineDelegate#dismissTimeoutDialog}, then
+     * either {@link #onCancel} or {@link #onRetry} will be called once.
+     */
+    public interface TimeoutDialogListener {
+        /**
+         * This method is called when user cancels the dialog in any way.
+         */
+        void onCancel();
+
+        /**
+         * This method is called when user clicks retry button.
+         */
+        void onRetry();
+    }
+
+    private final Context mContext;
+
+    private Dialog mProgressDialog;
+    private AlertDialog mTimeoutAlertDialog;
+
+    public ConfirmSyncDataStateMachineDelegate(Context context) {
+        mContext = context;
+    }
+
+    /**
+     * Shows progress dialog. Will dismiss previous progress dialog, if any.
+     *
+     * @param listener The {@link ProgressDialogListener} that will be notified about user actions.
+     */
+    public void showProgressDialog(final ProgressDialogListener listener) {

[msarda, 2017/03/28 11:09:02]

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.

[bsazonov, 2017/03/28 12:59:58]

Done. I think that "Management" fits better than "Managed" here. WDYT?

+        dismissProgressDialog();

[msarda, 2017/03/28 11:09:02]

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?

[bsazonov, 2017/03/28 12:59:58]

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.

[msarda, 2017/03/28 13:41:05]

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.

[bsazonov, 2017/03/29 13:05:05]

Done. I've added dismissAllDialogs instead of separate dismisses for every
dialog.

+        AlertDialog.Builder builder = new AlertDialog.Builder(mContext, R.style.AlertDialogTheme);

[msarda, 2017/03/28 11:09:02]

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()

[bsazonov, 2017/03/28 12:59:58]

See next comment.

+        LayoutInflater inflater = LayoutInflater.from(builder.getContext());

[msarda, 2017/03/28 11:09:02]

Stupid question: Is builder.getContext() the same as mContext? If so, use
mContext here.

[bsazonov, 2017/03/28 12:59:58]

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.

+        builder.setView(inflater.inflate(R.layout.signin_progress_bar_dialog, null));
+        builder.setNegativeButton(R.string.cancel, new DialogInterface.OnClickListener() {
+            @Override
+            public void onClick(DialogInterface dialogInterface, int i) {

[msarda, 2017/03/28 11:09:02]

For consistency, use dialog instead of dialogInterface here.

[bsazonov, 2017/03/28 12:59:58]

Done.

+                listener.onCancel();

[msarda, 2017/03/28 11:09:02]

For consistency, please call dialogInterface.cancel() here

[bsazonov, 2017/03/28 12:59:58]

Done.

+            }
+        });
+        builder.setOnCancelListener(new DialogInterface.OnCancelListener() {
+            @Override
+            public void onCancel(DialogInterface dialogInterface) {
+                listener.onCancel();
+            }
+        });
+        mProgressDialog = builder.create();
+        mProgressDialog.show();
+    }
+
+    /**
+     * Dismisses progress dialog.
+     */
+    public void dismissProgressDialog() {
+        if (mProgressDialog != null) mProgressDialog.dismiss();
+        mProgressDialog = null;
+    }
+
+    /**
+     * Shows timeout dialog. Will dismiss previous timeout dialog, if any.
+     *
+     * @param listener The {@link TimeoutDialogListener} that will be notified about user actions.
+     */
+    public void showTimeoutDialog(final TimeoutDialogListener listener) {

[msarda, 2017/03/28 11:09:02]

Same here: showFetchManagedPolicyTimeoutDialog

[bsazonov, 2017/03/28 12:59:58]

Done.

+        dismissTimeoutDialog();
+        mTimeoutAlertDialog =
+                new AlertDialog.Builder(mContext, R.style.AlertDialogTheme)
+                        .setTitle(R.string.sign_in_timeout_title)
+                        .setMessage(R.string.sign_in_timeout_message)
+                        .setNegativeButton(R.string.cancel,
+                                new DialogInterface.OnClickListener() {
+                                    @Override
+                                    public void onClick(DialogInterface dialog, int which) {
+                                        dialog.cancel();
+                                    }
+                                })
+                        .setPositiveButton(R.string.retry,
+                                new DialogInterface.OnClickListener() {
+                                    @Override
+                                    public void onClick(DialogInterface dialog, int which) {
+                                        listener.onRetry();
+                                    }
+                                })
+                        .setOnCancelListener(new DialogInterface.OnCancelListener() {
+                            @Override
+                            public void onCancel(DialogInterface dialogInterface) {
+                                listener.onCancel();
+                            }
+                        })
+                        .create();
+        mTimeoutAlertDialog.show();
+    }
+
+    /**
+     * Dismisses timeout dialog.
+     */
+    public void dismissTimeoutDialog() {
+        if (mTimeoutAlertDialog != null) mTimeoutAlertDialog.dismiss();
+        mTimeoutAlertDialog = null;
+    }
+}