Set timeout in sync setup

chrome/browser/ui/webui/sync_setup_handler.cc

 // Copyright (c) 2012 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.
 
 #include "chrome/browser/ui/webui/sync_setup_handler.h"
 
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
@@ skipping 266 matching lines @@
   localized_strings->SetString("createAccountLinkHTML",
       GetStringFUTF16(IDS_SYNC_CREATE_ACCOUNT_PREFIX, create_account));
 
   string16 sync_benefits_url(
       UTF8ToUTF16(google_util::StringAppendGoogleLocaleParam(
           chrome::kSyncLearnMoreURL)));
   localized_strings->SetString("promoLearnMoreURL", sync_benefits_url);
 
   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 },
     { "invalidCredentials", IDS_SYNC_INVALID_USER_CREDENTIALS },
     { "signin", IDS_SYNC_SIGNIN },
     { "couldNotConnect", IDS_SYNC_LOGIN_COULD_NOT_CONNECT },
     { "unrecoverableError", IDS_SYNC_UNRECOVERABLE_ERROR },
     { "errorLearnMore", IDS_LEARN_MORE },
     { "unrecoverableErrorHelpURL", IDS_SYNC_UNRECOVERABLE_ERROR_HELP_URL },
     { "cannotAccessAccount", IDS_SYNC_CANNOT_ACCESS_ACCOUNT },
@@ skipping 232 matching lines @@
   StringValue page("login");
   web_ui()->CallJavascriptFunction(
       "SyncSetupOverlay.showSyncSetupPage", page, args);
 }
 
 // TODO(kochi): Handle error conditions (timeout, other failures).
 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);
+}
 
 void SyncSetupHandler::RecordSignin() {
   // By default, do nothing - subclasses can override.
 }
 
 void SyncSetupHandler::DisplayGaiaSuccessAndClose() {
   RecordSignin();
   web_ui()->CallJavascriptFunction("SyncSetupOverlay.showSuccessAndClose");
 }
 
@@ skipping 103 matching lines @@
 }
 
 void SyncSetupHandler::GaiaCredentialsValid() {
   DCHECK(IsActiveLogin());
 
   // Gaia credentials are valid - update the UI.
   DisplayGaiaSuccessAndSettingUp();
 }
 
 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).
   // On ChromeOS, this condition should trigger the orange badge on wrench menu
   // and prompt to sign out.
 #if !defined(OS_CHROMEOS)
   DisplayGaiaLogin(GetSyncService()->unrecoverable_error_detected());
 #else
   CloseOverlay();
 #endif
 }
 
 Profile* SyncSetupHandler::GetProfile() const {
   return Profile::FromWebUI(web_ui());
 }
 
 ProfileSyncService* SyncSetupHandler::GetSyncService() const {
   return ProfileSyncServiceFactory::GetForProfile(GetProfile());
 }
 
 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.
   if (GetSyncService()->HasSyncSetupCompleted())
     DisplayGaiaSuccessAndClose();
   else
     DisplayConfigureSync(false, false);
 }
 
 void SyncSetupHandler::HandleConfigure(const ListValue* args) {
@@ skipping 324 matching lines @@
     if (i != current_profile_index && AreUserNamesEqual(
         cache.GetUserNameOfProfileAtIndex(i), username_utf16)) {
       *error_message = l10n_util::GetStringUTF16(
           IDS_SYNC_USER_NAME_IN_USE_ERROR);
       return false;
     }
   }
 
   return true;
 }