Signing in to sync as a different user is redirecting to sync settings

chrome/browser/ui/sync/one_click_signin_helper.cc

Index: chrome/browser/ui/sync/one_click_signin_helper.cc
diff --git a/chrome/browser/ui/sync/one_click_signin_helper.cc b/chrome/browser/ui/sync/one_click_signin_helper.cc
index 90b0f3be5c68e5db1139c945060b91b06fd2de50..a50ee9156b13ed69257246bc7ab061fe60a4fa98 100644
--- a/chrome/browser/ui/sync/one_click_signin_helper.cc
+++ b/chrome/browser/ui/sync/one_click_signin_helper.cc
@@ -136,14 +136,14 @@ void StartSync(const StartSyncArgs& args,
   int action = one_click_signin::HISTOGRAM_MAX;
   switch (args.auto_accept) {
     case OneClickSigninHelper::AUTO_ACCEPT_EXPLICIT:
-      action = one_click_signin::HISTOGRAM_AUTO_WITH_DEFAULTS;
-      break;
-    case OneClickSigninHelper::AUTO_ACCEPT_ACCEPTED:
       action =
           start_mode == OneClickSigninSyncStarter::SYNC_WITH_DEFAULT_SETTINGS ?
               one_click_signin::HISTOGRAM_AUTO_WITH_DEFAULTS :
               one_click_signin::HISTOGRAM_AUTO_WITH_ADVANCED;
       break;
+    case OneClickSigninHelper::AUTO_ACCEPT_ACCEPTED:
+      action = one_click_signin::HISTOGRAM_AUTO_WITH_DEFAULTS;
+      break;
     case OneClickSigninHelper::AUTO_ACCEPT_NONE:
       action =
           start_mode == OneClickSigninSyncStarter::SYNC_WITH_DEFAULT_SETTINGS ?
@@ -993,16 +993,15 @@ void OneClickSigninHelper::DidStopLoading(
         return;
       }
 
-      // In explicit sign ins, the user may have checked the box
+      // In explicit sign ins, the user may have changed the box
       // "Let me choose what to sync".  This is reflected as a change in the
       // source of the continue URL.  Make one last check of the current URL
-      // to see if there is a valid source and its set to settings.  If so,
-      // it overrides the current source.
+      // to see if there is a valid source.  If so, it overrides the
+      // current source.
       SyncPromoUI::Source source =
           SyncPromoUI::GetSourceForSyncPromoURL(url);
-      if (source == SyncPromoUI::SOURCE_SETTINGS &&
-          source_ != SyncPromoUI::SOURCE_SETTINGS) {
-        source_ = SyncPromoUI::SOURCE_SETTINGS;
+      if (source != source_) {

[Andrew T Wilson (Slow), 2013/02/14 18:18:49]

Why not just do source_ = source (why have an if at all?) If they are identical,
then source_=source is a no-op anyway.

[Roger Tawa OOO till Jul 10th, 2013/02/14 18:53:35]

We need the if to set the bool variable.  I've also renamed the bool and added
some commentary, let me know if this explains what is going better.  Also, even
though |force_same_tab_navigation| is being set to true only if the new source
is SETTINGS, OneClickSigninSyncStarter only used it in that case anyway.  So the
code is actually equivalent to the way it was before, but hopefully a little
clearer about what is going on.

+        source_ = source;
         last_minute_source_change = true;
       }
     }
@@ -1053,8 +1052,8 @@ void OneClickSigninHelper::DidStopLoading(
       break;
     case AUTO_ACCEPT_EXPLICIT: {
       OneClickSigninSyncStarter::StartSyncMode start_mode =
-          source_ == SyncPromoUI::SOURCE_SETTINGS ?
-              OneClickSigninSyncStarter::CONFIGURE_SYNC_FIRST :
+          source_ == SyncPromoUI::SOURCE_SETTINGS ?
+              OneClickSigninSyncStarter::CONFIGURE_SYNC_FIRST :
               OneClickSigninSyncStarter::SYNC_WITH_DEFAULT_SETTINGS;
 
       // If the new email address is different from the email address that
@@ -1082,7 +1081,7 @@ void OneClickSigninHelper::DidStopLoading(
             IDS_ONE_CLICK_SIGNIN_CONFIRM_EMAIL_DIALOG_CANCEL_BUTTON);
       }
 
-      if (last_minute_source_change &&
+      if (source_ == SyncPromoUI::SOURCE_SETTINGS &&
           SyncPromoUI::GetSourceForSyncPromoURL(continue_url_) ==
           SyncPromoUI::SOURCE_WEBSTORE_INSTALL) {
         redirect_url_ = continue_url_;