Set timeout in sync setup

chrome/browser/ui/webui/sync_setup_handler.cc

Index: chrome/browser/ui/webui/sync_setup_handler.cc
diff --git a/chrome/browser/ui/webui/sync_setup_handler.cc b/chrome/browser/ui/webui/sync_setup_handler.cc
index dfc6dfe0c6e8d2861626ddb94dea95b1c7959791..b2fa04eae0f960d22a38776e4eea4c943644c0f4 100644
--- a/chrome/browser/ui/webui/sync_setup_handler.cc
+++ b/chrome/browser/ui/webui/sync_setup_handler.cc
@@ -284,6 +284,8 @@ void SyncSetupHandler::GetStaticLocalizedValues(
 
   static OptionsStringResource resources[] = {
     { "syncSetupConfigureTitle", IDS_SYNC_SETUP_CONFIGURE_TITLE },
+    { "syncSetupTimeoutTitle", IDS_SYNC_SETUP_TIME_OUT_TITLE },
+    { "syncSetupTimeoutContent", IDS_SYNC_SETUP_TIME_OUT_CONTENT },
     { "cannotBeBlank", IDS_SYNC_CANNOT_BE_BLANK },
     { "emailLabel", IDS_SYNC_LOGIN_EMAIL_NEW_LINE },
     { "passwordLabel", IDS_SYNC_LOGIN_PASSWORD_NEW_LINE },
@@ -536,6 +538,23 @@ void SyncSetupHandler::DisplaySpinner() {
   configuring_sync_ = true;
   StringValue page("spinner");
   DictionaryValue args;
+
+  const int kTimeoutSec = 30;
+  backend_start_timer_.reset(new base::OneShotTimer<SyncSetupHandler>());

[Andrew T Wilson (Slow), 2012/06/20 00:12:02]

Seems like we should never get called with a non-empty backend_start_timer_ -
should we add a DCHECK(backend_start_timer_.get()) before calling reset()?

[peria, 2012/06/20 04:06:03]

I think it is not necessary to put DCHECK() here, because reset() checks if the
scoped pointer figures a non-empty OneShotTimer instance.
And if we put DCHECK() here, it fails in the first call of this method because
backend_start_timer_ is not instantiated yet.

[Andrew T Wilson (Slow), 2012/06/20 05:23:41]

Sorry, I meant to say "DCHECK(!backend_start_timer_.get()) to make sure it's
never already set when we call this routine".

But I don't feel that strongly about it, so it's fine as-is.

[peria, 2012/06/20 09:20:17]

I got it. Thank you for your explanation.
But it can also fail, because this method can be called many times (described
repro-steps below).  So I decided not to put it.

1. Click "Setup Sync"
2.  DisplaySpinner() is called
3.  backend_start_timer_ is set
4. Click "Cancel"
5. Click "Setup Sync" again
6.  DisplaySpinner() is called
7.  DCHECK fails

[Andrew T Wilson (Slow), 2012/06/20 16:40:27]

Isn't that a bug, then? Shouldn't the timeout timer be reset when you cancel?
That's kind of why I suggested adding that DCHECK.

[peria, 2012/06/21 06:45:32]

Yes, it is, even though it works without erros.

Again, I traced the source in HEAD, and found that this method was called twice
in one click of "Set up Sync" button.
There are two entry points from JavaScript layer, HandleAttachHandler() and
HandleShowSetupUI().  So I replace this code into HandleShowSetupUI() with
DCHECK.

Would you please take another look?

+  backend_start_timer_->Start(FROM_HERE,
+                              base::TimeDelta::FromSeconds(kTimeoutSec),
+                              this, &SyncSetupHandler::DisplayTimeout);
+  web_ui()->CallJavascriptFunction(
+      "SyncSetupOverlay.showSyncSetupPage", page, args);
+}
+
+// TODO(kochi): Handle error conditions other than timeout.
+// http://crbug.com/128692
+void SyncSetupHandler::DisplayTimeout() {
+  configuring_sync_ = false;

[Andrew T Wilson (Slow), 2012/06/20 00:12:02]

I'm slightly nervous about leaving sync setup running in the background while
the timeout dialog is here. Perhaps we should call CloseSyncSetup() first,
before displaying the timeout page?

[peria, 2012/06/20 04:06:03]

SGTM.
Done.

+  StringValue page("timeout");
+  DictionaryValue args;
+
   web_ui()->CallJavascriptFunction(
       "SyncSetupOverlay.showSyncSetupPage", page, args);
 }
@@ -662,6 +681,10 @@ void SyncSetupHandler::GaiaCredentialsValid() {
 }
 
 void SyncSetupHandler::SigninFailed(const GoogleServiceAuthError& error) {
+  // Stop a timer to handle timeout in waiting for checking network connection.
+  if (backend_start_timer_.get())

[Andrew T Wilson (Slow), 2012/06/20 00:12:02]

No need to do a .get() first - just call reset().

[peria, 2012/06/20 04:06:03]

Done.

+    backend_start_timer_.reset();
+
   last_signin_error_ = error;
   // Got a failed signin - this is either just a typical auth error, or a
   // sync error (treat sync errors as "fatal errors" - i.e. non-auth errors).
@@ -684,6 +707,10 @@ ProfileSyncService* SyncSetupHandler::GetSyncService() const {
 
 void SyncSetupHandler::SigninSuccess() {
   DCHECK(GetSyncService()->sync_initialized());
+  // Stop a timer to handle timeout in waiting for checking network connection.
+  if (backend_start_timer_.get())

[Andrew T Wilson (Slow), 2012/06/20 00:12:02]

Just call reset(), don't bother checking get() first.

Also, should we clear the timer in CloseOverlay() also, in case the user cancels
out of the spinner?

[peria, 2012/06/20 04:06:03]

Done.

+    backend_start_timer_.reset();
+
   // If we have signed in while sync is already setup, it must be due to some
   // kind of re-authentication flow. In that case, just close the signin dialog
   // rather than forcing the user to go through sync configuration.