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

Issue 11970002: Added Client-Hints. (Closed)

Created:
6 years, 2 months ago by bengr (incorrect)
Modified:
5 years, 5 months ago
CC:
chromium-reviews, joi+watch-content_chromium.org, cbentzel+watch_chromium.org, darin-cc_chromium.org, Randy Smith (Not in Mondays)
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

Patch Set 1 #

Patch Set 2 : Nit #

Total comments: 28

Patch Set 3 : Added unit tests #

Total comments: 16

Patch Set 4 : Addressed comments and put behind flag #

Patch Set 5 : Nit #

Patch Set 6 : Restricted client hints to a very simple initial set #

Patch Set 7 : Changed Client Hint names to match spec #

Total comments: 14

Patch Set 8 : Used WeakPtr and addressed comments #

Patch Set 9 : Changed ClientHints to use WeakPtr and addressed comments #

Total comments: 14

Patch Set 10 : Removed RefCountedThreadSafe, added tests. #

Total comments: 12

Patch Set 11 : Addressed comments #

Total comments: 11

Patch Set 12 : Added Init #

Patch Set 13 : Made float literals explicit in tests #

Unified diffs Side-by-side diffs Delta from patch set Stats (+184 lines, -0 lines) Patch
M chrome/browser/io_thread.cc View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/net/chrome_network_delegate.h View 1 2 3 4 5 6 7 8 9 3 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/net/chrome_network_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +13 lines, -0 lines 0 comments Download
A chrome/browser/net/client_hints.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +52 lines, -0 lines 0 comments Download
A chrome/browser/net/client_hints.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +57 lines, -0 lines 0 comments Download
A chrome/browser/net/client_hints_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +45 lines, -0 lines 0 comments Download
M chrome/browser/profiles/profile_io_data.cc View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/chrome_tests_unit.gypi View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_switches.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_switches.cc View 1 2 3 4 5 6 7 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 40 (0 generated)
bengr (incorrect)
Ryan, I haven't written tests yet for this code, but it seems to do the ...
6 years, 2 months ago (2013-01-16 00:10:03 UTC) #1
cbentzel
Please add a bug to track this.
6 years, 2 months ago (2013-01-16 16:40:00 UTC) #2
bengr (incorrect)
Bug added.
6 years, 2 months ago (2013-01-16 17:54:34 UTC) #3
Ryan Sleevi
No clue what to suggest to you for tests. Perhaps someone more familiar with chrome/browser/net ...
6 years, 2 months ago (2013-01-16 23:19:56 UTC) #4
bengr (incorrect)
https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/chrome_network_delegate.cc File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/chrome_network_delegate.cc#newcode284 chrome/browser/net/chrome_network_delegate.cc:284: client_hints_ = ClientHints::create(); On 2013/01/16 23:19:56, Ryan Sleevi wrote: ...
6 years, 2 months ago (2013-01-17 01:50:44 UTC) #5
Ryan Sleevi
https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/client_hints.cc File chrome/browser/net/client_hints.cc (right): https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/client_hints.cc#newcode77 chrome/browser/net/client_hints.cc:77: screen_info_hints += base::StringPrintf(", dpr=%0.3g", dpr); StringAppendF(screen_info_hints, ", dpr=%0.3g", dpr); ...
6 years, 2 months ago (2013-01-17 01:57:56 UTC) #6
bengr (incorrect)
Hi, I'm hoping for the following reviews: mattm: everything in chrome/browser/net thestig: the gypi in ...
6 years, 2 months ago (2013-01-17 02:02:35 UTC) #7
Jói
content/port and content/public LGTM, but see question below. https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/chrome_network_delegate.cc File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/chrome_network_delegate.cc#newcode389 chrome/browser/net/chrome_network_delegate.cc:389: // ...
6 years, 2 months ago (2013-01-17 04:43:21 UTC) #8
Randy Smith (Not in Mondays)
I shouldn't do reviews on anything in content/public/browser other than the download files (see comment ...
6 years, 2 months ago (2013-01-17 18:10:49 UTC) #9
jam
https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/client_hints.cc File chrome/browser/net/client_hints.cc (right): https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/client_hints.cc#newcode59 chrome/browser/net/client_hints.cc:59: #if defined(OS_POSIX) || defined(USE_AURA) why are you doing this ...
6 years, 2 months ago (2013-01-17 18:14:35 UTC) #10
bengr (incorrect)
I put client hints behind a flag, removed the interface changes to RenderWidgetHostView and hopefully ...
6 years, 2 months ago (2013-01-18 02:11:59 UTC) #11
jam
+darin for his thoughts on the feature it seems like a problem that as implemented, ...
6 years, 2 months ago (2013-01-18 18:07:28 UTC) #12
bengr (incorrect)
At least on linux desktop chrome, this data will be available to a request made ...
6 years, 2 months ago (2013-01-18 23:50:08 UTC) #13
bengr (incorrect)
So how should I proceed with this patch?
6 years, 1 month ago (2013-01-22 22:36:59 UTC) #14
Ryan Sleevi
On 2013/01/22 22:36:59, bengr wrote: > So how should I proceed with this patch? ben: ...
6 years, 1 month ago (2013-01-23 00:38:49 UTC) #15
bengr (incorrect)
No, but a good start would be a review from mattm on everything in chrome/browser/net. ...
6 years, 1 month ago (2013-01-23 16:20:29 UTC) #16
bengr (incorrect)
Hi all, we've taken another pass over the Client Hints spec and as a result, ...
6 years, 1 month ago (2013-02-07 22:39:02 UTC) #17
Ryan Sleevi
https://codereview.chromium.org/11970002/diff/32001/chrome/browser/net/chrome_network_delegate.cc File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/11970002/diff/32001/chrome/browser/net/chrome_network_delegate.cc#newcode285 chrome/browser/net/chrome_network_delegate.cc:285: if (client_hints_ != NULL) { Just use a bool ...
6 years, 1 month ago (2013-02-08 22:12:53 UTC) #18
igrigorik
https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/client_hints.cc File chrome/browser/net/client_hints.cc (right): https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/client_hints.cc#newcode62 chrome/browser/net/client_hints.cc:62: screen_info.deviceScaleFactor = 0.0; Is there a specific reason for ...
6 years, 1 month ago (2013-02-09 17:06:30 UTC) #19
bengr
https://codereview.chromium.org/11970002/diff/32001/chrome/browser/net/chrome_network_delegate.cc File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/11970002/diff/32001/chrome/browser/net/chrome_network_delegate.cc#newcode285 chrome/browser/net/chrome_network_delegate.cc:285: if (client_hints_ != NULL) { On 2013/02/08 22:12:53, Ryan ...
6 years ago (2013-02-26 01:57:20 UTC) #20
Ryan Sleevi
https://codereview.chromium.org/11970002/diff/40002/chrome/browser/net/chrome_network_delegate.cc File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/11970002/diff/40002/chrome/browser/net/chrome_network_delegate.cc#newcode398 chrome/browser/net/chrome_network_delegate.cc:398: << " for " << request->url().spec(); nit: Is it ...
6 years ago (2013-02-26 02:21:04 UTC) #21
bengr
https://codereview.chromium.org/11970002/diff/40002/chrome/browser/net/chrome_network_delegate.cc File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/11970002/diff/40002/chrome/browser/net/chrome_network_delegate.cc#newcode398 chrome/browser/net/chrome_network_delegate.cc:398: << " for " << request->url().spec(); On 2013/02/26 02:21:04, ...
6 years ago (2013-02-26 18:19:53 UTC) #22
Ryan Sleevi
https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client_hints.cc File chrome/browser/net/client_hints.cc (right): https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client_hints.cc#newcode24 chrome/browser/net/client_hints.cc:24: ClientHints::ClientHints() : screen_hints_(""), weak_ptr_factory_(this) { You will need to ...
6 years ago (2013-02-26 18:26:16 UTC) #23
bengr
https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client_hints.cc File chrome/browser/net/client_hints.cc (right): https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client_hints.cc#newcode24 chrome/browser/net/client_hints.cc:24: ClientHints::ClientHints() : screen_hints_(""), weak_ptr_factory_(this) { On 2013/02/26 18:26:16, Ryan ...
6 years ago (2013-02-26 18:41:16 UTC) #24
bengr
PTAL. On 2013/02/26 18:41:16, bengr1 wrote: > https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client_hints.cc > File chrome/browser/net/client_hints.cc (right): > > https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client_hints.cc#newcode24 ...
6 years ago (2013-02-26 23:01:03 UTC) #25
Ryan Sleevi
lgtm
6 years ago (2013-02-26 23:45:32 UTC) #26
bengr
mmenke@: I have an LGTM from sleevi@, but I don't see him in the OWNERS ...
6 years ago (2013-02-27 00:25:10 UTC) #27
Lei Zhang
lgtm
6 years ago (2013-02-27 00:27:07 UTC) #28
mmenke
https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/chrome_network_delegate.cc File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/chrome_network_delegate.cc#newcode306 chrome/browser/net/chrome_network_delegate.cc:306: client_hints_->RetrieveScreenInfo(); I don't think the network delegate should care ...
6 years ago (2013-02-27 21:06:05 UTC) #29
bengr
https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/chrome_network_delegate.cc File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/chrome_network_delegate.cc#newcode306 chrome/browser/net/chrome_network_delegate.cc:306: client_hints_->RetrieveScreenInfo(); On 2013/02/27 21:06:05, mmenke wrote: > I don't ...
6 years ago (2013-03-06 00:34:19 UTC) #30
Ryan Sleevi
https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/client_hints.cc File chrome/browser/net/client_hints.cc (right): https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/client_hints.cc#newcode39 chrome/browser/net/client_hints.cc:39: base::Owned(info))); On 2013/03/06 00:34:19, bengr1 wrote: > I presume ...
6 years ago (2013-03-06 00:38:18 UTC) #31
bengr
On 2013/03/06 00:38:18, Ryan Sleevi wrote: > https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/client_hints.cc > File chrome/browser/net/client_hints.cc (right): > > https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/client_hints.cc#newcode39 ...
6 years ago (2013-03-06 00:41:54 UTC) #32
mmenke
On 2013/03/06 00:41:54, bengr1 wrote: > On 2013/03/06 00:38:18, Ryan Sleevi wrote: > > > ...
6 years ago (2013-03-06 00:44:50 UTC) #33
mmenke
On 2013/03/06 00:44:50, mmenke wrote: > On 2013/03/06 00:41:54, bengr1 wrote: > > On 2013/03/06 ...
6 years ago (2013-03-06 01:08:18 UTC) #34
bengr
On 2013/03/06 00:44:50, mmenke wrote: > On 2013/03/06 00:41:54, bengr1 wrote: > > On 2013/03/06 ...
6 years ago (2013-03-06 01:10:43 UTC) #35
mmenke
On 2013/03/06 01:10:43, bengr1 wrote: > On 2013/03/06 00:44:50, mmenke wrote: > > On 2013/03/06 ...
6 years ago (2013-03-06 01:14:51 UTC) #36
mmenke
LGTM!
6 years ago (2013-03-06 19:05:08 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bengr@google.com/11970002/54001
6 years ago (2013-03-06 19:16:25 UTC) #38
mmenke
On 2013/03/06 19:16:25, I haz the power (commit-bot) wrote: > CQ is trying da patch. ...
5 years, 10 months ago (2013-05-23 15:03:02 UTC) #39
Yoav Weiss
5 years, 6 months ago (2013-09-07 14:31:02 UTC) #40
I've updated that patch at https://codereview.chromium.org/23654014
Now it applies, compiles and passes test on current Chromium.
I also:
* Removed the dh and dw hints, since AFAIK they are not part of the current
"Intent to implement".
* Fixed a locale related issue I encountered and added a test for it.

Powered by Google App Engine
This is Rietveld 408576698