Only track http and https in duplicate resource handler metrics

content/browser/renderer_host/duplicate_content_resource_handler.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 "content/browser/renderer_host/duplicate_content_resource_handler.h"
 
 #include <set>
 
 #include "base/lazy_instance.h"
 #include "base/logging.h"
@@ skipping 54 matching lines @@
   if (!next_handler_->OnWillRead(request_id, buf, buf_size, min_size))
     return false;
   read_buffer_ = *buf;
   return true;
 }
 
 bool DuplicateContentResourceHandler::OnReadCompleted(int request_id,
                                                       int bytes_read,
                                                       bool* defer) {
   PMurHash32_Process(&pmurhash_ph1_, &pmurhash_pcarry_,
                       read_buffer_->data(), bytes_read);

[gavinp, 2012/08/12 16:17:14]

The hash is calculated incrementally here. If the request isn't http or https,
isn't before this is called the time to stop?

[frankwang, 2012/08/13 04:32:17]

I missed this too and agree with Gavin. I also take back my L G T M. The
URLRequest is passed in from the resource dispatcher. To save performance, you
should have a check here to pass through to the next handler if it isn't http or
https.

   bytes_read_ += bytes_read;
   return next_handler_->OnReadCompleted(request_id, bytes_read, defer);
 }
 
 bool DuplicateContentResourceHandler::OnResponseCompleted(
     int request_id,
     const net::URLRequestStatus& status,
     const std::string& security_info) {
 
   if (status.is_success())
     RecordContentMetrics();
 
   return next_handler_->OnResponseCompleted(request_id, status, security_info);
 }
 
 void DuplicateContentResourceHandler::RecordContentMetrics() {
+  // Ignore everything that's not http/https. Specifically, exclude data and
+  // blob URLs which can be generated by content and cause the maintained sets
+  // to grow without bounds.
+  if (!request_->url().SchemeIs("http") && !request_->url().SchemeIs("https")) {

[gavinp, 2012/08/09 20:20:45]

I'm taking my LGTM back. This code calculates the hash on every resource, when
it could save some time by just setting a bool early on.

[scottmg, 2012/08/09 20:24:56]

Sorry, you lost me? There's a return here, so if the request is not http and not
https we early out.

[gavinp, 2012/08/12 16:17:14]

See my comment on line 74, above.

On 2012/08/09 20:24:56, scottmg wrote:

+    bytes_read_ = 0;
+    read_buffer_ = NULL;
+    return;
+  }
   MH_UINT32 contents_hash = PMurHash32_Result(pmurhash_ph1_,
                                               pmurhash_pcarry_, bytes_read_);
-
   // Combine the contents_hash with the url, so we can test if future content
   // identical resources have the same original url or not.
-  MH_UINT32 hashed_with_url;
   const std::string& url_spec = request_->url().spec();
   PMurHash32_Process(&pmurhash_ph1_, &pmurhash_pcarry_,
                      url_spec.data(), url_spec.length());
-  hashed_with_url = PMurHash32_Result(pmurhash_ph1_, pmurhash_pcarry_,
-                                      url_spec.length() + bytes_read_);
+  MH_UINT32 hashed_with_url = PMurHash32_Result(

[gavinp, 2012/08/09 18:32:00]

nice cleanup.

+      pmurhash_ph1_, pmurhash_pcarry_, url_spec.length() + bytes_read_);
 
   DVLOG(4) << "url: " << url_spec;
   DVLOG(4) << "contents hash: " << contents_hash;
   DVLOG(4) << "hash with url: " << hashed_with_url;
 
   std::set<MH_UINT32>* content_matches =
       GlobalDuplicateRecords::GetInstance()->content_matches();
   std::set<MH_UINT32>* content_and_url_matches =
       GlobalDuplicateRecords::GetInstance()->content_and_url_matches();
 
@@ skipping 12 matching lines @@
                                resource_type_, ResourceType::LAST_TYPE);
   }
   content_matches->insert(contents_hash);
   content_and_url_matches->insert(hashed_with_url);
 
   bytes_read_ = 0;
   read_buffer_ = NULL;
 }
 
 }  // namespace content