Data Reduction Proxy Interstitials

chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc

Index: chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
diff --git a/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc b/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
index a23756dd2d63df6c802ecb7030244570ac62eebc..4de73e61fefe16bf17b76b86928fdaffe4deee95 100644
--- a/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
+++ b/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
@@ -88,6 +88,8 @@
 
 #if defined(OS_ANDROID)
 #include "chrome/browser/android/intercept_download_resource_throttle.h"
+#include "components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.h"
+#include "components/data_reduction_proxy/core/common/data_reduction_proxy_params.h"
 #include "components/navigation_interception/intercept_navigation_delegate.h"
 #endif
 
@@ -109,6 +111,7 @@ using extensions::StreamsPrivateAPI;
 #endif
 
 #if defined(OS_ANDROID)
+using data_reduction_proxy::DataReductionProxyUIService;
 using navigation_interception::InterceptNavigationDelegate;
 #endif
 
@@ -522,6 +525,24 @@ void ChromeResourceDispatcherHostDelegate::AppendStandardResourceThrottles(
   }
 #endif
 
+#if defined(OS_ANDROID)

[mmenke, 2015/01/16 20:15:27]

All these OS_ANDROID checks seem very regression prone, and just a bad idea. 
Should either add a new #define for them
(ENABLE_DATA_REDUCTION_PROXY_DEBUGGING), which at makes it clear which ifdefs
are related how, or have a member of DRPPArams that we only set to true on
Android when we have the corresponding commandline option set, and look at that
everywhere.  The latter option will bload up binary size a little, of course
(And makes the platform restriction fairly pointless).

[megjablon, 2015/01/17 02:21:48]

Done.

+  if (io_data->IsDataReductionProxyEnabled() &&
+      data_reduction_proxy::DataReductionProxyParams::
+      IsInterstitalsCommandLineSwitchOn()) {

[mmenke, 2015/01/16 20:15:27]

Also suggest indenting IsInterstitalsCommandLineSwitchOn four more.

[mmenke, 2015/01/16 20:15:27]

I think "IsInterstitalsCommandLineSwitchOn" could be clearer.  Maybe
"ForceDataProxy" or "WarnIfNoDataProxy" or something.

[megjablon, 2015/01/17 02:21:48]

Done.

+    data_reduction_proxy::DataReductionProxyParams* params =
+        io_data->data_reduction_proxy_params();
+    data_reduction_proxy::DataReductionProxyUIService* service =
+        io_data->data_reduction_proxy_ui_service();
+    DCHECK(params);
+    DCHECK(service);
+
+    throttles->push_back(
+        new data_reduction_proxy::DataReductionProxyResourceThrottle(

[mmenke, 2015/01/16 20:15:27]

Suggest renaming this the "DataReductionProxyDebugResourceThrottle"

[megjablon, 2015/01/17 02:21:48]

Done.

+            request, resource_type, service, params));
+
+  }

[mmenke, 2015/01/16 20:15:27]

Think it's better if the RDHDelegate knows as little as possible about what the
service is up to.  So I suggest having only the following here:

scoped_ptr<content::ResourceThrottle> data_reduction_proxy_throttle =
data_reduction_proxy::DataReductionProxyResourceThrottle::MaybeCreate(...);
if (data_reduction_proxy_throttle )
  throttles.push_back(data_reduction_proxy_throttle.release());

The static MaybeCreate method checks the relevant command line flag, and if the
DRP is enabled, etc.

You may actually want to remove the enabled check - at least I assume when using
the flag, you'd care if the DRP were disabled due to the probe results.

[megjablon, 2015/01/17 02:21:48]

Done.

+#endif
+
 #if defined(ENABLE_SUPERVISED_USERS)
   bool is_subresource_request =
       resource_type != content::RESOURCE_TYPE_MAIN_FRAME;