Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11970002:
Added Client-Hints. (Closed)
|
Created:
7 years, 11 months ago by bengr (incorrect) Modified:
7 years, 2 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. |
DescriptionAdded Client-Hints.
See: https://github.com/igrigorik/http-client-hints/blob/master/draft-grigorik-http-client-hints-00.txt
BUG=170388
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 #
Messages
Total messages: 40 (0 generated)
Ryan, I haven't written tests yet for this code, but it seems to do the trick. Let me know what you think.
Please add a bug to track this.
Bug added.
No clue what to suggest to you for tests. Perhaps someone more familiar with chrome/browser/net can provide some hints? In the absolute worst case, you can use the net::TestServer and have the test server check, but that feels like big overkill. https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:284: client_hints_ = ClientHints::create(); Why a static method? Why not new ClientHints? https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... File chrome/browser/net/client_hints.cc (right): https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... chrome/browser/net/client_hints.cc:7: #include "chrome/browser/net/client_hints.h" style: This should be first, followed by a new line - http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Stream... https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... chrome/browser/net/client_hints.cc:11: #include "third_party/WebKit/Source/Platform/chromium/public/WebScreenInfo.h" I didn't think this was legal? I thought only the webkit glue was allowed to pull in these headers. https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... chrome/browser/net/client_hints.cc:35: if (it != screen_hints_.end()) { style: Put it as error handling first if (it == screen_hints_.end()) return false; return true; https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... chrome/browser/net/client_hints.cc:46: std::pair<int, int>(process_id, render_view_id)].hints_str; You're forcing a double-find for the DCHECK. Further, this seems like there can be a synchronization issue (hence your DCHECK), and I'd much rather see true safety here. If a renderer/process gets a spurious message sent and then kills itself, this could corrupt browser process, right? https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... chrome/browser/net/client_hints.cc:54: if (render_view_host != NULL && render_view_host->GetView() != NULL) { style: Put error handling first, with early return if (render_view_host == NULL || render_view_host->GetView() == NULL) return; WebKit::WebScreenInfo ... https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... chrome/browser/net/client_hints.cc:73: std::ostringstream value; NACK: This is forbidden in the style guide. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Stream... use base::StringPrintF() and call it a day. https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... File chrome/browser/net/client_hints.h (right): https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... chrome/browser/net/client_hints.h:14: class ScreenHints { This should be a struct, not a class, given that all members are public. https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... chrome/browser/net/client_hints.h:20: float dpr; style: Do not abbreviate dpr like this. https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... chrome/browser/net/client_hints.h:27: } These typedefs should appear before any class definitions. https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... chrome/browser/net/client_hints.h:37: ~ClientHints(); style: create() should be Create() - although it's entirely unclear why it's necessary BUG: ~ClientHints should not have a public destructor. It MUST be private/protected and friended only to RefCountedThreadSafe<ClientHints> https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... chrome/browser/net/client_hints.h:51: ClientHints(); Do you expect people to be subclassing this? You have no virtual methods, so if you were trying to use the factory pattern via Create(), then this should be private. https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... chrome/browser/net/client_hints.h:56: int width, int height, style: viewport_width, viewport_height ? https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... chrome/browser/net/client_hints.h:57: float dpr); style: meaningful name
https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:284: client_hints_ = ClientHints::create(); On 2013/01/16 23:19:56, Ryan Sleevi wrote: > Why a static method? Why not new ClientHints? Done. https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... File chrome/browser/net/client_hints.cc (right): https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... chrome/browser/net/client_hints.cc:7: #include "chrome/browser/net/client_hints.h" On 2013/01/16 23:19:56, Ryan Sleevi wrote: > style: This should be first, followed by a new line - > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Stream... Done. https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... chrome/browser/net/client_hints.cc:11: #include "third_party/WebKit/Source/Platform/chromium/public/WebScreenInfo.h" On 2013/01/16 23:19:56, Ryan Sleevi wrote: > I didn't think this was legal? I thought only the webkit glue was allowed to > pull in these headers. Done. https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... chrome/browser/net/client_hints.cc:35: if (it != screen_hints_.end()) { On 2013/01/16 23:19:56, Ryan Sleevi wrote: > style: Put it as error handling first > > if (it == screen_hints_.end()) > return false; > > return true; Done. https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... chrome/browser/net/client_hints.cc:46: std::pair<int, int>(process_id, render_view_id)].hints_str; Removed double-find. I don't understand your second point, though. On 2013/01/16 23:19:56, Ryan Sleevi wrote: > You're forcing a double-find for the DCHECK. Further, this seems like there can > be a synchronization issue (hence your DCHECK), and I'd much rather see true > safety here. If a renderer/process gets a spurious message sent and then kills > itself, this could corrupt browser process, right? https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... chrome/browser/net/client_hints.cc:54: if (render_view_host != NULL && render_view_host->GetView() != NULL) { On 2013/01/16 23:19:56, Ryan Sleevi wrote: > style: Put error handling first, with early return > > if (render_view_host == NULL || render_view_host->GetView() == NULL) > return; > > WebKit::WebScreenInfo ... Done. https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... chrome/browser/net/client_hints.cc:73: std::ostringstream value; On 2013/01/16 23:19:56, Ryan Sleevi wrote: > NACK: This is forbidden in the style guide. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Stream... > > use base::StringPrintF() and call it a day. Done. https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... File chrome/browser/net/client_hints.h (right): https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... chrome/browser/net/client_hints.h:14: class ScreenHints { On 2013/01/16 23:19:56, Ryan Sleevi wrote: > This should be a struct, not a class, given that all members are public. Done. https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... chrome/browser/net/client_hints.h:20: float dpr; On 2013/01/16 23:19:56, Ryan Sleevi wrote: > style: Do not abbreviate dpr like this. Done. https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... chrome/browser/net/client_hints.h:27: } On 2013/01/16 23:19:56, Ryan Sleevi wrote: > These typedefs should appear before any class definitions. Done. https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... chrome/browser/net/client_hints.h:37: ~ClientHints(); On 2013/01/16 23:19:56, Ryan Sleevi wrote: > style: create() should be Create() - although it's entirely unclear why it's > necessary > > BUG: ~ClientHints should not have a public destructor. It MUST be > private/protected and friended only to RefCountedThreadSafe<ClientHints> Done. https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... chrome/browser/net/client_hints.h:51: ClientHints(); I don't expect people to be subclassing this. Done. On 2013/01/16 23:19:56, Ryan Sleevi wrote: > Do you expect people to be subclassing this? You have no virtual methods, so if > you were trying to use the factory pattern via Create(), then this should be > private. https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... chrome/browser/net/client_hints.h:56: int width, int height, On 2013/01/16 23:19:56, Ryan Sleevi wrote: > style: viewport_width, viewport_height ? Done. https://codereview.chromium.org/11970002/diff/2001/chrome/browser/net/client_... chrome/browser/net/client_hints.h:57: float dpr); On 2013/01/16 23:19:56, Ryan Sleevi wrote: > style: meaningful name Done.
https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/client_hin... File chrome/browser/net/client_hints.cc (right): https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/client_hin... chrome/browser/net/client_hints.cc:77: screen_info_hints += base::StringPrintf(", dpr=%0.3g", dpr); StringAppendF(screen_info_hints, ", dpr=%0.3g", dpr); https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/client_hin... File chrome/browser/net/client_hints.h (right): https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/client_hin... chrome/browser/net/client_hints.h:15: typedef std::map<std::pair<int, int>, ScreenHints> ScreenMap; nit: Should this be a dependent type of ClientHints, rather than in the global namespace ClientHints::ScreenMap And does this need to be a public type? https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/client_hin... chrome/browser/net/client_hints.h:30: } I meant including this forward decl. Your typedefs and forward decls should appear before your classes. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/client_hin... chrome/browser/net/client_hints.h:31: No suitable namespace here? We use chrome_browser_net { } predominantly in here, it seems. https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/client_hin... chrome/browser/net/client_hints.h:59: ~ClientHints(); nit: newline between the friend decls https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/client_hin... File chrome/browser/net/client_hints_unittest.cc (right): https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/client_hin... chrome/browser/net/client_hints_unittest.cc:11: nit: unnecessary newline
Hi, I'm hoping for the following reviews: mattm: everything in chrome/browser/net thestig: the gypi in chrome/ joi and jam: content/port/browser/ rdsmith: content/public/browser/
content/port and content/public LGTM, but see question below. https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/chrome_net... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/chrome_net... chrome/browser/net/chrome_network_delegate.cc:389: // TODO(bengr): Put behind experimental flag. Should this be resolved before commit?
I shouldn't do reviews on anything in content/public/browser other than the download files (see comment in OWNERS file). If Joi reviewed content/public/browser/ along with content/public, you should be fine. Joi?
https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/client_hin... File chrome/browser/net/client_hints.cc (right): https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/client_hin... chrome/browser/net/client_hints.cc:59: #if defined(OS_POSIX) || defined(USE_AURA) why are you doing this only for these too? GetScreenInfo appears to be an internal detail of content, which is why it's in content/port (and not content/public). chrome layer is the one that manages teh browser size, why should it go to content to ask something it should know itself?
I put client hints behind a flag, removed the interface changes to RenderWidgetHostView and hopefully addressed all other comments. https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/client_hin... File chrome/browser/net/client_hints.cc (right): https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/client_hin... chrome/browser/net/client_hints.cc:59: #if defined(OS_POSIX) || defined(USE_AURA) I now get the device pixel ratio more directly. On 2013/01/17 18:14:35, John Abd-El-Malek wrote: > why are you doing this only for these too? > > GetScreenInfo appears to be an internal detail of content, which is why it's in > content/port (and not content/public). > > chrome layer is the one that manages teh browser size, why should it go to > content to ask something it should know itself? https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/client_hin... chrome/browser/net/client_hints.cc:77: screen_info_hints += base::StringPrintf(", dpr=%0.3g", dpr); On 2013/01/17 01:57:56, Ryan Sleevi wrote: > StringAppendF(screen_info_hints, ", dpr=%0.3g", dpr); Done. https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/client_hin... File chrome/browser/net/client_hints.h (right): https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/client_hin... chrome/browser/net/client_hints.h:15: typedef std::map<std::pair<int, int>, ScreenHints> ScreenMap; On 2013/01/17 01:57:56, Ryan Sleevi wrote: > nit: Should this be a dependent type of ClientHints, rather than in the global > namespace > > ClientHints::ScreenMap > > And does this need to be a public type? Done. https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/client_hin... chrome/browser/net/client_hints.h:30: } On 2013/01/17 01:57:56, Ryan Sleevi wrote: > I meant including this forward decl. > > Your typedefs and forward decls should appear before your classes. > > See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... Done. https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/client_hin... chrome/browser/net/client_hints.h:31: On 2013/01/17 01:57:56, Ryan Sleevi wrote: > No suitable namespace here? We use chrome_browser_net { } predominantly in here, > it seems. Done. https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/client_hin... chrome/browser/net/client_hints.h:59: ~ClientHints(); On 2013/01/17 01:57:56, Ryan Sleevi wrote: > nit: newline between the friend decls Done. https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/client_hin... File chrome/browser/net/client_hints_unittest.cc (right): https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/client_hin... chrome/browser/net/client_hints_unittest.cc:11: On 2013/01/17 01:57:56, Ryan Sleevi wrote: > nit: unnecessary newline Done.
+darin for his thoughts on the feature it seems like a problem that as implemented, (at least) the first request for a render view won't have this data.
At least on linux desktop chrome, this data will be available to a request made from any existing tab, since ChromeNetworkDelegate intercepts requests for chrome://newtab, though I'll have to look into whether this is true for other platforms or for opening a link in a new tab. On 2013/01/18 18:07:28, John Abd-El-Malek wrote: > +darin for his thoughts on the feature > > it seems like a problem that as implemented, (at least) the first request for a > render view won't have this data.
So how should I proceed with this patch?
On 2013/01/22 22:36:59, bengr wrote: > So how should I proceed with this patch? ben: You have a lot of reviewers on this CL. Was this directed at any one in particular?
No, but a good start would be a review from mattm on everything in chrome/browser/net. (I believe you wanted to defer to him; otherwise a review from you would be fine.) Once that's squared away, the rest should be straightforward. On 2013/01/23 00:38:49, Ryan Sleevi wrote: > On 2013/01/22 22:36:59, bengr wrote: > > So how should I proceed with this patch? > > ben: You have a lot of reviewers on this CL. Was this directed at any one in > particular?
Hi all, we've taken another pass over the Client Hints spec and as a result, I've greatly simplified this CL. Could I kindly have the following reviews: thesig: chrome/chrome_browser.gyp battre: chrome/browser/extensions/api/web_request/... mmenke: everything else.
https://codereview.chromium.org/11970002/diff/32001/chrome/browser/net/chrome... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/11970002/diff/32001/chrome/browser/net/chrome... chrome/browser/net/chrome_network_delegate.cc:285: if (client_hints_ != NULL) { Just use a bool check, rather than NULL if (client_hints_) { } This works regardless of the storage used for the client hints (eg: scoped_ptr.get() semantics, implicit pointers, etc) https://codereview.chromium.org/11970002/diff/32001/chrome/browser/net/chrome... chrome/browser/net/chrome_network_delegate.cc:397: << " for " << request->url().spec(); Align your streams - see http://dev.chromium.org/developers/coding-style https://codereview.chromium.org/11970002/diff/32001/chrome/browser/net/chrome... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/11970002/diff/32001/chrome/browser/net/chrome... chrome/browser/net/chrome_network_delegate.h:55: bool enable_client_hints); Is there a reason this needs to be part of the constructor, as opposed to having set_xxx methods like we do for everything else? https://codereview.chromium.org/11970002/diff/32001/chrome/browser/net/client... File chrome/browser/net/client_hints.cc (right): https://codereview.chromium.org/11970002/diff/32001/chrome/browser/net/client... chrome/browser/net/client_hints.cc:22: base::Bind(&ClientHints::FetchScreenInfoOnUIThread, this)); For example: namespace { typedef base::Callback<void(int, int, float)> ClientHintsCallback; void FetchScreenInfoOnUIThread(const ClientHintsCallback& cb) { DCHECK(... ::UI)); gfx::Display display = gfx::Screen::GetNativeScreen()->GetPrimaryDisplay(); cb.Run(display.GetSizeInPixel().width(), display.GetSizeInPixel().height(), display.device_scale_factor()); } } bool ClientHints::RetrieveScreenInfo() { DCHECK(... IO)); ClientHintsCallback cb = base::Bind( &BrowserThread::PostTask, BrowserThread::IO, FROM_HERE, base::Bind(&ClientHints::UpdateScreenInfo, weak_ptr_factory_.GetWeakPtr())); return BrowserThread::PostTask( BrowserThread::UI, FROM_HERE, base::Bind(&FetchScreenInfoOnUIThread, cb)); } No refcounting needed, and you still end up with 'safety' The extra bind to PostTask could alternatively be done with PostTaskAndReplyWithResult (which only handles one result value, not the three you're using), but that actually ends up with even more indirection and either exposing the struct publically or exposing the UpdateScreenInfo publicly, which I wanted to avoid. https://codereview.chromium.org/11970002/diff/32001/chrome/browser/net/client... File chrome/browser/net/client_hints.h (right): https://codereview.chromium.org/11970002/diff/32001/chrome/browser/net/client... chrome/browser/net/client_hints.h:17: nit: delete blank line https://codereview.chromium.org/11970002/diff/32001/chrome/browser/net/client... chrome/browser/net/client_hints.h:18: class ClientHints : public base::RefCountedThreadSafe<ClientHints> { Why is this RefCounted? If it's for the UI<->IO thread switch, can you please re-design it to use WeakPtr, since refcounting is a 'big hammer' to use for something that is really an internal implementation detail?
https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/client_hin... File chrome/browser/net/client_hints.cc (right): https://codereview.chromium.org/11970002/diff/9/chrome/browser/net/client_hin... chrome/browser/net/client_hints.cc:62: screen_info.deviceScaleFactor = 0.0; Is there a specific reason for 0.0? I think by default we should assume 1.0 - aka, physical pixel count is the same as CSS pixel count. https://codereview.chromium.org/11970002/diff/32001/chrome/browser/net/chrome... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/11970002/diff/32001/chrome/browser/net/chrome... chrome/browser/net/chrome_network_delegate.cc:393: std::string client_hints_value = client_hints_->GetScreenInfoHints(); Hmm, this would fetch screen.{width,height} on every request, correct? If so, orientation changes would switch the w/h values in the header. Is there a simpler approach to just report them as fixed portrait mode? The idea being.. we don't want to communicate orientation, just the device width and height, which is fixed.
https://codereview.chromium.org/11970002/diff/32001/chrome/browser/net/chrome... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/11970002/diff/32001/chrome/browser/net/chrome... chrome/browser/net/chrome_network_delegate.cc:285: if (client_hints_ != NULL) { On 2013/02/08 22:12:53, Ryan Sleevi wrote: > Just use a bool check, rather than NULL > > if (client_hints_) { } > > This works regardless of the storage used for the client hints (eg: > scoped_ptr.get() semantics, implicit pointers, etc) Done. https://codereview.chromium.org/11970002/diff/32001/chrome/browser/net/chrome... chrome/browser/net/chrome_network_delegate.cc:393: std::string client_hints_value = client_hints_->GetScreenInfoHints(); It does not fetch screen height/width on every request. It gets the hints string on every request. The updating of that string is triggered by ClientHints::RetrieveScreenInfo(), which only happens once when client hints is first enabled. On 2013/02/09 17:06:30, igrigorik wrote: > Hmm, this would fetch screen.{width,height} on every request, correct? If so, > orientation changes would switch the w/h values in the header. Is there a > simpler approach to just report them as fixed portrait mode? The idea being.. we > don't want to communicate orientation, just the device width and height, which > is fixed. https://codereview.chromium.org/11970002/diff/32001/chrome/browser/net/chrome... chrome/browser/net/chrome_network_delegate.cc:397: << " for " << request->url().spec(); On 2013/02/08 22:12:53, Ryan Sleevi wrote: > Align your streams - see http://dev.chromium.org/developers/coding-style Done. https://codereview.chromium.org/11970002/diff/32001/chrome/browser/net/chrome... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/11970002/diff/32001/chrome/browser/net/chrome... chrome/browser/net/chrome_network_delegate.h:55: bool enable_client_hints); On 2013/02/08 22:12:53, Ryan Sleevi wrote: > Is there a reason this needs to be part of the constructor, as opposed to having > set_xxx methods like we do for everything else? Done. https://codereview.chromium.org/11970002/diff/32001/chrome/browser/net/client... File chrome/browser/net/client_hints.cc (right): https://codereview.chromium.org/11970002/diff/32001/chrome/browser/net/client... chrome/browser/net/client_hints.cc:22: base::Bind(&ClientHints::FetchScreenInfoOnUIThread, this)); I couldn't get your suggestion to compile. I wound up using PostTaskAndReply and replied with a struct. The struct is exposed as a public member of ClientHints. I tried declaring it in an anonymous namespace, which worked fine for client_hint.{h,cc}, but I got an odd error when I tried to build the tests: In file included from ../../chrome/browser/net/client_hints_unittest.cc:5: ../../chrome/browser/net/client_hints.h:46:8: error: function 'ClientHints::UpdateScreenInfo' has internal linkage but is not defined [-Werror,-Wundefined-internal] void UpdateScreenInfo(ScreenInfo* info); ^ ../../chrome/browser/net/client_hints_unittest.cc:21:20: note: used here client_hints_->UpdateScreenInfo(&info); On 2013/02/08 22:12:53, Ryan Sleevi wrote: > For example: > > namespace { > typedef base::Callback<void(int, int, float)> ClientHintsCallback; > > void FetchScreenInfoOnUIThread(const ClientHintsCallback& cb) { > DCHECK(... ::UI)); > gfx::Display display = gfx::Screen::GetNativeScreen()->GetPrimaryDisplay(); > cb.Run(display.GetSizeInPixel().width(), > display.GetSizeInPixel().height(), > display.device_scale_factor()); > } > > } > > bool ClientHints::RetrieveScreenInfo() { > DCHECK(... IO)); > ClientHintsCallback cb = > base::Bind( > &BrowserThread::PostTask, > BrowserThread::IO, > FROM_HERE, > base::Bind(&ClientHints::UpdateScreenInfo, > weak_ptr_factory_.GetWeakPtr())); > return BrowserThread::PostTask( > BrowserThread::UI, FROM_HERE, > base::Bind(&FetchScreenInfoOnUIThread, cb)); > } > > > No refcounting needed, and you still end up with 'safety' > > The extra bind to PostTask could alternatively be done with > PostTaskAndReplyWithResult (which only handles one result value, not the three > you're using), but that actually ends up with even more indirection and either > exposing the struct publically or exposing the UpdateScreenInfo publicly, which > I wanted to avoid. https://codereview.chromium.org/11970002/diff/32001/chrome/browser/net/client... File chrome/browser/net/client_hints.h (right): https://codereview.chromium.org/11970002/diff/32001/chrome/browser/net/client... chrome/browser/net/client_hints.h:17: On 2013/02/08 22:12:53, Ryan Sleevi wrote: > nit: delete blank line Done. https://codereview.chromium.org/11970002/diff/32001/chrome/browser/net/client... chrome/browser/net/client_hints.h:18: class ClientHints : public base::RefCountedThreadSafe<ClientHints> { On 2013/02/08 22:12:53, Ryan Sleevi wrote: > Why is this RefCounted? If it's for the UI<->IO thread switch, can you please > re-design it to use WeakPtr, since refcounting is a 'big hammer' to use for > something that is really an internal implementation detail? Done.
https://codereview.chromium.org/11970002/diff/40002/chrome/browser/net/chrome... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/11970002/diff/40002/chrome/browser/net/chrome... chrome/browser/net/chrome_network_delegate.cc:398: << " for " << request->url().spec(); nit: Is it necessary to DVLOG this? Seems like you'll be able to snarf it in net-internals. https://codereview.chromium.org/11970002/diff/40002/chrome/browser/net/client... File chrome/browser/net/client_hints.cc (right): https://codereview.chromium.org/11970002/diff/40002/chrome/browser/net/client... chrome/browser/net/client_hints.cc:13: using content::BrowserThread; These are generally discouraged except when they make significant sense (eg: WK types). It seems unnecessary here. https://codereview.chromium.org/11970002/diff/40002/chrome/browser/net/client... chrome/browser/net/client_hints.cc:16: typedef base::Callback<void(int, int, float)> ClientHintsCallback; nit: Unused https://codereview.chromium.org/11970002/diff/40002/chrome/browser/net/client... File chrome/browser/net/client_hints.h (right): https://codereview.chromium.org/11970002/diff/40002/chrome/browser/net/client... chrome/browser/net/client_hints.h:16: class ClientHints : public base::RefCountedThreadSafe<ClientHints> { This is still RefCounted, but you said you were going WeakPtr. The goal is to eliminate RefCounting from being part of the public interface (and ideally, keep it from being introduced in the private / implementation details part) https://codereview.chromium.org/11970002/diff/40002/chrome/browser/net/client... chrome/browser/net/client_hints.h:35: }; Usually we place these declarations first (part of the public) - http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... https://codereview.chromium.org/11970002/diff/40002/chrome/browser/net/client... File chrome/browser/net/client_hints_unittest.cc (right): https://codereview.chromium.org/11970002/diff/40002/chrome/browser/net/client... chrome/browser/net/client_hints_unittest.cc:15: virtual void SetUp() OVERRIDE { 1) No need to use OVERRIDE for testing::Test overrides 2) No need to use SetUp/TearDown - simply using the constructor / destructor is more than sufficient - see https://groups.google.com/a/chromium.org/d/topic/chromium-dev/x-D34rZAkc4/dis... https://codereview.chromium.org/11970002/diff/40002/chrome/browser/net/client... chrome/browser/net/client_hints_unittest.cc:31: TEST_F(ClientHintsTest, HintsWellFormatted) { Are there any other edge cases you can be testing here? Negative values? "nice round" pixel ratio values (1.00 ratio)? Do you need to test the round up / round down behaviour?
https://codereview.chromium.org/11970002/diff/40002/chrome/browser/net/chrome... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/11970002/diff/40002/chrome/browser/net/chrome... chrome/browser/net/chrome_network_delegate.cc:398: << " for " << request->url().spec(); On 2013/02/26 02:21:04, Ryan Sleevi wrote: > nit: Is it necessary to DVLOG this? Seems like you'll be able to snarf it in > net-internals. Done. https://codereview.chromium.org/11970002/diff/40002/chrome/browser/net/client... File chrome/browser/net/client_hints.cc (right): https://codereview.chromium.org/11970002/diff/40002/chrome/browser/net/client... chrome/browser/net/client_hints.cc:13: using content::BrowserThread; On 2013/02/26 02:21:04, Ryan Sleevi wrote: > These are generally discouraged except when they make significant sense (eg: WK > types). It seems unnecessary here. Done. https://codereview.chromium.org/11970002/diff/40002/chrome/browser/net/client... chrome/browser/net/client_hints.cc:16: typedef base::Callback<void(int, int, float)> ClientHintsCallback; On 2013/02/26 02:21:04, Ryan Sleevi wrote: > nit: Unused Done. https://codereview.chromium.org/11970002/diff/40002/chrome/browser/net/client... File chrome/browser/net/client_hints.h (right): https://codereview.chromium.org/11970002/diff/40002/chrome/browser/net/client... chrome/browser/net/client_hints.h:16: class ClientHints : public base::RefCountedThreadSafe<ClientHints> { Oops. I meant to get rid of this. Done. On 2013/02/26 02:21:04, Ryan Sleevi wrote: > This is still RefCounted, but you said you were going WeakPtr. > > The goal is to eliminate RefCounting from being part of the public interface > (and ideally, keep it from being introduced in the private / implementation > details part) https://codereview.chromium.org/11970002/diff/40002/chrome/browser/net/client... chrome/browser/net/client_hints.h:35: }; On 2013/02/26 02:21:04, Ryan Sleevi wrote: > Usually we place these declarations first (part of the public) - > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... Done. https://codereview.chromium.org/11970002/diff/40002/chrome/browser/net/client... File chrome/browser/net/client_hints_unittest.cc (right): https://codereview.chromium.org/11970002/diff/40002/chrome/browser/net/client... chrome/browser/net/client_hints_unittest.cc:15: virtual void SetUp() OVERRIDE { On 2013/02/26 02:21:04, Ryan Sleevi wrote: > 1) No need to use OVERRIDE for testing::Test overrides > 2) No need to use SetUp/TearDown - simply using the constructor / destructor is > more than sufficient - see > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/x-D34rZAkc4/dis... Done. https://codereview.chromium.org/11970002/diff/40002/chrome/browser/net/client... chrome/browser/net/client_hints_unittest.cc:31: TEST_F(ClientHintsTest, HintsWellFormatted) { Not really. These values should be positive. I suppose I could discard the hint if otherwise. Done. On 2013/02/26 02:21:04, Ryan Sleevi wrote: > Are there any other edge cases you can be testing here? > > Negative values? "nice round" pixel ratio values (1.00 ratio)? Do you need to > test the round up / round down behaviour?
https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client... File chrome/browser/net/client_hints.cc (right): https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client... chrome/browser/net/client_hints.cc:24: ClientHints::ClientHints() : screen_hints_(""), weak_ptr_factory_(this) { You will need to rewrite this (to suppress an MSVC warning) as ClientHints::ClientHints : weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {} Note that I also removed the screen_hints("") - there is no need to initialize to an empty string here, let the default ctor handle that. https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client... chrome/browser/net/client_hints.cc:30: bool ClientHints::RetrieveScreenInfo() { DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO)) Simply because with the PTAR pattern, you are able to call from any thread. https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client... chrome/browser/net/client_hints.cc:37: base::Owned(info))); nit: indent to (, following function-call indenting. https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client... chrome/browser/net/client_hints.cc:45: if (info->height > 0 && info->width > 0 && info->pixel_ratio > 0.0) nit: braces here, for a multi-line body (even if it's wrapped) https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client... File chrome/browser/net/client_hints.h (right): https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client... chrome/browser/net/client_hints.h:23: }; I just realized you made this declaration public solely for testing. Can you not just friend this to the test as well, and then move this ~line 41? https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client... chrome/browser/net/client_hints.h:40: friend class base::RefCountedThreadSafe<ClientHints>; unnecessary
https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client... File chrome/browser/net/client_hints.cc (right): https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client... chrome/browser/net/client_hints.cc:24: ClientHints::ClientHints() : screen_hints_(""), weak_ptr_factory_(this) { On 2013/02/26 18:26:16, Ryan Sleevi wrote: > You will need to rewrite this (to suppress an MSVC warning) as > > ClientHints::ClientHints : > weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {} > > Note that I also removed the screen_hints("") - there is no need to initialize > to an empty string here, let the default ctor handle that. Done. https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client... chrome/browser/net/client_hints.cc:30: bool ClientHints::RetrieveScreenInfo() { On 2013/02/26 18:26:16, Ryan Sleevi wrote: > DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO)) > > Simply because with the PTAR pattern, you are able to call from any thread. Done. https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client... chrome/browser/net/client_hints.cc:37: base::Owned(info))); On 2013/02/26 18:26:16, Ryan Sleevi wrote: > nit: indent to (, following function-call indenting. Done. https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client... chrome/browser/net/client_hints.cc:45: if (info->height > 0 && info->width > 0 && info->pixel_ratio > 0.0) On 2013/02/26 18:26:16, Ryan Sleevi wrote: > nit: braces here, for a multi-line body (even if it's wrapped) Done. https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client... File chrome/browser/net/client_hints.h (right): https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client... chrome/browser/net/client_hints.h:23: }; FetchScreenInfoOnUIThread also needs ScreenInfo. On 2013/02/26 18:26:16, Ryan Sleevi wrote: > I just realized you made this declaration public solely for testing. > > Can you not just friend this to the test as well, and then move this ~line 41? https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client... chrome/browser/net/client_hints.h:40: friend class base::RefCountedThreadSafe<ClientHints>; On 2013/02/26 18:26:16, Ryan Sleevi wrote: > unnecessary Done.
PTAL. On 2013/02/26 18:41:16, bengr1 wrote: > https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client... > File chrome/browser/net/client_hints.cc (right): > > https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client... > chrome/browser/net/client_hints.cc:24: ClientHints::ClientHints() : > screen_hints_(""), weak_ptr_factory_(this) { > On 2013/02/26 18:26:16, Ryan Sleevi wrote: > > You will need to rewrite this (to suppress an MSVC warning) as > > > > ClientHints::ClientHints : > > weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {} > > > > Note that I also removed the screen_hints("") - there is no need to initialize > > to an empty string here, let the default ctor handle that. > > Done. > > https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client... > chrome/browser/net/client_hints.cc:30: bool ClientHints::RetrieveScreenInfo() { > On 2013/02/26 18:26:16, Ryan Sleevi wrote: > > DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO)) > > > > Simply because with the PTAR pattern, you are able to call from any thread. > > Done. > > https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client... > chrome/browser/net/client_hints.cc:37: base::Owned(info))); > On 2013/02/26 18:26:16, Ryan Sleevi wrote: > > nit: indent to (, following function-call indenting. > > Done. > > https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client... > chrome/browser/net/client_hints.cc:45: if (info->height > 0 && info->width > 0 > && info->pixel_ratio > 0.0) > On 2013/02/26 18:26:16, Ryan Sleevi wrote: > > nit: braces here, for a multi-line body (even if it's wrapped) > > Done. > > https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client... > File chrome/browser/net/client_hints.h (right): > > https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client... > chrome/browser/net/client_hints.h:23: }; > FetchScreenInfoOnUIThread also needs ScreenInfo. > > On 2013/02/26 18:26:16, Ryan Sleevi wrote: > > I just realized you made this declaration public solely for testing. > > > > Can you not just friend this to the test as well, and then move this ~line 41? > > https://codereview.chromium.org/11970002/diff/40003/chrome/browser/net/client... > chrome/browser/net/client_hints.h:40: friend class > base::RefCountedThreadSafe<ClientHints>; > On 2013/02/26 18:26:16, Ryan Sleevi wrote: > > unnecessary > > Done.
lgtm
mmenke@: I have an LGTM from sleevi@, but I don't see him in the OWNERS file for
chrome/browser/net, so could you take a look at my changes in that directory?
Also, could you look at chrome/browser/profiles/profile_io_data.cc?
thestig@: Can I have a review for chrome/browser/io_thread.cc, the gypi in
chrome/, and chrome_switches.{cc,h}?
lgtm
https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/chrome... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/chrome... chrome/browser/net/chrome_network_delegate.cc:306: client_hints_->RetrieveScreenInfo(); I don't think the network delegate should care what client_hints_ needs to do to get initialized. Suggest just an init() function. https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/client... File chrome/browser/net/client_hints.cc (right): https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/client... chrome/browser/net/client_hints.cc:39: base::Owned(info))); What if the user changes screen resolution? https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/client... chrome/browser/net/client_hints.cc:42: std::string ClientHints::GetScreenInfoHints() { optional: Could just make this return a const string&. Admittedly, one string copy per request isn't going to matter, but seems completely unnecessary. https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/client... File chrome/browser/net/client_hints_unittest.cc (right): https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/client... chrome/browser/net/client_hints_unittest.cc:19: }; nit: Add line break. https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/client... chrome/browser/net/client_hints_unittest.cc:43: nit: Remove blank line.
https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/chrome... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/chrome... chrome/browser/net/chrome_network_delegate.cc:306: client_hints_->RetrieveScreenInfo(); On 2013/02/27 21:06:05, mmenke wrote: > I don't think the network delegate should care what client_hints_ needs to do to > get initialized. Suggest just an init() function. Done. https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/client... File chrome/browser/net/client_hints.cc (right): https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/client... chrome/browser/net/client_hints.cc:39: base::Owned(info))); I presume that screen resolution changes are much rarer events than Chrome restarts. In fact on iOS and Android devices, afaik, screen resolution can't be changed. I therefore didn't see the point in the added code complexity to support screen resolution changes mid browsing session. On 2013/02/27 21:06:05, mmenke wrote: > What if the user changes screen resolution? https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/client... chrome/browser/net/client_hints.cc:42: std::string ClientHints::GetScreenInfoHints() { On 2013/02/27 21:06:05, mmenke wrote: > optional: Could just make this return a const string&. Admittedly, one string > copy per request isn't going to matter, but seems completely unnecessary. Done. https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/client... File chrome/browser/net/client_hints_unittest.cc (right): https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/client... chrome/browser/net/client_hints_unittest.cc:19: }; On 2013/02/27 21:06:05, mmenke wrote: > nit: Add line break. Done. https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/client... chrome/browser/net/client_hints_unittest.cc:43: On 2013/02/27 21:06:05, mmenke wrote: > nit: Remove blank line. Done.
https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/client... File chrome/browser/net/client_hints.cc (right): https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/client... chrome/browser/net/client_hints.cc:39: base::Owned(info))); On 2013/03/06 00:34:19, bengr1 wrote: > I presume that screen resolution changes are much rarer events than Chrome > restarts. In fact on iOS and Android devices, afaik, screen resolution can't be > changed. I therefore didn't see the point in the added code complexity to > support screen resolution changes mid browsing session. > > On 2013/02/27 21:06:05, mmenke wrote: > > What if the user changes screen resolution? > Why isn't an orientation change considered a resolution change? I would expect it to be
On 2013/03/06 00:38:18, Ryan Sleevi wrote: > https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/client... > File chrome/browser/net/client_hints.cc (right): > > https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/client... > chrome/browser/net/client_hints.cc:39: base::Owned(info))); > On 2013/03/06 00:34:19, bengr1 wrote: > > I presume that screen resolution changes are much rarer events than Chrome > > restarts. In fact on iOS and Android devices, afaik, screen resolution can't > be > > changed. I therefore didn't see the point in the added code complexity to > > support screen resolution changes mid browsing session. > > > > On 2013/02/27 21:06:05, mmenke wrote: > > > What if the user changes screen resolution? > > > > Why isn't an orientation change considered a resolution change? I would expect > it to be The current thinking is that orientation will be conveyed separately.
On 2013/03/06 00:41:54, bengr1 wrote: > On 2013/03/06 00:38:18, Ryan Sleevi wrote: > > > https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/client... > > File chrome/browser/net/client_hints.cc (right): > > > > > https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/client... > > chrome/browser/net/client_hints.cc:39: base::Owned(info))); > > On 2013/03/06 00:34:19, bengr1 wrote: > > > I presume that screen resolution changes are much rarer events than Chrome > > > restarts. In fact on iOS and Android devices, afaik, screen resolution can't > > be > > > changed. I therefore didn't see the point in the added code complexity to > > > support screen resolution changes mid browsing session. > > > > > > On 2013/02/27 21:06:05, mmenke wrote: > > > > What if the user changes screen resolution? > > > > > > > Why isn't an orientation change considered a resolution change? I would expect > > it to be > > The current thinking is that orientation will be conveyed separately. Separate from orientation or separate as in not a client hint? If this manages to catch on (And I think there's a definite need), I have no doubt that we'll get bug reports on handling resolution changes.
On 2013/03/06 00:44:50, mmenke wrote: > On 2013/03/06 00:41:54, bengr1 wrote: > > On 2013/03/06 00:38:18, Ryan Sleevi wrote: > > > > > > https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/client... > > > File chrome/browser/net/client_hints.cc (right): > > > > > > > > > https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/client... > > > chrome/browser/net/client_hints.cc:39: base::Owned(info))); > > > On 2013/03/06 00:34:19, bengr1 wrote: > > > > I presume that screen resolution changes are much rarer events than Chrome > > > > restarts. In fact on iOS and Android devices, afaik, screen resolution > can't > > > be > > > > changed. I therefore didn't see the point in the added code complexity to > > > > support screen resolution changes mid browsing session. > > > > > > > > On 2013/02/27 21:06:05, mmenke wrote: > > > > > What if the user changes screen resolution? > > > > > > > > > > Why isn't an orientation change considered a resolution change? I would > expect > > > it to be > > > > The current thinking is that orientation will be conveyed separately. > > Separate from orientation or separate as in not a client hint? > > If this manages to catch on (And I think there's a definite need), I have no > doubt that we'll get bug reports on handling resolution changes. Think the big case here would be a combination of hooking up an always-on (Or always-in-suspect) device to an external monitor. Fine not to worry about it now, but it's something we should be thinking about.
On 2013/03/06 00:44:50, mmenke wrote: > On 2013/03/06 00:41:54, bengr1 wrote: > > On 2013/03/06 00:38:18, Ryan Sleevi wrote: > > > > > > https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/client... > > > File chrome/browser/net/client_hints.cc (right): > > > > > > > > > https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/client... > > > chrome/browser/net/client_hints.cc:39: base::Owned(info))); > > > On 2013/03/06 00:34:19, bengr1 wrote: > > > > I presume that screen resolution changes are much rarer events than Chrome > > > > restarts. In fact on iOS and Android devices, afaik, screen resolution > can't > > > be > > > > changed. I therefore didn't see the point in the added code complexity to > > > > support screen resolution changes mid browsing session. > > > > > > > > On 2013/02/27 21:06:05, mmenke wrote: > > > > > What if the user changes screen resolution? > > > > > > > > > > Why isn't an orientation change considered a resolution change? I would > expect > > > it to be > > > > The current thinking is that orientation will be conveyed separately. > > Separate from orientation or separate as in not a client hint? > > If this manages to catch on (And I think there's a definite need), I have no > doubt that we'll get bug reports on handling resolution changes. Orientation will be another client hint. The point of the current CL was to get a dead simple hint in place to see how well it works and what issues it exposes. Future CLs might report on more dynamic hints, e.g., orientation, network type, etc. Would you mind if we postpone handling screen resolution dynamics until a future CL that adds orientation? (Client hints is behind a flag.)
On 2013/03/06 01:10:43, bengr1 wrote: > On 2013/03/06 00:44:50, mmenke wrote: > > On 2013/03/06 00:41:54, bengr1 wrote: > > > On 2013/03/06 00:38:18, Ryan Sleevi wrote: > > > > > > > > > > https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/client... > > > > File chrome/browser/net/client_hints.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/11970002/diff/47003/chrome/browser/net/client... > > > > chrome/browser/net/client_hints.cc:39: base::Owned(info))); > > > > On 2013/03/06 00:34:19, bengr1 wrote: > > > > > I presume that screen resolution changes are much rarer events than > Chrome > > > > > restarts. In fact on iOS and Android devices, afaik, screen resolution > > can't > > > > be > > > > > changed. I therefore didn't see the point in the added code complexity > to > > > > > support screen resolution changes mid browsing session. > > > > > > > > > > On 2013/02/27 21:06:05, mmenke wrote: > > > > > > What if the user changes screen resolution? > > > > > > > > > > > > > Why isn't an orientation change considered a resolution change? I would > > expect > > > > it to be > > > > > > The current thinking is that orientation will be conveyed separately. > > > > Separate from orientation or separate as in not a client hint? > > > > If this manages to catch on (And I think there's a definite need), I have no > > doubt that we'll get bug reports on handling resolution changes. > > Orientation will be another client hint. The point of the current CL was to get > a dead simple hint in place to see how well it works and what issues it exposes. > Future CLs might report on more dynamic hints, e.g., orientation, network type, > etc. > > Would you mind if we postpone handling screen resolution dynamics until a future > CL that adds orientation? > > (Client hints is behind a flag.) Ahh, we posted at the same time. Just to be sure you didn't miss it: I'm fine with putting it off for now. I'll take another quick look at the CL tomorrow, and expect to sign off on it then.
LGTM!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bengr@google.com/11970002/54001
On 2013/03/06 19:16:25, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/bengr%40google.com/11970002/54001 ben: Are you still planning to land this?
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. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
