DuplicateContentResourceHandler to monitor resources and track how many times th…

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/logging.h"
+#include "base/metrics/histogram.h"
+#include "content/browser/renderer_host/resource_request_info_impl.h"
+#include "net/base/io_buffer.h"
+#include "net/url_request/url_request.h"
+
+namespace content {
+namespace {

[gavinp, 2012/07/31 19:26:18]

Why not move the anon namespace out of content? It doesn't reference anything in
content.

[frankwang, 2012/07/31 20:26:18]

Done.

[darin (slow to review), 2012/07/31 20:35:22]

Sorry to contradict, but I think it is generally better to place the anonymous
namespace within the containing namespace.  That way, we are consistent from
file to file.  Sometimes the anon namespace will need to reference things from
the content:: namespace.  The last thing I want to see in source files under
src/content is 'content::Foo' or 'using content::Foo'.  Those just add noise to
the source.  There's no downside to putting the anon namespace inside the
content namespace, right?

+
+class GlobalDuplicateRecords {
+ public:
+  static GlobalDuplicateRecords* GetInstance() {
+    static GlobalDuplicateRecords records;
+    return &records;
+  }
+
+  std::set<MH_UINT32>* content_matches() {
+    return &content_matches_;
+  }
+
+  std::set<MH_UINT32>* content_and_url_matches() {
+    return &content_and_url_matches_;
+  }
+
+ private:
+  GlobalDuplicateRecords() {}
+  ~GlobalDuplicateRecords() {}
+
+  std::set<MH_UINT32> content_matches_;
+  std::set<MH_UINT32> content_and_url_matches_;
+};
+
+}  // namespace
+
+DuplicateContentResourceHandler::DuplicateContentResourceHandler(
+    scoped_ptr<ResourceHandler> next_handler,
+    ResourceType::Type resource_type,
+    net::URLRequest* request)
+    : LayeredResourceHandler(next_handler.Pass()),
+      resource_type_(resource_type),
+      bytes_read_(0),
+      request_(request),
+      pmurhash_ph1_(0),
+      pmurhash_pcarry_(0) {
+}
+
+DuplicateContentResourceHandler::~DuplicateContentResourceHandler() {
+}
+
+bool DuplicateContentResourceHandler::OnWillRead(
+    int request_id,
+    net::IOBuffer** buf,
+    int* buf_size,
+    int min_size) {
+  DCHECK_EQ(-1, min_size);
+
+  if (!next_handler_->OnWillRead(request_id, buf, buf_size, min_size))
+    return false;
+  read_buffer_ = *buf;
+  return true;
+}
+
+bool DuplicateContentResourceHandler::OnReadCompleted(

[gavinp, 2012/07/31 19:26:18]

You can move the first parameter up here, and indent the rest; saving a line.
Same for OnWillRead above as well.

[frankwang, 2012/07/31 20:26:18]

Is that the format? It seems like the parameters need to be on a different line
according to darin's guideline.

[gavinp, 2012/07/31 20:29:16]

Each parameter must be on its own line, but it is preferred to move them up to
the declaration line.  From the chromium coding standard:

For function declarations and definitions, put each argument on a separate line
unless everything fits on one line. You should only break this rule when the
arguments have some logical association like "int x, int y" or "char* str, int
str_len" where putting a few arguments together makes semantic sense.  Do NOT
collapse function arguments together just because you have "too many":

void OK1(TypeA first_argument, TypeB second_argument, TypeC third_argument);

// Prefer this to the next form when it's possible.
void OK2(TypeA first_argument,
         TypeB second_argument,
         TypeC thir_argument);

void OK3(
    TypeA first_argument,
    TypeB second_argument,
    TypeC third_argument);

// Only legal when |second_argument| and |third_argument| are intimately
// related, e.g. (x, y) coords.
void SpecialCase(TypeA first_argument,
                 TypeB second_argument, TypeC third_argument,
                 TypeD fourth_argument);

// Not legal, args are collapsed but overall decl doesn't fit on one line
void BadWrappingStyle(
    TypeA first_argument, TypeB second_argument, TypeC third_argument);

// Perhaps there is a way to break things up into functions with fewer
arguments...
void MyFunctionTakesTooManyArguments(Type first_argument, Type second_argument,
    Type third_argument, Type fourth_argument, Type fifth_argument,
    Type sixth_argument, Type seventh_argument, Type etc_argument);


Please change your functions to the OK2 format above where it fits.

[darin (slow to review), 2012/07/31 20:35:22]

gavin is correct.  sorry, i assumed since you had placed the method name on a
separate line that the OK2 format would not have worked.  i only suggested OK3
format assuming that OK2 format would not have worked.  sorry for the confusion
(and welcome to chromium style nit picking)!!

+    int request_id,
+    int bytes_read,
+    bool* defer) {
+  PMurHash32_Process(&pmurhash_ph1_, &pmurhash_pcarry_,
+                      read_buffer_->data(), bytes_read);
+  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() {
+  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_);
+
+  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();
+
+  const bool did_match_contents = content_matches->count(contents_hash) > 0;
+  const bool did_match_contents_and_url =
+      content_and_url_matches->count(hashed_with_url) > 0;
+
+  UMA_HISTOGRAM_BOOLEAN("Duplicate.Hits", did_match_contents);
+  UMA_HISTOGRAM_BOOLEAN("Duplicate.HitsSameUrl", did_match_contents &&

[gavinp, 2012/07/31 19:26:18]

Could we break this line after the ", " for better readability, without gaining
a line? looks to me like it fits.

[frankwang, 2012/07/31 20:26:18]

Yup, it does.

+                         did_match_contents_and_url);
+  if (did_match_contents && !did_match_contents_and_url) {
+    content_and_url_matches->insert(hashed_with_url);
+    UMA_HISTOGRAM_CUSTOM_COUNTS("Duplicate.Size.HashHitUrlMiss", bytes_read_,
+                                 1, 0x7FFFFFFF, 50);
+    UMA_HISTOGRAM_ENUMERATION("Duplicate.ResourceType.HashHitUrlMiss",
+                               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