Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(22)

Issue 23654014: Updated Client-Hints patch (Closed)

Created:
6 years ago by Yoav Weiss
Modified:
6 years ago
Reviewers:
bengr, mmenke
CC:
chromium-reviews, cbentzel+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

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.

Patch Set 1 #

Total comments: 26

Patch Set 2 : Fixed issues raised in review by mmenke and bengr #

Unified diffs Side-by-side diffs Delta from patch set Stats (+182 lines, -0 lines) Patch
M AUTHORS View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/io_thread.cc View 1 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/net/chrome_network_delegate.h View 1 3 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/net/chrome_network_delegate.cc View 1 4 chunks +12 lines, -0 lines 0 comments Download
A chrome/browser/net/client_hints.h View 1 1 chunk +42 lines, -0 lines 0 comments Download
A chrome/browser/net/client_hints.cc View 1 1 chunk +62 lines, -0 lines 0 comments Download
A chrome/browser/net/client_hints_unittest.cc View 1 1 chunk +48 lines, -0 lines 0 comments Download
M chrome/browser/profiles/profile_io_data.cc View 1 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/chrome_tests_unit.gypi View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_switches.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_switches.cc View 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 10 (0 generated)
Rik
https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hints.cc File chrome/browser/net/client_hints.cc (right): https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hints.cc#newcode45 chrome/browser/net/client_hints.cc:45: return screen_hints_; The screen_hints aren't updated if the devicePixelRatio ...
6 years ago (2013-09-07 20:24:58 UTC) #1
Yoav Weiss
On 2013/09/07 20:24:58, Rik wrote: > https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hints.cc > File chrome/browser/net/client_hints.cc (right): > > https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hints.cc#newcode45 > ...
6 years ago (2013-09-07 21:02:03 UTC) #2
mmenke
[+bengr] Hey Yoav, just for future reference, the best way to get the attention of ...
6 years ago (2013-09-09 16:37:13 UTC) #3
mmenke
A bunch of minor style comments. Haven't looked at the tests yet. https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/chrome_network_delegate.cc File chrome/browser/net/chrome_network_delegate.cc ...
6 years ago (2013-09-09 16:55:29 UTC) #4
Nicholas Shanks
https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hints.cc File chrome/browser/net/client_hints.cc (right): https://codereview.chromium.org/23654014/diff/1/chrome/browser/net/client_hints.cc#newcode16 chrome/browser/net/client_hints.cc:16: gfx::Display display = gfx::Screen::GetNativeScreen()->GetPrimaryDisplay(); Should this not be using ...
6 years ago (2013-09-11 10:22:46 UTC) #5
bengr
I think the biggest issues with the code are that the code doesn't (1) pick ...
6 years ago (2013-09-11 16:32:35 UTC) #6
bengr
On 2013/09/11 16:32:35, bengr1 wrote: > I think the biggest issues with the code are ...
6 years ago (2013-09-11 16:35:12 UTC) #7
Yoav Weiss
I believe I addressed all the small issues raised in the review. > I think ...
6 years ago (2013-09-12 09:12:45 UTC) #8
mmenke
Yoav: Since Ben is taking over the issue, mind if I close this one, to ...
6 years ago (2013-09-16 18:55:06 UTC) #9
Yoav Weiss
6 years ago (2013-09-16 19:01:24 UTC) #10
Message was sent while issue was closed.
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 :)

Powered by Google App Engine
This is Rietveld 408576698