Updated Client-Hints patch

Updated Client-Hints patch

Specification: https://raw.github.com/igrigorik/http-client-hints/master/draft-grigorik-http-client-hints-01.txt

BUG=170388
CONTRIBUTOR= bengr@chromium.org

This CL updates https://codereview.chromium.org/11970002

This is an update to Ben Greenstein's patch, so it'd apply, build and pass tests.
I've removed the dw and dh hints, since AFAIK, they are not part of the current "Intent to implement".
I also fixed a locale related issue I've encountered, and added a non-regression test for that.

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

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
File chrome/browser/net/client_hints.cc (right):

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
chrome/browser/net/client_hints.cc:45: return screen_hints_;
The screen_hints aren't updated if the devicePixelRatio changes. Should they?

[Yoav Weiss, 2013-09-07 21:02:03]

On 2013/09/07 20:24:58, Rik wrote:
>
https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
> File chrome/browser/net/client_hints.cc (right):
> 
>
https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
> chrome/browser/net/client_hints.cc:45: return screen_hints_;
> The screen_hints aren't updated if the devicePixelRatio changes. Should they?

IMO, they should. OTOH, we cannot poll the UI thread synchronously before
issuing requests.
I'm not sure how, but the best way to do this would be to get notifications upon
DPR change. Otherwise, periodic async polling may work too.
I don't think this a blocking issue before landing this as an experimental
feature behind a runtime flag.

[mmenke, 2013-09-09 16:37:13]

[+bengr]

Hey Yoav, just for future reference, the best way to get the attention of
reviewers is to set the reviewer's field of the CL and then publish a message. 
The first message will automatically list them as reviewers, so no need to say
anything, though it doesn't hurt.  If you add reviewers later, best to add them
as reviewers and then publish a comment along the lines of "[+someguy]:  Please
take a look at the FooDelgateViewHostTabThingy."

I'll skim through this now, but I'll largely defer to ben on this one.

[mmenke, 2013-09-09 16:55:29]

A bunch of minor style comments.  Haven't looked at the tests yet.

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/chrome_net...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/chrome_net...
chrome/browser/net/chrome_network_delegate.cc:624: if (client_hints_ != NULL) {
This is done a lot, but I believe "if (client_hints_) {" is the preferred style
for scoped_ptrs, now that it's supported.

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/chrome_net...
chrome/browser/net/chrome_network_delegate.cc:625: std::string
client_hints_value = client_hints_->GetScreenInfoHints();
Suggest just inlining this call below.  Not a huge performance issue, but it
removes a string copy.

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/chrome_net...
chrome/browser/net/chrome_network_delegate.cc:625: std::string
client_hints_value = client_hints_->GetScreenInfoHints();
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.

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/chrome_net...
File chrome/browser/net/chrome_network_delegate.h (right):

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/chrome_net...
chrome/browser/net/chrome_network_delegate.h:14: #include
"chrome/browser/net/client_hints.h"
This should be forward declared instead of included in the header.  Should go
right before "class ConnectInterceptor" below, to be in alphabetical order.

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/chrome_net...
chrome/browser/net/chrome_network_delegate.h:95: void set_enable_client_hints();
Only inlined functions should use this naming style.  And generally if you call
something that's not inlined, the calling function shouldn't be inlined either.

So this should be "SetEnableClientHints()".  I actually think the "set" is
redundant, and "EnableClientHints()" would be a little clearer.

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
File chrome/browser/net/client_hints.cc (right):

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
chrome/browser/net/client_hints.cc:23: : weak_ptr_factory_(this) {
nit:  4 space indent (Or put this on the previous line)

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
chrome/browser/net/client_hints.cc:55: it++)
nit:  These should be aligned with the "std" of the first line.  You can also
put the it++ on the previous line.

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
chrome/browser/net/client_hints.cc:55: it++)
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.

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
File chrome/browser/net/client_hints.h (right):

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
chrome/browser/net/client_hints.h:15: // draft-grigorik-http-client-hints-00.txt
Nit:  Put the URL in one line.  You'll get a presubmit warning about it, but go
ahead and ignore it.

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
chrome/browser/net/client_hints.h:38: const std::string& GetScreenInfoHints();
nit:  "const std::string& GetScreenInfoHints() const;"

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

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
File chrome/browser/net/client_hints.cc (right):

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
chrome/browser/net/client_hints.cc:16: gfx::Display display =
gfx::Screen::GetNativeScreen()->GetPrimaryDisplay();
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:35]

I think the biggest issues with the code are that the code doesn't (1) pick up
dpr changes during a session, or (2) support multiple displays with different
dprs.

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/chrome_net...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/chrome_net...
chrome/browser/net/chrome_network_delegate.cc:625: std::string
client_hints_value = client_hints_->GetScreenInfoHints();
On 2013/09/09 16:55:30, mmenke wrote:
> 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.

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

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
File chrome/browser/net/client_hints.cc (right):

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
chrome/browser/net/client_hints.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
2013, remove (c)

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
chrome/browser/net/client_hints.cc:16: gfx::Display display =
gfx::Screen::GetNativeScreen()->GetPrimaryDisplay();
On 2013/09/11 10:22:46, Nicholas Shanks wrote:
> 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?

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

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
chrome/browser/net/client_hints.cc:45: return screen_hints_;
On 2013/09/07 20:24:58, Rik wrote:
> The screen_hints aren't updated if the devicePixelRatio changes. Should they?

I think they should be.

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
chrome/browser/net/client_hints.cc:48: void
ClientHints::UpdateScreenInfo(ClientHints::ScreenInfo* info) {
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.

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
chrome/browser/net/client_hints.cc:51: // Make sure CH value doesn't change
according to the machine's locale
nit: "sure the Client Hints value"

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
File chrome/browser/net/client_hints.h (right):

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
chrome/browser/net/client_hints.h:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
2013, and remove (c).

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
chrome/browser/net/client_hints.h:18: // Contains information about the client
device.
I believe the current thinking is that we'll only be reporting dpr, so this
struct isn't needed.

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
chrome/browser/net/client_hints.h:33: // Returns client hints pertaining to
screen information. The format is:
Update comment to reflect that we are only reporting dpr.

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
chrome/browser/net/client_hints.h:37: // screen.height, screen.width, and
window.devicePixelRatio.
remove screen.height and screen.width.

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
chrome/browser/net/client_hints.h:43: void UpdateScreenInfo(ScreenInfo* info);
This can be a float* device_pixel_ratio.

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
File chrome/browser/net/client_hints_unittest.cc (right):

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
chrome/browser/net/client_hints_unittest.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
2013, remove (c)

https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
chrome/browser/net/client_hints_unittest.cc:26: UpdateScreenInfo(100, 200,
1.567f);
remove device width/height from tests because they won't be used.

https://codereview.chromium.org/23654014/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/23654014/diff/1/chrome/chrome_browser.gypi#ne...
chrome/chrome_browser.gypi:1150: 'browser/net/client_hints.cc',
alphabetize.

[bengr, 2013-09-11 16:35:12]

On 2013/09/11 16:32:35, bengr1 wrote:
> I think the biggest issues with the code are that the code doesn't (1) pick up
> dpr changes during a session, or (2) support multiple displays with different
> dprs.
> 
>
https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/chrome_net...
> File chrome/browser/net/chrome_network_delegate.cc (right):
> 
>
https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/chrome_net...
> chrome/browser/net/chrome_network_delegate.cc:625: std::string
> client_hints_value = client_hints_->GetScreenInfoHints();
> On 2013/09/09 16:55:30, mmenke wrote:
> > 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.
> 
> I like GetString(), or even ToString().
> 
>
https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
> File chrome/browser/net/client_hints.cc (right):
> 
>
https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
> chrome/browser/net/client_hints.cc:1: // Copyright (c) 2012 The Chromium
> Authors. All rights reserved.
> 2013, remove (c)
> 
>
https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
> chrome/browser/net/client_hints.cc:16: gfx::Display display =
> gfx::Screen::GetNativeScreen()->GetPrimaryDisplay();
> On 2013/09/11 10:22:46, Nicholas Shanks wrote:
> > 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?
> 
> This should really be the dpi of the device where the Tab associated with the
> request is being displayed.
> 
>
https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
> chrome/browser/net/client_hints.cc:45: return screen_hints_;
> On 2013/09/07 20:24:58, Rik wrote:
> > The screen_hints aren't updated if the devicePixelRatio changes. Should
they?
> 
> I think they should be.
> 
>
https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
> chrome/browser/net/client_hints.cc:48: void
> ClientHints::UpdateScreenInfo(ClientHints::ScreenInfo* info) {
> 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.
> 
>
https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
> chrome/browser/net/client_hints.cc:51: // Make sure CH value doesn't change
> according to the machine's locale
> nit: "sure the Client Hints value"
> 
>
https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
> File chrome/browser/net/client_hints.h (right):
> 
>
https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
> chrome/browser/net/client_hints.h:1: // Copyright (c) 2012 The Chromium
Authors.
> All rights reserved.
> 2013, and remove (c).
> 
>
https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
> chrome/browser/net/client_hints.h:18: // Contains information about the client
> device.
> I believe the current thinking is that we'll only be reporting dpr, so this
> struct isn't needed.
> 
>
https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
> chrome/browser/net/client_hints.h:33: // Returns client hints pertaining to
> screen information. The format is:
> Update comment to reflect that we are only reporting dpr.
> 
>
https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
> chrome/browser/net/client_hints.h:37: // screen.height, screen.width, and
> window.devicePixelRatio.
> remove screen.height and screen.width.
> 
>
https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
> chrome/browser/net/client_hints.h:43: void UpdateScreenInfo(ScreenInfo* info);
> This can be a float* device_pixel_ratio.
> 
>
https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
> File chrome/browser/net/client_hints_unittest.cc (right):
> 
>
https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
> chrome/browser/net/client_hints_unittest.cc:1: // Copyright (c) 2012 The
> Chromium Authors. All rights reserved.
> 2013, remove (c)
> 
>
https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hin...
> chrome/browser/net/client_hints_unittest.cc:26: UpdateScreenInfo(100, 200,
> 1.567f);
> remove device width/height from tests because they won't be used.
> 
> https://codereview.chromium.org/23654014/diff/1/chrome/chrome_browser.gypi
> File chrome/chrome_browser.gypi (right):
> 
>
https://codereview.chromium.org/23654014/diff/1/chrome/chrome_browser.gypi#ne...
> chrome/chrome_browser.gypi:1150: 'browser/net/client_hints.cc',
> alphabetize.

Yoav, also, I had planned to finish my CL in the next week. If you'd like, I
could merge your changes back into my CL and finish it off. Let me know what
you'd like to do.

[Yoav Weiss, 2013-09-12 09:12:45]

I believe I addressed all the small issues raised in the review.

> I think the biggest issues with the code are that the code doesn't (1) pick up
> dpr changes during a session, or (2) support multiple displays with different
> dprs.

I did not address these larger issues. I agree that they should be addressed.

> Yoav, also, I had planned to finish my CL in the next week. If you'd like, I
> could merge your changes back into my CL and finish it off. Let me know what
> you'd like to do.

Ben, the reason I started working on updating your patch was the responsive
image meetup, that took place on 10/9.
I wanted to arrive there with working code and better understanding of
implementation constraints of CH. 
Since the remaining issues seem to require more familiarity with Chromium's
architecture than I currently have (esp. issue #1), I think it would be best if
you would continue this work.

[mmenke, 2013-09-16 18:55:06]

Yoav:  Since Ben is taking over the issue, mind if I close this one, to get it
off of my pending review list?  Or feel free to close it yourself.

On 2013/09/12 09:12:45, Yoav Weiss wrote:
> I believe I addressed all the small issues raised in the review.
> 
> > I think the biggest issues with the code are that the code doesn't (1) pick
up
> > dpr changes during a session, or (2) support multiple displays with
different
> > dprs.
> 
> I did not address these larger issues. I agree that they should be addressed.
> 
> > Yoav, also, I had planned to finish my CL in the next week. If you'd like, I
> > could merge your changes back into my CL and finish it off. Let me know what
> > you'd like to do.
> 
> Ben, the reason I started working on updating your patch was the responsive
> image meetup, that took place on 10/9.
> I wanted to arrive there with working code and better understanding of
> implementation constraints of CH. 
> Since the remaining issues seem to require more familiarity with Chromium's
> architecture than I currently have (esp. issue #1), I think it would be best
if
> you would continue this work.

[Yoav Weiss, 2013-09-16 19:01:24]

On 2013/09/16 18:55:06, mmenke wrote:
> Yoav:  Since Ben is taking over the issue, mind if I close this one, to get it
> off of my pending review list?  Or feel free to close it yourself.
> 


Closed. Thanks for reviewing :)