With browser instant extended enabled, signing in should redirect to chrome://apps instead of to NT…

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 12 matching lines @@
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/defaults.h"
 #include "chrome/browser/google/google_util.h"
 #include "chrome/browser/infobars/infobar_service.h"
 #include "chrome/browser/prefs/scoped_user_pref_update.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_info_cache.h"
 #include "chrome/browser/profiles/profile_io_data.h"
 #include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/search/search.h"
 #include "chrome/browser/signin/signin_manager.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/signin/signin_names_io_thread.h"
 #include "chrome/browser/sync/profile_sync_service.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/browser/sync/sync_prefs.h"
 #include "chrome/browser/tab_contents/tab_util.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/chrome_pages.h"
@@ skipping 72 matching lines @@
                                 one_click_signin::HISTOGRAM_MAX);
       break;
     case SyncPromoUI::SOURCE_WEBSTORE_INSTALL:
       UMA_HISTOGRAM_ENUMERATION("Signin.WebstoreInstallActions", action,
                                 one_click_signin::HISTOGRAM_MAX);
       break;
     case SyncPromoUI::SOURCE_APP_LAUNCHER:
       UMA_HISTOGRAM_ENUMERATION("Signin.AppLauncherActions", action,
                                 one_click_signin::HISTOGRAM_MAX);
       break;
+    case SyncPromoUI::SOURCE_APPS_PAGE_LINK:
+      UMA_HISTOGRAM_ENUMERATION("Signin.AppsPageLinkActions", action,
+                                one_click_signin::HISTOGRAM_MAX);
+      break;
     default:
       NOTREACHED() << "Invalid Source";
       return;
   }
   UMA_HISTOGRAM_ENUMERATION("Signin.AllAccessPointActions", action,
                             one_click_signin::HISTOGRAM_MAX);
 }
 
 void LogOneClickHistogramValue(int action) {
   UMA_HISTOGRAM_ENUMERATION("Signin.OneClickActions", action,
@@ skipping 81 matching lines @@
   }
 }
 
 void ClearPendingEmailOnIOThread(content::ResourceContext* context) {
   ProfileIOData* io_data = ProfileIOData::FromResourceContext(context);
   DCHECK(io_data);
   io_data->set_reverse_autologin_pending_email(std::string());
 }
 
 // Determines the source of the sign in and the continue URL.  Its either one
-// of the known sign in access point (first run, NTP, menu, settings) or its
-// an implicit sign in via another Google property.  In the former case,
+// of the known sign in access point (first run, NTP, Apps page, menu, settings)
+// or its an implicit sign in via another Google property.  In the former case,
 // "service" is also checked to make sure its "chromiumsync".
 SyncPromoUI::Source GetSigninSource(const GURL& url, GURL* continue_url) {
   std::string value;
   net::GetValueForKeyInQuery(url, "service", &value);
   bool possibly_an_explicit_signin = value == "chromiumsync";
 
   // Find the final continue URL for this sign in.  In some cases, Gaia can
   // continue to itself, with the original continue URL buried under a couple
   // of layers of indirection.  Peel those layers away.  The final destination
   // can also be "IsGaiaSignonRealm" so stop if we get to the end (but be sure
@@ skipping 620 matching lines @@
       const std::string& token = tokens[i];
       if (token == "accepted") {
         auto_accept = AUTO_ACCEPT_ACCEPTED;
       } else if (token == "configure") {
         auto_accept = AUTO_ACCEPT_CONFIGURE;
       } else if (token == "rejected-for-profile") {
         auto_accept = AUTO_ACCEPT_REJECTED_FOR_PROFILE;
       }
     }
 
-    // If this is an explicit sign in (i.e., first run, NTP, menu,settings)
-    // then force the auto accept type to explicit.
+    // If this is an explicit sign in (i.e., first run, NTP, Apps page, menu,
+    // settings) then force the auto accept type to explicit.
     source = GetSigninSource(request->url(), &continue_url);
     if (source != SyncPromoUI::SOURCE_UNKNOWN)
       auto_accept = AUTO_ACCEPT_EXPLICIT;
   }
 
   if (auto_accept != AUTO_ACCEPT_NONE) {
     VLOG(1) << "OneClickSigninHelper::ShowInfoBarIfPossible:"
             << " auto_accept=" << auto_accept;
   }
 
@@ skipping 68 matching lines @@
   if (!session_index.empty())
     helper->session_index_ = session_index;
 
   if (!email.empty())
     helper->email_ = email;
 
   if (continue_url.is_valid())
     helper->continue_url_ = continue_url;
 }
 
-void OneClickSigninHelper::RedirectToNTP(bool show_bubble) {
-  VLOG(1) << "OneClickSigninHelper::RedirectToNTP";
+void OneClickSigninHelper::RedirectToAppsPage(bool show_bubble) {
+  VLOG(1) << "OneClickSigninHelper::RedirectToAppsPage";

[Roger Tawa OOO till Jul 10th, 2013/03/22 14:23:23]

Please rename to RedirectToNtpOrAppsPage

[kmadhusu, 2013/03/22 15:03:09]

Done.

 
-  // Redirect to NTP with sign in bubble visible.
+  // Redirect to NTP/Apps page with sign in bubble visible.
   content::WebContents* contents = web_contents();
   Profile* profile =
       Profile::FromBrowserContext(contents->GetBrowserContext());
   PrefService* pref_service = profile->GetPrefs();
   if (show_bubble) {
     pref_service->SetBoolean(prefs::kSyncPromoShowNTPBubble, true);
     pref_service->SetString(prefs::kSyncPromoErrorMessage, error_message_);
   }
 
-  content::OpenURLParams params(
-      GURL(chrome::kChromeUINewTabURL),
-      content::Referrer(),
-      CURRENT_TAB,
-      content::PAGE_TRANSITION_AUTO_TOPLEVEL,
-      false);
+  GURL url(chrome::kChromeUINewTabURL);
+  if ((source_ == SyncPromoUI::SOURCE_APPS_PAGE_LINK) ||
+      (source_ == SyncPromoUI::SOURCE_MENU &&

[Roger Tawa OOO till Jul 10th, 2013/03/22 14:23:23]

Why does the menu redirect to the apps page?

[kmadhusu, 2013/03/22 15:03:09]

If there is an error during "signing to chrome" from wrench menu, we want to
display the error message in the apps page signin bubble. With instant extended
enabled, we can only add signin bubble in the apps page.

[Roger Tawa OOO till Jul 10th, 2013/03/22 20:44:01]

if that is the case with instant extended, then I'm not sure the condition is
correct.  We need to show the bubble for success and failures.  If the NTP can
no longer show the bubble when instant extended is enabled, then we should
always go to apps page.

I think the original code is patchset 1 is correct.  Or maybe the following
might be better:

GURL(chrome::search::IsInstantExtendedAPIEnabled() && show_bubble ?
chrome::kChromeUIAppsURL : chrome::kChromeUINewTabURL)

[kmadhusu, 2013/03/22 21:29:06]

Done. 
We should not use the "show_bubble" value to figure out the redirect page.

From my understanding of this code, we first load the redirect page without the
signin bubble and then once the signin request is complete, we reload the
redirect page with the success/failure result in the signin bubble.

With instant extended enabled, if we use "show_bubble" in the above said
condition, we will show the new tab page first and then we will show the apps
page. I don't think that will be a good user experience. WDYT?

+       chrome::search::IsInstantExtendedAPIEnabled())) {
+    // Navigate to Apps page.
+    url = GURL(chrome::kChromeUIAppsURL);
+  }
+  content::OpenURLParams params(url,
+                                content::Referrer(),
+                                CURRENT_TAB,
+                                content::PAGE_TRANSITION_AUTO_TOPLEVEL,
+                                false);
   contents->OpenURL(params);
 
   error_message_.clear();
 }
 
 void OneClickSigninHelper::RedirectToSignin() {
   VLOG(1) << "OneClickSigninHelper::RedirectToSignin";
 
   // Extract the existing sounce=X value.  Default to "2" if missing.
   SyncPromoUI::Source source =
       SyncPromoUI::GetSourceForSyncPromoURL(continue_url_);
   if (source == SyncPromoUI::SOURCE_UNKNOWN)
     source = SyncPromoUI::SOURCE_MENU;
   GURL page = SyncPromoUI::GetSyncPromoURL(GURL(), source, false);
 
   content::WebContents* contents = web_contents();
   contents->GetController().LoadURL(page,
                                     content::Referrer(),
                                     content::PAGE_TRANSITION_AUTO_TOPLEVEL,
                                     std::string());
 }
 
 void OneClickSigninHelper::CleanTransientState() {
   VLOG(1) << "OneClickSigninHelper::CleanTransientState";
   showing_signin_ = false;
   email_.clear();
   password_.clear();
   auto_accept_ = AUTO_ACCEPT_NONE;
-  source_ = SyncPromoUI::SOURCE_UNKNOWN;

[Roger Tawa OOO till Jul 10th, 2013/03/22 14:23:23]

This method is meant to reset the state of OneClickSigninHelper.  This should
not be removed.  Why do you need to remove it?

[kmadhusu, 2013/03/22 15:03:09]

OneClickSigninHelper::SigninSuccess() is called when signin request is complete.
We redirect to NTP or Apps page from OneClickSigninHelper::SigninSuccess(). If
we reset |source_| to SyncPromoUI::SOURCE_UNKNOWN here, we will lose the source
details while calling RedirectToAppsPage() from
OneClickSigninHelper::SigninSuccess() function. That's why I removed from here
and reset |source_|  at lines 1335 and 1135.

The other way to achieve the same is to store the SyncPromoUI::Source detail in
StateTracker. But it doesn't seem right.  Please let me know if there any better
to find the source details in OneClickSigninHelper::SigninSuccess().

[Roger Tawa OOO till Jul 10th, 2013/03/22 20:44:01]

Once you fix RedirectToNtpOrAppsPage() to no longer use |source_|, this problem
goes away.

[kmadhusu, 2013/03/22 21:29:06]

Done.

   switched_to_advanced_ = false;
   original_source_ = SyncPromoUI::SOURCE_UNKNOWN;
   continue_url_ = GURL();
 
   // Post to IO thread to clear pending email.
   Profile* profile =
       Profile::FromBrowserContext(web_contents()->GetBrowserContext());
   content::BrowserThread::PostTask(
       content::BrowserThread::IO, FROM_HERE,
       base::Bind(&ClearPendingEmailOnIOThread,
@@ skipping 33 matching lines @@
   const GURL url = contents->GetURL();
   Profile* profile =
       Profile::FromBrowserContext(contents->GetBrowserContext());
   VLOG(1) << "OneClickSigninHelper::DidStopLoading: url=" << url.spec();
 
   // If an error has already occured during the sign in flow, make sure to
   // display it to the user and abort the process.  Do this only for
   // explicit sign ins.
   if (!error_message_.empty() && auto_accept_ == AUTO_ACCEPT_EXPLICIT) {
     VLOG(1) << "OneClickSigninHelper::DidStopLoading: error=" << error_message_;
-    RedirectToNTP(true);
+    RedirectToAppsPage(true);
     return;
   }
 
   if (AreWeShowingSignin(url, source_, email_)) {
     if (!showing_signin_) {
       if (source_ == SyncPromoUI::SOURCE_UNKNOWN)
         LogOneClickHistogramValue(one_click_signin::HISTOGRAM_SHOWN);
       else
         LogHistogramValue(source_, one_click_signin::HISTOGRAM_SHOWN);
     }
@@ skipping 11 matching lines @@
         continue_url_.ReplaceComponents(replacements));
 
   // If there is no valid email or password yet, there is nothing to do.
   if (email_.empty() || password_.empty()) {
     VLOG(1) << "OneClickSigninHelper::DidStopLoading: nothing to do";
     if (continue_url_match && auto_accept_ == AUTO_ACCEPT_EXPLICIT)
       RedirectToSignin();
     std::string unused_value;
     if (net::GetValueForKeyInQuery(url, "ntp", &unused_value)) {
       SyncPromoUI::SetUserSkippedSyncPromo(profile);
-      RedirectToNTP(false);
+      RedirectToAppsPage(false);
     }
     return;
   }
 
   // When the user uses the first-run, ntp, or hotdog menu to sign in, then have
   // the option of checking the the box "Let me choose what to sync".  When the
   // sign in process started, the source parameter in the continue URL may have
   // indicated one of the three options above.  However, once this box is
   // checked, the source parameter will indicate settings.  This will only be
   // communicated back to chrome when Gaia redirects to the continue URL, and
@@ skipping 13 matching lines @@
     // If this is not an explicit sign in, we don't need to check if we landed
     // on the right continue URL.  This is important because the continue URL
     // may itself lead to a redirect, which means this function will never see
     // the continue URL go by.
     if (auto_accept_ == AUTO_ACCEPT_EXPLICIT) {
       DCHECK(source_ != SyncPromoUI::SOURCE_UNKNOWN);
       if (!continue_url_match) {
         VLOG(1) << "OneClickSigninHelper::DidStopLoading: invalid url='"
                 << url.spec()
                 << "' expected continue url=" << continue_url_;
+        source_ = SyncPromoUI::SOURCE_UNKNOWN;
         CleanTransientState();
         return;
       }
 
       // 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.  If so, it overrides the
       // current source.
       //
@@ skipping 98 matching lines @@
           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
-      // 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
-      // redirect back to webstore.
+      // NTP/Apps page after sign in completes. In the case of the settings
+      // page, it will get closed by SyncSetupHandler. In the case of webstore,
+      // it will redirect back to webstore.
       if (source_ != SyncPromoUI::SOURCE_SETTINGS &&
           source_ != SyncPromoUI::SOURCE_WEBSTORE_INSTALL) {
         signin_tracker_.reset(new SigninTracker(profile, this));
-        // Show the NTP, but don't show the signed-in bubble yet.
-        RedirectToNTP(false);
+        RedirectToAppsPage(false);
       }
       break;
     }
     case AUTO_ACCEPT_REJECTED_FOR_PROFILE:
       AddEmailToOneClickRejectedList(profile, email_);
       LogOneClickHistogramValue(one_click_signin::HISTOGRAM_REJECTED);
       break;
     default:
       NOTREACHED() << "Invalid auto_accept=" << auto_accept_;
       break;
@@ skipping 43 matching lines @@
         error_message_.clear();
         break;
       case GoogleServiceAuthError::SERVICE_UNAVAILABLE:
         error_message_ = l10n_util::GetStringUTF8(IDS_SYNC_UNRECOVERABLE_ERROR);
         break;
       default:
         error_message_ = l10n_util::GetStringUTF8(IDS_SYNC_ERROR_SIGNING_IN);
         break;
     }
   }
-
-  RedirectToNTP(true);
-  signin_tracker_.reset();
+  RedirectOnSigninComplete();
 }
 
 void OneClickSigninHelper::SigninSuccess() {
-  // Signed in now, so show the signed-in bubble.
-  RedirectToNTP(true);
+  RedirectOnSigninComplete();
+}
+
+void OneClickSigninHelper::RedirectOnSigninComplete() {
+  // Show the result in the sign-in bubble.
+  RedirectToAppsPage(true);
   signin_tracker_.reset();
+  source_ = SyncPromoUI::SOURCE_UNKNOWN;
 }