Avoid the long merge session throttle interstitial page for NTP on Chrome OS

chrome/browser/search/search.cc

 // Copyright 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/search/search.h"
 
 #include <stddef.h>
 
 #include "base/command_line.h"
 #include "base/metrics/field_trial.h"
@@ skipping 24 matching lines @@
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/web_contents.h"
 
 #if defined(ENABLE_SUPERVISED_USERS)
 #include "chrome/browser/supervised_user/supervised_user_service.h"
 #include "chrome/browser/supervised_user/supervised_user_service_factory.h"
 #include "chrome/browser/supervised_user/supervised_user_url_filter.h"
 #endif
 
+#if defined(OS_CHROMEOS)
+#include "chrome/browser/chromeos/login/signin/merge_session_throttling_utils.h"
+#endif  // defined(OS_CHROMEOS)
+
 namespace search {
 
 namespace {
 
 const char kPrefetchSearchResultsOnSRP[] = "prefetch_results_srp";
 const char kPrerenderInstantUrlOnOmniboxFocus[] =
     "prerender_instant_url_on_omnibox_focus";
 
 // Controls whether to use the alternate Instant search base URL. This allows
 // experimentation of Instant search.
@@ skipping 177 matching lines @@
     return NEW_TAB_URL_INCOGNITO;
   if (!new_tab_url.is_valid())
     return NEW_TAB_URL_NOT_SET;
   if (!new_tab_url.SchemeIsCryptographic())
     return NEW_TAB_URL_INSECURE;
   if (!IsURLAllowedForSupervisedUser(new_tab_url, profile))
     return NEW_TAB_URL_BLOCKED;
   return NEW_TAB_URL_VALID;
 }
 
+// On Chrome OS, if the session hasn't merged yet, we need to avoid loading the
+// remote NTP because that will trigger showing the merge session throttle
+// interstitial page, which can show for 5+ seconds. crbug.com/591530.
+bool ShouldShowLocalNewTab(const GURL& url, Profile* profile) {
+#if defined(OS_CHROMEOS)
+  if (!merge_session_throttling_utils::ShouldDelayUrl(url) ||
+      !merge_session_throttling_utils::ShouldDelayRequest(profile)) {

[Marc Treib, 2016/04/21 19:53:38]

Can you please swap the if around, to
if (ShouldDelayUrl(...) && ShouldDelayRequest(...)) return true;

Less negation makes for easier reading, and that way the "default case" for CrOS
and other platforms matches up.

Now that I look at it like this: Is it correct that you only want to show the
local NTP if *both* ShouldDelay functions return true? If so, would it make
sense to combine them into one ShouldDelay(profile, url) function?

[afakhry, 2016/04/22 00:41:14]

Done.

Regarding combining them into one function, I see both are used separately in a
couple of places. So I don't think we should combine them.

+    return false;
+  }
+
+  return true;
+#else
+  return false;
+#endif  // defined(OS_CHROMEOS)
+}
+
 // Used to look up the URL to use for the New Tab page. Also tracks how we
 // arrived at that URL so it can be logged with UMA.
 struct NewTabURLDetails {
   NewTabURLDetails(const GURL& url, NewTabURLState state)
       : url(url), state(state) {}
 
   static NewTabURLDetails ForProfile(Profile* profile) {
     const GURL local_url(chrome::kChromeSearchLocalNtpUrl);
 
     base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
     if (command_line->HasSwitch(switches::kForceLocalNtp))
       return NewTabURLDetails(local_url, NEW_TAB_URL_VALID);
 
     TemplateURL* template_url = GetDefaultSearchProviderTemplateURL(profile);
     if (!profile || !template_url)
       return NewTabURLDetails(local_url, NEW_TAB_URL_BAD);
 
     GURL search_provider_url = TemplateURLRefToGURL(
         template_url->new_tab_url_ref(), UIThreadSearchTermsData(profile),
         false, false);
+
+    if (ShouldShowLocalNewTab(search_provider_url, profile))
+      return NewTabURLDetails(local_url, NEW_TAB_URL_VALID);
+
     NewTabURLState state = IsValidNewTabURL(profile, search_provider_url);
     switch (state) {
       case NEW_TAB_URL_VALID:
         // We can use the search provider's page.
         return NewTabURLDetails(search_provider_url, state);
       case NEW_TAB_URL_INCOGNITO:
         // Incognito has its own New Tab.
         return NewTabURLDetails(GURL(), state);
       default:
         // Use the local New Tab otherwise.
@@ skipping 312 matching lines @@
       kUseAltInstantURL, false, flags);
 }
 
 bool ShouldUseSearchPathForInstant() {
   FieldTrialFlags flags;
   return GetFieldTrialInfo(&flags) && GetBoolValueForFlagWithDefault(
       kUseSearchPathForInstant, false, flags);
 }
 
 }  // namespace search