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

chrome/browser/ui/sync/one_click_signin_helper.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/sync/one_click_signin_helper.h"
 
 #include <algorithm>
 #include <functional>
 #include <utility>
 #include <vector>
@@ skipping 118 matching lines @@
   }
   // The starter deletes itself once its done.
   new OneClickSigninSyncStarter(args.profile, args.browser, args.session_index,
                                 args.email, args.password, start_mode,
                                 /* force_same_tab_navigation */
                                 args.last_minute_source_change);
 
   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 ?
               one_click_signin::HISTOGRAM_WITH_DEFAULTS :
               one_click_signin::HISTOGRAM_WITH_ADVANCED;
       break;
     case OneClickSigninHelper::AUTO_ACCEPT_CONFIGURE:
       DCHECK(start_mode == OneClickSigninSyncStarter::CONFIGURE_SYNC_FIRST);
       action = one_click_signin::HISTOGRAM_AUTO_WITH_ADVANCED;
       break;
@@ skipping 829 matching lines @@
       const bool continue_url_match = (url.ReplaceComponents(replacements) ==
           continue_url_.ReplaceComponents(replacements));
       if (!continue_url_match) {
         VLOG(1) << "OneClickSigninHelper::DidStopLoading: invalid url='"
                 << url.spec()
                 << "' expected continue url=" << continue_url_;
         CleanTransientState();
         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;
       }
     }
   }
 
   Browser* browser = chrome::FindBrowserWithWebContents(contents);
   Profile* profile =
       Profile::FromBrowserContext(contents->GetBrowserContext());
 
   VLOG(1) << "OneClickSigninHelper::DidStopLoading: signin is go."
@@ skipping 59 matching lines @@
                 start_mode));
       } else {
         StartExplicitSync(
             StartSyncArgs(profile, browser, auto_accept_, session_index_,
                           email_, password_, last_minute_source_change),
             contents,
             start_mode,
             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_;
         ProfileSyncService* sync_service =
           ProfileSyncServiceFactory::GetForProfile(profile);
         if (sync_service)
           sync_service->AddObserver(this);
       }
 
       // If this explicit sign in is not from settings page/webstore, show the
@@ skipping 68 matching lines @@
         break;
     }
   }
 
   RedirectToNTP();
 }
 
 void OneClickSigninHelper::SigninSuccess() {
   RedirectToNTP();
 }