Data Reduction Proxy Interstitials

chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.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/renderer_host/chrome_resource_dispatcher_host_delegate.h"
 
 #include <string>
 #include <vector>
 
 #include "base/base64.h"
@@ skipping 70 matching lines @@
 #endif
 
 #if defined(USE_SYSTEM_PROTOBUF)
 #include <google/protobuf/repeated_field.h>
 #else
 #include "third_party/protobuf/src/google/protobuf/repeated_field.h"
 #endif
 
 #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
 
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/chromeos/login/signin/merge_session_throttle.h"
 // TODO(oshima): Enable this for other platforms.
 #include "chrome/browser/renderer_host/offline_resource_throttle.h"
 #endif
 
 using content::BrowserThread;
 using content::RenderViewHost;
 using content::ResourceDispatcherHostLoginDelegate;
 using content::ResourceRequestInfo;
 using content::ResourceType;
 
 #if defined(ENABLE_EXTENSIONS)
 using extensions::Extension;
 using extensions::StreamsPrivateAPI;
 #endif
 
 #if defined(OS_ANDROID)
+using data_reduction_proxy::DataReductionProxyUIService;
 using navigation_interception::InterceptNavigationDelegate;
 #endif
 
 namespace {
 
 ExternalProtocolHandler::Delegate* g_external_protocol_handler_delegate = NULL;
 
 void NotifyDownloadInitiatedOnUI(int render_process_id, int render_view_id) {
   RenderViewHost* rvh = RenderViewHost::FromID(render_process_id,
                                                render_view_id);
@@ skipping 393 matching lines @@
     content::ResourceThrottle* throttle =
         SafeBrowsingResourceThrottleFactory::Create(request,
                                                     resource_context,
                                                     resource_type,
                                                     safe_browsing_.get());
     if (throttle)
       throttles->push_back(throttle);
   }
 #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;
   throttles->push_back(new SupervisedUserResourceThrottle(
         request, !is_subresource_request,
         io_data->supervised_user_url_filter()));
 #endif
 
 #if defined(ENABLE_EXTENSIONS)
   content::ResourceThrottle* throttle =
@@ skipping 215 matching lines @@
                                        url_request->GetTotalReceivedBytes()));
   }
 }
 
 // static
 void ChromeResourceDispatcherHostDelegate::
     SetExternalProtocolHandlerDelegateForTesting(
     ExternalProtocolHandler::Delegate* delegate) {
   g_external_protocol_handler_delegate = delegate;
 }