Introducing the OffDomainInclusionDetector to analyze resource requests for suspicious activity.

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 289 matching lines @@
 
   return true;
 }
 
 void ChromeResourceDispatcherHostDelegate::RequestBeginning(
     net::URLRequest* request,
     content::ResourceContext* resource_context,
     content::AppCacheService* appcache_service,
     ResourceType resource_type,
     ScopedVector<content::ResourceThrottle>* throttles) {
+  if (safe_browsing_)
+    safe_browsing_->OnResourceRequest(request);

[Nico, 2014/11/14 19:52:58]

nit: if someone helpfully deletes this in the future, I don't think any tests
would fail…

[gab, 2014/11/18 17:23:19]

Right, but this class itself is highly undertested already (it only has 3 smalls
tests in chrome_resource_dispatcher_host_delegate_browsertest.cc which appear to
be way too little to provide significant coverage of the logic in this class).

The simple addition in this CL could be unit tested easily if the dependencies
were injected (rather than obtained from g_browser_process) and could be mocked.
But it seems out-of-scope for this CL to modify dependency injection and
introduce unit tests for this class just for this.

I could also add a browser test which is a bit simpler (doesn't need a full
class overhaul as the real dependencies can be used), and simply hook a
TestSafeBrowsingService (just like the current TestDispatcherHostDelegate in
ChromeResourceDispatcherHostDelegateBrowserTest).
But a browser test seems overkill to test this...

In short, I don't think there is a good testing solution for this code as-is and
I think many many things in this class could use tests before this code is the
most important to test here.

Once the OffDomainInclusionDetector is fully hooked to provide incident reports,
I will add integration tests which will exercise this code (and before then I
will definitely notice if this line goes away as I'll be using this code daily).

+
   const ResourceRequestInfo* info = ResourceRequestInfo::ForRequest(request);
   bool is_prerendering =
       info->GetVisibilityState() == blink::WebPageVisibilityStatePrerender;
   if (is_prerendering) {
     // Requests with the IGNORE_LIMITS flag set (i.e., sync XHRs)
     // should remain at MAXIMUM_PRIORITY.
     if (request->load_flags() & net::LOAD_IGNORE_LIMITS) {
       DCHECK_EQ(request->priority(), net::MAXIMUM_PRIORITY);
     } else {
       request->SetPriority(net::IDLE);
@@ skipping 434 matching lines @@
                                        url_request->GetTotalReceivedBytes()));
   }
 }
 
 // static
 void ChromeResourceDispatcherHostDelegate::
     SetExternalProtocolHandlerDelegateForTesting(
     ExternalProtocolHandler::Delegate* delegate) {
   g_external_protocol_handler_delegate = delegate;
 }