Updated Client-Hints patch

chrome/browser/net/client_hints.cc

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.

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

2013, remove (c)

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/net/client_hints.h"
+
+#include "base/bind.h"
+#include "base/strings/stringprintf.h"
+#include "content/public/browser/browser_thread.h"
+#include "ui/gfx/screen.h"
+
+namespace {
+
+void FetchScreenInfoOnUIThread(ClientHints::ScreenInfo* info) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  gfx::Display display = gfx::Screen::GetNativeScreen()->GetPrimaryDisplay();

[Nicholas Shanks, 2013/09/11 10:22:46]

Should this not be using the display that (the majority of) the page will be
rendered to, rather than the primary display? I often have my IDE window on my
laptop's primary, HighDPI display and the web browser on a secondary (standard
DPI) display. I imagine it could easily be the other way around for some users.
Perhaps use the highest DPR of all connected screens?

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

This should really be the dpi of the device where the Tab associated with the
request is being displayed.

+  info->pixel_ratio = display.device_scale_factor();
+}
+
+}  // namespace
+
+ClientHints::ClientHints()
+  : weak_ptr_factory_(this) {

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

nit:  4 space indent (Or put this on the previous line)

+}
+
+ClientHints::~ClientHints() {
+}
+
+void ClientHints::Init() {
+  RetrieveScreenInfo();
+}
+
+bool ClientHints::RetrieveScreenInfo() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
+  ClientHints::ScreenInfo* info = new ClientHints::ScreenInfo();
+  return content::BrowserThread::PostTaskAndReply(
+      content::BrowserThread::UI,
+      FROM_HERE,
+      base::Bind(&FetchScreenInfoOnUIThread, info),
+      base::Bind(&ClientHints::UpdateScreenInfo, weak_ptr_factory_.GetWeakPtr(),
+                 base::Owned(info)));
+}
+
+const std::string& ClientHints::GetScreenInfoHints() {
+  return screen_hints_;

[Rik, 2013/09/07 20:24:58]

The screen_hints aren't updated if the devicePixelRatio changes. Should they?

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

I think they should be.

+}
+
+void ClientHints::UpdateScreenInfo(ClientHints::ScreenInfo* info) {

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

It doesn't look like info is being modified. If you retain ScreenInfo, pass it
in as a const reference. I suggest just passing in a float (dpr) by value.

+  if (info->pixel_ratio > 0.0) {
+    std::string pixel_ratio = base::StringPrintf("%.2f", info->pixel_ratio);
+    // Make sure CH value doesn't change according to the machine's locale

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

nit: "sure the Client Hints value"

+    std::locale locale;
+    for (std::string::iterator it = pixel_ratio.begin();
+            it != pixel_ratio.end();
+            it++)

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

nit:  These should be aligned with the "std" of the first line.  You can also
put the it++ on the previous line.

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

Iterators should use pre-increments, for efficiency (++it).  Otherwise, they
have to both increment the value and return the old value, so the compiler will
need to make a copy of the old value.  Smart compilers may optimize that out,
but best not to rely on it.

+      if (!std::isdigit(*it, locale))
+        *it = '.';
+    screen_hints_ = std::string("dpr=") + pixel_ratio;
+  }
+}
+