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

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 7e4904b73b579cd2dac920db23a458a6477f4c36..48df587049d4c1ae94821e7f66cdc55a65815843 100644
--- a/chrome/browser/ui/sync/one_click_signin_helper.cc
+++ b/chrome/browser/ui/sync/one_click_signin_helper.cc
@@ -30,6 +30,7 @@
 #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"
@@ -122,6 +123,10 @@ void LogHistogramValue(SyncPromoUI::Source source, int action) {
       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;
@@ -223,8 +228,8 @@ void ClearPendingEmailOnIOThread(content::ResourceContext* context) {
 }
 
 // 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;
@@ -865,8 +870,8 @@ void OneClickSigninHelper::ShowInfoBarIfPossible(net::URLRequest* request,
       }
     }
 
-    // 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;
@@ -955,10 +960,10 @@ void OneClickSigninHelper::ShowInfoBarUIThread(
     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());
@@ -968,12 +973,18 @@ void OneClickSigninHelper::RedirectToNTP(bool show_bubble) {
     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();
@@ -1002,7 +1013,6 @@ void OneClickSigninHelper::CleanTransientState() {
   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();
@@ -1056,7 +1066,7 @@ void OneClickSigninHelper::DidStopLoading(
   // explicit sign ins.
   if (!error_message_.empty() && auto_accept_ == AUTO_ACCEPT_EXPLICIT) {
     VLOG(1) << "OneClickSigninHelper::DidStopLoading: error=" << error_message_;
-    RedirectToNTP(true);
+    RedirectToAppsPage(true);
     return;
   }
 
@@ -1088,7 +1098,7 @@ void OneClickSigninHelper::DidStopLoading(
     std::string unused_value;
     if (net::GetValueForKeyInQuery(url, "ntp", &unused_value)) {
       SyncPromoUI::SetUserSkippedSyncPromo(profile);
-      RedirectToNTP(false);
+      RedirectToAppsPage(false);
     }
     return;
   }
@@ -1122,6 +1132,7 @@ void OneClickSigninHelper::DidStopLoading(
         VLOG(1) << "OneClickSigninHelper::DidStopLoading: invalid url='"
                 << url.spec()
                 << "' expected continue url=" << continue_url_;
+        source_ = SyncPromoUI::SOURCE_UNKNOWN;
         CleanTransientState();
         return;
       }
@@ -1240,14 +1251,13 @@ void OneClickSigninHelper::DidStopLoading(
       }
 
       // 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;
     }
@@ -1311,13 +1321,16 @@ void OneClickSigninHelper::SigninFailed(const GoogleServiceAuthError& error) {
         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;
 }