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;

[Roger Tawa OOO till Jul 10th, 2013/02/13 19:39:29]

This change is not related to this bug, but I noticed that the actions here were
reversed.  This would cause the wrong  histogram action to be recorded at line
162 below.

     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_) {

[guohui, 2013/02/13 21:59:55]

Why do we care about source changes other than from non-settings to settings?

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

As mentioned in the description, it is now possible to go from settings to
non-settings, due to a change in the gaia sign in page.  When this code was
originally written, it was not possible for this to happen.

+        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 61 matching lines @@
         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 &&
           SyncPromoUI::GetSourceForSyncPromoURL(continue_url_) ==
           SyncPromoUI::SOURCE_WEBSTORE_INSTALL) {

[Roger Tawa OOO till Jul 10th, 2013/02/13 19:39:29]

Hui: how does this change affect signing in from the webstore?  Should there be
an extra check for "source_ == SyncPromoUI::SOURCE_SETTINGS"?

[guohui, 2013/02/13 21:59:55]

Before this CL, last_minute_source_change is only set to true if the new source
is SOURCE_SETTINGS, thus there is no need to explicitly check for 'source_ ==
settings'.

[guohui, 2013/02/13 22:15:30]

And yes, if last_minute_source_change is true in all source change cases, then
we need add another check here for 'source_ == settings'.

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

OK, I added the check.  But I think the last minute check is no longer needed. 
What do you think?

[guohui, 2013/02/14 16:43:29]

yes, if you add a check for 'source_ == settings', then last minute check is not
needed.

         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
       // NTP after sign in completes.  In the case of the settings page, it will
       // get closed by SyncSetupHandler. In the case of webstore, it will
@@ skipping 66 matching lines @@
         break;
     }
   }
 
   RedirectToNTP();
 }
 
 void OneClickSigninHelper::SigninSuccess() {
   RedirectToNTP();
 }