[sync] Prevent flash of error icon while loading syncSetup page

chrome/browser/ui/webui/settings/people_handler.cc

 // Copyright 2015 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/settings/people_handler.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 11 matching lines @@
 #include "chrome/browser/profiles/profile_window.h"
 #include "chrome/browser/signin/chrome_signin_helper.h"
 #include "chrome/browser/signin/signin_error_controller_factory.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/signin/signin_promo.h"
 #include "chrome/browser/signin/signin_ui_util.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/browser/sync/sync_ui_util.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/chrome_pages.h"
 #include "chrome/browser/ui/singleton_tabs.h"
 #include "chrome/browser/ui/user_manager.h"
 #include "chrome/browser/ui/webui/profile_helper.h"
 #include "chrome/browser/ui/webui/signin/login_ui_service.h"
 #include "chrome/browser/ui/webui/signin/login_ui_service_factory.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/common/url_constants.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/autofill/core/common/autofill_constants.h"
 #include "components/autofill/core/common/autofill_pref_names.h"
 #include "components/browser_sync/profile_sync_service.h"
 #include "components/prefs/pref_service.h"
 #include "components/signin/core/browser/signin_error_controller.h"
 #include "components/signin/core/browser/signin_header_helper.h"
 #include "components/signin/core/browser/signin_metrics.h"
 #include "components/signin/core/common/profile_management_switches.h"
 #include "components/signin/core/common/signin_pref_names.h"
@@ skipping 141 matching lines @@
 PeopleHandler::~PeopleHandler() {
   // Early exit if running unit tests (no actual WebUI is attached).
   if (!web_ui())
     return;
 
   // This case is hit when the user performs a back navigation.
   CloseSyncSetup();
 }
 
 void PeopleHandler::RegisterMessages() {
+  MaybeAcquireSyncBlocker();
   web_ui()->RegisterMessageCallback(
       "SyncSetupDidClosePage",
       base::Bind(&PeopleHandler::OnDidClosePage, base::Unretained(this)));
   web_ui()->RegisterMessageCallback(
       "SyncSetupSetDatatypes",
       base::Bind(&PeopleHandler::HandleSetDatatypes, base::Unretained(this)));
   web_ui()->RegisterMessageCallback(
       "SyncSetupSetEncryption",
       base::Bind(&PeopleHandler::HandleSetEncryption, base::Unretained(this)));
   web_ui()->RegisterMessageCallback(
@@ skipping 409 matching lines @@
   sync_blocker_.reset();
 
   configuring_sync_ = false;
 }
 
 void PeopleHandler::OpenSyncSetup() {
   // Notify services that login UI is now active.
   GetLoginUIService()->SetLoginUI(this);
 
   ProfileSyncService* service = GetSyncService();
-  if (service)
+  if (service && !sync_blocker_)
     sync_blocker_ = service->GetSetupInProgressHandle();
 
   // There are several different UI flows that can bring the user here:
   // 1) Signin promo.
   // 2) Normal signin through settings page (IsAuthenticated() is false).
   // 3) Previously working credentials have expired.
   // 4) User is signed in, but has stopped sync via the google dashboard, and
   //    signout is prohibited by policy so we need to force a re-auth.
   // 5) User clicks [Advanced Settings] button on options page while already
   //    logged in.
@@ skipping 40 matching lines @@
     }
     return;
   }
 
   // User is already logged in. They must have brought up the config wizard
   // via the "Advanced..." button or through One-Click signin (cases 4-6), or
   // they are re-enabling sync after having disabled it (case 7).
   PushSyncPrefs();
 }
 
+// If we're directly loading the sync setup page, we acquire a
+// SetupInProgressHandle early in order to prevent a lapse in
+// ProfileSyncService's "SetupInProgress" status. This lapse previously occured
+// between when the sync confirmation dialog was closed and when the sync setup
+// page hadn't yet fired the SyncSetupShowSetupUI event.
+void PeopleHandler::MaybeAcquireSyncBlocker() {

[tommycli, 2017/07/17 16:44:22]

I've been trained to not use MaybeFoo() generally, since then the reader
naturally asks "Maybe? Why maybe? under what conditions?"

In this case, maybe you can rename to: InitailizeSyncBlocker -- if the caller
truly does not need to know the details within...

[Patrick Noland, 2017/07/17 20:39:11]

Done.

+  if (!web_ui())
+    return;
+  WebContents* web_contents = web_ui()->GetWebContents();
+  if (web_contents) {
+    ProfileSyncService* service = GetSyncService();
+    const GURL current_url = web_contents->GetVisibleURL();
+    if (service &&
+        current_url == chrome::GetSettingsUrl(chrome::kSyncSetupSubPage)) {
+      sync_blocker_ = service->GetSetupInProgressHandle();
+    }
+  }
+}
+
 void PeopleHandler::FocusUI() {
   WebContents* web_contents = web_ui()->GetWebContents();
   web_contents->GetDelegate()->ActivateContents(web_contents);
 }
 
 void PeopleHandler::CloseUI() {
   CloseSyncSetup();
   FireWebUIListener("page-status-changed", base::Value(kDonePageStatus));
 }
 
@@ skipping 206 matching lines @@
   base::FilePath profile_file_path = profile_->GetPath();
   ProfileMetrics::LogProfileSyncSignIn(profile_file_path);
 
   // We're done configuring, so notify ProfileSyncService that it is OK to
   // start syncing.
   sync_blocker_.reset();
   service->SetFirstSetupComplete();
 }
 
 }  // namespace settings