Update prerender policy for custom tabs

chrome/browser/prerender/prerender_manager.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/prerender/prerender_manager.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <functional>
@@ skipping 279 matching lines @@
   if (!IsOmniboxEnabled(profile_))
     return NULL;
   return AddPrerender(ORIGIN_OMNIBOX, url, content::Referrer(), size,
                       session_storage_namespace);
 }
 
 PrerenderHandle* PrerenderManager::AddPrerenderFromExternalRequest(
     const GURL& url,
     const content::Referrer& referrer,
     SessionStorageNamespace* session_storage_namespace,
-    const gfx::Size& size) {
-  return AddPrerender(ORIGIN_EXTERNAL_REQUEST, url, referrer, size,
-                      session_storage_namespace);
+    const gfx::Size& size,
+    bool prerender_on_mobile) {

[mmenke, 2016/03/14 20:38:01]

This is weird, for a couple reasons:

* This method is only used on mobile.
* Presumably we know at build time if we're on mobile.
* It can be false on mobile.

I also am not a fan of bool arguments passed in by value - "false" just sin't
too meaningful.

I suggest either we make this an enum class within PrerenderManager (And rename
it - prerender_on_cellular?) or make a new method
AddPrerenderOnCellularFromExternalRequest (Or somesuch)

[Yusuf, 2016/03/14 23:08:19]

Done. Added AddPrerenderOnCellularFromExternalRequest and renamed all relevant
calls and variables to say cellular instead of mobile. Removed custom tabs
mentions.

+  Origin origin = prerender_on_mobile
+      ? ORIGIN_EXTERNAL_REQUEST_FORCED_CUSTOMTABS

[mmenke, 2016/03/14 20:38:01]

Mapping "prerender_on_mobile == true" to
"ORIGIN_EXTERNAL_REQUEST_FORCED_CUSTOMTABS" seems weird - PrerenderManager
shouldn't know that only "CUSTOMTABS" (?) requests prerender on cellular
connections.

[Yusuf, 2016/03/14 23:08:20]

Done.

+      : ORIGIN_EXTERNAL_REQUEST;
+  return AddPrerender(origin, url, referrer, size, session_storage_namespace);
 }
 
 PrerenderHandle* PrerenderManager::AddPrerenderForInstant(
     const GURL& url,
     content::SessionStorageNamespace* session_storage_namespace,
     const gfx::Size& size) {
   DCHECK(search::ShouldPrefetchSearchResults());
   return AddPrerender(ORIGIN_INSTANT, url, content::Referrer(), size,
                       session_storage_namespace);
 }
@@ skipping 468 matching lines @@
 bool PrerenderManager::DoesSubresourceURLHaveValidScheme(const GURL& url) {
   return DoesURLHaveValidScheme(url) || url == GURL(url::kAboutBlankURL);
 }
 
 base::DictionaryValue* PrerenderManager::GetAsValue() const {
   DCHECK(CalledOnValidThread());
   base::DictionaryValue* dict_value = new base::DictionaryValue();
   dict_value->Set("history", prerender_history_->GetEntriesAsValue());
   dict_value->Set("active", GetActivePrerendersAsValue());
   dict_value->SetBoolean("enabled",
       GetPredictionStatus() == NetworkPredictionStatus::ENABLED);

[mmenke, 2016/03/14 20:38:01]

The reason for disabling seems more relevant here, now that we're selectively
overriding it.  Seems like we should output which of the three states we're in
here.

I'd really like to get rid of GetPredictionStatus(), now that it's not accurate
for all origins, but the other two pre-existing calls are a bit funky.

[Yusuf, 2016/03/14 23:08:20]

By three states do you mean NetworkPredictionStatus values? (disable,
disabled_network, enable)

Sorry, I am not clear on what you mean by the two pre-existing values.

[mmenke, 2016/03/15 14:34:21]

The two other pre-existing calls to GetPredictionStatus(), besides this one and
the one you replaced with the new method.  It's weird to have separate notions
of general prediction status, an per-origin prediction status...

Regardless, now that we have magic to ignore the disable on cellular state in
certain cases, it seems like we should really be distinguishing the reasons
prerendering was disabled here, since they have different behaviors now.

[Yusuf, 2016/03/20 06:48:29]

Done. Added a disabled note. Removed two calls to GetPredictionStatus(). Now the
only ones left are the ones here and the one in AddProfileNetworkBytesIfEnabled
both of which is not aware of origin.

   dict_value->SetBoolean("omnibox_enabled", IsOmniboxEnabled(profile_));
   // If prerender is disabled via a flag this method is not even called.
   std::string enabled_note;
   if (IsControlGroup())
     enabled_note += "(Control group: Not actually prerendering) ";
   if (IsNoUseGroup())
     enabled_note += "(No-use group: Not swapping in prerendered pages) ";
   if (GetMode() == PRERENDER_MODE_EXPERIMENT_15MIN_TTL_GROUP)
     enabled_note +=
         "(15 min TTL group: Extended prerender eviction to 15 mins) ";
@@ skipping 125 matching lines @@
 
   GURL url = url_arg;
   GURL alias_url;
   if (IsControlGroup() && MaybeGetQueryStringBasedAliasURL(url, &alias_url))
     url = alias_url;
 
   // From here on, we will record a FinalStatus so we need to register with the
   // histogram tracking.
   histograms_->RecordPrerender(origin, url_arg);
 
-  NetworkPredictionStatus prerendering_status = GetPredictionStatus();
+  NetworkPredictionStatus prerendering_status =
+      GetPredictionStatusForOrigin(origin);
   if (prerendering_status != NetworkPredictionStatus::ENABLED) {
     FinalStatus final_status =
         prerendering_status == NetworkPredictionStatus::DISABLED_DUE_TO_NETWORK
             ? FINAL_STATUS_CELLULAR_NETWORK
             : FINAL_STATUS_PRERENDERING_DISABLED;
     RecordFinalStatusWithoutCreatingPrerenderContents(url, origin,
                                                       final_status);
     return nullptr;
   }
 
@@ skipping 330 matching lines @@
   DCHECK_GE(recent_profile_bytes, 0);
   histograms_->RecordNetworkBytes(
       origin, used, prerender_bytes, recent_profile_bytes);
 }
 
 NetworkPredictionStatus PrerenderManager::GetPredictionStatus() const {
   DCHECK(CalledOnValidThread());
   return CanPrefetchAndPrerenderUI(profile_->GetPrefs());
 }
 
+NetworkPredictionStatus PrerenderManager::GetPredictionStatusForOrigin(
+    Origin origin) const {
+  NetworkPredictionStatus prediction_status = GetPredictionStatus();
+  if (prediction_status == NetworkPredictionStatus::DISABLED_DUE_TO_NETWORK
+      && origin == ORIGIN_EXTERNAL_REQUEST_FORCED_CUSTOMTABS) {

[mmenke, 2016/03/14 20:38:01]

Do we really want to do this?  How large are AMP pages?  I'm concerned both
about the performance cost and the bandwidth, particularly in emerging markets,
where people are often on 2G connections, and data expenses make up a
significant portion of people's income.

[Yusuf, 2016/03/14 23:08:20]

AMP in general is pretty cheap.
On our end, we never call the external request call on low end phones and also
heavily throttle calls that doesn't end up following with a corresponding intent
launch and ban apps from using this codepath that repeatedly get caught doing
this. 

+    prediction_status = NetworkPredictionStatus::ENABLED;
+  }
+  return prediction_status;
+}
+
 void PrerenderManager::AddProfileNetworkBytesIfEnabled(int64_t bytes) {
   DCHECK_GE(bytes, 0);
   if (GetPredictionStatus() == NetworkPredictionStatus::ENABLED &&
       ActuallyPrerendering())
     profile_network_bytes_ += bytes;
 }
 
 void PrerenderManager::AddPrerenderProcessHost(
     content::RenderProcessHost* process_host) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
@@ skipping 13 matching lines @@
 }
 
 void PrerenderManager::RenderProcessHostDestroyed(
     content::RenderProcessHost* host) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   size_t erased = prerender_process_hosts_.erase(host);
   DCHECK_EQ(1u, erased);
 }
 
 }  // namespace prerender