Updated Client-Hints patch

chrome/browser/net/chrome_network_delegate.cc

Index: chrome/browser/net/chrome_network_delegate.cc
diff --git a/chrome/browser/net/chrome_network_delegate.cc b/chrome/browser/net/chrome_network_delegate.cc
index e02dca60f1ef848d035d1a6d7053f86ba7388893..232c18ba31a4f90c8ef66bb92ce369687de1b333 100644
--- a/chrome/browser/net/chrome_network_delegate.cc
+++ b/chrome/browser/net/chrome_network_delegate.cc
@@ -28,6 +28,7 @@
 #include "chrome/browser/extensions/extension_process_manager.h"
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/google/google_util.h"
+#include "chrome/browser/net/client_hints.h"
 #include "chrome/browser/net/connect_interceptor.h"
 #include "chrome/browser/net/load_time_stats.h"
 #include "chrome/browser/performance_monitor/performance_monitor.h"
@@ -79,6 +80,7 @@ bool ChromeNetworkDelegate::g_never_throttle_requests_ = false;
 namespace {
 
 const char kDNTHeader[] = "DNT";
+const char kClientHintsHeader[] = "CH";
 
 // If the |request| failed due to problems with a proxy, forward the error to
 // the proxy extension API.
@@ -530,6 +532,11 @@ void ChromeNetworkDelegate::set_predictor(
       new chrome_browser_net::ConnectInterceptor(predictor));
 }
 
+void ChromeNetworkDelegate::set_enable_client_hints() {
+  client_hints_.reset(new ClientHints());
+  client_hints_->Init();
+}
+
 // static
 void ChromeNetworkDelegate::NeverThrottleRequests() {
   g_never_throttle_requests_ = true;
@@ -614,6 +621,12 @@ int ChromeNetworkDelegate::OnBeforeURLRequest(
   if (enable_do_not_track_ && enable_do_not_track_->GetValue())
     request->SetExtraRequestHeaderByName(kDNTHeader, "1", true /* override */);
 
+  if (client_hints_ != NULL) {

[mmenke, 2013/09/09 16:55:30]

This is done a lot, but I believe "if (client_hints_) {" is the preferred style
for scoped_ptrs, now that it's supported.

+    std::string client_hints_value = client_hints_->GetScreenInfoHints();

[mmenke, 2013/09/09 16:55:30]

Suggest just inlining this call below.  Not a huge performance issue, but it
removes a string copy.

[mmenke, 2013/09/09 16:55:30]

I think this function name (GetScreenInfoHints()) is a little odd.  Seems like
client_hints_ should handle all the logic of what hints we want and don't want
itself, and the calling code shouldn't know or care if it includes screen info,
or what.

I'd suggest just calling it GetClientHintsString, or even just GetString, since
we already know it's a ClientHints object we're calling.

Another option would be to make the ClientHints object do all the work, and have
it work like client_hints_->AddHintsToRequest(request_), and then have the
ClientHints object do all the work.  Other reviewers may not like this idea.

[bengr, 2013/09/11 16:32:36]

I like GetString(), or even ToString().

+    request->SetExtraRequestHeaderByName(kClientHintsHeader,
+                                         client_hints_value, true);
+  }
+
   bool force_safe_search = force_google_safe_search_ &&
                            force_google_safe_search_->GetValue();