Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1310743003:
Consistently use LoFi for an entire page (Closed)
|
Created:
5 years, 4 months ago by megjablon Modified:
5 years, 2 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, cbentzel+watch_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, tbansal1, clamy Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConsistently use LoFi for an entire page
Store the LoFi state in the renderer and use that state to
add LoFi request headers when appropriate. Introduce
a state variable that has three values: LOFI_DEFAULT,
LOFI_ON, and LOFI_OFF. The snackbar triggering does not
use this state yet, but still looks at the last mainframe LoFi
value. Single image loads also use the bypass cache flag up to
willSendRequest. These will be fixed in follow up cls.
The LoFi state is not preserved from navigation to navigation,
and iframes inherit the status. Renderer initiated navigations
always start with state LOFI_DEFAULT whereas browser initiated
navigations can force LOFI_ON or LOFI_OFF.
BUG=524652
Committed: https://crrev.com/d5ac7d52b323d04178bba9cc746c623438d48751
Cr-Commit-Position: refs/heads/master@{#355679}
Patch Set 1 : test fixes #
Total comments: 64
Patch Set 2 : remove log statement and fix ResponseInfo::DeepCopy #
Total comments: 55
Patch Set 3 : addressing comments #
Total comments: 16
Patch Set 4 : use LoFiDefault in RequestNavigationParams constructor #
Total comments: 6
Patch Set 5 : using UserData and adressing nasko comments #
Total comments: 23
Patch Set 6 : addressing comments #Patch Set 7 : move DRP parts to https://codereview.chromium.org/1363673004/ #
Total comments: 23
Patch Set 8 : mmenke comments #
Total comments: 6
Patch Set 9 : mmenke comments #
Total comments: 17
Patch Set 10 : nasko comments #
Total comments: 2
Patch Set 11 : fix #
Total comments: 2
Patch Set 12 : rebase #
Total comments: 1
Patch Set 13 : added test and rebased #
Total comments: 14
Patch Set 14 : test fix #
Total comments: 42
Patch Set 15 : test comment fixes #
Total comments: 20
Patch Set 16 : mmenke comments #
Total comments: 13
Patch Set 17 : addressing comments #
Total comments: 18
Patch Set 18 : bengr comments and rebase #Depends on Patchset: Messages
Total messages: 125 (36 generated)
megjablon@chromium.org changed reviewers: + bengr@chromium.org
The structure looks good. I'm assuming LoFi will continue to work after you land this CL, even though not all features have moved over to this architecture, right? https://codereview.chromium.org/1310743003/diff/20001/chrome/renderer/page_lo... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/1310743003/diff/20001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.cc:418: bool in_lofi_enabled = Rename as in_lofi_enabled_group, or lofi_available. https://codereview.chromium.org/1310743003/diff/20001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.cc:422: bool in_lofi_control = Rename as in_lofi_control_group, or lofi_control_group. https://codereview.chromium.org/1310743003/diff/20001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.cc:543: data_reduction_proxy::params::GetLoFiFieldTrialName()) == It's probably worth the extra few lines of code to have methods in an anonymous namespace for these. E.g.: bool IsInLoFiControlGroup() { return base::FieldTrialList::FindFullName( data_reduction_proxy::params::GetLoFiFieldTrialName()) == kControl; } Alternatively, you could have one function return an enum. I'm fine with either. https://codereview.chromium.org/1310743003/diff/20001/chrome/renderer/page_lo... File chrome/renderer/page_load_histograms.h (right): https://codereview.chromium.org/1310743003/diff/20001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.h:50: void Dump(blink::WebFrame* frame, bool lofi_used); Add a description of this new parameter to the comment. https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.h (right): https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.h:29: void OnDataReductionProxyStatus(const net::HostPortPair& proxy_server, Why not just return a bool? https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter_unittest.cc (left): https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter_unittest.cc:62: } tests[] = { Why don't we need all of these test cases? https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (left): https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:465: LoFiStatus DataReductionProxyConfig::GetLoFiStatus() const { More deleted code. Why? https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:165: lofi_off_(false), Why this change? Does this mean lofi is on until we learn it shouldn't be? https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:775: if (params::IsLoFiAlwaysOnViaFlags()) { Remove curly braces. https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:190: // Returns true only if Lo-Fi "q=low" header should be added to the Chrome only if -> only when Same for line 300 https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:300: // // Returns true only if Lo-Fi "q=low" header should be added to the Chrome Remove the second //. On the next line too. https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:363: bool lofi_off_; Please add a comment. https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc (left): https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:861: TEST_F(DataReductionProxyConfigTest, LoFiOn) { More unexplained deletions. https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:93: // Values of change in the state of Auto Lo-Fi request headers. Auto Lo-Fi request header state changes. https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:94: // Possible Lo-Fi headers are: empty (""), low ("low"). Please make sure you grabbed the current version of this. I think, for example, that this comment says '... low("q=low")' https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:264: // Some requests, such as pings, don't go through will send request and get Name the method instead of "will send request". https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h (right): https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:141: bool previous_state_lofi_on_; Add a comment. https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc (right): https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:253: // Lo-Fi already off. Lo-Fi should not be used. Lo-Fi is already off. Add "is" on 267, 282, and 311 as well https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:351: 0, // not in enabled field trial, UMA is not recorded // not in enabled field trial si UMA should not be recorded https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc (right): https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:166: DCHECK(!(request->lofi_state() == net::LOFI_DEFAULT)); This won't compile on a release build. Do DCHECK(!request || !(request->lofi_state() == net::LOFI_DEFAULT)) https://codereview.chromium.org/1310743003/diff/20001/content/child/request_e... File content/child/request_extra_data.h (right): https://codereview.chromium.org/1310743003/diff/20001/content/child/request_e... content/child/request_extra_data.h:121: void set_lofi_state(LoFiState lofi_state) { Add a blank line above. https://codereview.chromium.org/1310743003/diff/20001/content/common/navigati... File content/common/navigation_params.h (right): https://codereview.chromium.org/1310743003/diff/20001/content/common/navigati... content/common/navigation_params.h:28: // Add the LoFi header to the request. I would make the comments more declarative, e.g.: // Request a LoFi version of the resource. // Request a normal (non-LoFi) version of the resource. // Let the browser process decide whether or not to request the LoFi version. https://codereview.chromium.org/1310743003/diff/20001/content/common/navigati... content/common/navigation_params.h:274: // Whether or not the LoFi header should be added to the request or if the Not necessary or desirable imho to describe how LoFi is requested. https://codereview.chromium.org/1310743003/diff/20001/content/common/resource... File content/common/resource_messages.h (right): https://codereview.chromium.org/1310743003/diff/20001/content/common/resource... content/common/resource_messages.h:273: // TODO(megjablon): Add comment. Add it. :) https://codereview.chromium.org/1310743003/diff/20001/content/public/common/r... File content/public/common/resource_response_info.h (right): https://codereview.chromium.org/1310743003/diff/20001/content/public/common/r... content/public/common/resource_response_info.h:133: // TODO(megjablon): Add comment. Again. https://codereview.chromium.org/1310743003/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1310743003/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:2654: lofi_state_ = extra_data->is_lofi() ? LOFI_ON : LOFI_OFF; What is lofi_state_ if !extra_data? Maybe do: lofi_state_ = extra_data && extra_data->is_lofi() ? LOFI_ON : LOFI_OFF; Alternatively: if (extra_data && extra_data->is_lofi()) lofi_state_ = LOFI_ON; else lofi_state_ = LOFI_OFF; https://codereview.chromium.org/1310743003/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:3401: Remove these blank lines. https://codereview.chromium.org/1310743003/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:4451: LOG(WARNING) << common_params.url << ": " << request_params.lofi_state; Remove the warning. https://codereview.chromium.org/1310743003/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1310743003/diff/20001/content/renderer/render... content/renderer/render_frame_impl.h:1011: // decision should be left up to the network-quality-based triggering logic. I wouldn't get into how the browser process decides. For example, we may add a user pref and use LOFI_DEFAULT to signify that the pref should be checked. https://codereview.chromium.org/1310743003/diff/20001/net/url_request/url_req... File net/url_request/url_request.h (right): https://codereview.chromium.org/1310743003/diff/20001/net/url_request/url_req... net/url_request/url_request.h:70: // Check with the network-quality-based triggering logic and add the header if See my previous comment about not specifying the implementation. https://codereview.chromium.org/1310743003/diff/20001/net/url_request/url_req... net/url_request/url_request.h:313: // TODO(megjablon): Add comment. Add it. https://codereview.chromium.org/1310743003/diff/20001/net/url_request/url_req... net/url_request/url_request.h:849: // TODO(megjablon): Add comment Add it.
https://codereview.chromium.org/1310743003/diff/20001/chrome/renderer/page_lo... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/1310743003/diff/20001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.cc:418: bool in_lofi_enabled = On 2015/08/25 00:00:01, bengr wrote: > Rename as in_lofi_enabled_group, or lofi_available. Done. https://codereview.chromium.org/1310743003/diff/20001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.cc:422: bool in_lofi_control = On 2015/08/25 00:00:01, bengr wrote: > Rename as in_lofi_control_group, or lofi_control_group. Done. https://codereview.chromium.org/1310743003/diff/20001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.cc:543: data_reduction_proxy::params::GetLoFiFieldTrialName()) == On 2015/08/25 00:00:01, bengr wrote: > It's probably worth the extra few lines of code to have methods in an anonymous > namespace for these. > > E.g.: > bool IsInLoFiControlGroup() { > return base::FieldTrialList::FindFullName( > data_reduction_proxy::params::GetLoFiFieldTrialName()) == kControl; > } > > Alternatively, you could have one function return an enum. I'm fine with either. Done. https://codereview.chromium.org/1310743003/diff/20001/chrome/renderer/page_lo... File chrome/renderer/page_load_histograms.h (right): https://codereview.chromium.org/1310743003/diff/20001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.h:50: void Dump(blink::WebFrame* frame, bool lofi_used); On 2015/08/25 00:00:01, bengr wrote: > Add a description of this new parameter to the comment. Done. https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.h (right): https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.h:29: void OnDataReductionProxyStatus(const net::HostPortPair& proxy_server, On 2015/08/25 00:00:01, bengr wrote: > Why not just return a bool? This is how IPC return values work. https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter_unittest.cc (left): https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter_unittest.cc:62: } tests[] = { On 2015/08/25 00:00:01, bengr wrote: > Why don't we need all of these test cases? The DRPMessageFilter no longer provides lofi state to the page load histograms. The state is now passed in through FrameWillClose and ClosePage. Renaming the method to OnIsDataReductionProxy. https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (left): https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:465: LoFiStatus DataReductionProxyConfig::GetLoFiStatus() const { On 2015/08/25 00:00:01, bengr wrote: > More deleted code. Why? LoFiStatus doesn't exist anymore, we base Lo-Fi being used on the new LoFiState and checking that we're not in the control group in DRPRequestOptions. Therefore, none of this code is necessary. https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:165: lofi_off_(false), On 2015/08/25 00:00:01, bengr wrote: > Why this change? Does this mean lofi is on until we learn it shouldn't be? lofi_off sets lofi off for the rest of the session. Once we enter the state, it isn't reset until a new session starts. The idea temporarily off doesn't exist anymore since that was set when lofi was being turned off due to bypassing the cache, which we are no longer using as a signal. We bypass the cache by setting the state to LOFI_OFF in RenderFrameImpl. This new value is simply used for implicit opt out. https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:775: if (params::IsLoFiAlwaysOnViaFlags()) { On 2015/08/25 00:00:01, bengr wrote: > Remove curly braces. Done. https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:190: // Returns true only if Lo-Fi "q=low" header should be added to the Chrome On 2015/08/25 00:00:01, bengr wrote: > only if -> only when > Same for line 300 Done. https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:300: // // Returns true only if Lo-Fi "q=low" header should be added to the Chrome On 2015/08/25 00:00:01, bengr wrote: > Remove the second //. On the next line too. Removed the method https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:363: bool lofi_off_; On 2015/08/25 00:00:01, bengr wrote: > Please add a comment. Done. https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc (left): https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:861: TEST_F(DataReductionProxyConfigTest, LoFiOn) { On 2015/08/25 00:00:01, bengr wrote: > More unexplained deletions. Not deleted, just moved. This in now in DRPNetworkDelegateUnittest. https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:947: TEST_F(DataReductionProxyConfigTest, LoFiStatusTransition) { We already test the transitions in DRPNetworkDelegateUnittest https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:93: // Values of change in the state of Auto Lo-Fi request headers. On 2015/08/25 00:00:01, bengr wrote: > Auto Lo-Fi request header state changes. Done. https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:94: // Possible Lo-Fi headers are: empty (""), low ("low"). On 2015/08/25 00:00:01, bengr wrote: > Please make sure you grabbed the current version of this. I think, for example, > that this comment says '... low("q=low")' Done. https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:264: // Some requests, such as pings, don't go through will send request and get On 2015/08/25 00:00:01, bengr wrote: > Name the method instead of "will send request". Done. https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h (right): https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:141: bool previous_state_lofi_on_; On 2015/08/25 00:00:01, bengr wrote: > Add a comment. Done. https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc (right): https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:253: // Lo-Fi already off. Lo-Fi should not be used. On 2015/08/25 00:00:02, bengr wrote: > Lo-Fi is already off. Add "is" on 267, 282, and 311 as well Done. https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:351: 0, // not in enabled field trial, UMA is not recorded On 2015/08/25 00:00:01, bengr wrote: > // not in enabled field trial si UMA should not be recorded Done. https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc (right): https://codereview.chromium.org/1310743003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:166: DCHECK(!(request->lofi_state() == net::LOFI_DEFAULT)); On 2015/08/25 00:00:02, bengr wrote: > This won't compile on a release build. > > Do DCHECK(!request || !(request->lofi_state() == net::LOFI_DEFAULT)) Done. https://codereview.chromium.org/1310743003/diff/20001/content/child/request_e... File content/child/request_extra_data.h (right): https://codereview.chromium.org/1310743003/diff/20001/content/child/request_e... content/child/request_extra_data.h:121: void set_lofi_state(LoFiState lofi_state) { On 2015/08/25 00:00:02, bengr wrote: > Add a blank line above. Moved above where there isn't spacing between setters and getters. https://codereview.chromium.org/1310743003/diff/20001/content/common/navigati... File content/common/navigation_params.h (right): https://codereview.chromium.org/1310743003/diff/20001/content/common/navigati... content/common/navigation_params.h:28: // Add the LoFi header to the request. On 2015/08/25 00:00:02, bengr wrote: > I would make the comments more declarative, e.g.: > > // Request a LoFi version of the resource. > > // Request a normal (non-LoFi) version of the resource. > > // Let the browser process decide whether or not to request the LoFi version. Done. https://codereview.chromium.org/1310743003/diff/20001/content/common/navigati... content/common/navigation_params.h:274: // Whether or not the LoFi header should be added to the request or if the On 2015/08/25 00:00:02, bengr wrote: > Not necessary or desirable imho to describe how LoFi is requested. Done. https://codereview.chromium.org/1310743003/diff/20001/content/common/resource... File content/common/resource_messages.h (right): https://codereview.chromium.org/1310743003/diff/20001/content/common/resource... content/common/resource_messages.h:273: // TODO(megjablon): Add comment. On 2015/08/25 00:00:02, bengr wrote: > Add it. :) Done. https://codereview.chromium.org/1310743003/diff/20001/content/public/common/r... File content/public/common/resource_response_info.h (right): https://codereview.chromium.org/1310743003/diff/20001/content/public/common/r... content/public/common/resource_response_info.h:133: // TODO(megjablon): Add comment. On 2015/08/25 00:00:02, bengr wrote: > Again. Done. https://codereview.chromium.org/1310743003/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1310743003/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:2654: lofi_state_ = extra_data->is_lofi() ? LOFI_ON : LOFI_OFF; On 2015/08/25 00:00:02, bengr wrote: > What is lofi_state_ if !extra_data? Maybe do: > > lofi_state_ = extra_data && extra_data->is_lofi() ? LOFI_ON : LOFI_OFF; > > > Alternatively: > > if (extra_data && extra_data->is_lofi()) > lofi_state_ = LOFI_ON; > else > lofi_state_ = LOFI_OFF; Done. https://codereview.chromium.org/1310743003/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:4451: LOG(WARNING) << common_params.url << ": " << request_params.lofi_state; On 2015/08/25 00:00:02, bengr wrote: > Remove the warning. Done. https://codereview.chromium.org/1310743003/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1310743003/diff/20001/content/renderer/render... content/renderer/render_frame_impl.h:1011: // decision should be left up to the network-quality-based triggering logic. On 2015/08/25 00:00:02, bengr wrote: > I wouldn't get into how the browser process decides. For example, we may add a > user pref and use LOFI_DEFAULT to signify that the pref should be checked. Done. https://codereview.chromium.org/1310743003/diff/20001/net/url_request/url_req... File net/url_request/url_request.h (right): https://codereview.chromium.org/1310743003/diff/20001/net/url_request/url_req... net/url_request/url_request.h:70: // Check with the network-quality-based triggering logic and add the header if On 2015/08/25 00:00:02, bengr wrote: > See my previous comment about not specifying the implementation. Done. https://codereview.chromium.org/1310743003/diff/20001/net/url_request/url_req... net/url_request/url_request.h:313: // TODO(megjablon): Add comment. On 2015/08/25 00:00:02, bengr wrote: > Add it. Done. https://codereview.chromium.org/1310743003/diff/20001/net/url_request/url_req... net/url_request/url_request.h:849: // TODO(megjablon): Add comment On 2015/08/25 00:00:02, bengr wrote: > Add it. Done.
megjablon@chromium.org changed reviewers: + davidben@chromium.org
davidben: content/* and net/*
Patchset #5 (id:80001) has been deleted
davidben@chromium.org changed reviewers: + creis@chromium.org, nasko@chromium.org
+creis and nasko who are probably the right people to review much of this, since there's OOPIF plumbing. I did an initial pass over it. I haven't looked at it more closely yet since it's a rather large CL and there's a few layering problems to be resolved. https://codereview.chromium.org/1310743003/diff/100001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/1310743003/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:298: data_reduction_proxy_request_options_->MaybeAddRequestHeader( When we were discussing design, it was mentioned that Blink's memory cache must be accounted for when adding headers conditionally in the browser process. What is the plan here? https://codereview.chromium.org/1310743003/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1310743003/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1234: new_request->set_lofi_state((net::LoFiState) request_data.lofi_state); Use C++-style static_cast<net::LofiState> in Chromium. This also comes from the renderer, so it's important to validate the value; nothing's stopping it from being outside the enum right now. https://codereview.chromium.org/1310743003/diff/100001/content/browser/loader... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/1310743003/diff/100001/content/browser/loader... content/browser/loader/resource_loader.cc:105: response->head.is_lofi = request->lofi_state() == net::LOFI_ON; Without the embedder colluding, there's nothing ensuring that LOFI_DEFAULT is handled correctly. If we're teaching //content about lofi state, //content should be interpreting LOFI_DEFAULT. If it needs to call out to something to query the bandwidth estimator, then //content can call out to its embedder via one of the top-level client/delegate interfaces to control what LOFI_DEFAULT means, with the default implementation being off. (Was there an Intent to Implement for this header? We're adding a new header to the web platform, so it needs to go through the Blink process.) https://codereview.chromium.org/1310743003/diff/100001/content/common/navigat... File content/common/navigation_params.h (right): https://codereview.chromium.org/1310743003/diff/100001/content/common/navigat... content/common/navigation_params.h:26: // sync with the enum in url_request.h. There is no reason for this. The enum shouldn't be in url_request.h at all. https://codereview.chromium.org/1310743003/diff/100001/content/common/navigat... content/common/navigation_params.h:206: int lofi_state); Use your enum, not an int. https://codereview.chromium.org/1310743003/diff/100001/content/common/navigat... content/common/navigation_params.h:275: int lofi_state; Use your enum, not an int. https://codereview.chromium.org/1310743003/diff/100001/content/common/resourc... File content/common/resource_messages.h (right): https://codereview.chromium.org/1310743003/diff/100001/content/common/resourc... content/common/resource_messages.h:275: IPC_STRUCT_MEMBER(int, lofi_state) This needs to be an enum, not an int. The IPC logic should also ensure thath invalid enum values are discarded. (nasko would know more about this.) https://codereview.chromium.org/1310743003/diff/100001/content/public/rendere... File content/public/renderer/render_view_observer.h (right): https://codereview.chromium.org/1310743003/diff/100001/content/public/rendere... content/public/renderer/render_view_observer.h:31: class CONTENT_EXPORT RenderViewObserver : public IPC::Listener, Isn't RenderViewObserver being deprecated? Why not RenderFrameObserver? https://codereview.chromium.org/1310743003/diff/100001/content/public/rendere... content/public/renderer/render_view_observer.h:56: virtual void FrameWillClose(blink::WebFrame* frame, bool lofi_used) {} This is a rather specific parameter to plumb into so general of a hook. This smells wrong. You should instead get the value out of the RenderFrame. https://codereview.chromium.org/1310743003/diff/100001/content/public/rendere... content/public/renderer/render_view_observer.h:70: virtual void ClosePage(bool lofi_used) {} Ditto. https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:729: weak_factory_(this) { lofi_state_ is not initialized. https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.h:223: void Initialize(LoFiState lofi_state = LOFI_DEFAULT); Style: no default arguments. https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argu... https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.h:1032: // decide. This is the copy of the field on the RenderFrameImpl, so "the resource" doesn't make sense. Document exactly what this means. (I thought RenderFrameImpl would never see LOFI_DEFAULT. It should always know which one it is. In my sketch it was just a boolean, but perhaps I didn't foresee some complication.) https://codereview.chromium.org/1310743003/diff/100001/net/url_request/url_re... File net/url_request/url_request.h (right): https://codereview.chromium.org/1310743003/diff/100001/net/url_request/url_re... net/url_request/url_request.h:850: LoFiState lofi_state_; Why is this on the URLRequest? It's not used by //net at all. //net is not just an HTTP stack for //content. Consumer-specific state needs to be in the consumer. //net can't know about it while //content needs to, so this should be in //content somewhere.
https://codereview.chromium.org/1310743003/diff/100001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/1310743003/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:298: data_reduction_proxy_request_options_->MaybeAddRequestHeader( On 2015/08/27 18:53:50, David Benjamin wrote: > When we were discussing design, it was mentioned that Blink's memory cache must > be accounted for when adding headers conditionally in the browser process. What > is the plan here? Initially, we are not going to do anything special with Blink's memory cache and we will not initially be using Vary to cache LoFi and non-Lofi resources separately. We will be using cache-control: private on LoFi resources so intermediaries don't get confused. https://codereview.chromium.org/1310743003/diff/100001/content/browser/loader... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/1310743003/diff/100001/content/browser/loader... content/browser/loader/resource_loader.cc:105: response->head.is_lofi = request->lofi_state() == net::LOFI_ON; On 2015/08/27 18:53:50, David Benjamin wrote: > Without the embedder colluding, there's nothing ensuring that LOFI_DEFAULT is > handled correctly. If we're teaching //content about lofi state, //content > should be interpreting LOFI_DEFAULT. If it needs to call out to something to > query the bandwidth estimator, then //content can call out to its embedder via > one of the top-level client/delegate interfaces to control what LOFI_DEFAULT > means, with the default implementation being off. > > (Was there an Intent to Implement for this header? We're adding a new header to > the web platform, so it needs to go through the Blink process.) The header isn't really a header right now; it's a directive within the data reduction proxy's "Chrome-Proxy" header. I.e., "Chrome-Proxy: q=low". I don't think we want the web platform knowing about this. When we move to a separate header 'RQ: low' we can remove the collusion. Is this ok with you? https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.h:1032: // decide. On 2015/08/27 18:53:50, David Benjamin wrote: > This is the copy of the field on the RenderFrameImpl, so "the resource" doesn't > make sense. Document exactly what this means. > > (I thought RenderFrameImpl would never see LOFI_DEFAULT. It should always know > which one it is. In my sketch it was just a boolean, but perhaps I didn't > foresee some complication.) Agreed this can/should be a bit.
https://codereview.chromium.org/1310743003/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1310743003/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1234: new_request->set_lofi_state((net::LoFiState) request_data.lofi_state); On 2015/08/27 18:53:50, David Benjamin wrote: > Use C++-style static_cast<net::LofiState> in Chromium. This also comes from the > renderer, so it's important to validate the value; nothing's stopping it from > being outside the enum right now. Done. https://codereview.chromium.org/1310743003/diff/100001/content/common/navigat... File content/common/navigation_params.h (right): https://codereview.chromium.org/1310743003/diff/100001/content/common/navigat... content/common/navigation_params.h:26: // sync with the enum in url_request.h. On 2015/08/27 18:53:50, David Benjamin wrote: > There is no reason for this. The enum shouldn't be in url_request.h at all. Using URLRequest in DataReductionProxyNetworkDelegate::OnBeforeSendProxyHeadersInternal is how we add q=low to the Chrome-Proxy header. https://codereview.chromium.org/1310743003/diff/100001/content/common/navigat... content/common/navigation_params.h:206: int lofi_state); On 2015/08/27 18:53:50, David Benjamin wrote: > Use your enum, not an int. Done. https://codereview.chromium.org/1310743003/diff/100001/content/common/navigat... content/common/navigation_params.h:275: int lofi_state; On 2015/08/27 18:53:50, David Benjamin wrote: > Use your enum, not an int. Using the enum instead of an int breaks IPC compilation. RequestNavigationParams is sent over IPC using an IPC struct and I don't know of a way for an enum to be a member. https://codereview.chromium.org/1310743003/diff/100001/content/common/resourc... File content/common/resource_messages.h (right): https://codereview.chromium.org/1310743003/diff/100001/content/common/resourc... content/common/resource_messages.h:275: IPC_STRUCT_MEMBER(int, lofi_state) On 2015/08/27 18:53:50, David Benjamin wrote: > This needs to be an enum, not an int. The IPC logic should also ensure thath > invalid enum values are discarded. (nasko would know more about this.) Is there a way for an IPC struct to have enums? I couldn't figure out a way to do this. https://codereview.chromium.org/1310743003/diff/100001/content/public/rendere... File content/public/renderer/render_view_observer.h (right): https://codereview.chromium.org/1310743003/diff/100001/content/public/rendere... content/public/renderer/render_view_observer.h:31: class CONTENT_EXPORT RenderViewObserver : public IPC::Listener, On 2015/08/27 18:53:50, David Benjamin wrote: > Isn't RenderViewObserver being deprecated? Why not RenderFrameObserver? PageLoadHistograms is derived from RenderViewObserver where we report Lo-Fi specific PLT histograms. https://codereview.chromium.org/1310743003/diff/100001/content/public/rendere... content/public/renderer/render_view_observer.h:56: virtual void FrameWillClose(blink::WebFrame* frame, bool lofi_used) {} On 2015/08/27 18:53:50, David Benjamin wrote: > This is a rather specific parameter to plumb into so general of a hook. This > smells wrong. You should instead get the value out of the RenderFrame. Without PlageLoadHistograms being a subclass of RenderFrameObserver, we have to pass this value in. https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:729: weak_factory_(this) { On 2015/08/27 18:53:50, David Benjamin wrote: > lofi_state_ is not initialized. Done. https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.h:223: void Initialize(LoFiState lofi_state = LOFI_DEFAULT); On 2015/08/27 18:53:50, David Benjamin wrote: > Style: no default arguments. > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argu... Done. https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.h:1032: // decide. On 2015/08/27 21:14:07, bengr wrote: > On 2015/08/27 18:53:50, David Benjamin wrote: > > This is the copy of the field on the RenderFrameImpl, so "the resource" > doesn't > > make sense. Document exactly what this means. > > > > (I thought RenderFrameImpl would never see LOFI_DEFAULT. It should always know > > which one it is. In my sketch it was just a boolean, but perhaps I didn't > > foresee some complication.) > > Agreed this can/should be a bit. I don't think this can be a bit. We need this for the ability to turn off Lo-Fi on mainframe requests. RenderFrameHost sends a FrameMsg_Navigate message to RenderFrameImpl. This message includes RequestNavigationParams and for navigations where the user wants to explicitly disable LoFi, we add LOFI_OFF to those params. We store this in lofi_state_ in RenderFrameImpl::NavigateInternal and in RenderFrameImpl::willSendRequest we copy the lofi_state_ to RequestExtraData. If we only have a bit then we don't know if the mainframe request wants to explicitly turn on or off Lo-Fi or let the browser decide. https://codereview.chromium.org/1310743003/diff/100001/net/url_request/url_re... File net/url_request/url_request.h (right): https://codereview.chromium.org/1310743003/diff/100001/net/url_request/url_re... net/url_request/url_request.h:850: LoFiState lofi_state_; On 2015/08/27 18:53:50, David Benjamin wrote: > Why is this on the URLRequest? It's not used by //net at all. //net is not just > an HTTP stack for //content. Consumer-specific state needs to be in the > consumer. //net can't know about it while //content needs to, so this should be > in //content somewhere. We add q=low to the Chrome-Proxy header in DataReductionProxyNetworkDelegate::OnBeforeSendProxyHeadersInternal which gets a URLRequest.
https://codereview.chromium.org/1310743003/diff/100001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/1310743003/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:298: data_reduction_proxy_request_options_->MaybeAddRequestHeader( On 2015/08/27 21:14:07, bengr wrote: > On 2015/08/27 18:53:50, David Benjamin wrote: > > When we were discussing design, it was mentioned that Blink's memory cache > must > > be accounted for when adding headers conditionally in the browser process. > What > > is the plan here? > > Initially, we are not going to do anything special with Blink's memory cache and > we will not initially be using Vary to cache LoFi and non-Lofi resources > separately. We will be using cache-control: private on LoFi resources so > intermediaries don't get confused. > I don't think that works. Cache-Control: private only means that shared caches can't cache it, but this barely matters since connections to the DRP are over HTTPS anyway. Both caches in Chromium are private caches. I would be very surprised if Blink's cache discarded these. https://codereview.chromium.org/1310743003/diff/100001/content/browser/loader... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/1310743003/diff/100001/content/browser/loader... content/browser/loader/resource_loader.cc:105: response->head.is_lofi = request->lofi_state() == net::LOFI_ON; On 2015/08/27 21:14:07, bengr wrote: > On 2015/08/27 18:53:50, David Benjamin wrote: > > Without the embedder colluding, there's nothing ensuring that LOFI_DEFAULT is > > handled correctly. If we're teaching //content about lofi state, //content > > should be interpreting LOFI_DEFAULT. If it needs to call out to something to > > query the bandwidth estimator, then //content can call out to its embedder via > > one of the top-level client/delegate interfaces to control what LOFI_DEFAULT > > means, with the default implementation being off. > > > > (Was there an Intent to Implement for this header? We're adding a new header > to > > the web platform, so it needs to go through the Blink process.) > > The header isn't really a header right now; it's a directive within the data > reduction proxy's "Chrome-Proxy" header. I.e., "Chrome-Proxy: q=low". I don't > think we want the web platform knowing about this. When we move to a separate > header 'RQ: low' we can remove the collusion. Is this ok with you? Can we make it a separate header and do it properly? Otherwise this results in an extremely weird API at the //content boundary. The API contract between //content and its embedder is now "I set this lofi bit on every request, but honestly I haven't the slightest clue what it means. I'll just propagate it. There's also this LOFI_DEFAULT setting, but I don't know what to do with it either. If you intercept the URLRequest and mutate it early enough, then I'll read the value back and propagate as what you chose. Otherwise, uh, I'll make something up." https://codereview.chromium.org/1310743003/diff/100001/content/common/navigat... File content/common/navigation_params.h (right): https://codereview.chromium.org/1310743003/diff/100001/content/common/navigat... content/common/navigation_params.h:275: int lofi_state; On 2015/08/27 23:11:02, megjablon wrote: > On 2015/08/27 18:53:50, David Benjamin wrote: > > Use your enum, not an int. > > Using the enum instead of an int breaks IPC compilation. RequestNavigationParams > is sent over IPC using an IPC struct and I don't know of a way for an enum to be > a member. nasko would be a better person to answer this than me. It is definitely possible to send enums over IPC; content::ResourceType, for instance. It looks like you need this. This also resolves not checking the macros. https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_message_ma... https://codereview.chromium.org/1310743003/diff/100001/content/public/rendere... File content/public/renderer/render_view_observer.h (right): https://codereview.chromium.org/1310743003/diff/100001/content/public/rendere... content/public/renderer/render_view_observer.h:56: virtual void FrameWillClose(blink::WebFrame* frame, bool lofi_used) {} On 2015/08/27 23:11:02, megjablon wrote: > On 2015/08/27 18:53:50, David Benjamin wrote: > > This is a rather specific parameter to plumb into so general of a hook. This > > smells wrong. You should instead get the value out of the RenderFrame. > > Without PlageLoadHistograms being a subclass of RenderFrameObserver, we have to > pass this value in. I'll defer to nasko, but I would think it's possible to get the RenderFrameHost out of the WebFrame. Alternatively, it PageLoadHistograms is fairly small. Can we convert that to RenderFrameHostObserver? https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:3353: if (request.cachePolicy() == WebURLRequest::ReloadBypassingCache) Why isn't this coming from the navigation parameters? https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.h:1032: // decide. On 2015/08/27 23:11:02, megjablon wrote: > On 2015/08/27 21:14:07, bengr wrote: > > On 2015/08/27 18:53:50, David Benjamin wrote: > > > This is the copy of the field on the RenderFrameImpl, so "the resource" > > doesn't > > > make sense. Document exactly what this means. > > > > > > (I thought RenderFrameImpl would never see LOFI_DEFAULT. It should always > know > > > which one it is. In my sketch it was just a boolean, but perhaps I didn't > > > foresee some complication.) > > > > Agreed this can/should be a bit. > > I don't think this can be a bit. We need this for the ability to turn off Lo-Fi > on mainframe requests. RenderFrameHost sends a FrameMsg_Navigate message to > RenderFrameImpl. This message includes RequestNavigationParams and for > navigations where the user wants to explicitly disable LoFi, we add LOFI_OFF to > those params. We store this in lofi_state_ in RenderFrameImpl::NavigateInternal > and in RenderFrameImpl::willSendRequest we copy the lofi_state_ to > RequestExtraData. If we only have a bit then we don't know if the mainframe > request wants to explicitly turn on or off Lo-Fi or let the browser decide. That won't work anyway. Navigations are not synchronous. While the new navigation is in flight and hasn't committed, the document is still live and may make new requests. The navigation may not even commit (perhaps a download, perhaps stopped), in which case, the document will be around for arbitrarily long afterwards. For navigations, the value needs to get attached to the request earlier. When we were hashing this out, I specifically cited CreateURLRequestforNavigation. https://codereview.chromium.org/1310743003/diff/100001/net/url_request/url_re... File net/url_request/url_request.h (right): https://codereview.chromium.org/1310743003/diff/100001/net/url_request/url_re... net/url_request/url_request.h:850: LoFiState lofi_state_; On 2015/08/27 23:11:02, megjablon wrote: > On 2015/08/27 18:53:50, David Benjamin wrote: > > Why is this on the URLRequest? It's not used by //net at all. //net is not > just > > an HTTP stack for //content. Consumer-specific state needs to be in the > > consumer. //net can't know about it while //content needs to, so this should > be > > in //content somewhere. > > We add q=low to the Chrome-Proxy header in > DataReductionProxyNetworkDelegate::OnBeforeSendProxyHeadersInternal which gets a > URLRequest. There's other ways to staple consumer-specific state, such as UserData. This CL should not end up touching anything in //net.
I'll take a look at this CL in the next couple of days, as I just came back from being out. On 2015/08/31 23:43:24, David Benjamin wrote: > https://codereview.chromium.org/1310743003/diff/100001/components/data_reduct... > File > components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc > (right): > > https://codereview.chromium.org/1310743003/diff/100001/components/data_reduct... > components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:298: > data_reduction_proxy_request_options_->MaybeAddRequestHeader( > On 2015/08/27 21:14:07, bengr wrote: > > On 2015/08/27 18:53:50, David Benjamin wrote: > > > When we were discussing design, it was mentioned that Blink's memory cache > > must > > > be accounted for when adding headers conditionally in the browser process. > > What > > > is the plan here? > > > > Initially, we are not going to do anything special with Blink's memory cache > and > > we will not initially be using Vary to cache LoFi and non-Lofi resources > > separately. We will be using cache-control: private on LoFi resources so > > intermediaries don't get confused. > > > > I don't think that works. Cache-Control: private only means that shared caches > can't cache it, but this barely matters since connections to the DRP are over > HTTPS anyway. > > Both caches in Chromium are private caches. I would be very surprised if Blink's > cache discarded these. > > https://codereview.chromium.org/1310743003/diff/100001/content/browser/loader... > File content/browser/loader/resource_loader.cc (right): > > https://codereview.chromium.org/1310743003/diff/100001/content/browser/loader... > content/browser/loader/resource_loader.cc:105: response->head.is_lofi = > request->lofi_state() == net::LOFI_ON; > On 2015/08/27 21:14:07, bengr wrote: > > On 2015/08/27 18:53:50, David Benjamin wrote: > > > Without the embedder colluding, there's nothing ensuring that LOFI_DEFAULT > is > > > handled correctly. If we're teaching //content about lofi state, //content > > > should be interpreting LOFI_DEFAULT. If it needs to call out to something to > > > query the bandwidth estimator, then //content can call out to its embedder > via > > > one of the top-level client/delegate interfaces to control what LOFI_DEFAULT > > > means, with the default implementation being off. > > > > > > (Was there an Intent to Implement for this header? We're adding a new header > > to > > > the web platform, so it needs to go through the Blink process.) > > > > The header isn't really a header right now; it's a directive within the data > > reduction proxy's "Chrome-Proxy" header. I.e., "Chrome-Proxy: q=low". I don't > > think we want the web platform knowing about this. When we move to a separate > > header 'RQ: low' we can remove the collusion. Is this ok with you? > > Can we make it a separate header and do it properly? Otherwise this results in > an extremely weird API at the //content boundary. The API contract between > //content and its embedder is now "I set this lofi bit on every request, but > honestly I haven't the slightest clue what it means. I'll just propagate it. > There's also this LOFI_DEFAULT setting, but I don't know what to do with it > either. If you intercept the URLRequest and mutate it early enough, then I'll > read the value back and propagate as what you chose. Otherwise, uh, I'll make > something up." > > https://codereview.chromium.org/1310743003/diff/100001/content/common/navigat... > File content/common/navigation_params.h (right): > > https://codereview.chromium.org/1310743003/diff/100001/content/common/navigat... > content/common/navigation_params.h:275: int lofi_state; > On 2015/08/27 23:11:02, megjablon wrote: > > On 2015/08/27 18:53:50, David Benjamin wrote: > > > Use your enum, not an int. > > > > Using the enum instead of an int breaks IPC compilation. > RequestNavigationParams > > is sent over IPC using an IPC struct and I don't know of a way for an enum to > be > > a member. > > nasko would be a better person to answer this than me. It is definitely possible > to send enums over IPC; content::ResourceType, for instance. It looks like you > need this. This also resolves not checking the macros. > > https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_message_ma... > > https://codereview.chromium.org/1310743003/diff/100001/content/public/rendere... > File content/public/renderer/render_view_observer.h (right): > > https://codereview.chromium.org/1310743003/diff/100001/content/public/rendere... > content/public/renderer/render_view_observer.h:56: virtual void > FrameWillClose(blink::WebFrame* frame, bool lofi_used) {} > On 2015/08/27 23:11:02, megjablon wrote: > > On 2015/08/27 18:53:50, David Benjamin wrote: > > > This is a rather specific parameter to plumb into so general of a hook. This > > > smells wrong. You should instead get the value out of the RenderFrame. > > > > Without PlageLoadHistograms being a subclass of RenderFrameObserver, we have > to > > pass this value in. > > I'll defer to nasko, but I would think it's possible to get the RenderFrameHost > out of the WebFrame. Alternatively, it PageLoadHistograms is fairly small. Can > we convert that to RenderFrameHostObserver? I just commented on another CL for PageLoadHistograms that it should be converted to RenderFrameObserver. There is even a bug for that - crbug.com/472140. > https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... > File content/renderer/render_frame_impl.cc (right): > > https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... > content/renderer/render_frame_impl.cc:3353: if (request.cachePolicy() == > WebURLRequest::ReloadBypassingCache) > Why isn't this coming from the navigation parameters? > > https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... > File content/renderer/render_frame_impl.h (right): > > https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... > content/renderer/render_frame_impl.h:1032: // decide. > On 2015/08/27 23:11:02, megjablon wrote: > > On 2015/08/27 21:14:07, bengr wrote: > > > On 2015/08/27 18:53:50, David Benjamin wrote: > > > > This is the copy of the field on the RenderFrameImpl, so "the resource" > > > doesn't > > > > make sense. Document exactly what this means. > > > > > > > > (I thought RenderFrameImpl would never see LOFI_DEFAULT. It should always > > know > > > > which one it is. In my sketch it was just a boolean, but perhaps I didn't > > > > foresee some complication.) > > > > > > Agreed this can/should be a bit. > > > > I don't think this can be a bit. We need this for the ability to turn off > Lo-Fi > > on mainframe requests. RenderFrameHost sends a FrameMsg_Navigate message to > > RenderFrameImpl. This message includes RequestNavigationParams and for > > navigations where the user wants to explicitly disable LoFi, we add LOFI_OFF > to > > those params. We store this in lofi_state_ in > RenderFrameImpl::NavigateInternal > > and in RenderFrameImpl::willSendRequest we copy the lofi_state_ to > > RequestExtraData. If we only have a bit then we don't know if the mainframe > > request wants to explicitly turn on or off Lo-Fi or let the browser decide. > > That won't work anyway. Navigations are not synchronous. While the new > navigation is in flight and hasn't committed, the document is still live and may > make new requests. > > The navigation may not even commit (perhaps a download, perhaps stopped), in > which case, the document will be around for arbitrarily long afterwards. > > For navigations, the value needs to get attached to the request earlier. When we > were hashing this out, I specifically cited CreateURLRequestforNavigation. > > https://codereview.chromium.org/1310743003/diff/100001/net/url_request/url_re... > File net/url_request/url_request.h (right): > > https://codereview.chromium.org/1310743003/diff/100001/net/url_request/url_re... > net/url_request/url_request.h:850: LoFiState lofi_state_; > On 2015/08/27 23:11:02, megjablon wrote: > > On 2015/08/27 18:53:50, David Benjamin wrote: > > > Why is this on the URLRequest? It's not used by //net at all. //net is not > > just > > > an HTTP stack for //content. Consumer-specific state needs to be in the > > > consumer. //net can't know about it while //content needs to, so this should > > be > > > in //content somewhere. > > > > We add q=low to the Chrome-Proxy header in > > DataReductionProxyNetworkDelegate::OnBeforeSendProxyHeadersInternal which gets > a > > URLRequest. > > There's other ways to staple consumer-specific state, such as UserData. This CL > should not end up touching anything in //net.
https://codereview.chromium.org/1310743003/diff/100001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/1310743003/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:298: data_reduction_proxy_request_options_->MaybeAddRequestHeader( On 2015/08/31 23:43:24, David Benjamin wrote: > On 2015/08/27 21:14:07, bengr wrote: > > On 2015/08/27 18:53:50, David Benjamin wrote: > > > When we were discussing design, it was mentioned that Blink's memory cache > > must > > > be accounted for when adding headers conditionally in the browser process. > > What > > > is the plan here? > > > > Initially, we are not going to do anything special with Blink's memory cache > and > > we will not initially be using Vary to cache LoFi and non-Lofi resources > > separately. We will be using cache-control: private on LoFi resources so > > intermediaries don't get confused. > > > > I don't think that works. Cache-Control: private only means that shared caches > can't cache it, but this barely matters since connections to the DRP are over > HTTPS anyway. > > Both caches in Chromium are private caches. I would be very surprised if Blink's > cache discarded these. My comment about "cache-control: private" was meant to explain that intermediary caches wouldn't get confused by responses lacking Vary, because DRP resources aren't cached by them. They're not cached in intermediaries because most of our traffic is over HTTPS and because we use "cache-control: private" in case the request was proxied over HTTP. There's logic in the DRP to fall back to HTTP for various reasons. Now blink's cache could have a LoFi resource beyond when LoFi should be used, but we're OK with that for our use case. https://codereview.chromium.org/1310743003/diff/100001/content/browser/loader... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/1310743003/diff/100001/content/browser/loader... content/browser/loader/resource_loader.cc:105: response->head.is_lofi = request->lofi_state() == net::LOFI_ON; On 2015/08/31 23:43:24, David Benjamin wrote: > On 2015/08/27 21:14:07, bengr wrote: > > On 2015/08/27 18:53:50, David Benjamin wrote: > > > Without the embedder colluding, there's nothing ensuring that LOFI_DEFAULT > is > > > handled correctly. If we're teaching //content about lofi state, //content > > > should be interpreting LOFI_DEFAULT. If it needs to call out to something to > > > query the bandwidth estimator, then //content can call out to its embedder > via > > > one of the top-level client/delegate interfaces to control what LOFI_DEFAULT > > > means, with the default implementation being off. > > > > > > (Was there an Intent to Implement for this header? We're adding a new header > > to > > > the web platform, so it needs to go through the Blink process.) > > > > The header isn't really a header right now; it's a directive within the data > > reduction proxy's "Chrome-Proxy" header. I.e., "Chrome-Proxy: q=low". I don't > > think we want the web platform knowing about this. When we move to a separate > > header 'RQ: low' we can remove the collusion. Is this ok with you? > > Can we make it a separate header and do it properly? Otherwise this results in > an extremely weird API at the //content boundary. The API contract between > //content and its embedder is now "I set this lofi bit on every request, but > honestly I haven't the slightest clue what it means. I'll just propagate it. > There's also this LOFI_DEFAULT setting, but I don't know what to do with it > either. If you intercept the URLRequest and mutate it early enough, then I'll > read the value back and propagate as what you chose. Otherwise, uh, I'll make > something up." I see. We might be open to making a server change to support 'RQ: low'. If we can't, then for now we could possibly let the browser add q=low to the Chrome-Proxy request header whenever it sees RQ: low. I'm a little hesitant, though, to commit to using RQ, because the RFC is very much in flux, but agree it is a better approach. Megan, could you make the change to do it properly as per David's definition of "properly?" https://codereview.chromium.org/1310743003/diff/100001/net/url_request/url_re... File net/url_request/url_request.h (right): https://codereview.chromium.org/1310743003/diff/100001/net/url_request/url_re... net/url_request/url_request.h:850: LoFiState lofi_state_; On 2015/08/31 23:43:24, David Benjamin wrote: > On 2015/08/27 23:11:02, megjablon wrote: > > On 2015/08/27 18:53:50, David Benjamin wrote: > > > Why is this on the URLRequest? It's not used by //net at all. //net is not > > just > > > an HTTP stack for //content. Consumer-specific state needs to be in the > > > consumer. //net can't know about it while //content needs to, so this should > > be > > > in //content somewhere. > > > > We add q=low to the Chrome-Proxy header in > > DataReductionProxyNetworkDelegate::OnBeforeSendProxyHeadersInternal which gets > a > > URLRequest. > > There's other ways to staple consumer-specific state, such as UserData. This CL > should not end up touching anything in //net. I like this goal.
https://codereview.chromium.org/1310743003/diff/100001/content/browser/loader... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/1310743003/diff/100001/content/browser/loader... content/browser/loader/resource_loader.cc:105: response->head.is_lofi = request->lofi_state() == net::LOFI_ON; On 2015/09/02 19:01:36, bengr wrote: > On 2015/08/31 23:43:24, David Benjamin wrote: > > On 2015/08/27 21:14:07, bengr wrote: > > > On 2015/08/27 18:53:50, David Benjamin wrote: > > > > Without the embedder colluding, there's nothing ensuring that LOFI_DEFAULT > > is > > > > handled correctly. If we're teaching //content about lofi state, //content > > > > should be interpreting LOFI_DEFAULT. If it needs to call out to something > to > > > > query the bandwidth estimator, then //content can call out to its embedder > > via > > > > one of the top-level client/delegate interfaces to control what > LOFI_DEFAULT > > > > means, with the default implementation being off. > > > > > > > > (Was there an Intent to Implement for this header? We're adding a new > header > > > to > > > > the web platform, so it needs to go through the Blink process.) > > > > > > The header isn't really a header right now; it's a directive within the data > > > reduction proxy's "Chrome-Proxy" header. I.e., "Chrome-Proxy: q=low". I > don't > > > think we want the web platform knowing about this. When we move to a > separate > > > header 'RQ: low' we can remove the collusion. Is this ok with you? > > > > Can we make it a separate header and do it properly? Otherwise this results in > > an extremely weird API at the //content boundary. The API contract between > > //content and its embedder is now "I set this lofi bit on every request, but > > honestly I haven't the slightest clue what it means. I'll just propagate it. > > There's also this LOFI_DEFAULT setting, but I don't know what to do with it > > either. If you intercept the URLRequest and mutate it early enough, then I'll > > read the value back and propagate as what you chose. Otherwise, uh, I'll make > > something up." > > I see. We might be open to making a server change to support 'RQ: low'. If we > can't, then for now we could possibly let the browser add q=low to the > Chrome-Proxy request header whenever it sees RQ: low. I'm a little hesitant, > though, to commit to using RQ, because the RFC is very much in flux, but agree > it is a better approach. > > Megan, could you make the change to do it properly as per David's definition of > "properly?" @David, I'm not sure I follow completely. I understand the issue with leaving LOFI_DEFAULT uninterpreted, but you please explain how the separate header would be properly used here? Where would we add 'RQ: low'? I'm trying to understand the details of the design change you would like. https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:3353: if (request.cachePolicy() == WebURLRequest::ReloadBypassingCache) On 2015/08/31 23:43:24, David Benjamin wrote: > Why isn't this coming from the navigation parameters? This is for the single image and page reloads which still utilize our original "hackiness" which is intercepted here. I am working on follow up cls so that this will come from the navigation parameters, but this cl is already very large so those fixes will be in separate cls. https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.h:1032: // decide. On 2015/08/31 23:43:24, David Benjamin wrote: > On 2015/08/27 23:11:02, megjablon wrote: > > On 2015/08/27 21:14:07, bengr wrote: > > > On 2015/08/27 18:53:50, David Benjamin wrote: > > > > This is the copy of the field on the RenderFrameImpl, so "the resource" > > > doesn't > > > > make sense. Document exactly what this means. > > > > > > > > (I thought RenderFrameImpl would never see LOFI_DEFAULT. It should always > > know > > > > which one it is. In my sketch it was just a boolean, but perhaps I didn't > > > > foresee some complication.) > > > > > > Agreed this can/should be a bit. > > > > I don't think this can be a bit. We need this for the ability to turn off > Lo-Fi > > on mainframe requests. RenderFrameHost sends a FrameMsg_Navigate message to > > RenderFrameImpl. This message includes RequestNavigationParams and for > > navigations where the user wants to explicitly disable LoFi, we add LOFI_OFF > to > > those params. We store this in lofi_state_ in > RenderFrameImpl::NavigateInternal > > and in RenderFrameImpl::willSendRequest we copy the lofi_state_ to > > RequestExtraData. If we only have a bit then we don't know if the mainframe > > request wants to explicitly turn on or off Lo-Fi or let the browser decide. > > That won't work anyway. Navigations are not synchronous. While the new > navigation is in flight and hasn't committed, the document is still live and may > make new requests. > > The navigation may not even commit (perhaps a download, perhaps stopped), in > which case, the document will be around for arbitrarily long afterwards. > > For navigations, the value needs to get attached to the request earlier. When we > were hashing this out, I specifically cited CreateURLRequestforNavigation. So you're saying we should use CreateURLRequestforNavigation instead of NavigateInternal and immediately add the LoFiState there? How does this work with CreateURLRequestforNavigation not getting the RequestNavigationParams. Also, does the main frame request go through WillSendRequest after this?
https://codereview.chromium.org/1310743003/diff/100001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/1310743003/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:298: data_reduction_proxy_request_options_->MaybeAddRequestHeader( On 2015/09/02 19:01:36, bengr wrote: > On 2015/08/31 23:43:24, David Benjamin wrote: > > On 2015/08/27 21:14:07, bengr wrote: > > > On 2015/08/27 18:53:50, David Benjamin wrote: > > > > When we were discussing design, it was mentioned that Blink's memory cache > > > must > > > > be accounted for when adding headers conditionally in the browser process. > > > What > > > > is the plan here? > > > > > > Initially, we are not going to do anything special with Blink's memory cache > > and > > > we will not initially be using Vary to cache LoFi and non-Lofi resources > > > separately. We will be using cache-control: private on LoFi resources so > > > intermediaries don't get confused. > > > > > > > I don't think that works. Cache-Control: private only means that shared caches > > can't cache it, but this barely matters since connections to the DRP are over > > HTTPS anyway. > > > > Both caches in Chromium are private caches. I would be very surprised if > Blink's > > cache discarded these. > > My comment about "cache-control: private" was meant to explain that intermediary > caches wouldn't get confused by responses lacking Vary, because DRP resources > aren't cached by them. They're not cached in intermediaries because most of our > traffic is over HTTPS and because we use "cache-control: private" in case the > request was proxied over HTTP. There's logic in the DRP to fall back to HTTP for > various reasons. > > Now blink's cache could have a LoFi resource beyond when LoFi should be used, > but we're OK with that for our use case. That sounds very bad. I do not think we should ship a feature in that state. That means that a page may not be entirely in or not in lofi mode. When processes don't swap, it means that lofi mode will bleed into pages even after the network estimate is fixed. This is the sort of problem that looks bad when it happens, but is flaky and unreliable enough that it's very hard to reproduce or diagnose. We should strive especially hard to avoid that kind of bug. The right way to do this is to make sure Blink's cache is somehow aware of this. https://codereview.chromium.org/1310743003/diff/100001/content/browser/loader... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/1310743003/diff/100001/content/browser/loader... content/browser/loader/resource_loader.cc:105: response->head.is_lofi = request->lofi_state() == net::LOFI_ON; On 2015/09/02 21:25:41, megjablon wrote: > On 2015/09/02 19:01:36, bengr wrote: > > On 2015/08/31 23:43:24, David Benjamin wrote: > > > On 2015/08/27 21:14:07, bengr wrote: > > > > On 2015/08/27 18:53:50, David Benjamin wrote: > > > > > Without the embedder colluding, there's nothing ensuring that > LOFI_DEFAULT > > > is > > > > > handled correctly. If we're teaching //content about lofi state, > //content > > > > > should be interpreting LOFI_DEFAULT. If it needs to call out to > something > > to > > > > > query the bandwidth estimator, then //content can call out to its > embedder > > > via > > > > > one of the top-level client/delegate interfaces to control what > > LOFI_DEFAULT > > > > > means, with the default implementation being off. > > > > > > > > > > (Was there an Intent to Implement for this header? We're adding a new > > header > > > > to > > > > > the web platform, so it needs to go through the Blink process.) > > > > > > > > The header isn't really a header right now; it's a directive within the > data > > > > reduction proxy's "Chrome-Proxy" header. I.e., "Chrome-Proxy: q=low". I > > don't > > > > think we want the web platform knowing about this. When we move to a > > separate > > > > header 'RQ: low' we can remove the collusion. Is this ok with you? > > > > > > Can we make it a separate header and do it properly? Otherwise this results > in > > > an extremely weird API at the //content boundary. The API contract between > > > //content and its embedder is now "I set this lofi bit on every request, but > > > honestly I haven't the slightest clue what it means. I'll just propagate it. > > > There's also this LOFI_DEFAULT setting, but I don't know what to do with it > > > either. If you intercept the URLRequest and mutate it early enough, then > I'll > > > read the value back and propagate as what you chose. Otherwise, uh, I'll > make > > > something up." > > > > I see. We might be open to making a server change to support 'RQ: low'. If we > > can't, then for now we could possibly let the browser add q=low to the > > Chrome-Proxy request header whenever it sees RQ: low. I'm a little hesitant, > > though, to commit to using RQ, because the RFC is very much in flux, but agree > > it is a better approach. > > > > Megan, could you make the change to do it properly as per David's definition > of > > "properly?" > > @David, I'm not sure I follow completely. I understand the issue with leaving > LOFI_DEFAULT uninterpreted, but you please explain how the separate header would > be properly used here? Where would we add 'RQ: low'? I'm trying to understand > the details of the design change you would like. I don't think //content should manage state that it's clueless about. If it's to collude with the consumer to interpret LOFI_DEFAULT, there should be a //content-level hook for it. I also think it makes more sense for //content to set the header, which is much more easily done as a separate header. Although I care much more about the uninterpreted LOFI_DEFAULT. The separate header is much more important for caching. If your response depends on a header, you MUST include it in Vary. I assume you don't want to stick Chrome-Proxy into Vary since that'll affect everything. My understanding from our discussions was that lofi mode was to be a straight-up web platform feature (Blink process and everything) and a header that we simply send and servers can implement if they wish. The DRP's role is instead just a polyfill for sites that don't understand it. This is much more forward-looking as Chrome is otherwise very consistently pushing HTTPS and treating plain HTTP as deprecated. https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:3353: if (request.cachePolicy() == WebURLRequest::ReloadBypassingCache) On 2015/09/02 21:25:42, megjablon wrote: > On 2015/08/31 23:43:24, David Benjamin wrote: > > Why isn't this coming from the navigation parameters? > > This is for the single image and page reloads which still utilize our original > "hackiness" which is intercepted here. I am working on follow up cls so that > this will come from the navigation parameters, but this cl is already very large > so those fixes will be in separate cls. I see. This should at least have a TODO comment with a link to a bug so that people reading the code will realize what's going on. https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.h:1032: // decide. On 2015/09/02 21:25:42, megjablon wrote: > On 2015/08/31 23:43:24, David Benjamin wrote: > > On 2015/08/27 23:11:02, megjablon wrote: > > > On 2015/08/27 21:14:07, bengr wrote: > > > > On 2015/08/27 18:53:50, David Benjamin wrote: > > > > > This is the copy of the field on the RenderFrameImpl, so "the resource" > > > > doesn't > > > > > make sense. Document exactly what this means. > > > > > > > > > > (I thought RenderFrameImpl would never see LOFI_DEFAULT. It should > always > > > know > > > > > which one it is. In my sketch it was just a boolean, but perhaps I > didn't > > > > > foresee some complication.) > > > > > > > > Agreed this can/should be a bit. > > > > > > I don't think this can be a bit. We need this for the ability to turn off > > Lo-Fi > > > on mainframe requests. RenderFrameHost sends a FrameMsg_Navigate message to > > > RenderFrameImpl. This message includes RequestNavigationParams and for > > > navigations where the user wants to explicitly disable LoFi, we add LOFI_OFF > > to > > > those params. We store this in lofi_state_ in > > RenderFrameImpl::NavigateInternal > > > and in RenderFrameImpl::willSendRequest we copy the lofi_state_ to > > > RequestExtraData. If we only have a bit then we don't know if the mainframe > > > request wants to explicitly turn on or off Lo-Fi or let the browser decide. > > > > That won't work anyway. Navigations are not synchronous. While the new > > navigation is in flight and hasn't committed, the document is still live and > may > > make new requests. > > > > The navigation may not even commit (perhaps a download, perhaps stopped), in > > which case, the document will be around for arbitrarily long afterwards. > > > > For navigations, the value needs to get attached to the request earlier. When > we > > were hashing this out, I specifically cited CreateURLRequestforNavigation. > > So you're saying we should use CreateURLRequestforNavigation instead of > NavigateInternal and immediately add the LoFiState there? How does this work > with CreateURLRequestforNavigation not getting the RequestNavigationParams. I'm not super familiar with the distinctions between all the sets of parameters anymore. You'll have to ask nasko@ about that. Presumably you'll need to add another bit somewhere to pass lofi state down in lieu of reload-ignoring-cache, so it can be on whichever set is appropriate. > Also, does the main frame request go through WillSendRequest after this? The main frame request still goes through willSendRequest, yes, before PlzNavigate. After PlzNavigate, the renderer process isn't involved at all and you'll need to route this through browser-side navigation bits. (Actually, that's another thing we'll need to fix here.) To be clear, you *cannot* update lofi_state_ until didCommitProvisionalLoad. Consider the following situation: 1. Global state is lofi. 2. Load a page. It is in lofi mode. 3. Global state is not lofi. 4. Tap a link on page. It loads as not lofi mode. 5. However, that load turns into a download and doesn't complete. lofi_state_ must remain unchanged in this scenario.
Poked mostly through content/. My main concern is that this CL is sprawling the LoFi bit all over the place. It can be made much more constrained, which will result in a smaller CL. Also, stating some basic explanation about the expected behavior will be good to have in the CL description for historical/discoverability reasons. For example, do iframes inherit the status? Do we preserve the status from navigation to navigation? Does it differ whether it is browser initiated vs renderer initiated. https://codereview.chromium.org/1310743003/diff/120001/components/data_reduct... File components/data_reduction_proxy/content/common/data_reduction_proxy_messages.h (right): https://codereview.chromium.org/1310743003/diff/120001/components/data_reduct... components/data_reduction_proxy/content/common/data_reduction_proxy_messages.h:13: DataReductionProxyViewHostMsg_IsDataReductionProxy, IsDataReductionProxy is a bit ambiguous. What are we asking here? I think the old name was just fine or alternatively have "IsDataReductionProxyOn" or something similar. https://codereview.chromium.org/1310743003/diff/120001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/1310743003/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:1767: LOFI_DEFAULT); If all browser initiated navigations start with status LOFI_DEFAULT, then why do we need to plumb this value all the way from here through Navigator? Why not use the constant there, which will mean less invasive change? https://codereview.chromium.org/1310743003/diff/120001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1310743003/diff/120001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1234: if (request_data.lofi_state <= 2 && request_data.lofi_state >= 0) { Please, let's use named constants, not verbatim numbers. https://codereview.chromium.org/1310743003/diff/120001/content/common/navigat... File content/common/navigation_params.h (right): https://codereview.chromium.org/1310743003/diff/120001/content/common/navigat... content/common/navigation_params.h:275: int lofi_state; This structure is used for navigations, not for resource requests. s/resource/document/? https://codereview.chromium.org/1310743003/diff/120001/content/public/rendere... File content/public/renderer/render_view_observer.h (right): https://codereview.chromium.org/1310743003/diff/120001/content/public/rendere... content/public/renderer/render_view_observer.h:56: virtual void FrameWillClose(blink::WebFrame* frame, bool lofi_used) {} Instead of adding these as parameters, can't the implementation of the method just query the frame and ask it about its lofi state? https://codereview.chromium.org/1310743003/diff/120001/content/public/rendere... content/public/renderer/render_view_observer.h:70: virtual void ClosePage(bool lofi_used) {} Same as above. https://codereview.chromium.org/1310743003/diff/120001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1310743003/diff/120001/content/renderer/rende... content/renderer/render_frame_impl.cc:2701: if (extra_data && extra_data->is_lofi()) nit: Maybe use tertiary operator, which will fit all on one line. https://codereview.chromium.org/1310743003/diff/120001/net/url_request/url_re... File net/url_request/url_request.h (right): https://codereview.chromium.org/1310743003/diff/120001/net/url_request/url_re... net/url_request/url_request.h:65: enum LoFiState { This has already been defined elsewhere. Why can't we have it in one place?
https://codereview.chromium.org/1310743003/diff/100001/content/common/navigat... File content/common/navigation_params.h (right): https://codereview.chromium.org/1310743003/diff/100001/content/common/navigat... content/common/navigation_params.h:275: int lofi_state; On 2015/08/31 23:43:24, David Benjamin wrote: > On 2015/08/27 23:11:02, megjablon wrote: > > On 2015/08/27 18:53:50, David Benjamin wrote: > > > Use your enum, not an int. > > > > Using the enum instead of an int breaks IPC compilation. > RequestNavigationParams > > is sent over IPC using an IPC struct and I don't know of a way for an enum to > be > > a member. > > nasko would be a better person to answer this than me. It is definitely possible > to send enums over IPC; content::ResourceType, for instance. It looks like you > need this. This also resolves not checking the macros. > > https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_message_ma... Done. https://codereview.chromium.org/1310743003/diff/100001/content/common/resourc... File content/common/resource_messages.h (right): https://codereview.chromium.org/1310743003/diff/100001/content/common/resourc... content/common/resource_messages.h:275: IPC_STRUCT_MEMBER(int, lofi_state) On 2015/08/27 23:11:02, megjablon wrote: > On 2015/08/27 18:53:50, David Benjamin wrote: > > This needs to be an enum, not an int. The IPC logic should also ensure thath > > invalid enum values are discarded. (nasko would know more about this.) > > Is there a way for an IPC struct to have enums? I couldn't figure out a way to > do this. Done. https://codereview.chromium.org/1310743003/diff/100001/content/public/rendere... File content/public/renderer/render_view_observer.h (right): https://codereview.chromium.org/1310743003/diff/100001/content/public/rendere... content/public/renderer/render_view_observer.h:56: virtual void FrameWillClose(blink::WebFrame* frame, bool lofi_used) {} On 2015/08/31 23:43:24, David Benjamin wrote: > On 2015/08/27 23:11:02, megjablon wrote: > > On 2015/08/27 18:53:50, David Benjamin wrote: > > > This is a rather specific parameter to plumb into so general of a hook. This > > > smells wrong. You should instead get the value out of the RenderFrame. > > > > Without PlageLoadHistograms being a subclass of RenderFrameObserver, we have > to > > pass this value in. > > I'll defer to nasko, but I would think it's possible to get the RenderFrameHost > out of the WebFrame. Alternatively, it PageLoadHistograms is fairly small. Can > we convert that to RenderFrameHostObserver? Done. https://codereview.chromium.org/1310743003/diff/100001/content/public/rendere... content/public/renderer/render_view_observer.h:70: virtual void ClosePage(bool lofi_used) {} On 2015/08/27 18:53:50, David Benjamin wrote: > Ditto. Done. https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:3353: if (request.cachePolicy() == WebURLRequest::ReloadBypassingCache) On 2015/09/04 14:41:22, David Benjamin wrote: > On 2015/09/02 21:25:42, megjablon wrote: > > On 2015/08/31 23:43:24, David Benjamin wrote: > > > Why isn't this coming from the navigation parameters? > > > > This is for the single image and page reloads which still utilize our original > > "hackiness" which is intercepted here. I am working on follow up cls so that > > this will come from the navigation parameters, but this cl is already very > large > > so those fixes will be in separate cls. > > I see. This should at least have a TODO comment with a link to a bug so that > people reading the code will realize what's going on. Done. https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1310743003/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.h:1032: // decide. On 2015/09/04 14:41:22, David Benjamin wrote: > On 2015/09/02 21:25:42, megjablon wrote: > > On 2015/08/31 23:43:24, David Benjamin wrote: > > > On 2015/08/27 23:11:02, megjablon wrote: > > > > On 2015/08/27 21:14:07, bengr wrote: > > > > > On 2015/08/27 18:53:50, David Benjamin wrote: > > > > > > This is the copy of the field on the RenderFrameImpl, so "the > resource" > > > > > doesn't > > > > > > make sense. Document exactly what this means. > > > > > > > > > > > > (I thought RenderFrameImpl would never see LOFI_DEFAULT. It should > > always > > > > know > > > > > > which one it is. In my sketch it was just a boolean, but perhaps I > > didn't > > > > > > foresee some complication.) > > > > > > > > > > Agreed this can/should be a bit. > > > > > > > > I don't think this can be a bit. We need this for the ability to turn off > > > Lo-Fi > > > > on mainframe requests. RenderFrameHost sends a FrameMsg_Navigate message > to > > > > RenderFrameImpl. This message includes RequestNavigationParams and for > > > > navigations where the user wants to explicitly disable LoFi, we add > LOFI_OFF > > > to > > > > those params. We store this in lofi_state_ in > > > RenderFrameImpl::NavigateInternal > > > > and in RenderFrameImpl::willSendRequest we copy the lofi_state_ to > > > > RequestExtraData. If we only have a bit then we don't know if the > mainframe > > > > request wants to explicitly turn on or off Lo-Fi or let the browser > decide. > > > > > > That won't work anyway. Navigations are not synchronous. While the new > > > navigation is in flight and hasn't committed, the document is still live and > > may > > > make new requests. > > > > > > The navigation may not even commit (perhaps a download, perhaps stopped), in > > > which case, the document will be around for arbitrarily long afterwards. > > > > > > For navigations, the value needs to get attached to the request earlier. > When > > we > > > were hashing this out, I specifically cited CreateURLRequestforNavigation. > > > > So you're saying we should use CreateURLRequestforNavigation instead of > > NavigateInternal and immediately add the LoFiState there? How does this work > > with CreateURLRequestforNavigation not getting the RequestNavigationParams. > > I'm not super familiar with the distinctions between all the sets of parameters > anymore. You'll have to ask nasko@ about that. Presumably you'll need to add > another bit somewhere to pass lofi state down in lieu of reload-ignoring-cache, > so it can be on whichever set is appropriate. > > > Also, does the main frame request go through WillSendRequest after this? > > The main frame request still goes through willSendRequest, yes, before > PlzNavigate. After PlzNavigate, the renderer process isn't involved at all and > you'll need to route this through browser-side navigation bits. (Actually, > that's another thing we'll need to fix here.) > > To be clear, you *cannot* update lofi_state_ until didCommitProvisionalLoad. > Consider the following situation: > > 1. Global state is lofi. > 2. Load a page. It is in lofi mode. > 3. Global state is not lofi. > 4. Tap a link on page. It loads as not lofi mode. > 5. However, that load turns into a download and doesn't complete. > > lofi_state_ must remain unchanged in this scenario. @nasko can you take a look? https://codereview.chromium.org/1310743003/diff/120001/components/data_reduct... File components/data_reduction_proxy/content/common/data_reduction_proxy_messages.h (right): https://codereview.chromium.org/1310743003/diff/120001/components/data_reduct... components/data_reduction_proxy/content/common/data_reduction_proxy_messages.h:13: DataReductionProxyViewHostMsg_IsDataReductionProxy, On 2015/09/04 22:47:42, nasko (out until Sept 11) wrote: > IsDataReductionProxy is a bit ambiguous. What are we asking here? I think the > old name was just fine or alternatively have "IsDataReductionProxyOn" or > something similar. This doesn't tell us if the Data Reduction Proxy is on, but if the given proxy server is a Data Reduction Proxy. https://codereview.chromium.org/1310743003/diff/120001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/1310743003/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:1767: LOFI_DEFAULT); On 2015/09/04 22:47:42, nasko (out until Sept 11) wrote: > If all browser initiated navigations start with status LOFI_DEFAULT, then why do > we need to plumb this value all the way from here through Navigator? Why not use > the constant there, which will mean less invasive change? Done. https://codereview.chromium.org/1310743003/diff/120001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1310743003/diff/120001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1234: if (request_data.lofi_state <= 2 && request_data.lofi_state >= 0) { On 2015/09/04 22:47:42, nasko (out until Sept 11) wrote: > Please, let's use named constants, not verbatim numbers. Done. https://codereview.chromium.org/1310743003/diff/120001/content/common/navigat... File content/common/navigation_params.h (right): https://codereview.chromium.org/1310743003/diff/120001/content/common/navigat... content/common/navigation_params.h:275: int lofi_state; On 2015/09/04 22:47:42, nasko (out until Sept 11) wrote: > This structure is used for navigations, not for resource requests. > s/resource/document/? Done. https://codereview.chromium.org/1310743003/diff/120001/content/public/rendere... File content/public/renderer/render_view_observer.h (right): https://codereview.chromium.org/1310743003/diff/120001/content/public/rendere... content/public/renderer/render_view_observer.h:56: virtual void FrameWillClose(blink::WebFrame* frame, bool lofi_used) {} On 2015/09/04 22:47:42, nasko (out until Sept 11) wrote: > Instead of adding these as parameters, can't the implementation of the method > just query the frame and ask it about its lofi state? Done. https://codereview.chromium.org/1310743003/diff/120001/content/public/rendere... content/public/renderer/render_view_observer.h:70: virtual void ClosePage(bool lofi_used) {} On 2015/09/04 22:47:42, nasko (out until Sept 11) wrote: > Same as above. Done. https://codereview.chromium.org/1310743003/diff/120001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1310743003/diff/120001/content/renderer/rende... content/renderer/render_frame_impl.cc:2701: if (extra_data && extra_data->is_lofi()) On 2015/09/04 22:47:42, nasko (out until Sept 11) wrote: > nit: Maybe use tertiary operator, which will fit all on one line. Done. https://codereview.chromium.org/1310743003/diff/120001/net/url_request/url_re... File net/url_request/url_request.h (right): https://codereview.chromium.org/1310743003/diff/120001/net/url_request/url_re... net/url_request/url_request.h:65: enum LoFiState { On 2015/09/04 22:47:42, nasko (out until Sept 11) wrote: > This has already been defined elsewhere. Why can't we have it in one place? This is currently defined in content/common. Should I define it somewhere else? I saw this used elsewhere in the code: https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/re...
Megan, I've lost track of this CL. Where are we withe the major concerns of davidben and nasko, namely, that net/ really shouldn't need to know about the header, and that the CL is sprawling all over the place?
On 2015/09/11 12:33:56, bengr wrote: > Megan, I've lost track of this CL. Where are we withe the major concerns of > davidben and nasko, namely, that net/ really shouldn't need to know about the > header, and that the CL is sprawling all over the place? Yes those are the concerns, along with caching. I think we should move discussion of those issues back to the design doc since it is difficult to track here.
https://codereview.chromium.org/1310743003/diff/100001/net/url_request/url_re... File net/url_request/url_request.h (right): https://codereview.chromium.org/1310743003/diff/100001/net/url_request/url_re... net/url_request/url_request.h:850: LoFiState lofi_state_; On 2015/09/02 19:01:36, bengr wrote: > On 2015/08/31 23:43:24, David Benjamin wrote: > > On 2015/08/27 23:11:02, megjablon wrote: > > > On 2015/08/27 18:53:50, David Benjamin wrote: > > > > Why is this on the URLRequest? It's not used by //net at all. //net is not > > > just > > > > an HTTP stack for //content. Consumer-specific state needs to be in the > > > > consumer. //net can't know about it while //content needs to, so this > should > > > be > > > > in //content somewhere. > > > > > > We add q=low to the Chrome-Proxy header in > > > DataReductionProxyNetworkDelegate::OnBeforeSendProxyHeadersInternal which > gets > > a > > > URLRequest. > > > > There's other ways to staple consumer-specific state, such as UserData. This > CL > > should not end up touching anything in //net. > > I like this goal. Aren't there layering issues with creating and setting LoFi UserData in //content and then getting it in //components? I don't know that this is feasible.
https://codereview.chromium.org/1310743003/diff/100001/net/url_request/url_re... File net/url_request/url_request.h (right): https://codereview.chromium.org/1310743003/diff/100001/net/url_request/url_re... net/url_request/url_request.h:850: LoFiState lofi_state_; On 2015/09/11 20:58:03, megjablon wrote: > On 2015/09/02 19:01:36, bengr wrote: > > On 2015/08/31 23:43:24, David Benjamin wrote: > > > On 2015/08/27 23:11:02, megjablon wrote: > > > > On 2015/08/27 18:53:50, David Benjamin wrote: > > > > > Why is this on the URLRequest? It's not used by //net at all. //net is > not > > > > just > > > > > an HTTP stack for //content. Consumer-specific state needs to be in the > > > > > consumer. //net can't know about it while //content needs to, so this > > should > > > > be > > > > > in //content somewhere. > > > > > > > > We add q=low to the Chrome-Proxy header in > > > > DataReductionProxyNetworkDelegate::OnBeforeSendProxyHeadersInternal which > > gets > > > a > > > > URLRequest. > > > > > > There's other ways to staple consumer-specific state, such as UserData. This > > CL > > > should not end up touching anything in //net. > > > > I like this goal. > > Aren't there layering issues with creating and setting LoFi UserData in > //content and then getting it in //components? I don't know that this is > feasible. Portions of //components can depend on //content, right? Just not the core parts. So it should be possible to plumb that up. Although, since //content has to track this bit and know about it, it seems //content should have some interface out to its embedder for deciding when to use lofi mode. Maybe a ShouldEnableLofiMode() hook on the ResourceDispatcherHostDelegate or something which gets the ResourceContext or similar. Though that the interpretation of this bit is, to begin with, something that //content can't do is also pretty wonky since you put it in the DRP's header. I think the real problem is you're trying to coordinate too many related jobs across too many layers, which is why the interfaces across layers get very complex. (This is why I'd been advocating having a separate header. Then //content can know what that header means, insert it, and //chrome's and //components' role is just in implementing the ShouldEnableLofiMode() hook.)
https://codereview.chromium.org/1310743003/diff/100001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/1310743003/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:298: data_reduction_proxy_request_options_->MaybeAddRequestHeader( On 2015/09/04 14:41:22, David Benjamin wrote: > On 2015/09/02 19:01:36, bengr wrote: > > On 2015/08/31 23:43:24, David Benjamin wrote: > > > On 2015/08/27 21:14:07, bengr wrote: > > > > On 2015/08/27 18:53:50, David Benjamin wrote: > > > > > When we were discussing design, it was mentioned that Blink's memory > cache > > > > must > > > > > be accounted for when adding headers conditionally in the browser > process. > > > > What > > > > > is the plan here? > > > > > > > > Initially, we are not going to do anything special with Blink's memory > cache > > > and > > > > we will not initially be using Vary to cache LoFi and non-Lofi resources > > > > separately. We will be using cache-control: private on LoFi resources so > > > > intermediaries don't get confused. > > > > > > > > > > I don't think that works. Cache-Control: private only means that shared > caches > > > can't cache it, but this barely matters since connections to the DRP are > over > > > HTTPS anyway. > > > > > > Both caches in Chromium are private caches. I would be very surprised if > > Blink's > > > cache discarded these. > > > > My comment about "cache-control: private" was meant to explain that > intermediary > > caches wouldn't get confused by responses lacking Vary, because DRP resources > > aren't cached by them. They're not cached in intermediaries because most of > our > > traffic is over HTTPS and because we use "cache-control: private" in case the > > request was proxied over HTTP. There's logic in the DRP to fall back to HTTP > for > > various reasons. > > > > Now blink's cache could have a LoFi resource beyond when LoFi should be used, > > but we're OK with that for our use case. > > That sounds very bad. I do not think we should ship a feature in that state. > That means that a page may not be entirely in or not in lofi mode. When > processes don't swap, it means that lofi mode will bleed into pages even after > the network estimate is fixed. This is the sort of problem that looks bad when > it happens, but is flaky and unreliable enough that it's very hard to reproduce > or diagnose. We should strive especially hard to avoid that kind of bug. > > The right way to do this is to make sure Blink's cache is somehow aware of this. Since the user has turned on Data Saver, we want to preserve those data savings and this is how the feature will work. https://codereview.chromium.org/1310743003/diff/100001/content/browser/loader... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/1310743003/diff/100001/content/browser/loader... content/browser/loader/resource_loader.cc:105: response->head.is_lofi = request->lofi_state() == net::LOFI_ON; On 2015/09/04 14:41:22, David Benjamin wrote: > On 2015/09/02 21:25:41, megjablon wrote: > > On 2015/09/02 19:01:36, bengr wrote: > > > On 2015/08/31 23:43:24, David Benjamin wrote: > > > > On 2015/08/27 21:14:07, bengr wrote: > > > > > On 2015/08/27 18:53:50, David Benjamin wrote: > > > > > > Without the embedder colluding, there's nothing ensuring that > > LOFI_DEFAULT > > > > is > > > > > > handled correctly. If we're teaching //content about lofi state, > > //content > > > > > > should be interpreting LOFI_DEFAULT. If it needs to call out to > > something > > > to > > > > > > query the bandwidth estimator, then //content can call out to its > > embedder > > > > via > > > > > > one of the top-level client/delegate interfaces to control what > > > LOFI_DEFAULT > > > > > > means, with the default implementation being off. > > > > > > > > > > > > (Was there an Intent to Implement for this header? We're adding a new > > > header > > > > > to > > > > > > the web platform, so it needs to go through the Blink process.) > > > > > > > > > > The header isn't really a header right now; it's a directive within the > > data > > > > > reduction proxy's "Chrome-Proxy" header. I.e., "Chrome-Proxy: q=low". I > > > don't > > > > > think we want the web platform knowing about this. When we move to a > > > separate > > > > > header 'RQ: low' we can remove the collusion. Is this ok with you? > > > > > > > > Can we make it a separate header and do it properly? Otherwise this > results > > in > > > > an extremely weird API at the //content boundary. The API contract between > > > > //content and its embedder is now "I set this lofi bit on every request, > but > > > > honestly I haven't the slightest clue what it means. I'll just propagate > it. > > > > There's also this LOFI_DEFAULT setting, but I don't know what to do with > it > > > > either. If you intercept the URLRequest and mutate it early enough, then > > I'll > > > > read the value back and propagate as what you chose. Otherwise, uh, I'll > > make > > > > something up." > > > > > > I see. We might be open to making a server change to support 'RQ: low'. If > we > > > can't, then for now we could possibly let the browser add q=low to the > > > Chrome-Proxy request header whenever it sees RQ: low. I'm a little hesitant, > > > though, to commit to using RQ, because the RFC is very much in flux, but > agree > > > it is a better approach. > > > > > > Megan, could you make the change to do it properly as per David's definition > > of > > > "properly?" > > > > @David, I'm not sure I follow completely. I understand the issue with leaving > > LOFI_DEFAULT uninterpreted, but you please explain how the separate header > would > > be properly used here? Where would we add 'RQ: low'? I'm trying to understand > > the details of the design change you would like. > > I don't think //content should manage state that it's clueless about. If it's to > collude with the consumer to interpret LOFI_DEFAULT, there should be a > //content-level hook for it. > > I also think it makes more sense for //content to set the header, which is much > more easily done as a separate header. Although I care much more about the > uninterpreted LOFI_DEFAULT. > > The separate header is much more important for caching. If your response depends > on a header, you MUST include it in Vary. I assume you don't want to stick > Chrome-Proxy into Vary since that'll affect everything. > > My understanding from our discussions was that lofi mode was to be a straight-up > web platform feature (Blink process and everything) and a header that we simply > send and servers can implement if they wish. The DRP's role is instead just a > polyfill for sites that don't understand it. > > This is much more forward-looking as Chrome is otherwise very consistently > pushing HTTPS and treating plain HTTP as deprecated. How will this work for the navigation? Since we make the decision to use LoFi on a page on the network side, we have no way of knowing prior to that in //content if we want to add the header or not.
Patchset #9 (id:180001) has been deleted
I think content looks good. Few minor comments. https://codereview.chromium.org/1310743003/diff/200001/content/public/rendere... File content/public/renderer/render_frame.h (right): https://codereview.chromium.org/1310743003/diff/200001/content/public/rendere... content/public/renderer/render_frame.h:155: virtual bool LoFiOn() = 0; IsLoFiOn https://codereview.chromium.org/1310743003/diff/200001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1310743003/diff/200001/content/renderer/rende... content/renderer/render_frame_impl.cc:4500: lofi_state_ = (LoFiState) request_params.lofi_state; Why do we need a cast here? https://codereview.chromium.org/1310743003/diff/200001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/1310743003/diff/200001/content/renderer/rende... content/renderer/render_view_impl.cc:718: main_render_frame_->Initialize(LOFI_DEFAULT); Inline this as RF default value.
https://codereview.chromium.org/1310743003/diff/200001/content/public/rendere... File content/public/renderer/render_frame.h (right): https://codereview.chromium.org/1310743003/diff/200001/content/public/rendere... content/public/renderer/render_frame.h:155: virtual bool LoFiOn() = 0; On 2015/09/17 23:09:00, nasko (slow to review) wrote: > IsLoFiOn Done. https://codereview.chromium.org/1310743003/diff/200001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1310743003/diff/200001/content/renderer/rende... content/renderer/render_frame_impl.cc:4500: lofi_state_ = (LoFiState) request_params.lofi_state; On 2015/09/17 23:09:00, nasko (slow to review) wrote: > Why do we need a cast here? Done. https://codereview.chromium.org/1310743003/diff/200001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/1310743003/diff/200001/content/renderer/rende... content/renderer/render_view_impl.cc:718: main_render_frame_->Initialize(LOFI_DEFAULT); On 2015/09/17 23:09:00, nasko (slow to review) wrote: > Inline this as RF default value. Done.
Just glanced at the CL. I'll take a full pass through it once nasko is happy. https://codereview.chromium.org/1310743003/diff/220001/components/data_reduct... File components/data_reduction_proxy/content/common/content_data_reduction_proxy_lofi_helper.cc (right): https://codereview.chromium.org/1310743003/diff/220001/components/data_reduct... components/data_reduction_proxy/content/common/content_data_reduction_proxy_lofi_helper.cc:28: void ContentDataReductionProxyLoFiHelper::set_lofi_state( These seem to be more complicated than typical getters and setters. Suggest changing the name to GetLoFiState and SetLoFiState, or RetrieveLoFiStateFromUserData and AddLoFiStateToUserData. https://codereview.chromium.org/1310743003/diff/220001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/1310743003/diff/220001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:779: if (net::NetworkChangeNotifier::IsConnectionCellular( How about return net::NetworkChangeNotifier::IsConnectionCellular( net::NetworkChangeNotifier::GetConnectionType()); ? https://codereview.chromium.org/1310743003/diff/220001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:789: if (IsNetworkQualityProhibitivelySlow(network_quality_estimator)) Same here: return IsNetworkQuality...
One quick comment to make sure this isn't forgotten (I'm going to be slow for the next two days since I'm on bug triage duty). https://codereview.chromium.org/1310743003/diff/220001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1310743003/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:4501: lofi_state_ = request_params.lofi_state; This still hasn't been resolved. lofi_state_ must only be updated on commit, not when you start a navigation. See previous discussions on whether this field was a LofiState or a bool.
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
Drive-by comments. No need to wait for my signoff before landing or anything like that. https://codereview.chromium.org/1310743003/diff/220001/content/public/common/... File content/public/common/data_reduction_proxy_lofi_user_data.h (right): https://codereview.chromium.org/1310743003/diff/220001/content/public/common/... content/public/common/data_reduction_proxy_lofi_user_data.h:20: LOFI_DEFAULT, This seems weird - seems to me like this should not be resolved to on/off on a per request basis, but on a per-navigation basis. Once a request is made, it either is using lo-fi, or it isn't. https://codereview.chromium.org/1310743003/diff/220001/content/public/common/... content/public/common/data_reduction_proxy_lofi_user_data.h:30: void set_lofi_state(LoFiState lofi_state) { lofi_state_ = lofi_state; } Should this just be a value in ResourceRequestInfoImpl instead? Particularly if the goal is to make this a web platform feature, rather than purely a DRP feature.
mmenke@chromium.org changed reviewers: - mmenke@chromium.org
https://codereview.chromium.org/1310743003/diff/220001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1310743003/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:4501: lofi_state_ = request_params.lofi_state; On 2015/09/21 17:58:15, David Benjamin wrote: > This still hasn't been resolved. lofi_state_ must only be updated on commit, not > when you start a navigation. See previous discussions on whether this field was > a LofiState or a bool. To clarify, this isn't a "layering; we'll fix this later" problem. It's a correctness problem. Until the navigation has committed, it's not correct to change any state relating to the current navigation. The state needs to flow at commit from the ResourceResponseInfo sent in ResourceMsg_ReceivedResponse to didCommitProvisionalLoad and be set at that point and only at that point. That bit then gets stapled to subresources at willSendRequest. Frame requests will hit a slightly different codepath. The key thing to always keep in mind is that navigations (frame requests) are completely different from subresources. It is expected that you will need to treat them different for this sort of thing. (Treating them the same is usually a red flag that something is wrong.) (In fact, I'm not sure this CL works at all for renderer-initiated navigations. Not every navigation hits this function.)
https://codereview.chromium.org/1310743003/diff/220001/components/data_reduct... File components/data_reduction_proxy/content/common/content_data_reduction_proxy_lofi_helper.cc (right): https://codereview.chromium.org/1310743003/diff/220001/components/data_reduct... components/data_reduction_proxy/content/common/content_data_reduction_proxy_lofi_helper.cc:28: void ContentDataReductionProxyLoFiHelper::set_lofi_state( On 2015/09/21 17:56:08, bengr wrote: > These seem to be more complicated than typical getters and setters. Suggest > changing the name to GetLoFiState and SetLoFiState, or > RetrieveLoFiStateFromUserData and AddLoFiStateToUserData. Done. https://codereview.chromium.org/1310743003/diff/220001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/1310743003/diff/220001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:779: if (net::NetworkChangeNotifier::IsConnectionCellular( On 2015/09/21 17:56:08, bengr wrote: > How about > return net::NetworkChangeNotifier::IsConnectionCellular( > net::NetworkChangeNotifier::GetConnectionType()); > ? Done. https://codereview.chromium.org/1310743003/diff/220001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:789: if (IsNetworkQualityProhibitivelySlow(network_quality_estimator)) On 2015/09/21 17:56:08, bengr wrote: > Same here: > return IsNetworkQuality... Done. https://codereview.chromium.org/1310743003/diff/220001/content/public/common/... File content/public/common/data_reduction_proxy_lofi_user_data.h (right): https://codereview.chromium.org/1310743003/diff/220001/content/public/common/... content/public/common/data_reduction_proxy_lofi_user_data.h:20: LOFI_DEFAULT, On 2015/09/21 18:03:48, mmenke wrote: > This seems weird - seems to me like this should not be resolved to on/off on a > per request basis, but on a per-navigation basis. Once a request is made, it > either is using lo-fi, or it isn't. LOFI_DEFAULT is only used by navigation requests to tell the Data Reduction Proxy component to use the NQE to determine if Lo-Fi should be used. After that the subresources will either get LOFI_ON or LOFI_OFF. https://codereview.chromium.org/1310743003/diff/220001/content/public/common/... content/public/common/data_reduction_proxy_lofi_user_data.h:30: void set_lofi_state(LoFiState lofi_state) { lofi_state_ = lofi_state; } On 2015/09/21 18:03:48, mmenke wrote: > Should this just be a value in ResourceRequestInfoImpl instead? Particularly if > the goal is to make this a web platform feature, rather than purely a DRP > feature. This sounds like a good and simpler idea to me. davidben@ and nasko@ any objections? https://codereview.chromium.org/1310743003/diff/220001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1310743003/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:4501: lofi_state_ = request_params.lofi_state; On 2015/09/21 18:09:01, David Benjamin wrote: > On 2015/09/21 17:58:15, David Benjamin wrote: > > This still hasn't been resolved. lofi_state_ must only be updated on commit, > not > > when you start a navigation. See previous discussions on whether this field > was > > a LofiState or a bool. > > To clarify, this isn't a "layering; we'll fix this later" problem. It's a > correctness problem. Until the navigation has committed, it's not correct to > change any state relating to the current navigation. The state needs to flow at > commit from the ResourceResponseInfo sent in ResourceMsg_ReceivedResponse to > didCommitProvisionalLoad and be set at that point and only at that point. That > bit then gets stapled to subresources at willSendRequest. Frame requests will > hit a slightly different codepath. > > The key thing to always keep in mind is that navigations (frame requests) are > completely different from subresources. It is expected that you will need to > treat them different for this sort of thing. (Treating them the same is usually > a red flag that something is wrong.) > > (In fact, I'm not sure this CL works at all for renderer-initiated navigations. > Not every navigation hits this function.) For renderer initiated navigations, we don't need to set Lo-Fi state because it will always be LOFI_DEFAULT, and we have the NQE decide. This is used by browser initiated navigations to force Lo-Fi on or off and will be used by the "Load images" snackbar (to be hooked up in a follow-up cl). Do you have any suggestions for a path to force the navigation to take the value of RequestNavigationParams that doesn't break this correctness? Maybe the problem is we're combining the navigation signal to force Lo-Fi on or off with the response value at commit that sets the state. These values would be the same for the navigations with forced Lo-Fi state, and that's why it was being set early here.
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
https://codereview.chromium.org/1310743003/diff/220001/content/public/common/... File content/public/common/data_reduction_proxy_lofi_user_data.h (right): https://codereview.chromium.org/1310743003/diff/220001/content/public/common/... content/public/common/data_reduction_proxy_lofi_user_data.h:20: LOFI_DEFAULT, On 2015/09/22 20:35:52, megjablon wrote: > On 2015/09/21 18:03:48, mmenke wrote: > > This seems weird - seems to me like this should not be resolved to on/off on a > > per request basis, but on a per-navigation basis. Once a request is made, it > > either is using lo-fi, or it isn't. > > LOFI_DEFAULT is only used by navigation requests to tell the Data Reduction > Proxy component to use the NQE to determine if Lo-Fi should be used. After that > the subresources will either get LOFI_ON or LOFI_OFF. That sounds reasonable - in that case, I think we should indeed just be storing a bool with each request, rather than a LoFiState, to make it clearer it just has one of two values.
https://codereview.chromium.org/1310743003/diff/220001/content/public/common/... File content/public/common/data_reduction_proxy_lofi_user_data.h (right): https://codereview.chromium.org/1310743003/diff/220001/content/public/common/... content/public/common/data_reduction_proxy_lofi_user_data.h:20: LOFI_DEFAULT, On 2015/09/22 20:38:12, mmenke wrote: > On 2015/09/22 20:35:52, megjablon wrote: > > On 2015/09/21 18:03:48, mmenke wrote: > > > This seems weird - seems to me like this should not be resolved to on/off on > a > > > per request basis, but on a per-navigation basis. Once a request is made, > it > > > either is using lo-fi, or it isn't. > > > > LOFI_DEFAULT is only used by navigation requests to tell the Data Reduction > > Proxy component to use the NQE to determine if Lo-Fi should be used. After > that > > the subresources will either get LOFI_ON or LOFI_OFF. > > That sounds reasonable - in that case, I think we should indeed just be storing > a bool with each request, rather than a LoFiState, to make it clearer it just > has one of two values. Maybe I didn't explain this right. We need to be able to tell the drp component to query the NQE sometimes, but also have the ability to force LOFI_ON or LOFI_OFF othertimes for navigation requests.
https://codereview.chromium.org/1310743003/diff/220001/content/public/common/... File content/public/common/data_reduction_proxy_lofi_user_data.h (right): https://codereview.chromium.org/1310743003/diff/220001/content/public/common/... content/public/common/data_reduction_proxy_lofi_user_data.h:20: LOFI_DEFAULT, On 2015/09/22 20:53:40, megjablon wrote: > On 2015/09/22 20:38:12, mmenke wrote: > > On 2015/09/22 20:35:52, megjablon wrote: > > > On 2015/09/21 18:03:48, mmenke wrote: > > > > This seems weird - seems to me like this should not be resolved to on/off > on > > a > > > > per request basis, but on a per-navigation basis. Once a request is made, > > it > > > > either is using lo-fi, or it isn't. > > > > > > LOFI_DEFAULT is only used by navigation requests to tell the Data Reduction > > > Proxy component to use the NQE to determine if Lo-Fi should be used. After > > that > > > the subresources will either get LOFI_ON or LOFI_OFF. > > > > That sounds reasonable - in that case, I think we should indeed just be > storing > > a bool with each request, rather than a LoFiState, to make it clearer it just > > has one of two values. > > Maybe I didn't explain this right. We need to be able to tell the drp component > to query the NQE sometimes, but also have the ability to force LOFI_ON or > LOFI_OFF othertimes for navigation requests. Right, but DataReductionProxyLoFiUserData is associated with URLRequests, not navigations, so while we want a LoFiState to be associated with a navigation, we just need a bool associated with a URLRequest...Or am I missing something?
https://codereview.chromium.org/1310743003/diff/220001/content/public/common/... File content/public/common/data_reduction_proxy_lofi_user_data.h (right): https://codereview.chromium.org/1310743003/diff/220001/content/public/common/... content/public/common/data_reduction_proxy_lofi_user_data.h:20: LOFI_DEFAULT, On 2015/09/22 20:57:07, mmenke wrote: > On 2015/09/22 20:53:40, megjablon wrote: > > On 2015/09/22 20:38:12, mmenke wrote: > > > On 2015/09/22 20:35:52, megjablon wrote: > > > > On 2015/09/21 18:03:48, mmenke wrote: > > > > > This seems weird - seems to me like this should not be resolved to > on/off > > on > > > a > > > > > per request basis, but on a per-navigation basis. Once a request is > made, > > > it > > > > > either is using lo-fi, or it isn't. > > > > > > > > LOFI_DEFAULT is only used by navigation requests to tell the Data > Reduction > > > > Proxy component to use the NQE to determine if Lo-Fi should be used. After > > > that > > > > the subresources will either get LOFI_ON or LOFI_OFF. > > > > > > That sounds reasonable - in that case, I think we should indeed just be > > storing > > > a bool with each request, rather than a LoFiState, to make it clearer it > just > > > has one of two values. > > > > Maybe I didn't explain this right. We need to be able to tell the drp > component > > to query the NQE sometimes, but also have the ability to force LOFI_ON or > > LOFI_OFF othertimes for navigation requests. > > Right, but DataReductionProxyLoFiUserData is associated with URLRequests, not > navigations, so while we want a LoFiState to be associated with a navigation, we > just need a bool associated with a URLRequest...Or am I missing something? The navigation creates a URLRequest and when that request hits DRPNetworkDelegate::OnBeforeSendProxyHeadersInternal the NQE may make the decision to use Lo-Fi or not on the URLRequest for the navigation. We need the three states in OnBeforeSendProxyHeadersInternal because LOFI_DEFAULT tells us to check the NQE for the request whereas LOFI_ON or LOFI_OFF just adds or doesn't add the header respectively.
I'm on bug triage duty today and yesterday, so I haven't had a chance to do a thorough review, but mmenke asked me to comment on the ResourceRequestInfoImpl thing, so here's some comments: https://codereview.chromium.org/1310743003/diff/220001/content/public/common/... File content/public/common/data_reduction_proxy_lofi_user_data.h (right): https://codereview.chromium.org/1310743003/diff/220001/content/public/common/... content/public/common/data_reduction_proxy_lofi_user_data.h:30: void set_lofi_state(LoFiState lofi_state) { lofi_state_ = lofi_state; } On 2015/09/22 20:35:52, megjablon wrote: > On 2015/09/21 18:03:48, mmenke wrote: > > Should this just be a value in ResourceRequestInfoImpl instead? Particularly > if > > the goal is to make this a web platform feature, rather than purely a DRP > > feature. > > This sounds like a good and simpler idea to me. davidben@ and nasko@ any > objections? Yes, please do that. But exposing a mutator here still makes the content/DRP boundary a little awkward, ResourceRequestInfo or DRPLoFiUserData. I would suggest going further and doing the following: ResourceRequestInfo does not have set_lofi_state or even expose the LoFiState enum. Instead, you have: bool ResourceRequestInfo::is_lofi() const Leave interpreting LOFI_DEFAULT and converting to bool to //content. //content doesn't have the bandwidth estimator, so we add a method to ResourceDispatcherHostDelegate or elsewhere like bool ShouldEnableLofiMode(/* stuff */). This function tells you whether, at this point in time, LOFI_DEFAULT means on or off. //content calls into that very early to get rid of LOFI_DEFAULT and turn it into a boolean. From there, the LofiState enum is gone. As previously discussed, I'm not a huge fan of //content having this bit and not knowing what it means, so I think it SHOULD be a header that //content also just sets. But I'm okay with holding that for now since it's not yet standardized. In the meantime, the semantics are: - //content has this notion of whether requests and documents and navigations are "lofi". It exposes this bit on ResourceRequestInfo. - //content lets you read this bit, but not set it. Maintaining this bit is //content's job. - As part of maintaining that bit, //content asks its embedder whether, at any point in time, bandwidth and such are low enough that "lofi" is on. This is used to bootstrap the bit on top-level navigations, implemented with LOFI_DEFAULT. //chrome will implement that hook by querying the DRP logic. - //content doesn't do anything with this bit it maintains, but, it does let you mutate the URLRequest before it goes out because it's always let you do that as part of intercepting the net stack. When intercepting the net stack, you're free to interpret that bit how you like and, notably, modify the DRP header. In the future, that last step will be replaced by //content just setting the Enable-Fancy-Standardized-Lofi-Mode header and then the DRP won't really be involved in this but for a server-side polyfill. And maybe to implement the ShouldEnableLofiMode hook, I dunno. How does that sound? https://codereview.chromium.org/1310743003/diff/220001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1310743003/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:4501: lofi_state_ = request_params.lofi_state; On 2015/09/22 20:35:52, megjablon wrote: > On 2015/09/21 18:09:01, David Benjamin wrote: > > On 2015/09/21 17:58:15, David Benjamin wrote: > > > This still hasn't been resolved. lofi_state_ must only be updated on commit, > > not > > > when you start a navigation. See previous discussions on whether this field > > was > > > a LofiState or a bool. > > > > To clarify, this isn't a "layering; we'll fix this later" problem. It's a > > correctness problem. Until the navigation has committed, it's not correct to > > change any state relating to the current navigation. The state needs to flow > at > > commit from the ResourceResponseInfo sent in ResourceMsg_ReceivedResponse to > > didCommitProvisionalLoad and be set at that point and only at that point. That > > bit then gets stapled to subresources at willSendRequest. Frame requests will > > hit a slightly different codepath. > > > > The key thing to always keep in mind is that navigations (frame requests) are > > completely different from subresources. It is expected that you will need to > > treat them different for this sort of thing. (Treating them the same is > usually > > a red flag that something is wrong.) > > > > (In fact, I'm not sure this CL works at all for renderer-initiated > navigations. > > Not every navigation hits this function.) > > For renderer initiated navigations, we don't need to set Lo-Fi state because it > will always be LOFI_DEFAULT, and we have the NQE decide. I think we were talking about different things. Though I see that didCommitProvisionalLoad *also* sets lofi_state_. Sorry, I missed that somehow. The part I was thinking of works fine for renderer-initiated navigations. All you actually want to do is ditch this line, switch it to a boolean, and then plumb the value for navigations differently. > This is used by browser > initiated navigations to force Lo-Fi on or off and will be used by the "Load > images" snackbar (to be hooked up in a follow-up cl). Do you have any > suggestions for a path to force the navigation to take the value of > RequestNavigationParams that doesn't break this correctness? Why not attach it to the extraData in CreateURLRequestForNavigation? You'll just need to make tweak the willSendRequest code to only inherit on subresource requests, but not navigations, since those are pretty different. That would translate to PlzNavigate well since you get a browser-side CreateURLRequestForNavigation-looking function, but not willSendRequest in the renderer because the renderer does not exist. > Maybe the problem > is we're combining the navigation signal to force Lo-Fi on or off with the > response value at commit that sets the state. Yes! That's it in a nutshell. Thanks for phrasing it much better than I did. :-) > These values would be the same for > the navigations with forced Lo-Fi state, and that's why it was being set early > here. They're not quite the same because a navigation may not commit, and the previous document may still issue subresource requests before this commit point.
https://codereview.chromium.org/1310743003/diff/220001/content/public/common/... File content/public/common/data_reduction_proxy_lofi_user_data.h (right): https://codereview.chromium.org/1310743003/diff/220001/content/public/common/... content/public/common/data_reduction_proxy_lofi_user_data.h:30: void set_lofi_state(LoFiState lofi_state) { lofi_state_ = lofi_state; } On 2015/09/22 22:20:50, David Benjamin wrote: > On 2015/09/22 20:35:52, megjablon wrote: > > On 2015/09/21 18:03:48, mmenke wrote: > > > Should this just be a value in ResourceRequestInfoImpl instead? Particularly > > if > > > the goal is to make this a web platform feature, rather than purely a DRP > > > feature. > > > > This sounds like a good and simpler idea to me. davidben@ and nasko@ any > > objections? > > Yes, please do that. But exposing a mutator here still makes the content/DRP > boundary a little awkward, ResourceRequestInfo or DRPLoFiUserData. I would > suggest going further and doing the following: > > ResourceRequestInfo does not have set_lofi_state or even expose the LoFiState > enum. Instead, you have: > > bool ResourceRequestInfo::is_lofi() const > > Leave interpreting LOFI_DEFAULT and converting to bool to //content. //content > doesn't have the bandwidth estimator, so we add a method to > ResourceDispatcherHostDelegate or elsewhere like bool ShouldEnableLofiMode(/* > stuff */). This function tells you whether, at this point in time, LOFI_DEFAULT > means on or off. > > //content calls into that very early to get rid of LOFI_DEFAULT and turn it into > a boolean. From there, the LofiState enum is gone. > > As previously discussed, I'm not a huge fan of //content having this bit and not > knowing what it means, so I think it SHOULD be a header that //content also just > sets. But I'm okay with holding that for now since it's not yet standardized. In > the meantime, the semantics are: > > - //content has this notion of whether requests and documents and navigations > are "lofi". It exposes this bit on ResourceRequestInfo. > > - //content lets you read this bit, but not set it. Maintaining this bit is > //content's job. > > - As part of maintaining that bit, //content asks its embedder whether, at any > point in time, bandwidth and such are low enough that "lofi" is on. This is used > to bootstrap the bit on top-level navigations, implemented with LOFI_DEFAULT. > //chrome will implement that hook by querying the DRP logic. > > - //content doesn't do anything with this bit it maintains, but, it does let you > mutate the URLRequest before it goes out because it's always let you do that as > part of intercepting the net stack. When intercepting the net stack, you're free > to interpret that bit how you like and, notably, modify the DRP header. > > In the future, that last step will be replaced by //content just setting the > Enable-Fancy-Standardized-Lofi-Mode header and then the DRP won't really be > involved in this but for a server-side polyfill. And maybe to implement the > ShouldEnableLofiMode hook, I dunno. > > How does that sound? As discussed offline, this makes me, at least, much happier.
https://codereview.chromium.org/1310743003/diff/220001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1310743003/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:4501: lofi_state_ = request_params.lofi_state; On 2015/09/22 22:20:50, David Benjamin wrote: > On 2015/09/22 20:35:52, megjablon wrote: > > On 2015/09/21 18:09:01, David Benjamin wrote: > > > On 2015/09/21 17:58:15, David Benjamin wrote: > > > > This still hasn't been resolved. lofi_state_ must only be updated on > commit, > > > not > > > > when you start a navigation. See previous discussions on whether this > field > > > was > > > > a LofiState or a bool. > > > > > > To clarify, this isn't a "layering; we'll fix this later" problem. It's a > > > correctness problem. Until the navigation has committed, it's not correct to > > > change any state relating to the current navigation. The state needs to flow > > at > > > commit from the ResourceResponseInfo sent in ResourceMsg_ReceivedResponse to > > > didCommitProvisionalLoad and be set at that point and only at that point. > That > > > bit then gets stapled to subresources at willSendRequest. Frame requests > will > > > hit a slightly different codepath. > > > > > > The key thing to always keep in mind is that navigations (frame requests) > are > > > completely different from subresources. It is expected that you will need to > > > treat them different for this sort of thing. (Treating them the same is > > usually > > > a red flag that something is wrong.) > > > > > > (In fact, I'm not sure this CL works at all for renderer-initiated > > navigations. > > > Not every navigation hits this function.) > > > > For renderer initiated navigations, we don't need to set Lo-Fi state because > it > > will always be LOFI_DEFAULT, and we have the NQE decide. > > I think we were talking about different things. Though I see that > didCommitProvisionalLoad *also* sets lofi_state_. Sorry, I missed that somehow. > The part I was thinking of works fine for renderer-initiated navigations. All > you actually want to do is ditch this line, switch it to a boolean, and then > plumb the value for navigations differently. > > > This is used by browser > > initiated navigations to force Lo-Fi on or off and will be used by the "Load > > images" snackbar (to be hooked up in a follow-up cl). Do you have any > > suggestions for a path to force the navigation to take the value of > > RequestNavigationParams that doesn't break this correctness? > > Why not attach it to the extraData in CreateURLRequestForNavigation? You'll just > need to make tweak the willSendRequest code to only inherit on subresource > requests, but not navigations, since those are pretty different. That would > translate to PlzNavigate well since you get a browser-side > CreateURLRequestForNavigation-looking function, but not willSendRequest in the > renderer because the renderer does not exist. > > > Maybe the problem > > is we're combining the navigation signal to force Lo-Fi on or off with the > > response value at commit that sets the state. > > Yes! That's it in a nutshell. Thanks for phrasing it much better than I did. :-) > > > These values would be the same for > > the navigations with forced Lo-Fi state, and that's why it was being set early > > here. > > They're not quite the same because a navigation may not commit, and the previous > document may still issue subresource requests before this commit point. This sounds good. I'm working on implementing this. This may be a simple question, but in willSendRequest how do I check if the request is a subresource?
https://codereview.chromium.org/1310743003/diff/220001/content/public/common/... File content/public/common/data_reduction_proxy_lofi_user_data.h (right): https://codereview.chromium.org/1310743003/diff/220001/content/public/common/... content/public/common/data_reduction_proxy_lofi_user_data.h:30: void set_lofi_state(LoFiState lofi_state) { lofi_state_ = lofi_state; } On 2015/09/22 22:36:28, mmenke wrote: > On 2015/09/22 22:20:50, David Benjamin wrote: > > On 2015/09/22 20:35:52, megjablon wrote: > > > On 2015/09/21 18:03:48, mmenke wrote: > > > > Should this just be a value in ResourceRequestInfoImpl instead? > Particularly > > > if > > > > the goal is to make this a web platform feature, rather than purely a DRP > > > > feature. > > > > > > This sounds like a good and simpler idea to me. davidben@ and nasko@ any > > > objections? > > > > Yes, please do that. But exposing a mutator here still makes the content/DRP > > boundary a little awkward, ResourceRequestInfo or DRPLoFiUserData. I would > > suggest going further and doing the following: > > > > ResourceRequestInfo does not have set_lofi_state or even expose the LoFiState > > enum. Instead, you have: > > > > bool ResourceRequestInfo::is_lofi() const > > > > Leave interpreting LOFI_DEFAULT and converting to bool to //content. //content > > doesn't have the bandwidth estimator, so we add a method to > > ResourceDispatcherHostDelegate or elsewhere like bool ShouldEnableLofiMode(/* > > stuff */). This function tells you whether, at this point in time, > LOFI_DEFAULT > > means on or off. > > > > //content calls into that very early to get rid of LOFI_DEFAULT and turn it > into > > a boolean. From there, the LofiState enum is gone. > > > > As previously discussed, I'm not a huge fan of //content having this bit and > not > > knowing what it means, so I think it SHOULD be a header that //content also > just > > sets. But I'm okay with holding that for now since it's not yet standardized. > In > > the meantime, the semantics are: > > > > - //content has this notion of whether requests and documents and navigations > > are "lofi". It exposes this bit on ResourceRequestInfo. > > > > - //content lets you read this bit, but not set it. Maintaining this bit is > > //content's job. > > > > - As part of maintaining that bit, //content asks its embedder whether, at any > > point in time, bandwidth and such are low enough that "lofi" is on. This is > used > > to bootstrap the bit on top-level navigations, implemented with LOFI_DEFAULT. > > //chrome will implement that hook by querying the DRP logic. > > > > - //content doesn't do anything with this bit it maintains, but, it does let > you > > mutate the URLRequest before it goes out because it's always let you do that > as > > part of intercepting the net stack. When intercepting the net stack, you're > free > > to interpret that bit how you like and, notably, modify the DRP header. > > > > In the future, that last step will be replaced by //content just setting the > > Enable-Fancy-Standardized-Lofi-Mode header and then the DRP won't really be > > involved in this but for a server-side polyfill. And maybe to implement the > > ShouldEnableLofiMode hook, I dunno. > > > > How does that sound? > > As discussed offline, this makes me, at least, much happier. Done! LTAL and let me know if you want to move anything around. I did my best from this description, but you may have a better idea of where things should be placed. https://codereview.chromium.org/1310743003/diff/220001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1310743003/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:4501: lofi_state_ = request_params.lofi_state; On 2015/09/23 01:39:04, megjablon wrote: > On 2015/09/22 22:20:50, David Benjamin wrote: > > On 2015/09/22 20:35:52, megjablon wrote: > > > On 2015/09/21 18:09:01, David Benjamin wrote: > > > > On 2015/09/21 17:58:15, David Benjamin wrote: > > > > > This still hasn't been resolved. lofi_state_ must only be updated on > > commit, > > > > not > > > > > when you start a navigation. See previous discussions on whether this > > field > > > > was > > > > > a LofiState or a bool. > > > > > > > > To clarify, this isn't a "layering; we'll fix this later" problem. It's a > > > > correctness problem. Until the navigation has committed, it's not correct > to > > > > change any state relating to the current navigation. The state needs to > flow > > > at > > > > commit from the ResourceResponseInfo sent in ResourceMsg_ReceivedResponse > to > > > > didCommitProvisionalLoad and be set at that point and only at that point. > > That > > > > bit then gets stapled to subresources at willSendRequest. Frame requests > > will > > > > hit a slightly different codepath. > > > > > > > > The key thing to always keep in mind is that navigations (frame requests) > > are > > > > completely different from subresources. It is expected that you will need > to > > > > treat them different for this sort of thing. (Treating them the same is > > > usually > > > > a red flag that something is wrong.) > > > > > > > > (In fact, I'm not sure this CL works at all for renderer-initiated > > > navigations. > > > > Not every navigation hits this function.) > > > > > > For renderer initiated navigations, we don't need to set Lo-Fi state because > > it > > > will always be LOFI_DEFAULT, and we have the NQE decide. > > > > I think we were talking about different things. Though I see that > > didCommitProvisionalLoad *also* sets lofi_state_. Sorry, I missed that > somehow. > > The part I was thinking of works fine for renderer-initiated navigations. All > > you actually want to do is ditch this line, switch it to a boolean, and then > > plumb the value for navigations differently. > > > > > This is used by browser > > > initiated navigations to force Lo-Fi on or off and will be used by the "Load > > > images" snackbar (to be hooked up in a follow-up cl). Do you have any > > > suggestions for a path to force the navigation to take the value of > > > RequestNavigationParams that doesn't break this correctness? > > > > Why not attach it to the extraData in CreateURLRequestForNavigation? You'll > just > > need to make tweak the willSendRequest code to only inherit on subresource > > requests, but not navigations, since those are pretty different. That would > > translate to PlzNavigate well since you get a browser-side > > CreateURLRequestForNavigation-looking function, but not willSendRequest in the > > renderer because the renderer does not exist. > > > > > Maybe the problem > > > is we're combining the navigation signal to force Lo-Fi on or off with the > > > response value at commit that sets the state. > > > > Yes! That's it in a nutshell. Thanks for phrasing it much better than I did. > :-) > > > > > These values would be the same for > > > the navigations with forced Lo-Fi state, and that's why it was being set > early > > > here. > > > > They're not quite the same because a navigation may not commit, and the > previous > > document may still issue subresource requests before this commit point. > > This sounds good. I'm working on implementing this. This may be a simple > question, but in willSendRequest how do I check if the request is a subresource? I took a stab at this. PTAL!
A friendly ping! :)
(Sorry this slipped through. Will get to it tomorrow!)
On 2015/09/28 22:54:22, David Benjamin wrote: > (Sorry this slipped through. Will get to it tomorrow!) David: You seem to be pretty loaded, and with me fleeing from the stale-while-revalidate CL... I'm happy to do a pass on this tomorrow.
https://codereview.chromium.org/1310743003/diff/300001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/1310743003/diff/300001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:747: io_data->data_reduction_proxy_io_data(); Why does this need to go through a new DRP class? If we want field trials to enable lofi mode only when using the DRP, I'd suggest putting that logic here (Setting a bool based on the trial on creation of this class, and checking if the DRP is enabled / will be used for the request here), rather than adding a separate class just for this one experiment. Or is there something I'm missing? https://codereview.chromium.org/1310743003/diff/300001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:749: if (url_request && data_reduction_proxy_io_data) { url_request can't be NULL, so shouldn't check for it. https://codereview.chromium.org/1310743003/diff/300001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:751: } nit: Remove braces. https://codereview.chromium.org/1310743003/diff/300001/chrome/renderer/page_l... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/1310743003/diff/300001/chrome/renderer/page_l... chrome/renderer/page_load_histograms.cc:924: data_reduction_proxy_was_used, render_frame->IsLoFiOn(), Suggest UsingLoFi (Or IsUsingLoFi), think that makes it clearer it's a per-frame setting, and not some global setting. Should also use a consistent name here and in the browser-side files. https://codereview.chromium.org/1310743003/diff/300001/components/data_reduct... File components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc (right): https://codereview.chromium.org/1310743003/diff/300001/components/data_reduct... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc:19: content::ResourceRequestInfo::ForRequest(request); ResourceRequestInfo will never exist at this point - we need to call this method to figure out the value to set its IsLoFi bit in the first place, before we can create it. https://codereview.chromium.org/1310743003/diff/300001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1310743003/diff/300001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1325: bool is_lofi; Maybe is_lofi -> use_lofi / using_lofi / is_using_lofi? modifies a bunch of files, but think it's clearer. "This request uses lofi" seems clearer than "this request is lofi" to me. https://codereview.chromium.org/1310743003/diff/300001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1326: if (request_data.lofi_state == LOFI_DEFAULT && I think this code makes more sense if LOFI_DEFAULT is renamed to LOFI_UNSPECIFIED. "Default" implies a single default value, but we're letting the delegate set it on a per-request basis, rather than using a default value. https://codereview.chromium.org/1310743003/diff/300001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1327: (load_flags & net::LOAD_MAIN_FRAME) != 0 && delegate_) { I don't see why we shouldn't call into the delegate on non-main frames, when lofi_state is is LOFI_DEFAULT. https://codereview.chromium.org/1310743003/diff/300001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1333: is_lofi = false; Question: Do favicons also inherit the lofi bit from the main page? That may be problematic, if they're shared via sync. https://codereview.chromium.org/1310743003/diff/300001/content/public/browser... File content/public/browser/resource_dispatcher_host_delegate.h (right): https://codereview.chromium.org/1310743003/diff/300001/content/public/browser... content/public/browser/resource_dispatcher_host_delegate.h:127: // Asks the embedder if Lo-Fi mode should be enabled for the given request. Should mention it's only called for requests with an unspecified lo-fi value. https://codereview.chromium.org/1310743003/diff/300001/content/renderer/rende... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1310743003/diff/300001/content/renderer/rende... content/renderer/render_frame_impl.h:226: void Initialize(bool is_lofi = false); The style guide doesn't generally allow default arguments.
https://codereview.chromium.org/1310743003/diff/300001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/1310743003/diff/300001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:747: io_data->data_reduction_proxy_io_data(); On 2015/09/29 16:35:57, mmenke wrote: > Why does this need to go through a new DRP class? > > If we want field trials to enable lofi mode only when using the DRP, I'd suggest > putting that logic here (Setting a bool based on the trial on creation of this > class, and checking if the DRP is enabled / will be used for the request here), > rather than adding a separate class just for this one experiment. > > Or is there something I'm missing? This isn't a new class. This is just adding a method to an existing class. ShouldEnableLoFiMode doesn't simply check the field trials and the DRP being enabled, it asks the network quality estimator if Lo-Fi should be enabled based on the connection. https://codereview.chromium.org/1310743003/diff/300001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:749: if (url_request && data_reduction_proxy_io_data) { On 2015/09/29 16:35:57, mmenke wrote: > url_request can't be NULL, so shouldn't check for it. Done. https://codereview.chromium.org/1310743003/diff/300001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:751: } On 2015/09/29 16:35:57, mmenke wrote: > nit: Remove braces. Done. https://codereview.chromium.org/1310743003/diff/300001/chrome/renderer/page_l... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/1310743003/diff/300001/chrome/renderer/page_l... chrome/renderer/page_load_histograms.cc:924: data_reduction_proxy_was_used, render_frame->IsLoFiOn(), On 2015/09/29 16:35:57, mmenke wrote: > Suggest UsingLoFi (Or IsUsingLoFi), think that makes it clearer it's a per-frame > setting, and not some global setting. Should also use a consistent name here > and in the browser-side files. I like this better. Done. https://codereview.chromium.org/1310743003/diff/300001/components/data_reduct... File components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc (right): https://codereview.chromium.org/1310743003/diff/300001/components/data_reduct... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc:19: content::ResourceRequestInfo::ForRequest(request); On 2015/09/29 16:35:57, mmenke wrote: > ResourceRequestInfo will never exist at this point - we need to call this method > to figure out the value to set its IsLoFi bit in the first place, before we can > create it. This is called from DRPNetworkDelegate::OnBeforeSendProxyHeadersInternal. I instrumented this and the ResourceRequestInfo exists from ResourceDispatcherHostImpl::BeginRequest which does happen before. https://codereview.chromium.org/1310743003/diff/300001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1310743003/diff/300001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1325: bool is_lofi; On 2015/09/29 16:35:57, mmenke wrote: > Maybe is_lofi -> use_lofi / using_lofi / is_using_lofi? modifies a bunch of > files, but think it's clearer. "This request uses lofi" seems clearer than > "this request is lofi" to me. Done. https://codereview.chromium.org/1310743003/diff/300001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1326: if (request_data.lofi_state == LOFI_DEFAULT && On 2015/09/29 16:35:57, mmenke wrote: > I think this code makes more sense if LOFI_DEFAULT is renamed to > LOFI_UNSPECIFIED. "Default" implies a single default value, but we're letting > the delegate set it on a per-request basis, rather than using a default value. Done. https://codereview.chromium.org/1310743003/diff/300001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1327: (load_flags & net::LOAD_MAIN_FRAME) != 0 && delegate_) { On 2015/09/29 16:35:57, mmenke wrote: > I don't see why we shouldn't call into the delegate on non-main frames, when > lofi_state is is LOFI_DEFAULT. Done. https://codereview.chromium.org/1310743003/diff/300001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1333: is_lofi = false; On 2015/09/29 16:35:57, mmenke wrote: > Question: Do favicons also inherit the lofi bit from the main page? That may > be problematic, if they're shared via sync. The proxy has logic so that icons and button images aren't "LoFi-ed". Testing it out, I haven't seen a favicon that got the placeholder, but I'll have to ask if favicons are explicitly filtered out or if they just happen to be detected by the logic that finds icons. https://codereview.chromium.org/1310743003/diff/300001/content/public/browser... File content/public/browser/resource_dispatcher_host_delegate.h (right): https://codereview.chromium.org/1310743003/diff/300001/content/public/browser... content/public/browser/resource_dispatcher_host_delegate.h:127: // Asks the embedder if Lo-Fi mode should be enabled for the given request. On 2015/09/29 16:35:57, mmenke wrote: > Should mention it's only called for requests with an unspecified lo-fi value. Done. https://codereview.chromium.org/1310743003/diff/300001/content/renderer/rende... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1310743003/diff/300001/content/renderer/rende... content/renderer/render_frame_impl.h:226: void Initialize(bool is_lofi = false); On 2015/09/29 16:35:58, mmenke wrote: > The style guide doesn't generally allow default arguments. @nasko who wanted this so we didn't have to touch as many files.
Patchset #16 (id:340001) has been deleted
We should have browser tests for this - tests that the RDHDelegate can set the mode on or off and that both the main frame resource and a subresource both get that priority. We should also have tests that other load types (reloads, renderer-initiated navigations, history navigations) get the expected lo-fi mode setting, going or not going through the RDHDelegate, as expected. I'd also like a test that a lofi mode tab doesn't request a lofi mode favicon. https://codereview.chromium.org/1310743003/diff/300001/components/data_reduct... File components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc (right): https://codereview.chromium.org/1310743003/diff/300001/components/data_reduct... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc:19: content::ResourceRequestInfo::ForRequest(request); On 2015/09/29 21:20:07, megjablon wrote: > On 2015/09/29 16:35:57, mmenke wrote: > > ResourceRequestInfo will never exist at this point - we need to call this > method > > to figure out the value to set its IsLoFi bit in the first place, before we > can > > create it. > > This is called from DRPNetworkDelegate::OnBeforeSendProxyHeadersInternal. I > instrumented this and the ResourceRequestInfo exists from > ResourceDispatcherHostImpl::BeginRequest which does happen before. Sorry, I thought that this is what was being called from ShouldEnableLoFiMode (Which seemed really odd to me) - I hadn't realized there was pre-existing code, thought this added all of the lofi stuff. https://codereview.chromium.org/1310743003/diff/360001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/1310743003/diff/360001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:733: return data_reduction_proxy_io_data->ShouldEnableLoFiMode(url_request); This method doesn't seem to exist. Also, I think the logic here makes more sense inlined than in a method of DRP's catch-all class. It doesn't, for instance, make any sense in Cronet's context, I believe, and Cronet also uses DRP_io_data. https://codereview.chromium.org/1310743003/diff/360001/components/data_reduct... File components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc (right): https://codereview.chromium.org/1310743003/diff/360001/components/data_reduct... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc:24: params::IsIncludedInLoFiEnabledFieldTrial()); optional: This is kinda weird. IsUsingLoFi can return false for requests that, according to content, are in fact using lofi. Maybe this ContentDataReductionProxyLoFiHelper::IsUsingLoFi should be ShouldUseLoFi? Also think this behavior is weird enough to warrant a comment. https://codereview.chromium.org/1310743003/diff/360001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1310743003/diff/360001/content/renderer/rende... content/renderer/render_frame_impl.cc:3397: extra_data->set_lofi_state(is_using_lofi_ ? LOFI_ON : LOFI_OFF); We should have tests for each of these cases.
I'm unfamiliar with the test infrastructure in content, can you recommend some similar tests for a starting reference? Also, the request header does get the LoFi directive for favicons, we just don't return a LoFi-ed version for them via the server. https://chromiumcodereview.appspot.com/1310743003/diff/360001/chrome/browser/... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://chromiumcodereview.appspot.com/1310743003/diff/360001/chrome/browser/... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:733: return data_reduction_proxy_io_data->ShouldEnableLoFiMode(url_request); On 2015/09/30 19:33:41, mmenke wrote: > This method doesn't seem to exist. > > Also, I think the logic here makes more sense inlined than in a method of DRP's > catch-all class. It doesn't, for instance, make any sense in Cronet's context, > I believe, and Cronet also uses DRP_io_data. Sorry this was split into two cls since it was getting enormous. The DRP portions of it are in the other one and ShouldEnableLoFiMode is created there. Inlining this here is not simple. There's a lot of code that is DRP specific and would require more massive changes. Plus we may see value in adding this in other contexts in the future. https://chromiumcodereview.appspot.com/1310743003/diff/360001/components/data... File components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc (right): https://chromiumcodereview.appspot.com/1310743003/diff/360001/components/data... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc:24: params::IsIncludedInLoFiEnabledFieldTrial()); On 2015/09/30 19:33:41, mmenke wrote: > optional: This is kinda weird. IsUsingLoFi can return false for requests that, > according to content, are in fact using lofi. Maybe this > ContentDataReductionProxyLoFiHelper::IsUsingLoFi should be ShouldUseLoFi? Also > think this behavior is weird enough to warrant a comment. Done. https://chromiumcodereview.appspot.com/1310743003/diff/360001/content/rendere... File content/renderer/render_frame_impl.cc (right): https://chromiumcodereview.appspot.com/1310743003/diff/360001/content/rendere... content/renderer/render_frame_impl.cc:3397: extra_data->set_lofi_state(is_using_lofi_ ? LOFI_ON : LOFI_OFF); On 2015/09/30 19:33:41, mmenke wrote: > We should have tests for each of these cases. I'm unfamiliar with the test infrastructure in content, can you recommend some similar tests for a starting reference?
On 2015/09/30 22:14:08, megjablon wrote: > I'm unfamiliar with the test infrastructure in content, can you recommend some > similar tests for a starting reference? We'll want a browser test. I'm happy to help here, but I have network triage duty the next two days. I'll be happy to help after that - I suspect it may take a bit of digging to come up with something reasonable. I'm sorry for the delay (And I completely understand your frustration with our slowness on this). > Also, the request header does get the LoFi directive for favicons, we just don't > return a LoFi-ed version for them via the server. Ok, I think that's a problem that we want to fix before shipping (In an actually shipping the non-DRP header sense, doesn't need to be taken care of in this CL) > https://chromiumcodereview.appspot.com/1310743003/diff/360001/chrome/browser/... > File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc > (right): > > https://chromiumcodereview.appspot.com/1310743003/diff/360001/chrome/browser/... > chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:733: > return data_reduction_proxy_io_data->ShouldEnableLoFiMode(url_request); > On 2015/09/30 19:33:41, mmenke wrote: > > This method doesn't seem to exist. > > > > Also, I think the logic here makes more sense inlined than in a method of > DRP's > > catch-all class. It doesn't, for instance, make any sense in Cronet's > context, > > I believe, and Cronet also uses DRP_io_data. > > Sorry this was split into two cls since it was getting enormous. The DRP > portions of it are in the other one and ShouldEnableLoFiMode is created there. > Inlining this here is not simple. There's a lot of code that is DRP specific and > would require more massive changes. Plus we may see value in adding this in > other contexts in the future. I'm a bit concerned that we're trying this too tightly with the DRP - this is a feature that makes a lot of sense when used with the DRP, but it also makes sense when used independently of it.
On 2015/09/30 22:21:48, mmenke wrote: > On 2015/09/30 22:14:08, megjablon wrote: > > I'm unfamiliar with the test infrastructure in content, can you recommend some > > similar tests for a starting reference? > > We'll want a browser test. I'm happy to help here, but I have network triage > duty the next two days. I'll be happy to help after that - I suspect it may > take a bit of digging to come up with something reasonable. I'm sorry for the > delay (And I completely understand your frustration with our slowness on this). That would be great. I'll start digging into this in the time being and see what I can come up with. > > > Also, the request header does get the LoFi directive for favicons, we just > don't > > return a LoFi-ed version for them via the server. > > Ok, I think that's a problem that we want to fix before shipping (In an actually > shipping the non-DRP header sense, doesn't need to be taken care of in this CL) I'll file a separate bug for this, but this cl is getting very cumbersome, so let's keep moving on without this. > > > > https://chromiumcodereview.appspot.com/1310743003/diff/360001/chrome/browser/... > > File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc > > (right): > > > > > https://chromiumcodereview.appspot.com/1310743003/diff/360001/chrome/browser/... > > chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:733: > > return data_reduction_proxy_io_data->ShouldEnableLoFiMode(url_request); > > On 2015/09/30 19:33:41, mmenke wrote: > > > This method doesn't seem to exist. > > > > > > Also, I think the logic here makes more sense inlined than in a method of > > DRP's > > > catch-all class. It doesn't, for instance, make any sense in Cronet's > > context, > > > I believe, and Cronet also uses DRP_io_data. > > > > Sorry this was split into two cls since it was getting enormous. The DRP > > portions of it are in the other one and ShouldEnableLoFiMode is created there. > > Inlining this here is not simple. There's a lot of code that is DRP specific > and > > would require more massive changes. Plus we may see value in adding this in > > other contexts in the future. > > I'm a bit concerned that we're trying this too tightly with the DRP - this is a > feature that makes a lot of sense when used with the DRP, but it also makes > sense when used independently of it. We have plans to make this a web platform feature, but for now we're doing the best we can with the DRP without making this too complicated. Most of this code will be rewritten once we have the plan for the web platform feature figured out.
On 2015/09/30 22:30:05, megjablon wrote: > On 2015/09/30 22:21:48, mmenke wrote: > > On 2015/09/30 22:14:08, megjablon wrote: > > > I'm unfamiliar with the test infrastructure in content, can you recommend > some > > > similar tests for a starting reference? > > > > We'll want a browser test. I'm happy to help here, but I have network triage > > duty the next two days. I'll be happy to help after that - I suspect it may > > take a bit of digging to come up with something reasonable. I'm sorry for the > > delay (And I completely understand your frustration with our slowness on > this). > > That would be great. I'll start digging into this in the time being and see what > I can come up with. > > > > > > Also, the request header does get the LoFi directive for favicons, we just > > don't > > > return a LoFi-ed version for them via the server. > > > > Ok, I think that's a problem that we want to fix before shipping (In an > actually > > shipping the non-DRP header sense, doesn't need to be taken care of in this > CL) > > I'll file a separate bug for this, but this cl is getting very cumbersome, so > let's keep moving on without this. I think this is reasonable. > > > > > > > > https://chromiumcodereview.appspot.com/1310743003/diff/360001/chrome/browser/... > > > File > chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc > > > (right): > > > > > > > > > https://chromiumcodereview.appspot.com/1310743003/diff/360001/chrome/browser/... > > > > chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:733: > > > return data_reduction_proxy_io_data->ShouldEnableLoFiMode(url_request); > > > On 2015/09/30 19:33:41, mmenke wrote: > > > > This method doesn't seem to exist. > > > > > > > > Also, I think the logic here makes more sense inlined than in a method of > > > DRP's > > > > catch-all class. It doesn't, for instance, make any sense in Cronet's > > > context, > > > > I believe, and Cronet also uses DRP_io_data. > > > > > > Sorry this was split into two cls since it was getting enormous. The DRP > > > portions of it are in the other one and ShouldEnableLoFiMode is created > there. > > > Inlining this here is not simple. There's a lot of code that is DRP specific > > and > > > would require more massive changes. Plus we may see value in adding this in > > > other contexts in the future. > > > > I'm a bit concerned that we're trying this too tightly with the DRP - this is > a > > feature that makes a lot of sense when used with the DRP, but it also makes > > sense when used independently of it. > > We have plans to make this a web platform feature, but for now we're doing the > best we can with the DRP without making this too complicated. Most of this code > will be rewritten once we have the plan for the web platform feature figured > out.
Just a few comments, mostly nits. https://chromiumcodereview.appspot.com/1310743003/diff/380001/chrome/renderer... File chrome/renderer/page_load_histograms.cc (right): https://chromiumcodereview.appspot.com/1310743003/diff/380001/chrome/renderer... chrome/renderer/page_load_histograms.cc:916: data_reduction_proxy_was_used, render_frame->IsUsingLoFi(), Is it possible to get this bit somehow through the DRP instead of RenderFrame? It will be great if we can keep it internal for now. https://chromiumcodereview.appspot.com/1310743003/diff/380001/content/common/... File content/common/navigation_params.h (right): https://chromiumcodereview.appspot.com/1310743003/diff/380001/content/common/... content/common/navigation_params.h:29: // Request a normal (non-Lo-Fi) version of the resource. nit: Empty line between the enum value and the comment. https://chromiumcodereview.appspot.com/1310743003/diff/380001/content/common/... content/common/navigation_params.h:105: LoFiState lofi_state; It might be good to add clamy@, as an FYI on this CL. https://chromiumcodereview.appspot.com/1310743003/diff/380001/content/public/... File content/public/browser/resource_dispatcher_host_delegate.h (right): https://chromiumcodereview.appspot.com/1310743003/diff/380001/content/public/... content/public/browser/resource_dispatcher_host_delegate.h:127: // Asks the embedder if Lo-Fi mode should be enabled for the given request. Is nit: s/Is/It is/ https://chromiumcodereview.appspot.com/1310743003/diff/380001/content/public/... File content/public/renderer/render_frame.h (right): https://chromiumcodereview.appspot.com/1310743003/diff/380001/content/public/... content/public/renderer/render_frame.h:161: // Whether or not to use Lo-Fi for this frame. To use or "is using"? All I see using this is the metrics code, which can't influence any decision making. https://chromiumcodereview.appspot.com/1310743003/diff/380001/content/rendere... File content/renderer/render_frame_impl.cc (right): https://chromiumcodereview.appspot.com/1310743003/diff/380001/content/rendere... content/renderer/render_frame_impl.cc:2254: child_render_frame->Initialize(is_using_lofi_); Since we've exposed from RenderFrame whether it is using LoFi or not, we can remove this parameter totally and inside Initialize just ask the parent frame for the boolean. https://chromiumcodereview.appspot.com/1310743003/diff/380001/content/rendere... File content/renderer/render_frame_impl.h (right): https://chromiumcodereview.appspot.com/1310743003/diff/380001/content/rendere... content/renderer/render_frame_impl.h:228: void Initialize(bool is_using_lofi = false); No default values are allowed by the style guide. What I suggested is initializing this to a default value in the method itself. AFAIR, we concluded that in all cases of Initialize, the value will be DEFAULT. https://chromiumcodereview.appspot.com/1310743003/diff/380001/content/rendere... content/renderer/render_frame_impl.h:1055: // Whether or not to use Lo-Fi for this frame. "Whether or not this RenderFrame is using Lo-Fi mode."
https://chromiumcodereview.appspot.com/1310743003/diff/380001/chrome/renderer... File chrome/renderer/page_load_histograms.cc (right): https://chromiumcodereview.appspot.com/1310743003/diff/380001/chrome/renderer... chrome/renderer/page_load_histograms.cc:916: data_reduction_proxy_was_used, render_frame->IsUsingLoFi(), On 2015/10/01 18:28:57, nasko (slow to review) wrote: > Is it possible to get this bit somehow through the DRP instead of RenderFrame? > It will be great if we can keep it internal for now. DRP doesn't have any idea what the state would be for a frame and only knows when the state changes from one ShouldEnableLoFiMode call to the next. Asking the DRP would just keep the same hackiness that we're trying to get rid of. https://chromiumcodereview.appspot.com/1310743003/diff/380001/content/common/... File content/common/navigation_params.h (right): https://chromiumcodereview.appspot.com/1310743003/diff/380001/content/common/... content/common/navigation_params.h:29: // Request a normal (non-Lo-Fi) version of the resource. On 2015/10/01 18:28:57, nasko (slow to review) wrote: > nit: Empty line between the enum value and the comment. Done. https://chromiumcodereview.appspot.com/1310743003/diff/380001/content/common/... content/common/navigation_params.h:105: LoFiState lofi_state; On 2015/10/01 18:28:57, nasko (slow to review) wrote: > It might be good to add clamy@, as an FYI on this CL. Done. https://chromiumcodereview.appspot.com/1310743003/diff/380001/content/public/... File content/public/browser/resource_dispatcher_host_delegate.h (right): https://chromiumcodereview.appspot.com/1310743003/diff/380001/content/public/... content/public/browser/resource_dispatcher_host_delegate.h:127: // Asks the embedder if Lo-Fi mode should be enabled for the given request. Is On 2015/10/01 18:28:57, nasko (slow to review) wrote: > nit: s/Is/It is/ Done. https://chromiumcodereview.appspot.com/1310743003/diff/380001/content/public/... File content/public/renderer/render_frame.h (right): https://chromiumcodereview.appspot.com/1310743003/diff/380001/content/public/... content/public/renderer/render_frame.h:161: // Whether or not to use Lo-Fi for this frame. On 2015/10/01 18:28:57, nasko (slow to review) wrote: > To use or "is using"? All I see using this is the metrics code, which can't > influence any decision making. Done. https://chromiumcodereview.appspot.com/1310743003/diff/380001/content/rendere... File content/renderer/render_frame_impl.cc (right): https://chromiumcodereview.appspot.com/1310743003/diff/380001/content/rendere... content/renderer/render_frame_impl.cc:2254: child_render_frame->Initialize(is_using_lofi_); On 2015/10/01 18:28:57, nasko (slow to review) wrote: > Since we've exposed from RenderFrame whether it is using LoFi or not, we can > remove this parameter totally and inside Initialize just ask the parent frame > for the boolean. Done. https://chromiumcodereview.appspot.com/1310743003/diff/380001/content/rendere... File content/renderer/render_frame_impl.h (right): https://chromiumcodereview.appspot.com/1310743003/diff/380001/content/rendere... content/renderer/render_frame_impl.h:228: void Initialize(bool is_using_lofi = false); On 2015/10/01 18:28:57, nasko (slow to review) wrote: > No default values are allowed by the style guide. What I suggested is > initializing this to a default value in the method itself. AFAIR, we concluded > that in all cases of Initialize, the value will be DEFAULT. Ah sorry I misunderstood. https://chromiumcodereview.appspot.com/1310743003/diff/380001/content/rendere... content/renderer/render_frame_impl.h:1055: // Whether or not to use Lo-Fi for this frame. On 2015/10/01 18:28:57, nasko (slow to review) wrote: > "Whether or not this RenderFrame is using Lo-Fi mode." Done.
LGTM on non-loader part of content/. Please ensure davidben@/mmenke@ sign off on the loader part. https://codereview.chromium.org/1310743003/diff/380001/chrome/renderer/page_l... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/1310743003/diff/380001/chrome/renderer/page_l... chrome/renderer/page_load_histograms.cc:916: data_reduction_proxy_was_used, render_frame->IsUsingLoFi(), On 2015/10/01 19:43:25, megjablon wrote: > On 2015/10/01 18:28:57, nasko (slow to review) wrote: > > Is it possible to get this bit somehow through the DRP instead of RenderFrame? > > It will be great if we can keep it internal for now. > > DRP doesn't have any idea what the state would be for a frame and only knows > when the state changes from one ShouldEnableLoFiMode call to the next. Asking > the DRP would just keep the same hackiness that we're trying to get rid of. Acknowledged. https://codereview.chromium.org/1310743003/diff/400001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1310743003/diff/400001/content/renderer/rende... content/renderer/render_frame_impl.cc:3396: else if (is_main_frame_) Was this change intentional?
https://codereview.chromium.org/1310743003/diff/400001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1310743003/diff/400001/content/renderer/rende... content/renderer/render_frame_impl.cc:3396: else if (is_main_frame_) On 2015/10/01 20:13:57, nasko (slow to review) wrote: > Was this change intentional? Whoops, nope. Fixed.
clamy@chromium.org changed reviewers: + clamy@chromium.org
Just a comment on the navigation_params. https://codereview.chromium.org/1310743003/diff/420001/content/common/navigat... File content/common/navigation_params.h (right): https://codereview.chromium.org/1310743003/diff/420001/content/common/navigat... content/common/navigation_params.h:107: LoFiState lofi_state; Why are we adding this parameter to this struct? I cannot find a single instance where it will be set to something else than LOFI_UNSPECIFIED. Also, if you add a parameter to this struct, you should update its constructor.
On 2015/09/30 22:30:05, megjablon wrote: > We have plans to make this a web platform feature, but for now we're doing the > best we can with the DRP without making this too complicated. Most of this code > will be rewritten once we have the plan for the web platform feature figured > out. Planning to rewrite a completely feature before you even land the initial implementation seems like a waste of everyone times - both yours and yours reviewer. Better to get things right (Or at least as right as you can) the first time through, rather than rushing things out the door, only to completely redo them later. There are certainly exceptions, like when you think there's a good chance you won't keep a feature, but I'd argue that's not the case here. Sure, it slows down initial launch, but it speeds up final launch, and is also likely to result in fewer bugs (Particularly bugs in production) to work through, since you land less code.
On 2015/10/02 16:58:08, mmenke wrote: > On 2015/09/30 22:30:05, megjablon wrote: > > We have plans to make this a web platform feature, but for now we're doing the > > best we can with the DRP without making this too complicated. Most of this > code > > will be rewritten once we have the plan for the web platform feature figured > > out. > > Planning to rewrite a completely feature before you even land the initial > implementation seems like a waste of everyone times - both yours and yours > reviewer. Better to get things right (Or at least as right as you can) the > first time through, rather than rushing things out the door, only to completely > redo them later. There are certainly exceptions, like when you think there's a > good chance you won't keep a feature, but I'd argue that's not the case here. > Sure, it slows down initial launch, but it speeds up final launch, and is also > likely to result in fewer bugs (Particularly bugs in production) to work > through, since you land less code. This will not require a rewrite of most of the feature, but moving the pieces you are suggesting be moved out of the DRP will happen as we figure out the details of the web platform feature. We are still investigating the proper triggers for the feature, and this code just helps fix some of our broken logic. As we experiment with Lo-Fi, we are learning and iterating on the design.
https://chromiumcodereview.appspot.com/1310743003/diff/420001/content/common/... File content/common/navigation_params.h (right): https://chromiumcodereview.appspot.com/1310743003/diff/420001/content/common/... content/common/navigation_params.h:107: LoFiState lofi_state; On 2015/10/02 16:08:30, clamy wrote: > Why are we adding this parameter to this struct? I cannot find a single instance > where it will be set to something else than LOFI_UNSPECIFIED. Also, if you add a > parameter to this struct, you should update its constructor. It's going to be used in a follow up cl that switches the snackbar that reloads the page with images to using this mechanism. Added to the constructor.
Patchset #20 (id:440001) has been deleted
Patchset #3 (id:40001) has been deleted
For tests, content/browser/loader/resource_dispatcher_host_browsertest.cc already inject a RDHDelegate. You should be able to fairly easily follow that example to set one which returns a specific value in response to ShouldEnableLoFiMode calls, and has expectations for ResourceInfo::is_lofi values for RequestBeginning calls, based on URL. Suggested test cases (Checking main frame and some subresource - an image, maybe, in each case): Start with LOFI_UNSPECIFIED and return true/false. Start navigation with LOFI_ON/LOFI_OFF, and make sure the expected value is returned. Reload and make sure expectations match, and navigate back and make sure it's what's expected (I'm not sure what is expected in either of those two paths - if we should retain the value, in which case we should start with LOFI_UNSPECIFIED, reload, have one test case which it's turned on and one off, and make sure after reload, the values are the same, and ShouldEnableLofi was not called again, if we should check again, then have the delegate toggle the value between loads)
On 2015/10/02 17:45:21, megjablon wrote: > On 2015/10/02 16:58:08, mmenke wrote: > > On 2015/09/30 22:30:05, megjablon wrote: > > > We have plans to make this a web platform feature, but for now we're doing > the > > > best we can with the DRP without making this too complicated. Most of this > > code > > > will be rewritten once we have the plan for the web platform feature figured > > > out. > > > > Planning to rewrite a completely feature before you even land the initial > > implementation seems like a waste of everyone times - both yours and yours > > reviewer. Better to get things right (Or at least as right as you can) the > > first time through, rather than rushing things out the door, only to > completely > > redo them later. There are certainly exceptions, like when you think there's > a > > good chance you won't keep a feature, but I'd argue that's not the case here. > > Sure, it slows down initial launch, but it speeds up final launch, and is also > > likely to result in fewer bugs (Particularly bugs in production) to work > > through, since you land less code. > > This will not require a rewrite of most of the feature, but moving the pieces > you are suggesting be moved out of the DRP will happen as we figure out the > details of the web platform feature. We are still investigating the proper > triggers for the feature, and this code just helps fix some of our broken logic. > As we experiment with Lo-Fi, we are learning and iterating on the design. Sorry, I misunderstood "most of this code will be rewritten".
On 2015/10/05 15:32:41, mmenke wrote: > For tests, content/browser/loader/resource_dispatcher_host_browsertest.cc > already inject a RDHDelegate. You should be able to fairly easily follow that > example to set one which returns a specific value in response to > ShouldEnableLoFiMode calls, and has expectations for ResourceInfo::is_lofi > values for RequestBeginning calls, based on URL. > > Suggested test cases (Checking main frame and some subresource - an image, > maybe, in each case): Start with LOFI_UNSPECIFIED and return true/false. Start > navigation with LOFI_ON/LOFI_OFF, and make sure the expected value is returned. > Reload and make sure expectations match, and navigate back and make sure it's > what's expected (I'm not sure what is expected in either of those two paths - if > we should retain the value, in which case we should start with LOFI_UNSPECIFIED, > reload, have one test case which it's turned on and one off, and make sure after > reload, the values are the same, and ShouldEnableLofi was not called again, if > we should check again, then have the delegate toggle the value between loads) This is super helpful, thanks! For the reload and navigating back, would these navigations force an eviction from the cache? We can reassess the network quality and call ShouldEnableLoFiMode again on these navigations as long as that isn't happening.
On 2015/10/06 18:03:25, megjablon wrote: > On 2015/10/05 15:32:41, mmenke wrote: > > For tests, content/browser/loader/resource_dispatcher_host_browsertest.cc > > already inject a RDHDelegate. You should be able to fairly easily follow that > > example to set one which returns a specific value in response to > > ShouldEnableLoFiMode calls, and has expectations for ResourceInfo::is_lofi > > values for RequestBeginning calls, based on URL. > > > > Suggested test cases (Checking main frame and some subresource - an image, > > maybe, in each case): Start with LOFI_UNSPECIFIED and return true/false. > Start > > navigation with LOFI_ON/LOFI_OFF, and make sure the expected value is > returned. > > Reload and make sure expectations match, and navigate back and make sure it's > > what's expected (I'm not sure what is expected in either of those two paths - > if > > we should retain the value, in which case we should start with > LOFI_UNSPECIFIED, > > reload, have one test case which it's turned on and one off, and make sure > after > > reload, the values are the same, and ShouldEnableLofi was not called again, if > > we should check again, then have the delegate toggle the value between loads) > > This is super helpful, thanks! For the reload and navigating back, would these > navigations force an eviction from the cache? We can reassess the network > quality and call ShouldEnableLoFiMode again on these navigations as long as that > isn't happening. Back/forward typically reload preferring what's in the cache (i.e. don't even bother revalidating). We don't want to hit the network there if we can avoid it since you're going back to whatever the user was on last time. Reload will force a *revalidation*, but it won't ignore the cache entry completely. It will send a conditionalized request. The only time you should need to completely ignore the cache entry is if the server did caching wrong (e.g. failing to honor Vary headers, or just completely messing up ETags). That's what a shift-reload does.
https://chromiumcodereview.appspot.com/1310743003/diff/480001/content/rendere... File content/renderer/render_frame_impl.cc (right): https://chromiumcodereview.appspot.com/1310743003/diff/480001/content/rendere... content/renderer/render_frame_impl.cc:431: extra_data->set_lofi_state(common_params.lofi_state); I'm finding that setting this extra data here doesn't always get passed through to willSendRequest. I'm currently trying to test a reload with LOFI_OFF and there's no extra_data on the request at willSendRequest. I'm having trouble finding all the different paths that could cause this extra_data to be thrown out. What were the objections to storing the LoFiState in this class until the navigation commits?
On 2015/10/06 20:35:49, David Benjamin wrote: > On 2015/10/06 18:03:25, megjablon wrote: > > On 2015/10/05 15:32:41, mmenke wrote: > > > For tests, content/browser/loader/resource_dispatcher_host_browsertest.cc > > > already inject a RDHDelegate. You should be able to fairly easily follow > that > > > example to set one which returns a specific value in response to > > > ShouldEnableLoFiMode calls, and has expectations for ResourceInfo::is_lofi > > > values for RequestBeginning calls, based on URL. > > > > > > Suggested test cases (Checking main frame and some subresource - an image, > > > maybe, in each case): Start with LOFI_UNSPECIFIED and return true/false. > > Start > > > navigation with LOFI_ON/LOFI_OFF, and make sure the expected value is > > returned. > > > Reload and make sure expectations match, and navigate back and make sure > it's > > > what's expected (I'm not sure what is expected in either of those two paths > - > > if > > > we should retain the value, in which case we should start with > > LOFI_UNSPECIFIED, > > > reload, have one test case which it's turned on and one off, and make sure > > after > > > reload, the values are the same, and ShouldEnableLofi was not called again, > if > > > we should check again, then have the delegate toggle the value between > loads) > > > > This is super helpful, thanks! For the reload and navigating back, would these > > navigations force an eviction from the cache? We can reassess the network > > quality and call ShouldEnableLoFiMode again on these navigations as long as > that > > isn't happening. > > Back/forward typically reload preferring what's in the cache (i.e. don't even > bother revalidating). We don't want to hit the network there if we can avoid it > since you're going back to whatever the user was on last time. > > Reload will force a *revalidation*, but it won't ignore the cache entry > completely. It will send a conditionalized request. The only time you should > need to completely ignore the cache entry is if the server did caching wrong > (e.g. failing to honor Vary headers, or just completely messing up ETags). > That's what a shift-reload does. Maybe use no-store in the tests, then? I basically want to make sure we remember, or not, the lofi mode, as expected.
Patchset #21 (id:500001) has been deleted
On 2015/10/07 14:47:55, mmenke wrote: > On 2015/10/06 20:35:49, David Benjamin wrote: > > On 2015/10/06 18:03:25, megjablon wrote: > > > On 2015/10/05 15:32:41, mmenke wrote: > > > > For tests, content/browser/loader/resource_dispatcher_host_browsertest.cc > > > > already inject a RDHDelegate. You should be able to fairly easily follow > > that > > > > example to set one which returns a specific value in response to > > > > ShouldEnableLoFiMode calls, and has expectations for ResourceInfo::is_lofi > > > > values for RequestBeginning calls, based on URL. > > > > > > > > Suggested test cases (Checking main frame and some subresource - an image, > > > > maybe, in each case): Start with LOFI_UNSPECIFIED and return true/false. > > > Start > > > > navigation with LOFI_ON/LOFI_OFF, and make sure the expected value is > > > returned. > > > > Reload and make sure expectations match, and navigate back and make sure > > it's > > > > what's expected (I'm not sure what is expected in either of those two > paths > > - > > > if > > > > we should retain the value, in which case we should start with > > > LOFI_UNSPECIFIED, > > > > reload, have one test case which it's turned on and one off, and make sure > > > after > > > > reload, the values are the same, and ShouldEnableLofi was not called > again, > > if > > > > we should check again, then have the delegate toggle the value between > > loads) > > > > > > This is super helpful, thanks! For the reload and navigating back, would > these > > > navigations force an eviction from the cache? We can reassess the network > > > quality and call ShouldEnableLoFiMode again on these navigations as long as > > that > > > isn't happening. > > > > Back/forward typically reload preferring what's in the cache (i.e. don't even > > bother revalidating). We don't want to hit the network there if we can avoid > it > > since you're going back to whatever the user was on last time. > > > > Reload will force a *revalidation*, but it won't ignore the cache entry > > completely. It will send a conditionalized request. The only time you should > > need to completely ignore the cache entry is if the server did caching wrong > > (e.g. failing to honor Vary headers, or just completely messing up ETags). > > That's what a shift-reload does. > > Maybe use no-store in the tests, then? I basically want to make sure we > remember, or not, the lofi mode, as expected. Added tests! PTAL
https://codereview.chromium.org/1310743003/diff/520001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_browsertest.cc (right): https://codereview.chromium.org/1310743003/diff/520001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:531: : public ResourceDispatcherHostDelegate { This class seems very sensitive to receiving unexpected requests, or not receiving requests (Favicon request after navigation, for instance, could cause confusion). Suggest two watch URLs - one for the main frame, one for a subresource, and asserting we see them on destruction, and ignore all others, and make sure we see them once and only once (First check can be done on destruction, other can be done in RequestBeginning). And same for ShouldEnableLoFiMode with the main frame one. Can also check the order, out of paranoia (If we see the subframe request first, even if it has the right flag, it may be a request from the previous navigation or something). https://codereview.chromium.org/1310743003/diff/520001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:533: ShouldEnableLoFiModeResourceDispatcherHostDelegate(GURL watch_url) nit: const GURL& https://codereview.chromium.org/1310743003/diff/520001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:563: void SetWatchUrl(GURL watch_url) { const GURL& https://codereview.chromium.org/1310743003/diff/520001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:583: ResourceDispatcherHost::Get()->SetDelegate(&delegate); Hrm...This doesn't seem threadsafe - the RDH lives on the IO thread, we're on the IO thread. We should post a task to do this. https://codereview.chromium.org/1310743003/diff/520001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:583: ResourceDispatcherHost::Get()->SetDelegate(&delegate); Teardown also does seem threadsafe - we should restore the original delegate before deleting the delegate we injected. https://codereview.chromium.org/1310743003/diff/520001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:589: EXPECT_TRUE(delegate.should_enable_lofi_mode_called()); Each of these should be a separate test (the reload one and disabled one will need some extra navigations, of course). We should create the delegate, set it up, set it on the RDH (On the IOThread, by posting a task), and then start the tests. For the requests that need extra navigations, you can inject a second the preliminary navigation.
https://chromiumcodereview.appspot.com/1310743003/diff/520001/content/browser... File content/browser/loader/resource_dispatcher_host_browsertest.cc (right): https://chromiumcodereview.appspot.com/1310743003/diff/520001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:531: : public ResourceDispatcherHostDelegate { On 2015/10/08 19:06:09, mmenke wrote: > This class seems very sensitive to receiving unexpected requests, or not > receiving requests (Favicon request after navigation, for instance, could cause > confusion). > > Suggest two watch URLs - one for the main frame, one for a subresource, and > asserting we see them on destruction, and ignore all others, and make sure we > see them once and only once (First check can be done on destruction, other can > be done in RequestBeginning). And same for ShouldEnableLoFiMode with the main > frame one. > > Can also check the order, out of paranoia (If we see the subframe request first, > even if it has the right flag, it may be a request from the previous navigation > or something). Done. https://chromiumcodereview.appspot.com/1310743003/diff/520001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:533: ShouldEnableLoFiModeResourceDispatcherHostDelegate(GURL watch_url) On 2015/10/08 19:06:09, mmenke wrote: > nit: const GURL& Done. https://chromiumcodereview.appspot.com/1310743003/diff/520001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:563: void SetWatchUrl(GURL watch_url) { On 2015/10/08 19:06:09, mmenke wrote: > const GURL& Done. https://chromiumcodereview.appspot.com/1310743003/diff/520001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:583: ResourceDispatcherHost::Get()->SetDelegate(&delegate); On 2015/10/08 19:06:09, mmenke wrote: > Hrm...This doesn't seem threadsafe - the RDH lives on the IO thread, we're on > the IO thread. We should post a task to do this. I'm not sure I follow. Why do we need to post a task to the IO thread if we're on the IO thread? All the other browser tests simply set a delegate in this manner and then some of them set it to a nullptr at the end of the test, which I've added. https://chromiumcodereview.appspot.com/1310743003/diff/520001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:589: EXPECT_TRUE(delegate.should_enable_lofi_mode_called()); On 2015/10/08 19:06:09, mmenke wrote: > Each of these should be a separate test (the reload one and disabled one will > need some extra navigations, of course). We should create the delegate, set it > up, set it on the RDH (On the IOThread, by posting a task), and then start the > tests. > > For the requests that need extra navigations, you can inject a second the > preliminary navigation. Done.
quick response. I'll take another look tomorrow. https://chromiumcodereview.appspot.com/1310743003/diff/520001/content/browser... File content/browser/loader/resource_dispatcher_host_browsertest.cc (right): https://chromiumcodereview.appspot.com/1310743003/diff/520001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:583: ResourceDispatcherHost::Get()->SetDelegate(&delegate); On 2015/10/08 22:20:40, megjablon wrote: > On 2015/10/08 19:06:09, mmenke wrote: > > Hrm...This doesn't seem threadsafe - the RDH lives on the IO thread, we're on > > the IO thread. We should post a task to do this. > > I'm not sure I follow. Why do we need to post a task to the IO thread if we're > on the IO thread? All the other browser tests simply set a delegate in this > manner and then some of them set it to a nullptr at the end of the test, which > I've added. Sorry, that was a typo - we're on the UI thread, which is the problem.
Patchset #23 (id:560001) has been deleted
https://chromiumcodereview.appspot.com/1310743003/diff/520001/content/browser... File content/browser/loader/resource_dispatcher_host_browsertest.cc (right): https://chromiumcodereview.appspot.com/1310743003/diff/520001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:583: ResourceDispatcherHost::Get()->SetDelegate(&delegate); On 2015/10/08 22:25:15, mmenke wrote: > On 2015/10/08 22:20:40, megjablon wrote: > > On 2015/10/08 19:06:09, mmenke wrote: > > > Hrm...This doesn't seem threadsafe - the RDH lives on the IO thread, we're > on > > > the IO thread. We should post a task to do this. > > > > I'm not sure I follow. Why do we need to post a task to the IO thread if we're > > on the IO thread? All the other browser tests simply set a delegate in this > > manner and then some of them set it to a nullptr at the end of the test, which > > I've added. > > Sorry, that was a typo - we're on the UI thread, which is the problem. Is this feasible? I'm just getting test failed with no useful output when I do it this way. Also, why do none of the other test ResourceDispatcherHostDelegates do this?
https://chromiumcodereview.appspot.com/1310743003/diff/520001/content/browser... File content/browser/loader/resource_dispatcher_host_browsertest.cc (right): https://chromiumcodereview.appspot.com/1310743003/diff/520001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:583: ResourceDispatcherHost::Get()->SetDelegate(&delegate); On 2015/10/08 23:39:07, megjablon wrote: > On 2015/10/08 22:25:15, mmenke wrote: > > On 2015/10/08 22:20:40, megjablon wrote: > > > On 2015/10/08 19:06:09, mmenke wrote: > > > > Hrm...This doesn't seem threadsafe - the RDH lives on the IO thread, we're > > on > > > > the IO thread. We should post a task to do this. > > > > > > I'm not sure I follow. Why do we need to post a task to the IO thread if > we're > > > on the IO thread? All the other browser tests simply set a delegate in this > > > manner and then some of them set it to a nullptr at the end of the test, > which > > > I've added. > > > > Sorry, that was a typo - we're on the UI thread, which is the problem. > > Is this feasible? I'm just getting test failed with no useful output when I do > it this way. Also, why do none of the other test ResourceDispatcherHostDelegates > do this? Tests that do that are just crossing their fingers and hoping the RDH isn't doing anything when there are no navigations pending, which seems rather problematic. Tests that do that also tend to delete the Delegate long before the RDH, which also seems really bad. If you upload the code that's giving you the problem, I'll take a look at it.
On 2015/10/09 15:31:56, mmenke wrote: > https://chromiumcodereview.appspot.com/1310743003/diff/520001/content/browser... > File content/browser/loader/resource_dispatcher_host_browsertest.cc (right): > > https://chromiumcodereview.appspot.com/1310743003/diff/520001/content/browser... > content/browser/loader/resource_dispatcher_host_browsertest.cc:583: > ResourceDispatcherHost::Get()->SetDelegate(&delegate); > On 2015/10/08 23:39:07, megjablon wrote: > > On 2015/10/08 22:25:15, mmenke wrote: > > > On 2015/10/08 22:20:40, megjablon wrote: > > > > On 2015/10/08 19:06:09, mmenke wrote: > > > > > Hrm...This doesn't seem threadsafe - the RDH lives on the IO thread, > we're > > > on > > > > > the IO thread. We should post a task to do this. > > > > > > > > I'm not sure I follow. Why do we need to post a task to the IO thread if > > we're > > > > on the IO thread? All the other browser tests simply set a delegate in > this > > > > manner and then some of them set it to a nullptr at the end of the test, > > which > > > > I've added. > > > > > > Sorry, that was a typo - we're on the UI thread, which is the problem. > > > > Is this feasible? I'm just getting test failed with no useful output when I do > > it this way. Also, why do none of the other test > ResourceDispatcherHostDelegates > > do this? > > Tests that do that are just crossing their fingers and hoping the RDH isn't > doing anything when there are no navigations pending, which seems rather > problematic. Tests that do that also tend to delete the Delegate long before > the RDH, which also seems really bad. > > If you upload the code that's giving you the problem, I'll take a look at it. I've uploaded the code. I'm getting the following error: ERROR:kill_posix.cc(79) Unable to terminate process group 29589: No such process
On 2015/10/09 19:53:25, megjablon wrote: > On 2015/10/09 15:31:56, mmenke wrote: > > > https://chromiumcodereview.appspot.com/1310743003/diff/520001/content/browser... > > File content/browser/loader/resource_dispatcher_host_browsertest.cc (right): > > > > > https://chromiumcodereview.appspot.com/1310743003/diff/520001/content/browser... > > content/browser/loader/resource_dispatcher_host_browsertest.cc:583: > > ResourceDispatcherHost::Get()->SetDelegate(&delegate); > > On 2015/10/08 23:39:07, megjablon wrote: > > > On 2015/10/08 22:25:15, mmenke wrote: > > > > On 2015/10/08 22:20:40, megjablon wrote: > > > > > On 2015/10/08 19:06:09, mmenke wrote: > > > > > > Hrm...This doesn't seem threadsafe - the RDH lives on the IO thread, > > we're > > > > on > > > > > > the IO thread. We should post a task to do this. > > > > > > > > > > I'm not sure I follow. Why do we need to post a task to the IO thread if > > > we're > > > > > on the IO thread? All the other browser tests simply set a delegate in > > this > > > > > manner and then some of them set it to a nullptr at the end of the test, > > > which > > > > > I've added. > > > > > > > > Sorry, that was a typo - we're on the UI thread, which is the problem. > > > > > > Is this feasible? I'm just getting test failed with no useful output when I > do > > > it this way. Also, why do none of the other test > > ResourceDispatcherHostDelegates > > > do this? > > > > Tests that do that are just crossing their fingers and hoping the RDH isn't > > doing anything when there are no navigations pending, which seems rather > > problematic. Tests that do that also tend to delete the Delegate long before > > the RDH, which also seems really bad. > > > > If you upload the code that's giving you the problem, I'll take a look at it. > > I've uploaded the code. I'm getting the following error: > > ERROR:kill_posix.cc(79) Unable to terminate process group 29589: No such process You can't make reference counted variables on the stack - you're basically calling delete on a variable on the stack, which is bad. Remove reference counting and use base::Unretained. And get rid of all those FRIEND_TEST_ALL_PREFIXES, and make the destructor public again.
On 2015/10/09 19:56:59, mmenke wrote: > On 2015/10/09 19:53:25, megjablon wrote: > > On 2015/10/09 15:31:56, mmenke wrote: > > > > > > https://chromiumcodereview.appspot.com/1310743003/diff/520001/content/browser... > > > File content/browser/loader/resource_dispatcher_host_browsertest.cc (right): > > > > > > > > > https://chromiumcodereview.appspot.com/1310743003/diff/520001/content/browser... > > > content/browser/loader/resource_dispatcher_host_browsertest.cc:583: > > > ResourceDispatcherHost::Get()->SetDelegate(&delegate); > > > On 2015/10/08 23:39:07, megjablon wrote: > > > > On 2015/10/08 22:25:15, mmenke wrote: > > > > > On 2015/10/08 22:20:40, megjablon wrote: > > > > > > On 2015/10/08 19:06:09, mmenke wrote: > > > > > > > Hrm...This doesn't seem threadsafe - the RDH lives on the IO thread, > > > we're > > > > > on > > > > > > > the IO thread. We should post a task to do this. > > > > > > > > > > > > I'm not sure I follow. Why do we need to post a task to the IO thread > if > > > > we're > > > > > > on the IO thread? All the other browser tests simply set a delegate in > > > this > > > > > > manner and then some of them set it to a nullptr at the end of the > test, > > > > which > > > > > > I've added. > > > > > > > > > > Sorry, that was a typo - we're on the UI thread, which is the problem. > > > > > > > > Is this feasible? I'm just getting test failed with no useful output when > I > > do > > > > it this way. Also, why do none of the other test > > > ResourceDispatcherHostDelegates > > > > do this? > > > > > > Tests that do that are just crossing their fingers and hoping the RDH isn't > > > doing anything when there are no navigations pending, which seems rather > > > problematic. Tests that do that also tend to delete the Delegate long > before > > > the RDH, which also seems really bad. > > > > > > If you upload the code that's giving you the problem, I'll take a look at > it. > > > > I've uploaded the code. I'm getting the following error: > > > > ERROR:kill_posix.cc(79) Unable to terminate process group 29589: No such > process > > You can't make reference counted variables on the stack - you're basically > calling delete on a variable on the stack, which is bad. Remove reference > counting and use base::Unretained. And get rid of all those > FRIEND_TEST_ALL_PREFIXES, and make the destructor public again. Done! Fix uploaded. Thanks.
megjablon@chromium.org changed reviewers: + thestig@chromium.org
thestig: chrome/*
On 2015/10/09 20:05:41, megjablon wrote: > On 2015/10/09 19:56:59, mmenke wrote: > > On 2015/10/09 19:53:25, megjablon wrote: > > > On 2015/10/09 15:31:56, mmenke wrote: > > > > > > > > > > https://chromiumcodereview.appspot.com/1310743003/diff/520001/content/browser... > > > > File content/browser/loader/resource_dispatcher_host_browsertest.cc > (right): > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/1310743003/diff/520001/content/browser... > > > > content/browser/loader/resource_dispatcher_host_browsertest.cc:583: > > > > ResourceDispatcherHost::Get()->SetDelegate(&delegate); > > > > On 2015/10/08 23:39:07, megjablon wrote: > > > > > On 2015/10/08 22:25:15, mmenke wrote: > > > > > > On 2015/10/08 22:20:40, megjablon wrote: > > > > > > > On 2015/10/08 19:06:09, mmenke wrote: > > > > > > > > Hrm...This doesn't seem threadsafe - the RDH lives on the IO > thread, > > > > we're > > > > > > on > > > > > > > > the IO thread. We should post a task to do this. > > > > > > > > > > > > > > I'm not sure I follow. Why do we need to post a task to the IO > thread > > if > > > > > we're > > > > > > > on the IO thread? All the other browser tests simply set a delegate > in > > > > this > > > > > > > manner and then some of them set it to a nullptr at the end of the > > test, > > > > > which > > > > > > > I've added. > > > > > > > > > > > > Sorry, that was a typo - we're on the UI thread, which is the problem. > > > > > > > > > > Is this feasible? I'm just getting test failed with no useful output > when > > I > > > do > > > > > it this way. Also, why do none of the other test > > > > ResourceDispatcherHostDelegates > > > > > do this? > > > > > > > > Tests that do that are just crossing their fingers and hoping the RDH > isn't > > > > doing anything when there are no navigations pending, which seems rather > > > > problematic. Tests that do that also tend to delete the Delegate long > > before > > > > the RDH, which also seems really bad. > > > > > > > > If you upload the code that's giving you the problem, I'll take a look at > > it. > > > > > > I've uploaded the code. I'm getting the following error: > > > > > > ERROR:kill_posix.cc(79) Unable to terminate process group 29589: No such > > process > > > > You can't make reference counted variables on the stack - you're basically > > calling delete on a variable on the stack, which is bad. Remove reference > > counting and use base::Unretained. And get rid of all those > > FRIEND_TEST_ALL_PREFIXES, and make the destructor public again. > > Done! Fix uploaded. Thanks. Sorry, forgot this was in my queue! I'll try to get to it today, if not, it's top of my list tomorrow.
Want to do a full pass tomorrow. https://codereview.chromium.org/1310743003/diff/620001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_browsertest.cc (right): https://codereview.chromium.org/1310743003/diff/620001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:531: class LoFiModeResourceDispatcherHostDelegate This needs to be in an anonymous namespace. https://codereview.chromium.org/1310743003/diff/620001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:531: class LoFiModeResourceDispatcherHostDelegate Should document this class. https://codereview.chromium.org/1310743003/diff/620001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:536: const GURL& iframe_url) Should include gurl.h (Yea, should have already been included in this file) https://codereview.chromium.org/1310743003/diff/620001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:544: void RequestBeginning(net::URLRequest* request, Should include net/url_request/url_request.h https://codereview.chromium.org/1310743003/diff/620001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:552: mainframe_url_seen_ = true; nit: main_frame_... https://codereview.chromium.org/1310743003/diff/620001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:553: } else if (request->url() == subresource_url_) { EXPECT_TRUE(mainframe_url_seen);? https://codereview.chromium.org/1310743003/diff/620001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:556: } else if (request->url() == iframe_url_) { EXPECT_TRUE(mainframe_url_seen);? https://codereview.chromium.org/1310743003/diff/620001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:592: void CheckResourcesRequested() { const https://codereview.chromium.org/1310743003/diff/620001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:596: if (!iframe_url_.is_empty()) Don't think we need to handle empty urls (See comment further down about not testing after the second nav in the only test that does that) https://codereview.chromium.org/1310743003/diff/620001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:600: bool should_enable_lofi_mode_called() { const https://codereview.chromium.org/1310743003/diff/620001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:607: GURL iframe_url_; Suggest a blank line between the URLs and the bools. https://codereview.chromium.org/1310743003/diff/620001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:612: bool should_enable_lofi_mode_called_; You need to initialize all of the bools in the constructor. https://codereview.chromium.org/1310743003/diff/620001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:612: bool should_enable_lofi_mode_called_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1310743003/diff/620001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:619: LoFiModeResourceDispatcherHostDelegate delegate( Should making a new test fixture, and installing the delegate in SetUpOnMainThread. That ensures it outlives the RDH, which it currently doesn't do. Makes this code much more robust against regressions. Can also set up the embedded test server in the text fixture, in SetUp (Make sure to call the base classes methods in both cases) https://codereview.chromium.org/1310743003/diff/620001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:628: base::Unretained(&delegate))); include bind.h https://codereview.chromium.org/1310743003/diff/620001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:654: delegate.Reset(false); not needed. https://codereview.chromium.org/1310743003/diff/620001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:658: delegate.CheckResourcesRequested(); Not really safe to call this on the UI thread - should either use PostTaskAndReply with a RunLoop, or a lock to protect access to everything (bools and the GURLs) on both threads. https://codereview.chromium.org/1310743003/diff/620001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:677: delegate.Reset(false); Not needed. https://codereview.chromium.org/1310743003/diff/620001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:684: delegate.Reset(true); Again, should be called on the other thread, or use locks. Not going to annotate all cases of this. https://codereview.chromium.org/1310743003/diff/620001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:715: GURL()); Maybe just navigate to about:blank, so we don't have to care about this? Can also remove the SetUrls method, and make the GURLs in the delegate const. https://codereview.chromium.org/1310743003/diff/620001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:743: delegate.CheckResourcesRequested(); I don't think this last navigation is needed. If back works, I'm willing to take it on faith that forward does.
chrome/ lgtm
https://chromiumcodereview.appspot.com/1310743003/diff/620001/content/browser... File content/browser/loader/resource_dispatcher_host_browsertest.cc (right): https://chromiumcodereview.appspot.com/1310743003/diff/620001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:531: class LoFiModeResourceDispatcherHostDelegate On 2015/10/12 20:19:29, mmenke wrote: > This needs to be in an anonymous namespace. Done. https://chromiumcodereview.appspot.com/1310743003/diff/620001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:531: class LoFiModeResourceDispatcherHostDelegate On 2015/10/12 20:19:28, mmenke wrote: > Should document this class. Done. https://chromiumcodereview.appspot.com/1310743003/diff/620001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:536: const GURL& iframe_url) On 2015/10/12 20:19:29, mmenke wrote: > Should include gurl.h (Yea, should have already been included in this file) Done. https://chromiumcodereview.appspot.com/1310743003/diff/620001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:544: void RequestBeginning(net::URLRequest* request, On 2015/10/12 20:19:29, mmenke wrote: > Should include net/url_request/url_request.h Done. https://chromiumcodereview.appspot.com/1310743003/diff/620001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:552: mainframe_url_seen_ = true; On 2015/10/12 20:19:28, mmenke wrote: > nit: main_frame_... Done. https://chromiumcodereview.appspot.com/1310743003/diff/620001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:553: } else if (request->url() == subresource_url_) { On 2015/10/12 20:19:29, mmenke wrote: > EXPECT_TRUE(mainframe_url_seen);? Done. https://chromiumcodereview.appspot.com/1310743003/diff/620001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:556: } else if (request->url() == iframe_url_) { On 2015/10/12 20:19:28, mmenke wrote: > EXPECT_TRUE(mainframe_url_seen);? Done. https://chromiumcodereview.appspot.com/1310743003/diff/620001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:592: void CheckResourcesRequested() { On 2015/10/12 20:19:29, mmenke wrote: > const This doesn't work with binding, do you know why? https://chromiumcodereview.appspot.com/1310743003/diff/620001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:596: if (!iframe_url_.is_empty()) On 2015/10/12 20:19:29, mmenke wrote: > Don't think we need to handle empty urls (See comment further down about not > testing after the second nav in the only test that does that) Done. https://chromiumcodereview.appspot.com/1310743003/diff/620001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:600: bool should_enable_lofi_mode_called() { On 2015/10/12 20:19:29, mmenke wrote: > const Done. https://chromiumcodereview.appspot.com/1310743003/diff/620001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:607: GURL iframe_url_; On 2015/10/12 20:19:28, mmenke wrote: > Suggest a blank line between the URLs and the bools. Done. https://chromiumcodereview.appspot.com/1310743003/diff/620001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:612: bool should_enable_lofi_mode_called_; On 2015/10/12 20:19:29, mmenke wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://chromiumcodereview.appspot.com/1310743003/diff/620001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:612: bool should_enable_lofi_mode_called_; On 2015/10/12 20:19:28, mmenke wrote: > You need to initialize all of the bools in the constructor. Done. https://chromiumcodereview.appspot.com/1310743003/diff/620001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:619: LoFiModeResourceDispatcherHostDelegate delegate( On 2015/10/12 20:19:29, mmenke wrote: > Should making a new test fixture, and installing the delegate in > SetUpOnMainThread. That ensures it outlives the RDH, which it currently doesn't > do. Makes this code much more robust against regressions. Can also set up the > embedded test server in the text fixture, in SetUp (Make sure to call the base > classes methods in both cases) Done. https://chromiumcodereview.appspot.com/1310743003/diff/620001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:628: base::Unretained(&delegate))); On 2015/10/12 20:19:29, mmenke wrote: > include bind.h Done. https://chromiumcodereview.appspot.com/1310743003/diff/620001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:654: delegate.Reset(false); On 2015/10/12 20:19:29, mmenke wrote: > not needed. Done. https://chromiumcodereview.appspot.com/1310743003/diff/620001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:658: delegate.CheckResourcesRequested(); On 2015/10/12 20:19:28, mmenke wrote: > Not really safe to call this on the UI thread - should either use > PostTaskAndReply with a RunLoop, or a lock to protect access to everything > (bools and the GURLs) on both threads. Done. https://chromiumcodereview.appspot.com/1310743003/diff/620001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:677: delegate.Reset(false); On 2015/10/12 20:19:29, mmenke wrote: > Not needed. Done. https://chromiumcodereview.appspot.com/1310743003/diff/620001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:684: delegate.Reset(true); On 2015/10/12 20:19:29, mmenke wrote: > Again, should be called on the other thread, or use locks. Not going to > annotate all cases of this. Done. https://chromiumcodereview.appspot.com/1310743003/diff/620001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:715: GURL()); On 2015/10/12 20:19:29, mmenke wrote: > Maybe just navigate to about:blank, so we don't have to care about this? Can > also remove the SetUrls method, and make the GURLs in the delegate const. Done. https://chromiumcodereview.appspot.com/1310743003/diff/620001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:743: delegate.CheckResourcesRequested(); On 2015/10/12 20:19:29, mmenke wrote: > I don't think this last navigation is needed. If back works, I'm willing to > take it on faith that forward does. Done.
LGTM, modulo comments. https://codereview.chromium.org/1310743003/diff/640001/components/data_reduct... File components/data_reduction_proxy/content/browser/content_lofi_decider.cc (right): https://codereview.chromium.org/1310743003/diff/640001/components/data_reduct... components/data_reduction_proxy/content/browser/content_lofi_decider.cc:22: } nit: Remove braces https://codereview.chromium.org/1310743003/diff/640001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_browsertest.cc (right): https://codereview.chromium.org/1310743003/diff/640001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:554: should_enable_lofi_mode_called_(false) {} child classes should have explicit override destructors. https://codereview.chromium.org/1310743003/diff/640001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:561: ScopedVector<ResourceThrottle>* throttles) override { Should have DCHECK_CURRENTLY_ON(BrowserThread::IO); in all these methods, except the constructor. https://codereview.chromium.org/1310743003/diff/640001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:564: return; Should probably just ignore everything that doesn't match one of the three expected URLs, and remove ignore_url https://codereview.chromium.org/1310743003/diff/640001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:625: class LoFiResourceDispatcherHostBrowserTest : public ContentBrowserTest { Should have a destructor with override https://codereview.chromium.org/1310743003/diff/640001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:652: run_loop.Run(); The PostTask SetUpOnMainThread() doesn't need to wait for the reply, and neither does this. Should be consistent about whether you wait for the reply when you don't need it. I suggest either not waiting for it when you don't have to, or just make a method that always waits, and use that everywhere. e.g. void PostTaskToIOThreadAndWait(const base::Callback& callback) { // Do the RunLoop + PostTaskAndReply here. } https://codereview.chromium.org/1310743003/diff/640001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:673: // Navigate with ShouldEnableLoFiMode true. nit: Maybe "Navigate with ShouldEnableLoFiMode returning true." (For all of these). Or rephrase in some other way that makes it clearer ShouldEnableLoFiModeOn is a method, not a value. https://codereview.chromium.org/1310743003/diff/640001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:724: Reset(true); May want to go from true to false here, just to do the opposite of the above test. https://codereview.chromium.org/1310743003/diff/640001/content/public/common/... File content/public/common/resource_response_info.h (right): https://codereview.chromium.org/1310743003/diff/640001/content/public/common/... content/public/common/resource_response_info.h:134: bool is_using_lofi; BUG: You need to initialize this to false in ResourceResponseInfo() https://codereview.chromium.org/1310743003/diff/640001/content/public/rendere... File content/public/renderer/render_frame.h (right): https://codereview.chromium.org/1310743003/diff/640001/content/public/rendere... content/public/renderer/render_frame.h:162: virtual bool IsUsingLoFi() = 0; const?
https://chromiumcodereview.appspot.com/1310743003/diff/640001/components/data... File components/data_reduction_proxy/content/browser/content_lofi_decider.cc (right): https://chromiumcodereview.appspot.com/1310743003/diff/640001/components/data... components/data_reduction_proxy/content/browser/content_lofi_decider.cc:22: } On 2015/10/13 15:42:49, mmenke wrote: > nit: Remove braces Done. https://chromiumcodereview.appspot.com/1310743003/diff/640001/content/browser... File content/browser/loader/resource_dispatcher_host_browsertest.cc (right): https://chromiumcodereview.appspot.com/1310743003/diff/640001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:554: should_enable_lofi_mode_called_(false) {} On 2015/10/13 15:42:49, mmenke wrote: > child classes should have explicit override destructors. Done. https://chromiumcodereview.appspot.com/1310743003/diff/640001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:561: ScopedVector<ResourceThrottle>* throttles) override { On 2015/10/13 15:42:49, mmenke wrote: > Should have DCHECK_CURRENTLY_ON(BrowserThread::IO); in all these methods, except > the constructor. Done. https://chromiumcodereview.appspot.com/1310743003/diff/640001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:564: return; On 2015/10/13 15:42:49, mmenke wrote: > Should probably just ignore everything that doesn't match one of the three > expected URLs, and remove ignore_url Done. https://chromiumcodereview.appspot.com/1310743003/diff/640001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:625: class LoFiResourceDispatcherHostBrowserTest : public ContentBrowserTest { On 2015/10/13 15:42:49, mmenke wrote: > Should have a destructor with override Done. https://chromiumcodereview.appspot.com/1310743003/diff/640001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:652: run_loop.Run(); On 2015/10/13 15:42:49, mmenke wrote: > The PostTask SetUpOnMainThread() doesn't need to wait for the reply, and neither > does this. Should be consistent about whether you wait for the reply when you > don't need it. > > I suggest either not waiting for it when you don't have to, or just make a > method that always waits, and use that everywhere. > > e.g. > > void PostTaskToIOThreadAndWait(const base::Callback& callback) { > // Do the RunLoop + PostTaskAndReply here. > } Done. https://chromiumcodereview.appspot.com/1310743003/diff/640001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:673: // Navigate with ShouldEnableLoFiMode true. On 2015/10/13 15:42:49, mmenke wrote: > nit: Maybe "Navigate with ShouldEnableLoFiMode returning true." (For all of > these). Or rephrase in some other way that makes it clearer > ShouldEnableLoFiModeOn is a method, not a value. Done. https://chromiumcodereview.appspot.com/1310743003/diff/640001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:724: Reset(true); On 2015/10/13 15:42:49, mmenke wrote: > May want to go from true to false here, just to do the opposite of the above > test. Done. https://chromiumcodereview.appspot.com/1310743003/diff/640001/content/public/... File content/public/common/resource_response_info.h (right): https://chromiumcodereview.appspot.com/1310743003/diff/640001/content/public/... content/public/common/resource_response_info.h:134: bool is_using_lofi; On 2015/10/13 15:42:49, mmenke wrote: > BUG: You need to initialize this to false in ResourceResponseInfo() Done. https://chromiumcodereview.appspot.com/1310743003/diff/640001/content/public/... File content/public/renderer/render_frame.h (right): https://chromiumcodereview.appspot.com/1310743003/diff/640001/content/public/... content/public/renderer/render_frame.h:162: virtual bool IsUsingLoFi() = 0; On 2015/10/13 15:42:49, mmenke wrote: > const? Done.
I think it is mostly ready from my perspective, once this round of comments is addressed. https://codereview.chromium.org/1310743003/diff/660001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1310743003/diff/660001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1740: LOFI_UNSPECIFIED); This should always be specified as OFF. Why would we ever want to navigate to an interstitial page and load a LoFi version of it?! https://codereview.chromium.org/1310743003/diff/660001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_browsertest.cc (right): https://codereview.chromium.org/1310743003/diff/660001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:672: IN_PROC_BROWSER_TEST_F(LoFiResourceDispatcherHostBrowserTest, Please add a comment above each test describing what case it is trying to test. https://codereview.chromium.org/1310743003/diff/660001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:676: NavigateToURLBlockUntilNavigationsComplete( Better alternative is EXPECT_TRUE(NavigateToURL(...)); https://codereview.chromium.org/1310743003/diff/660001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:731: WaitForLoadStop(shell()->web_contents()); What is this waiting for? At this point there shouldn't be any loads that are ongoing. https://codereview.chromium.org/1310743003/diff/660001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1310743003/diff/660001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1333: } Since this identical code is used in two places in this file, why not pull it into a helper function in the anonymous namespace and call it from both places? You can even call inlined in the constructor belo, since it will just return a boolean and you can save yourself declaring a local variable. https://codereview.chromium.org/1310743003/diff/660001/content/public/browser... File content/public/browser/resource_dispatcher_host_delegate.h (right): https://codereview.chromium.org/1310743003/diff/660001/content/public/browser... content/public/browser/resource_dispatcher_host_delegate.h:128: // is only called for main frame requests with an unspecified Lo-Fi value. nit: "main frame navigation requests", since we don't call this for subresource requests, right?
https://codereview.chromium.org/1310743003/diff/660001/content/public/browser... File content/public/browser/resource_dispatcher_host_delegate.h (right): https://codereview.chromium.org/1310743003/diff/660001/content/public/browser... content/public/browser/resource_dispatcher_host_delegate.h:128: // is only called for main frame requests with an unspecified Lo-Fi value. On 2015/10/15 16:54:43, nasko (slow to review) wrote: > nit: "main frame navigation requests", since we don't call this for subresource > requests, right? Effectively we don't - subresources inherit values from main frame....Main frame requests may start out with UNSPECIFIED, but once this assigns a value, then it's passed back to the renderer, and used for all subresource. That having been said...There could certainly be some path where subresources are requested before it's been set for the main frame, so I agree that more weasily language is probably warranted.
On 2015/10/15 17:04:28, mmenke wrote: > https://codereview.chromium.org/1310743003/diff/660001/content/public/browser... > File content/public/browser/resource_dispatcher_host_delegate.h (right): > > https://codereview.chromium.org/1310743003/diff/660001/content/public/browser... > content/public/browser/resource_dispatcher_host_delegate.h:128: // is only > called for main frame requests with an unspecified Lo-Fi value. > On 2015/10/15 16:54:43, nasko (slow to review) wrote: > > nit: "main frame navigation requests", since we don't call this for > subresource > > requests, right? > > Effectively we don't - subresources inherit values from main frame....Main frame > requests may start out with UNSPECIFIED, but once this assigns a value, then > it's passed back to the renderer, and used for all subresource. That having > been said...There could certainly be some path where subresources are requested > before it's been set for the main frame, so I agree that more weasily language > is probably warranted. How would we request a subresource before committing the main document? And at commit time we note the flag and stamp it on all subsequent requests.
On 2015/10/15 17:11:47, nasko (slow to review) wrote: > On 2015/10/15 17:04:28, mmenke wrote: > > > https://codereview.chromium.org/1310743003/diff/660001/content/public/browser... > > File content/public/browser/resource_dispatcher_host_delegate.h (right): > > > > > https://codereview.chromium.org/1310743003/diff/660001/content/public/browser... > > content/public/browser/resource_dispatcher_host_delegate.h:128: // is only > > called for main frame requests with an unspecified Lo-Fi value. > > On 2015/10/15 16:54:43, nasko (slow to review) wrote: > > > nit: "main frame navigation requests", since we don't call this for > > subresource > > > requests, right? > > > > Effectively we don't - subresources inherit values from main frame....Main > frame > > requests may start out with UNSPECIFIED, but once this assigns a value, then > > it's passed back to the renderer, and used for all subresource. That having > > been said...There could certainly be some path where subresources are > requested > > before it's been set for the main frame, so I agree that more weasily language > > is probably warranted. > > How would we request a subresource before committing the main document? And at > commit time we note the flag and stamp it on all subsequent requests. The page itself certainly couldn't. Could any other Chrome module living in the renderer make a request that early? I couldn't tell you. What about plugin processes? What about ServiceWorkers?
On 2015/10/15 17:19:41, mmenke wrote: > On 2015/10/15 17:11:47, nasko (slow to review) wrote: > > On 2015/10/15 17:04:28, mmenke wrote: > > > > > > https://codereview.chromium.org/1310743003/diff/660001/content/public/browser... > > > File content/public/browser/resource_dispatcher_host_delegate.h (right): > > > > > > > > > https://codereview.chromium.org/1310743003/diff/660001/content/public/browser... > > > content/public/browser/resource_dispatcher_host_delegate.h:128: // is only > > > called for main frame requests with an unspecified Lo-Fi value. > > > On 2015/10/15 16:54:43, nasko (slow to review) wrote: > > > > nit: "main frame navigation requests", since we don't call this for > > > subresource > > > > requests, right? > > > > > > Effectively we don't - subresources inherit values from main frame....Main > > frame > > > requests may start out with UNSPECIFIED, but once this assigns a value, then > > > it's passed back to the renderer, and used for all subresource. That having > > > been said...There could certainly be some path where subresources are > > requested > > > before it's been set for the main frame, so I agree that more weasily > language > > > is probably warranted. > > > > How would we request a subresource before committing the main document? And at > > commit time we note the flag and stamp it on all subsequent requests. > > The page itself certainly couldn't. Could any other Chrome module living in the > renderer make a request that early? I couldn't tell you. What about plugin > processes? What about ServiceWorkers? (For plugin processes, the issue isn't what happens before "commit", but if the requests are coming directly from the plugin process, they may not get the flag. For service workers, they can make requests unassociated with any navigation. And even when they make requests associated with a navigate, do they copy these flags around? I'd guess not.
On 2015/10/15 17:22:28, mmenke wrote: > On 2015/10/15 17:19:41, mmenke wrote: > > On 2015/10/15 17:11:47, nasko (slow to review) wrote: > > > On 2015/10/15 17:04:28, mmenke wrote: > > > > > > > > > > https://codereview.chromium.org/1310743003/diff/660001/content/public/browser... > > > > File content/public/browser/resource_dispatcher_host_delegate.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1310743003/diff/660001/content/public/browser... > > > > content/public/browser/resource_dispatcher_host_delegate.h:128: // is only > > > > called for main frame requests with an unspecified Lo-Fi value. > > > > On 2015/10/15 16:54:43, nasko (slow to review) wrote: > > > > > nit: "main frame navigation requests", since we don't call this for > > > > subresource > > > > > requests, right? > > > > > > > > Effectively we don't - subresources inherit values from main frame....Main > > > frame > > > > requests may start out with UNSPECIFIED, but once this assigns a value, > then > > > > it's passed back to the renderer, and used for all subresource. That > having > > > > been said...There could certainly be some path where subresources are > > > requested > > > > before it's been set for the main frame, so I agree that more weasily > > language > > > > is probably warranted. > > > > > > How would we request a subresource before committing the main document? And > at > > > commit time we note the flag and stamp it on all subsequent requests. > > > > The page itself certainly couldn't. Could any other Chrome module living in > the > > renderer make a request that early? I couldn't tell you. What about plugin > > processes? What about ServiceWorkers? > > (For plugin processes, the issue isn't what happens before "commit", but if the > requests are coming directly from the plugin process, they may not get the flag. > For service workers, they can make requests unassociated with any navigation. > And even when they make requests associated with a navigate, do they copy these > flags around? I'd guess not. I don't see any way this CL interacts with ServiceWorker. Maybe a good idea to clarify how it is supposed to work with LoFi. bengr@, can you shed some light?
On 2015/10/15 17:42:59, nasko (slow to review) wrote: > On 2015/10/15 17:22:28, mmenke wrote: > > On 2015/10/15 17:19:41, mmenke wrote: > > > On 2015/10/15 17:11:47, nasko (slow to review) wrote: > > > > On 2015/10/15 17:04:28, mmenke wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1310743003/diff/660001/content/public/browser... > > > > > File content/public/browser/resource_dispatcher_host_delegate.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1310743003/diff/660001/content/public/browser... > > > > > content/public/browser/resource_dispatcher_host_delegate.h:128: // is > only > > > > > called for main frame requests with an unspecified Lo-Fi value. > > > > > On 2015/10/15 16:54:43, nasko (slow to review) wrote: > > > > > > nit: "main frame navigation requests", since we don't call this for > > > > > subresource > > > > > > requests, right? > > > > > > > > > > Effectively we don't - subresources inherit values from main > frame....Main > > > > frame > > > > > requests may start out with UNSPECIFIED, but once this assigns a value, > > then > > > > > it's passed back to the renderer, and used for all subresource. That > > having > > > > > been said...There could certainly be some path where subresources are > > > > requested > > > > > before it's been set for the main frame, so I agree that more weasily > > > language > > > > > is probably warranted. > > > > > > > > How would we request a subresource before committing the main document? > And > > at > > > > commit time we note the flag and stamp it on all subsequent requests. > > > > > > The page itself certainly couldn't. Could any other Chrome module living in > > the > > > renderer make a request that early? I couldn't tell you. What about plugin > > > processes? What about ServiceWorkers? > > > > (For plugin processes, the issue isn't what happens before "commit", but if > the > > requests are coming directly from the plugin process, they may not get the > flag. > > For service workers, they can make requests unassociated with any navigation. > > > And even when they make requests associated with a navigate, do they copy > these > > flags around? I'd guess not. > > I don't see any way this CL interacts with ServiceWorker. Maybe a good idea to > clarify how it is supposed to work with LoFi. bengr@, can you shed some light? From my understanding, ServiceWorker requests will have Lo-Fi unspecified and get the queried Lo-Fi value. This aligns with our fetching when the network is slow and caching policy for Lo-Fi, being that we leave them in the cache and evict them the same as any non-Lo-Fi resource.
https://chromiumcodereview.appspot.com/1310743003/diff/660001/content/browser... File content/browser/frame_host/render_frame_host_impl.cc (right): https://chromiumcodereview.appspot.com/1310743003/diff/660001/content/browser... content/browser/frame_host/render_frame_host_impl.cc:1740: LOFI_UNSPECIFIED); On 2015/10/15 16:54:43, nasko (slow to review) wrote: > This should always be specified as OFF. Why would we ever want to navigate to an > interstitial page and load a LoFi version of it?! Done. https://chromiumcodereview.appspot.com/1310743003/diff/660001/content/browser... File content/browser/loader/resource_dispatcher_host_browsertest.cc (right): https://chromiumcodereview.appspot.com/1310743003/diff/660001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:672: IN_PROC_BROWSER_TEST_F(LoFiResourceDispatcherHostBrowserTest, On 2015/10/15 16:54:43, nasko (slow to review) wrote: > Please add a comment above each test describing what case it is trying to test. Done. https://chromiumcodereview.appspot.com/1310743003/diff/660001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:676: NavigateToURLBlockUntilNavigationsComplete( On 2015/10/15 16:54:43, nasko (slow to review) wrote: > Better alternative is EXPECT_TRUE(NavigateToURL(...)); Done. https://chromiumcodereview.appspot.com/1310743003/diff/660001/content/browser... content/browser/loader/resource_dispatcher_host_browsertest.cc:731: WaitForLoadStop(shell()->web_contents()); On 2015/10/15 16:54:43, nasko (slow to review) wrote: > What is this waiting for? At this point there shouldn't be any loads that are > ongoing. Done. https://chromiumcodereview.appspot.com/1310743003/diff/660001/content/browser... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://chromiumcodereview.appspot.com/1310743003/diff/660001/content/browser... content/browser/loader/resource_dispatcher_host_impl.cc:1333: } On 2015/10/15 16:54:43, nasko (slow to review) wrote: > Since this identical code is used in two places in this file, why not pull it > into a helper function in the anonymous namespace and call it from both places? > You can even call inlined in the constructor belo, since it will just return a > boolean and you can save yourself declaring a local variable. Done. https://chromiumcodereview.appspot.com/1310743003/diff/660001/content/public/... File content/public/browser/resource_dispatcher_host_delegate.h (right): https://chromiumcodereview.appspot.com/1310743003/diff/660001/content/public/... content/public/browser/resource_dispatcher_host_delegate.h:128: // is only called for main frame requests with an unspecified Lo-Fi value. On 2015/10/15 17:04:28, mmenke wrote: > On 2015/10/15 16:54:43, nasko (slow to review) wrote: > > nit: "main frame navigation requests", since we don't call this for > subresource > > requests, right? > > Effectively we don't - subresources inherit values from main frame....Main frame > requests may start out with UNSPECIFIED, but once this assigns a value, then > it's passed back to the renderer, and used for all subresource. That having > been said...There could certainly be some path where subresources are requested > before it's been set for the main frame, so I agree that more weasily language > is probably warranted. Changed this to just "called for requests." It will be called for anything that goes through ResourceDispatcherHostImpl::BeginRequest with LOFI_UNSPECIFIED. Paths where the main frame hasn't set the state yet or there isn't a main frame request will also call this.
Looks good. Thanks for hanging in there, Megan. Just a few more comments from me... https://codereview.chromium.org/1310743003/diff/680001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/1310743003/diff/680001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:726: net::URLRequest* url_request, Can this be a const& instead? https://codereview.chromium.org/1310743003/diff/680001/chrome/renderer/page_l... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/1310743003/diff/680001/chrome/renderer/page_l... chrome/renderer/page_load_histograms.cc:913: content::RenderFrame::FromWebFrame(frame); It looks like this can return null, but then you use it in DumpHistograms without checking. https://codereview.chromium.org/1310743003/diff/680001/components/data_reduct... File components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc (right): https://codereview.chromium.org/1310743003/diff/680001/components/data_reduct... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc:21: if (request_info) { I prefer: if (!request_info) return false; return request_info->IsUsingLoFi() ... https://codereview.chromium.org/1310743003/diff/680001/components/data_reduct... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc:22: // The Lo-Fi directive should not be added for users in the Lo-Fi field It isn't the user who is in the group, it is the browser instance. That would help getting around assigning a gender on the next line. :) https://codereview.chromium.org/1310743003/diff/680001/components/data_reduct... File components/data_reduction_proxy/content/browser/content_lofi_decider.cc (right): https://codereview.chromium.org/1310743003/diff/680001/components/data_reduct... components/data_reduction_proxy/content/browser/content_lofi_decider.cc:8: #include "net/url_request/url_request.h" It looks like you can forward declare the URLRequest instead. https://codereview.chromium.org/1310743003/diff/680001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_browsertest.cc (right): https://codereview.chromium.org/1310743003/diff/680001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:537: // Checks whether the given urls are requested, set the Lo-Fi state, and have The comment is a little awkward. How about: Checks whether the given urls are requested, and that IsUsingLofi() returns the appropriate value when the Lo-Fi state is set. https://codereview.chromium.org/1310743003/diff/680001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:672: // Test that navigating with ShouldEnableLoFiMode returning true. navigating does what? https://codereview.chromium.org/1310743003/diff/680001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:682: // Test that navigating with ShouldEnableLoFiMode returning false. that navigating does what?
https://codereview.chromium.org/1310743003/diff/680001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/1310743003/diff/680001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:726: net::URLRequest* url_request, On 2015/10/20 16:30:51, bengr wrote: > Can this be a const& instead? Done. https://codereview.chromium.org/1310743003/diff/680001/chrome/renderer/page_l... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/1310743003/diff/680001/chrome/renderer/page_l... chrome/renderer/page_load_histograms.cc:913: content::RenderFrame::FromWebFrame(frame); On 2015/10/20 16:30:51, bengr wrote: > It looks like this can return null, but then you use it in DumpHistograms > without checking. Done. https://codereview.chromium.org/1310743003/diff/680001/components/data_reduct... File components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc (right): https://codereview.chromium.org/1310743003/diff/680001/components/data_reduct... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc:21: if (request_info) { On 2015/10/20 16:30:51, bengr wrote: > I prefer: > > if (!request_info) > return false; > > return request_info->IsUsingLoFi() ... > Sorry this class should have been removed. This got messed up in the rebasing. https://codereview.chromium.org/1310743003/diff/680001/components/data_reduct... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc:22: // The Lo-Fi directive should not be added for users in the Lo-Fi field On 2015/10/20 16:30:51, bengr wrote: > It isn't the user who is in the group, it is the browser instance. That would > help getting around assigning a gender on the next line. :) Sorry this class should have been removed. This got messed up in the rebasing. https://codereview.chromium.org/1310743003/diff/680001/components/data_reduct... File components/data_reduction_proxy/content/browser/content_lofi_decider.cc (right): https://codereview.chromium.org/1310743003/diff/680001/components/data_reduct... components/data_reduction_proxy/content/browser/content_lofi_decider.cc:8: #include "net/url_request/url_request.h" On 2015/10/20 16:30:51, bengr wrote: > It looks like you can forward declare the URLRequest instead. Done. https://codereview.chromium.org/1310743003/diff/680001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_browsertest.cc (right): https://codereview.chromium.org/1310743003/diff/680001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:537: // Checks whether the given urls are requested, set the Lo-Fi state, and have On 2015/10/20 16:30:51, bengr wrote: > The comment is a little awkward. How about: > > Checks whether the given urls are requested, and that IsUsingLofi() returns the > appropriate value when the Lo-Fi state is set. Done. https://codereview.chromium.org/1310743003/diff/680001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:672: // Test that navigating with ShouldEnableLoFiMode returning true. On 2015/10/20 16:30:51, bengr wrote: > navigating does what? Done. https://codereview.chromium.org/1310743003/diff/680001/content/browser/loader... content/browser/loader/resource_dispatcher_host_browsertest.cc:682: // Test that navigating with ShouldEnableLoFiMode returning false. On 2015/10/20 16:30:51, bengr wrote: > that navigating does what? Done.
LGTM, with one last comment. https://codereview.chromium.org/1310743003/diff/680001/components/data_reduct... File components/data_reduction_proxy/content/browser/content_lofi_decider.cc (right): https://codereview.chromium.org/1310743003/diff/680001/components/data_reduct... components/data_reduction_proxy/content/browser/content_lofi_decider.cc:8: #include "net/url_request/url_request.h" On 2015/10/20 17:46:35, megjablon wrote: > On 2015/10/20 16:30:51, bengr wrote: > > It looks like you can forward declare the URLRequest instead. > > Done. You should forward declare it.
https://codereview.chromium.org/1310743003/diff/680001/components/data_reduct... File components/data_reduction_proxy/content/browser/content_lofi_decider.cc (right): https://codereview.chromium.org/1310743003/diff/680001/components/data_reduct... components/data_reduction_proxy/content/browser/content_lofi_decider.cc:8: #include "net/url_request/url_request.h" On 2015/10/21 20:58:25, bengr wrote: > On 2015/10/20 17:46:35, megjablon wrote: > > On 2015/10/20 16:30:51, bengr wrote: > > > It looks like you can forward declare the URLRequest instead. > > > > Done. > > You should forward declare it. It's already forward declared in the header file.
Patchset #31 (id:740001) has been deleted
Patchset #32 (id:780001) has been deleted
Patchset #31 (id:760001) has been deleted
Patchset #31 (id:800001) has been deleted
The CQ bit was checked by megjablon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org, thestig@chromium.org, mmenke@chromium.org, bengr@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1310743003/#ps860001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1310743003/860001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1310743003/860001
Message was sent while issue was closed.
Committed patchset #33 (id:860001)
Message was sent while issue was closed.
Patchset 33 (id:??) landed as https://crrev.com/d5ac7d52b323d04178bba9cc746c623438d48751 Cr-Commit-Position: refs/heads/master@{#355679}
Message was sent while issue was closed.
Patchset #30 (id:720001) has been deleted
Message was sent while issue was closed.
Patchset #1 (id:1) has been deleted
Message was sent while issue was closed.
Patchset #6 (id:160001) has been deleted
Message was sent while issue was closed.
Patchset #12 (id:320001) has been deleted
Message was sent while issue was closed.
Patchset #5 (id:140001) has been deleted
Message was sent while issue was closed.
Patchset #8 (id:260001) has been deleted
Message was sent while issue was closed.
Patchset #18 (id:580001) has been deleted
Message was sent while issue was closed.
Patchset #24 (id:820001) has been deleted
Message was sent while issue was closed.
Patchset #14 (id:460001) has been deleted
Message was sent while issue was closed.
Patchset #23 (id:840001) has been deleted
Message was sent while issue was closed.
Patchset #17 (id:600001) has been deleted
Message was sent while issue was closed.
Patchset #8 (id:280001) has been deleted
Message was sent while issue was closed.
Patchset #2 (id:60001) has been deleted
Message was sent while issue was closed.
Patchset #19 (id:700001) has been deleted
Message was sent while issue was closed.
Patchset #14 (id:540001) has been deleted |
