Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1363673004:
[DRP] Consistently use LoFi for an entire page (Closed)
Description[DRP] Consistently use LoFi for an entire page
Data Reduction Proxy changes for the cl that stores the LoFi state in
the renderer and uses that state to add LoFi request headers when
appropriate. Creates the method ShouldEnableLoFiMode which the renderer
will use to get the Lo-Fi state for a navigation. The renderer will then
add lofi_on to the request via its ResourceRequestInfo.
DRPNetworkDelegate will add the "q=low" header when lofi_on is true.
The snackbar triggering does not use this state yet, but
still looks at the last mainframe LoFi value.
This cl turns off Lo-Fi until the renderer follow up cl is landed.
BUG=524652
Committed: https://crrev.com/61d87091c812e803e060854d24877c513814661b
Cr-Commit-Position: refs/heads/master@{#355674}
Patch Set 1 #
Total comments: 18
Patch Set 2 : addressing tbansal comments #
Total comments: 10
Patch Set 3 : check config #
Total comments: 5
Patch Set 4 : comments and adding IsLoFiOnViaFlags #
Total comments: 14
Patch Set 5 : nits and test fixes #
Total comments: 18
Patch Set 6 : addressing bengr comments #Patch Set 7 : rebase and fix experiment control header #
Total comments: 49
Patch Set 8 : bengr comments #
Total comments: 14
Patch Set 9 : thestig comment #
Total comments: 6
Patch Set 10 : bengr comments #
Total comments: 8
Patch Set 11 : final comments #Dependent Patchsets: Messages
Total messages: 62 (27 generated)
megjablon@chromium.org changed reviewers: + bengr@chromium.org
tbansal@chromium.org changed reviewers: + tbansal@chromium.org
drive-by review. https://codereview.chromium.org/1363673004/diff/1/chrome/renderer/page_load_h... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/1363673004/diff/1/chrome/renderer/page_load_h... chrome/renderer/page_load_histograms.cc:70: bool IsInLoFiEnabledGroup() { It would be preferable if IsInLoFiEnabledGroup() is read from DRP code. This just spills the logic to multiple places. https://codereview.chromium.org/1363673004/diff/1/chrome/renderer/page_load_h... chrome/renderer/page_load_histograms.cc:237: bool lofi_on, Can you please explain what |lofi_on| in comments? Does it mean Lo-Fi was used or Lo-Fi could have been used? https://codereview.chromium.org/1363673004/diff/1/components/data_reduction_p... File components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.h (right): https://codereview.chromium.org/1363673004/diff/1/components/data_reduction_p... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.h:17: // Handles getting if a request is in Lo-Fi mode from a request. May be remove "from a request" https://codereview.chromium.org/1363673004/diff/1/components/data_reduction_p... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.h:24: bool IsLoFi(net::URLRequest* request) const override; Method comments. Also, may be use const net::URLRequest& request. That makes it clear that IsLoFi() is not going to modify contents of |request|. https://codereview.chromium.org/1363673004/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/1363673004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:887: IsIncludedInLoFiControlFieldTrial()) { So, this returns true even if user is in control group. https://codereview.chromium.org/1363673004/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/1363673004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:188: bool IsLoFiOff() const { return lofi_off_; } May be rename the method to lofi_off() if it is simply returning the value. https://codereview.chromium.org/1363673004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:201: bool ShouldTurnOnLoFiOnMainFrameRequest( The comment is not correct anymore. The method returns true even if user is in Control group. In that case, we should not add q=low to the CP header. Please update the comments for this method and the above. https://codereview.chromium.org/1363673004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:362: // If true, Lo-Fi is turned off for the rest of the session. Is Lo-Fi turned off if the user is in control group? Alternatively, what does it mean by Lo-Fi not turned off? May be add a comment to explain what can not happen once |lofi_off_| is true. https://codereview.chromium.org/1363673004/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h (right): https://codereview.chromium.org/1363673004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:93: bool ShouldEnableLoFiMode(net::URLRequest* url_request); Is this comment correct? Does this function return true if user is in control group and network is slow?
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/1363673004/diff/1/chrome/renderer/page_load_h... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/1363673004/diff/1/chrome/renderer/page_load_h... chrome/renderer/page_load_histograms.cc:70: bool IsInLoFiEnabledGroup() { On 2015/09/28 17:49:02, tbansal1 wrote: > It would be preferable if IsInLoFiEnabledGroup() is read from DRP code. This > just spills the logic to multiple places. Done. https://codereview.chromium.org/1363673004/diff/1/chrome/renderer/page_load_h... chrome/renderer/page_load_histograms.cc:237: bool lofi_on, On 2015/09/28 17:49:02, tbansal1 wrote: > Can you please explain what |lofi_on| in comments? Does it mean Lo-Fi was used > or Lo-Fi could have been used? Done. https://codereview.chromium.org/1363673004/diff/1/components/data_reduction_p... File components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.h (right): https://codereview.chromium.org/1363673004/diff/1/components/data_reduction_p... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.h:17: // Handles getting if a request is in Lo-Fi mode from a request. On 2015/09/28 17:49:02, tbansal1 wrote: > May be remove "from a request" Done. https://codereview.chromium.org/1363673004/diff/1/components/data_reduction_p... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.h:24: bool IsLoFi(net::URLRequest* request) const override; On 2015/09/28 17:49:02, tbansal1 wrote: > Method comments. > > Also, may be use const net::URLRequest& request. > That makes it clear that IsLoFi() is not going to modify contents of |request|. Done. https://codereview.chromium.org/1363673004/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/1363673004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:887: IsIncludedInLoFiControlFieldTrial()) { On 2015/09/28 17:49:02, tbansal1 wrote: > So, this returns true even if user is in control group. Yes, we keep track of is_lofi in the frame for both the control and enabled group now and simply don't add the header in DRPRequestParams now for the control group. https://codereview.chromium.org/1363673004/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/1363673004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:188: bool IsLoFiOff() const { return lofi_off_; } On 2015/09/28 17:49:02, tbansal1 wrote: > May be rename the method to lofi_off() if it is simply returning the value. Done. https://codereview.chromium.org/1363673004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:201: bool ShouldTurnOnLoFiOnMainFrameRequest( On 2015/09/28 17:49:02, tbansal1 wrote: > The comment is not correct anymore. The method returns true even if user is in > Control > group. In that case, we should not add q=low to the CP header. > > Please update the comments for this method and the above. Done. https://codereview.chromium.org/1363673004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:362: // If true, Lo-Fi is turned off for the rest of the session. On 2015/09/28 17:49:02, tbansal1 wrote: > Is Lo-Fi turned off if the user is in control group? Alternatively, > what does it mean by Lo-Fi not turned off? > > May be add a comment to explain what can not happen once |lofi_off_| is true. Done. https://codereview.chromium.org/1363673004/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h (right): https://codereview.chromium.org/1363673004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:93: bool ShouldEnableLoFiMode(net::URLRequest* url_request); On 2015/09/28 17:49:02, tbansal1 wrote: > Is this comment correct? Does this function return true if user is in control > group and network is slow? Done.
Let me know when the other CL stabilizes and I'll take a look. Thanks.
https://codereview.chromium.org/1363673004/diff/60001/chrome/renderer/page_lo... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/1363673004/diff/60001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.cc:225: bool lofi_on /* Whether the frame used the Lo-Fi request header. */, Is this correct? Are the users in control group using LoFi request header? I am probably misunderstanding this. It seems like this should be was_network_slow? https://codereview.chromium.org/1363673004/diff/60001/components/data_reducti... File components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.h (right): https://codereview.chromium.org/1363673004/diff/60001/components/data_reducti... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.h:24: // Returns true if the Lo-Fi directive should be added to the Chrome-Proxy Does this return true if the user is in Control group? https://codereview.chromium.org/1363673004/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/1363673004/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:862: if (params::IsLoFiAlwaysOnViaFlags()) { Remove parenthesis {...} https://codereview.chromium.org/1363673004/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h (right): https://codereview.chromium.org/1363673004/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:106: void SetLoFiModeActiveOnMainFrame(bool lo_fi_mode_active); Is |lo_fi_mode_active| true when the user is in control group and network is slow? https://codereview.chromium.org/1363673004/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc (right): https://codereview.chromium.org/1363673004/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:224: DCHECK(thread_checker_.CalledOnValidThread()); Please add a DCHECK_IMPLIES that if |is_lofi_on| is true, then it implies that either LoFi is enabled from command lien or the user is in Enabled group. Also, it might be worth adding a test that verifies LoFi is not enabled if user is in Control group (may be there is a test already). I am just worried about LoFi getting enabled when it is not supposed to :>
Patchset #3 (id:80001) has been deleted
https://codereview.chromium.org/1363673004/diff/60001/chrome/renderer/page_lo... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/1363673004/diff/60001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.cc:225: bool lofi_on /* Whether the frame used the Lo-Fi request header. */, On 2015/09/29 00:47:48, tbansal1 wrote: > Is this correct? Are the users in control group using LoFi request header? I am > probably misunderstanding this. It seems like this should be was_network_slow? How about a different description? I don't like the idea of using was_network_slow because the metrics we're reporting are based on it being suitable to use Lo-Fi "q=low" header. https://codereview.chromium.org/1363673004/diff/60001/components/data_reducti... File components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.h (right): https://codereview.chromium.org/1363673004/diff/60001/components/data_reducti... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.h:24: // Returns true if the Lo-Fi directive should be added to the Chrome-Proxy On 2015/09/29 00:47:48, tbansal1 wrote: > Does this return true if the user is in Control group? Yes, fixed. Moved the check in here. https://codereview.chromium.org/1363673004/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/1363673004/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:862: if (params::IsLoFiAlwaysOnViaFlags()) { On 2015/09/29 00:47:48, tbansal1 wrote: > Remove parenthesis {...} Done. https://codereview.chromium.org/1363673004/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h (right): https://codereview.chromium.org/1363673004/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:106: void SetLoFiModeActiveOnMainFrame(bool lo_fi_mode_active); On 2015/09/29 00:47:48, tbansal1 wrote: > Is |lo_fi_mode_active| true when the user is in control group and network is > slow? Fixed this so that this won't be set to true when in the control group. This is used by the UI so we don't want that to be activated when in the control group. https://codereview.chromium.org/1363673004/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc (right): https://codereview.chromium.org/1363673004/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:224: DCHECK(thread_checker_.CalledOnValidThread()); On 2015/09/29 00:47:48, tbansal1 wrote: > Please add a DCHECK_IMPLIES that if |is_lofi_on| is true, then > it implies that either LoFi is enabled from command lien or > the user is in Enabled group. > > Also, it might be worth adding a test that verifies LoFi is not > enabled if user is in Control group (may be there is a test already). > > I am just worried about LoFi getting enabled when it is not supposed to :> With the changes I just made I'll need to test this outside of the DRP component, or I can move the check for the "Control" group into DataReductionProxyNetworkDelegate::OnBeforeSendProxyHeadersInternal and write a test there.
Patchset #3 (id:80002) has been deleted
https://codereview.chromium.org/1363673004/diff/170001/components/data_reduct... File components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc (right): https://codereview.chromium.org/1363673004/diff/170001/components/data_reduct... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc:20: !params::IsIncludedInLoFiControlFieldTrial(); nit: this is outside commented section. https://codereview.chromium.org/1363673004/diff/170001/components/data_reduct... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc:20: !params::IsIncludedInLoFiControlFieldTrial(); This may be incorrect. e.g., if user has enabled Lo-Fi from flags, and user is in control field trial, this may return false. I think we want it to return true. https://codereview.chromium.org/1363673004/diff/170001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h (right): https://codereview.chromium.org/1363673004/diff/170001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h:102: bool is_lofi_on); nit: please document |is_lofi_on|.
https://codereview.chromium.org/1363673004/diff/170001/components/data_reduct... File components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc (right): https://codereview.chromium.org/1363673004/diff/170001/components/data_reduct... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc:20: !params::IsIncludedInLoFiControlFieldTrial(); On 2015/09/30 15:52:39, tbansal1 wrote: > This may be incorrect. e.g., if user has enabled Lo-Fi from flags, and user is > in > control field trial, this may return false. I think we want it to return true. Done. Both. https://codereview.chromium.org/1363673004/diff/170001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h (right): https://codereview.chromium.org/1363673004/diff/170001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h:102: bool is_lofi_on); On 2015/09/30 15:52:40, tbansal1 wrote: > nit: please document |is_lofi_on|. Done.
nits... https://codereview.chromium.org/1363673004/diff/190001/components/data_reduct... File components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc (right): https://codereview.chromium.org/1363673004/diff/190001/components/data_reduct... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc:20: // (params::IsLoFiOnViaFlags() || Please add DCHECKs here. e.g., if user is not in Enabled, and flags are not active, then this should not return true. This would trigger if users in Control get the LoFi. https://codereview.chromium.org/1363673004/diff/190001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/1363673004/diff/190001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:816: net::NetworkQualityEstimator* network_quality_estimator = nullptr; DCHECK(thread_checker...) https://codereview.chromium.org/1363673004/diff/190001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/1363673004/diff/190001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:194: bool ShouldEnableLoFiMode(net::URLRequest* request); can the argument be const net::URLRequest*? If you can pass by reference, that's even better. const net::URLRequest& request https://codereview.chromium.org/1363673004/diff/190001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h (right): https://codereview.chromium.org/1363673004/diff/190001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:94: bool ShouldEnableLoFiMode(net::URLRequest* url_request); can this be const net::URLRequest& request? https://codereview.chromium.org/1363673004/diff/190001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h (right): https://codereview.chromium.org/1363673004/diff/190001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h:96: // proxy authentication credentials. If |is_lofi_on| is true adds the "q=low" Duplicate comment? https://codereview.chromium.org/1363673004/diff/190001/components/data_reduct... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/1363673004/diff/190001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:85: return FieldTrialList::FindFullName(GetLoFiFieldTrialName()) == kEnabled; May be it would be simpler if this function and the one below returns false if the user has changed Lo-Fi flag. wdyt? https://codereview.chromium.org/1363673004/diff/190001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:101: kDataReductionProxyLoFiValueSlowConnectionsOnly; It might be easier to call IsLoFiAlwaysOnViaFlags() || IsLoFiCellularOnlyViaFlags()....
https://chromiumcodereview.appspot.com/1363673004/diff/190001/components/data... File components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc (right): https://chromiumcodereview.appspot.com/1363673004/diff/190001/components/data... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc:20: // (params::IsLoFiOnViaFlags() || On 2015/09/30 19:55:25, tbansal1 wrote: > Please add DCHECKs here. e.g., if user is not in Enabled, and flags are not > active, then this should not return true. This would trigger if users in Control > get the LoFi. Agreed online this doesn't work here. https://chromiumcodereview.appspot.com/1363673004/diff/190001/components/data... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://chromiumcodereview.appspot.com/1363673004/diff/190001/components/data... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:816: net::NetworkQualityEstimator* network_quality_estimator = nullptr; On 2015/09/30 19:55:25, tbansal1 wrote: > DCHECK(thread_checker...) Done. https://chromiumcodereview.appspot.com/1363673004/diff/190001/components/data... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://chromiumcodereview.appspot.com/1363673004/diff/190001/components/data... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:194: bool ShouldEnableLoFiMode(net::URLRequest* request); On 2015/09/30 19:55:25, tbansal1 wrote: > can the argument be const net::URLRequest*? If you can pass by reference, that's > even better. > const net::URLRequest& request Done. https://chromiumcodereview.appspot.com/1363673004/diff/190001/components/data... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h (right): https://chromiumcodereview.appspot.com/1363673004/diff/190001/components/data... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:94: bool ShouldEnableLoFiMode(net::URLRequest* url_request); On 2015/09/30 19:55:25, tbansal1 wrote: > can this be const net::URLRequest& request? Done. https://chromiumcodereview.appspot.com/1363673004/diff/190001/components/data... File components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h (right): https://chromiumcodereview.appspot.com/1363673004/diff/190001/components/data... components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h:96: // proxy authentication credentials. If |is_lofi_on| is true adds the "q=low" On 2015/09/30 19:55:25, tbansal1 wrote: > Duplicate comment? haha whoops https://chromiumcodereview.appspot.com/1363673004/diff/190001/components/data... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc (right): https://chromiumcodereview.appspot.com/1363673004/diff/190001/components/data... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:85: return FieldTrialList::FindFullName(GetLoFiFieldTrialName()) == kEnabled; On 2015/09/30 19:55:25, tbansal1 wrote: > May be it would be simpler if this function and the one below returns false if > the user has changed Lo-Fi flag. wdyt? I think I like explicitly asking about both because then you know where those two are being called what logic is being applied, otherwise this function becomes "is the field trial enabled unless overridden by flags" https://chromiumcodereview.appspot.com/1363673004/diff/190001/components/data... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:101: kDataReductionProxyLoFiValueSlowConnectionsOnly; On 2015/09/30 19:55:25, tbansal1 wrote: > It might be easier to call IsLoFiAlwaysOnViaFlags() || > IsLoFiCellularOnlyViaFlags().... Done.
Patchset #8 (id:210001) has been deleted
Patchset #8 (id:230001) has been deleted
Here's a start. In general I think we need to be clearer in our naming of methods to differential when the browser decides to be in lofi mode from when a request is made for the low fidelity version of the resource. E.g., IsLoFiActive() ShouldRequestLoFiResource(const net::URLRequest& request) I'll add more comments later. https://codereview.chromium.org/1363673004/diff/250001/chrome/renderer/page_l... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/1363673004/diff/250001/chrome/renderer/page_l... chrome/renderer/page_load_histograms.cc:225: bool lofi_on /* The current conditions may use the Lo-Fi request header. */, I expected to see the comma before the comment. Also, you could use single line comments, i.e., //. https://codereview.chromium.org/1363673004/diff/250001/components/data_reduct... File components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc (right): https://codereview.chromium.org/1363673004/diff/250001/components/data_reduct... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc:21: // params::IsIncludedInLoFiEnabledFieldTrial()) Why isn't this done? It seems pretty fundamental. https://codereview.chromium.org/1363673004/diff/250001/components/data_reduct... File components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.h (right): https://codereview.chromium.org/1363673004/diff/250001/components/data_reduct... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.h:24: // Returns true if the Lo-Fi directive should be added to the Chrome-Proxy You don't need the comment, because the interface has it. https://codereview.chromium.org/1363673004/diff/250001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/1363673004/diff/250001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:191: // Proxy header or when the user is in the Lo-Fi "Control" group and the Chrome-Proxy Also, I would say less about how the request will be modified to express the desire for low fidelity, and instead just say: // Returns true when Lo-Fi mode should be activated. https://codereview.chromium.org/1363673004/diff/250001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:274: // Returns true when the Lo-Fi "q=low" directive should be added to the Chrome I would get away from the mechanics. https://codereview.chromium.org/1363673004/diff/250001/components/data_reduct... File components/data_reduction_proxy/core/common/data_reduction_proxy_lofi_helper.h (right): https://codereview.chromium.org/1363673004/diff/250001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_lofi_helper.h:13: // An interface for the ContentDataReductionProxyLoFiHelper which creates This comment seems to be a layering violation. The interface shouldn't specify its subclass. // Interface to determine if a request should be made for a low fidelity // version of the resource. https://codereview.chromium.org/1363673004/diff/250001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_lofi_helper.h:16: class DataReductionProxyLoFiHelper { I'd really, really like to drop the DataReductionProxy prefix from all of our class names, because we already have the namespace. How about LoFiDecider? https://codereview.chromium.org/1363673004/diff/250001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_lofi_helper.h:20: // Returns true if the Lo-Fi directive should be added to the Chrome-Proxy Why do we need a class? Couldn't this just be an injected callback? https://codereview.chromium.org/1363673004/diff/250001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_lofi_helper.h:22: virtual bool ShouldUseLoFi(const net::URLRequest* request) const = 0; I understand why you have this interface, but the text is completely confusing. So is it: // Returns true if a low fidelity version of the resource should be requested. Also, the behavior is undefined for a null request, so I would use a const& instead. Finally, why not just have the helper add the header?
Patchset #9 (id:270001) has been deleted
https://chromiumcodereview.appspot.com/1363673004/diff/250001/chrome/renderer... File chrome/renderer/page_load_histograms.cc (right): https://chromiumcodereview.appspot.com/1363673004/diff/250001/chrome/renderer... chrome/renderer/page_load_histograms.cc:225: bool lofi_on /* The current conditions may use the Lo-Fi request header. */, On 2015/10/03 00:01:29, bengr wrote: > I expected to see the comma before the comment. Also, you could use single line > comments, i.e., //. Done. https://chromiumcodereview.appspot.com/1363673004/diff/250001/components/data... File components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc (right): https://chromiumcodereview.appspot.com/1363673004/diff/250001/components/data... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc:21: // params::IsIncludedInLoFiEnabledFieldTrial()) On 2015/10/03 00:01:29, bengr wrote: > Why isn't this done? It seems pretty fundamental. IsLoFi isn't in ResourceRequestInfo yet since it lives in content// and this cl doesn't touch content//. I added (params::IsLoFiOnViaFlags() || params::IsIncludedInLoFiEnabledFieldTrial()) to the return. https://chromiumcodereview.appspot.com/1363673004/diff/250001/components/data... File components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.h (right): https://chromiumcodereview.appspot.com/1363673004/diff/250001/components/data... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.h:24: // Returns true if the Lo-Fi directive should be added to the Chrome-Proxy On 2015/10/03 00:01:29, bengr wrote: > You don't need the comment, because the interface has it. Done. https://chromiumcodereview.appspot.com/1363673004/diff/250001/components/data... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://chromiumcodereview.appspot.com/1363673004/diff/250001/components/data... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:191: // Proxy header or when the user is in the Lo-Fi "Control" group and the On 2015/10/03 00:01:29, bengr wrote: > Chrome-Proxy > > Also, I would say less about how the request will be modified to express the > desire for low fidelity, and instead just say: > > // Returns true when Lo-Fi mode should be activated. Done. https://chromiumcodereview.appspot.com/1363673004/diff/250001/components/data... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:274: // Returns true when the Lo-Fi "q=low" directive should be added to the Chrome On 2015/10/03 00:01:29, bengr wrote: > I would get away from the mechanics. Done. https://chromiumcodereview.appspot.com/1363673004/diff/250001/components/data... File components/data_reduction_proxy/core/common/data_reduction_proxy_lofi_helper.h (right): https://chromiumcodereview.appspot.com/1363673004/diff/250001/components/data... components/data_reduction_proxy/core/common/data_reduction_proxy_lofi_helper.h:13: // An interface for the ContentDataReductionProxyLoFiHelper which creates On 2015/10/03 00:01:30, bengr wrote: > This comment seems to be a layering violation. The interface shouldn't specify > its subclass. > > // Interface to determine if a request should be made for a low fidelity > // version of the resource. Done. https://chromiumcodereview.appspot.com/1363673004/diff/250001/components/data... components/data_reduction_proxy/core/common/data_reduction_proxy_lofi_helper.h:16: class DataReductionProxyLoFiHelper { On 2015/10/03 00:01:30, bengr wrote: > I'd really, really like to drop the DataReductionProxy prefix from all of our > class names, because we already have the namespace. > > How about LoFiDecider? Done. https://chromiumcodereview.appspot.com/1363673004/diff/250001/components/data... components/data_reduction_proxy/core/common/data_reduction_proxy_lofi_helper.h:20: // Returns true if the Lo-Fi directive should be added to the Chrome-Proxy On 2015/10/03 00:01:30, bengr wrote: > Why do we need a class? Couldn't this just be an injected callback? We can't inject a callback into OnBeforeSendProxyHeadersInternal, which is where this is used, but we could have DataReductionProxyIOData own a callback that is created by DRPChromeIOData. That won't change much of this code, and it just seems cleaner to me to have this in its own class rather then have DRPChromeIOData create a callback for some logic that it knows nothing about. https://chromiumcodereview.appspot.com/1363673004/diff/250001/components/data... components/data_reduction_proxy/core/common/data_reduction_proxy_lofi_helper.h:22: virtual bool ShouldUseLoFi(const net::URLRequest* request) const = 0; On 2015/10/03 00:01:30, bengr wrote: > I understand why you have this interface, but the text is completely confusing. > So is it: > > // Returns true if a low fidelity version of the resource should be requested. Done. > Also, the behavior is undefined for a null request, so I would use a const& > instead. Done. > Finally, why not just have the helper add the header? This sounds good to me, but this cl is already large and I don't want to move around too much code at once. Can we make this a follow up cl? This would require pulling all the logic and tests from DRPRequestOptions.
On 2015/10/05 20:22:59, megjablon wrote: > https://chromiumcodereview.appspot.com/1363673004/diff/250001/chrome/renderer... > File chrome/renderer/page_load_histograms.cc (right): > > https://chromiumcodereview.appspot.com/1363673004/diff/250001/chrome/renderer... > chrome/renderer/page_load_histograms.cc:225: bool lofi_on /* The current > conditions may use the Lo-Fi request header. */, > On 2015/10/03 00:01:29, bengr wrote: > > I expected to see the comma before the comment. Also, you could use single > line > > comments, i.e., //. > > Done. > > https://chromiumcodereview.appspot.com/1363673004/diff/250001/components/data... > File > components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc > (right): > > https://chromiumcodereview.appspot.com/1363673004/diff/250001/components/data... > components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc:21: > // params::IsIncludedInLoFiEnabledFieldTrial()) > On 2015/10/03 00:01:29, bengr wrote: > > Why isn't this done? It seems pretty fundamental. > > IsLoFi isn't in ResourceRequestInfo yet since it lives in content// and this cl > doesn't touch content//. I added (params::IsLoFiOnViaFlags() || > params::IsIncludedInLoFiEnabledFieldTrial()) to the return. > > https://chromiumcodereview.appspot.com/1363673004/diff/250001/components/data... > File > components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.h > (right): > > https://chromiumcodereview.appspot.com/1363673004/diff/250001/components/data... > components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.h:24: > // Returns true if the Lo-Fi directive should be added to the Chrome-Proxy > On 2015/10/03 00:01:29, bengr wrote: > > You don't need the comment, because the interface has it. > > Done. > > https://chromiumcodereview.appspot.com/1363673004/diff/250001/components/data... > File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h > (right): > > https://chromiumcodereview.appspot.com/1363673004/diff/250001/components/data... > components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:191: > // Proxy header or when the user is in the Lo-Fi "Control" group and the > On 2015/10/03 00:01:29, bengr wrote: > > Chrome-Proxy > > > > Also, I would say less about how the request will be modified to express the > > desire for low fidelity, and instead just say: > > > > // Returns true when Lo-Fi mode should be activated. > > Done. > > https://chromiumcodereview.appspot.com/1363673004/diff/250001/components/data... > components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:274: > // Returns true when the Lo-Fi "q=low" directive should be added to the Chrome > On 2015/10/03 00:01:29, bengr wrote: > > I would get away from the mechanics. > > Done. > > https://chromiumcodereview.appspot.com/1363673004/diff/250001/components/data... > File > components/data_reduction_proxy/core/common/data_reduction_proxy_lofi_helper.h > (right): > > https://chromiumcodereview.appspot.com/1363673004/diff/250001/components/data... > components/data_reduction_proxy/core/common/data_reduction_proxy_lofi_helper.h:13: > // An interface for the ContentDataReductionProxyLoFiHelper which creates > On 2015/10/03 00:01:30, bengr wrote: > > This comment seems to be a layering violation. The interface shouldn't specify > > its subclass. > > > > // Interface to determine if a request should be made for a low fidelity > > // version of the resource. > > Done. > > https://chromiumcodereview.appspot.com/1363673004/diff/250001/components/data... > components/data_reduction_proxy/core/common/data_reduction_proxy_lofi_helper.h:16: > class DataReductionProxyLoFiHelper { > On 2015/10/03 00:01:30, bengr wrote: > > I'd really, really like to drop the DataReductionProxy prefix from all of our > > class names, because we already have the namespace. > > > > How about LoFiDecider? > > Done. > > https://chromiumcodereview.appspot.com/1363673004/diff/250001/components/data... > components/data_reduction_proxy/core/common/data_reduction_proxy_lofi_helper.h:20: > // Returns true if the Lo-Fi directive should be added to the Chrome-Proxy > On 2015/10/03 00:01:30, bengr wrote: > > Why do we need a class? Couldn't this just be an injected callback? > > We can't inject a callback into OnBeforeSendProxyHeadersInternal, which is where > this is used, but we could have DataReductionProxyIOData own a callback that is > created by DRPChromeIOData. That won't change much of this code, and it just > seems cleaner to me to have this in its own class rather then have > DRPChromeIOData create a callback for some logic that it knows nothing about. > > https://chromiumcodereview.appspot.com/1363673004/diff/250001/components/data... > components/data_reduction_proxy/core/common/data_reduction_proxy_lofi_helper.h:22: > virtual bool ShouldUseLoFi(const net::URLRequest* request) const = 0; > On 2015/10/03 00:01:30, bengr wrote: > > I understand why you have this interface, but the text is completely > confusing. > > So is it: > > > > // Returns true if a low fidelity version of the resource should be requested. > > Done. > > > Also, the behavior is undefined for a null request, so I would use a const& > > instead. > > Done. > > > Finally, why not just have the helper add the header? > > This sounds good to me, but this cl is already large and I don't want to move > around too much code at once. Can we make this a follow up cl? This would > require pulling all the logic and tests from DRPRequestOptions. ping :)
https://codereview.chromium.org/1363673004/diff/310001/chrome/renderer/page_l... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/1363673004/diff/310001/chrome/renderer/page_l... chrome/renderer/page_load_histograms.cc:226: bool is_using_lofi, How about lofi_active_for_page? Then the comment could be: // LoFi was used, unless part of the control group. https://codereview.chromium.org/1363673004/diff/310001/chrome/renderer/page_l... chrome/renderer/page_load_histograms.cc:443: } It looks like the indentation is incorrect here and on the following line. https://codereview.chromium.org/1363673004/diff/310001/chrome/renderer/page_l... chrome/renderer/page_load_histograms.cc:487: PLT_HISTOGRAM( These cases seem very repetitive. Can you put these in one or a small number of anonymous functions? https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... File components/data_reduction_proxy/content/browser/content_lofi_decider.cc (right): https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... components/data_reduction_proxy/content/browser/content_lofi_decider.cc:7: #include "components/data_reduction_proxy/core/common/data_reduction_proxy_params.h" None of these includes is needed. https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... components/data_reduction_proxy/content/browser/content_lofi_decider.cc:22: } // namespace data_reduction_roxy roxy -> proxy https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... File components/data_reduction_proxy/content/browser/content_lofi_decider.h (right): https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... components/data_reduction_proxy/content/browser/content_lofi_decider.h:17: // Handles getting if a request is in Lo-Fi mode. How about: // Class responsible for deciding whether a request // should be requested with low fidelity (Lo-Fi) or // not. Also, please say why this lives in content, and mention any assumptions about threading and lifetime. https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... File components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.h (right): https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.h:28: // a Data Reduction Proxy. Say that is_data_reduction_proxy must not be null. https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:818: net::NetworkQualityEstimator* network_quality_estimator = nullptr; How about: net::NetworkQualityEstimator* network_quality_estimator; network_quality_estimator = request.context() ? request.context()->network_quality_estimator() : nullptr; https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:192: bool ShouldEnableLoFiMode(const net::URLRequest& request); Why does this need a request? Please add to the comment. https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:273: // |network_quality_estimator| may be NULL. Please say a little more about why this method needs a network quality estimator. https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:355: // Store if the previous state of Lo-Fi was on, so that change in Lo-Fi status Store -> True? https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc (right): https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:797: // Lo-Fi off. Would you mind re-doing these comments, here and below? They are often incomplete sentences, redundant, and/or incomplete. For this one, for example, the comment seems to only tell us what the last paramter means. In the next group, lines 802 and 804 both tell us that it is not in the enabled field trial group. Etc. https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h (right): https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:91: // Returns true when Lo-Fi mode should be activated. Please say more about what Lo-Fi mode is: // Returns true when Lo-Fi mode should be activated. // When Lo-Fi mode is active, URL requests are modified // to request low fidelity versions of the resources. https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:180: LoFiDecider* lofi_decider = data_reduction_proxy_io_data_->lofi_decider(); Can this return null? https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:259: data_reduction_proxy_io_data_->lofi_decider() && Can this return null? https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h (right): https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:48: // connections switches. These aren't separate switches, are they? You might want to refer to the switch and associated values in this comment. https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:86: // Returns true if this client is part of Lo-Fi enabled field trial. ...is part of the "Enabled" group of the Lo-Fi field trial. https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:90: // Returns true if this client is part of Lo-Fi control field trial. ...is part of the "Control" group of the Lo-Fi field trial. https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... File components/data_reduction_proxy/core/common/lofi_decider.h (right): https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... components/data_reduction_proxy/core/common/lofi_decider.h:20: virtual bool IsUsingLoFiMode(const net::URLRequest& request) const = 0; I really don't understand the name. I'm presuming this is called before the request header is added. If so, this should be called ShouldUseLoFiMode(). Please explain in the comment and rename if needed.
Patchset #11 (id:330001) has been deleted
megjablon@chromium.org changed reviewers: + thestig@chromium.org
thestig: chrome/* https://chromiumcodereview.appspot.com/1363673004/diff/310001/chrome/renderer... File chrome/renderer/page_load_histograms.cc (right): https://chromiumcodereview.appspot.com/1363673004/diff/310001/chrome/renderer... chrome/renderer/page_load_histograms.cc:226: bool is_using_lofi, On 2015/10/12 21:06:40, bengr wrote: > How about lofi_active_for_page? Then the comment could be: > // LoFi was used, unless part of the control group. Done. https://chromiumcodereview.appspot.com/1363673004/diff/310001/chrome/renderer... chrome/renderer/page_load_histograms.cc:443: } On 2015/10/12 21:06:40, bengr wrote: > It looks like the indentation is incorrect here and on the following line. Indentation is good. https://chromiumcodereview.appspot.com/1363673004/diff/310001/chrome/renderer... chrome/renderer/page_load_histograms.cc:487: PLT_HISTOGRAM( On 2015/10/12 21:06:40, bengr wrote: > These cases seem very repetitive. Can you put these in one or a small number of > anonymous functions? That seems outside of the scope of this cl to fix. https://chromiumcodereview.appspot.com/1363673004/diff/310001/components/data... File components/data_reduction_proxy/content/browser/content_lofi_decider.cc (right): https://chromiumcodereview.appspot.com/1363673004/diff/310001/components/data... components/data_reduction_proxy/content/browser/content_lofi_decider.cc:7: #include "components/data_reduction_proxy/core/common/data_reduction_proxy_params.h" On 2015/10/12 21:06:41, bengr wrote: > None of these includes is needed. Done. https://chromiumcodereview.appspot.com/1363673004/diff/310001/components/data... components/data_reduction_proxy/content/browser/content_lofi_decider.cc:22: } // namespace data_reduction_roxy On 2015/10/12 21:06:41, bengr wrote: > roxy -> proxy Done. https://chromiumcodereview.appspot.com/1363673004/diff/310001/components/data... File components/data_reduction_proxy/content/browser/content_lofi_decider.h (right): https://chromiumcodereview.appspot.com/1363673004/diff/310001/components/data... components/data_reduction_proxy/content/browser/content_lofi_decider.h:17: // Handles getting if a request is in Lo-Fi mode. On 2015/10/12 21:06:41, bengr wrote: > How about: > > // Class responsible for deciding whether a request > // should be requested with low fidelity (Lo-Fi) or > // not. > > Also, please say why this lives in content, and mention any assumptions about > threading and lifetime. Done. https://chromiumcodereview.appspot.com/1363673004/diff/310001/components/data... File components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.h (right): https://chromiumcodereview.appspot.com/1363673004/diff/310001/components/data... components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.h:28: // a Data Reduction Proxy. On 2015/10/12 21:06:41, bengr wrote: > Say that is_data_reduction_proxy must not be null. Done. https://chromiumcodereview.appspot.com/1363673004/diff/310001/components/data... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://chromiumcodereview.appspot.com/1363673004/diff/310001/components/data... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:818: net::NetworkQualityEstimator* network_quality_estimator = nullptr; On 2015/10/12 21:06:41, bengr wrote: > How about: > > net::NetworkQualityEstimator* network_quality_estimator; > network_quality_estimator = request.context() ? > request.context()->network_quality_estimator() : nullptr; > Done. https://chromiumcodereview.appspot.com/1363673004/diff/310001/components/data... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://chromiumcodereview.appspot.com/1363673004/diff/310001/components/data... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:192: bool ShouldEnableLoFiMode(const net::URLRequest& request); On 2015/10/12 21:06:41, bengr wrote: > Why does this need a request? Please add to the comment. Done. https://chromiumcodereview.appspot.com/1363673004/diff/310001/components/data... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:273: // |network_quality_estimator| may be NULL. On 2015/10/12 21:06:41, bengr wrote: > Please say a little more about why this method needs a network quality > estimator. Done. https://chromiumcodereview.appspot.com/1363673004/diff/310001/components/data... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:355: // Store if the previous state of Lo-Fi was on, so that change in Lo-Fi status On 2015/10/12 21:06:41, bengr wrote: > Store -> True? Done. https://chromiumcodereview.appspot.com/1363673004/diff/310001/components/data... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc (right): https://chromiumcodereview.appspot.com/1363673004/diff/310001/components/data... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:797: // Lo-Fi off. On 2015/10/12 21:06:41, bengr wrote: > Would you mind re-doing these comments, here and below? They are often > incomplete sentences, redundant, and/or incomplete. For this one, for example, > the comment seems to only tell us what the last paramter means. In the next > group, lines 802 and 804 both tell us that it is not in the enabled field trial > group. Etc. Done. https://chromiumcodereview.appspot.com/1363673004/diff/310001/components/data... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h (right): https://chromiumcodereview.appspot.com/1363673004/diff/310001/components/data... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:91: // Returns true when Lo-Fi mode should be activated. On 2015/10/12 21:06:41, bengr wrote: > Please say more about what Lo-Fi mode is: > > // Returns true when Lo-Fi mode should be activated. > // When Lo-Fi mode is active, URL requests are modified > // to request low fidelity versions of the resources. Done. https://chromiumcodereview.appspot.com/1363673004/diff/310001/components/data... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://chromiumcodereview.appspot.com/1363673004/diff/310001/components/data... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:180: LoFiDecider* lofi_decider = data_reduction_proxy_io_data_->lofi_decider(); On 2015/10/12 21:06:41, bengr wrote: > Can this return null? Yes, if the Lo-Fi decider isn't set. This is only currently set in Chrome. It can also be null in tests. https://chromiumcodereview.appspot.com/1363673004/diff/310001/components/data... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:259: data_reduction_proxy_io_data_->lofi_decider() && On 2015/10/12 21:06:41, bengr wrote: > Can this return null? Yes, if the Lo-Fi decider isn't set. This is only currently set in Chrome. It can also be null in tests. https://chromiumcodereview.appspot.com/1363673004/diff/310001/components/data... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h (right): https://chromiumcodereview.appspot.com/1363673004/diff/310001/components/data... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:48: // connections switches. On 2015/10/12 21:06:41, bengr wrote: > These aren't separate switches, are they? You might want to refer to the switch > and associated values in this comment. Done. https://chromiumcodereview.appspot.com/1363673004/diff/310001/components/data... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:86: // Returns true if this client is part of Lo-Fi enabled field trial. On 2015/10/12 21:06:41, bengr wrote: > ...is part of the "Enabled" group of the Lo-Fi field trial. Done. https://chromiumcodereview.appspot.com/1363673004/diff/310001/components/data... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:90: // Returns true if this client is part of Lo-Fi control field trial. On 2015/10/12 21:06:41, bengr wrote: > ...is part of the "Control" group of the Lo-Fi field trial. Done. https://chromiumcodereview.appspot.com/1363673004/diff/310001/components/data... File components/data_reduction_proxy/core/common/lofi_decider.h (right): https://chromiumcodereview.appspot.com/1363673004/diff/310001/components/data... components/data_reduction_proxy/core/common/lofi_decider.h:20: virtual bool IsUsingLoFiMode(const net::URLRequest& request) const = 0; On 2015/10/12 21:06:41, bengr wrote: > I really don't understand the name. I'm presuming this is called before the > request header is added. If so, this should be called ShouldUseLoFiMode(). > Please explain in the comment and rename if needed. ShouldUseLoFiMode is reserved for when we call the logic that determines if we should activate Lo-Fi mode. This is done on navigations from ResourceDispatcherHostImpl. IsUsingLoFiMode is once we have set Lo-Fi mode to on for a page and then that state is copied to all requests.
I defer to the other reviewers. Happy to stamp when you have their approvals. https://chromiumcodereview.appspot.com/1363673004/diff/350001/chrome/browser/... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc (right): https://chromiumcodereview.appspot.com/1363673004/diff/350001/chrome/browser/... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:70: scoped_ptr<data_reduction_proxy::LoFiDecider> lofi_decider( Just combine with the next line?
https://chromiumcodereview.appspot.com/1363673004/diff/350001/chrome/browser/... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc (right): https://chromiumcodereview.appspot.com/1363673004/diff/350001/chrome/browser/... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:70: scoped_ptr<data_reduction_proxy::LoFiDecider> lofi_decider( On 2015/10/12 23:14:26, Lei Zhang wrote: > Just combine with the next line? Done.
https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc (right): https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:28: #include "net/base/load_flags.h" Is this include needed? https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc (right): https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:186: if (!prefs_ || params::IsLoFiOnViaFlags()) does this mean that if user has enabled Lo-Fi on cellular through flags, they may still get opted-out? https://codereview.chromium.org/1363673004/diff/370001/components/data_reduct... File components/data_reduction_proxy/content/browser/content_lofi_decider.h (right): https://codereview.chromium.org/1363673004/diff/370001/components/data_reduct... components/data_reduction_proxy/content/browser/content_lofi_decider.h:17: // Class responsible for deciding whether a request should be requested with low Is this class deciding if Lo-Fi should be used or not? Seems like decision is already made and stored in ResourceRequestInfo::IsUsingLoFi. This class is simply reading it? https://codereview.chromium.org/1363673004/diff/370001/components/data_reduct... components/data_reduction_proxy/content/browser/content_lofi_decider.h:20: // using content::ResourceRequestInfo::ForRequest. Lives in You mean owned by DRPIOData? https://codereview.chromium.org/1363673004/diff/370001/components/data_reduct... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/1363673004/diff/370001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:85: bool IsIncludedInLoFiEnabledFieldTrial() { nit: Match the declaration order with the definition order.
https://chromiumcodereview.appspot.com/1363673004/diff/370001/components/data... File components/data_reduction_proxy/content/browser/content_lofi_decider.h (right): https://chromiumcodereview.appspot.com/1363673004/diff/370001/components/data... components/data_reduction_proxy/content/browser/content_lofi_decider.h:17: // Class responsible for deciding whether a request should be requested with low On 2015/10/13 17:12:02, tbansal1 wrote: > Is this class deciding if Lo-Fi should be used or not? Seems like decision is > already > made and stored in ResourceRequestInfo::IsUsingLoFi. This class is simply > reading it? The plan is to move adding the header directly to this class. I agree this is not the best name, but I was ok with switching it to this because once this logic is moved here this is a better name. https://chromiumcodereview.appspot.com/1363673004/diff/370001/components/data... components/data_reduction_proxy/content/browser/content_lofi_decider.h:20: // using content::ResourceRequestInfo::ForRequest. Lives in On 2015/10/13 17:12:02, tbansal1 wrote: > You mean owned by DRPIOData? Done. https://chromiumcodereview.appspot.com/1363673004/diff/370001/components/data... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc (right): https://chromiumcodereview.appspot.com/1363673004/diff/370001/components/data... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:85: bool IsIncludedInLoFiEnabledFieldTrial() { On 2015/10/13 17:12:02, tbansal1 wrote: > nit: Match the declaration order with the definition order. Done.
https://codereview.chromium.org/1363673004/diff/310001/chrome/renderer/page_l... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/1363673004/diff/310001/chrome/renderer/page_l... chrome/renderer/page_load_histograms.cc:487: PLT_HISTOGRAM( On 2015/10/12 23:08:04, megjablon wrote: > On 2015/10/12 21:06:40, bengr wrote: > > These cases seem very repetitive. Can you put these in one or a small number > of > > anonymous functions? > > That seems outside of the scope of this cl to fix. OK. Please add a TODO. https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:273: // |network_quality_estimator| may be NULL. On 2015/10/12 23:08:04, megjablon wrote: > On 2015/10/12 21:06:41, bengr wrote: > > Please say a little more about why this method needs a network quality > > estimator. > > Done. I couldn't quite parse the comment addition. Please rewrite. https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:180: LoFiDecider* lofi_decider = data_reduction_proxy_io_data_->lofi_decider(); On 2015/10/12 23:08:05, megjablon wrote: > On 2015/10/12 21:06:41, bengr wrote: > > Can this return null? > > Yes, if the Lo-Fi decider isn't set. This is only currently set in Chrome. It > can also be null in tests. Well then, you have the potential for a crash, right? https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:259: data_reduction_proxy_io_data_->lofi_decider() && On 2015/10/12 23:08:05, megjablon wrote: > On 2015/10/12 21:06:41, bengr wrote: > > Can this return null? > > Yes, if the Lo-Fi decider isn't set. This is only currently set in Chrome. It > can also be null in tests. Again, you have to do something like: if (lofi_decider()) lofi_decider()->IsUsingLofiMode(...) https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... File components/data_reduction_proxy/content/browser/content_lofi_decider.h (right): https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... components/data_reduction_proxy/content/browser/content_lofi_decider.h:18: // fidelity (Lo-Fi) or not. Lives in content because the Lo-Fi mode state is Instead of "Lives in content because the Lo-Fi mode state is" say "Relies on the Lo-Fi mode state" https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... components/data_reduction_proxy/content/browser/content_lofi_decider.h:19: // stored in the request's content::ResourceRequestInfo which must be fetched nit: add a comma before which https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... components/data_reduction_proxy/content/browser/content_lofi_decider.h:20: // using content::ResourceRequestInfo::ForRequest. Lives in Is it owned by DRPIOData? If so, say "Owned by" instead of "Lives in" https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... File components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.cc (right): https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.cc:49: DCHECK(is_data_reduction_proxy); DCHECKing whether something is null isn't needed because the program will crash anyway. https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... File components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.h (right): https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.h:28: // a Data Reduction Proxy. |is_data_reduction_proxy| must not be null net: Add a period. https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:191: // state changes. |request| is used to get the Network Quality Estimator from NetworkQualityEstimator or network quality estimator
Patchset #14 (id:410001) has been deleted
https://codereview.chromium.org/1363673004/diff/310001/chrome/renderer/page_l... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/1363673004/diff/310001/chrome/renderer/page_l... chrome/renderer/page_load_histograms.cc:487: PLT_HISTOGRAM( On 2015/10/14 22:34:10, bengr wrote: > On 2015/10/12 23:08:04, megjablon wrote: > > On 2015/10/12 21:06:40, bengr wrote: > > > These cases seem very repetitive. Can you put these in one or a small number > > of > > > anonymous functions? > > > > That seems outside of the scope of this cl to fix. > > OK. Please add a TODO. Done. https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:273: // |network_quality_estimator| may be NULL. On 2015/10/14 22:34:10, bengr wrote: > On 2015/10/12 23:08:04, megjablon wrote: > > On 2015/10/12 21:06:41, bengr wrote: > > > Please say a little more about why this method needs a network quality > > > estimator. > > > > Done. > > I couldn't quite parse the comment addition. Please rewrite. Done. https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc (right): https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:28: #include "net/base/load_flags.h" On 2015/10/13 17:12:02, tbansal1 wrote: > Is this include needed? Done. https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:180: LoFiDecider* lofi_decider = data_reduction_proxy_io_data_->lofi_decider(); On 2015/10/14 22:34:10, bengr wrote: > On 2015/10/12 23:08:05, megjablon wrote: > > On 2015/10/12 21:06:41, bengr wrote: > > > Can this return null? > > > > Yes, if the Lo-Fi decider isn't set. This is only currently set in Chrome. It > > can also be null in tests. > > Well then, you have the potential for a crash, right? I check data_reduction_proxy_io_data_->lofi_decider() https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:259: data_reduction_proxy_io_data_->lofi_decider() && On 2015/10/14 22:34:10, bengr wrote: > On 2015/10/12 23:08:05, megjablon wrote: > > On 2015/10/12 21:06:41, bengr wrote: > > > Can this return null? > > > > Yes, if the Lo-Fi decider isn't set. This is only currently set in Chrome. It > > can also be null in tests. > > Again, you have to do something like: > if (lofi_decider()) > lofi_decider()->IsUsingLofiMode(...) I check data_reduction_proxy_io_data_->lofi_decider() https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... File components/data_reduction_proxy/content/browser/content_lofi_decider.h (right): https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... components/data_reduction_proxy/content/browser/content_lofi_decider.h:18: // fidelity (Lo-Fi) or not. Lives in content because the Lo-Fi mode state is On 2015/10/14 22:34:10, bengr wrote: > Instead of "Lives in content because the Lo-Fi mode state is" say "Relies on the > Lo-Fi mode state" Done. https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... components/data_reduction_proxy/content/browser/content_lofi_decider.h:19: // stored in the request's content::ResourceRequestInfo which must be fetched On 2015/10/14 22:34:10, bengr wrote: > nit: add a comma before which Done. https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... components/data_reduction_proxy/content/browser/content_lofi_decider.h:20: // using content::ResourceRequestInfo::ForRequest. Lives in On 2015/10/14 22:34:10, bengr wrote: > Is it owned by DRPIOData? If so, say "Owned by" instead of "Lives in" Done. https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... File components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.cc (right): https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.cc:49: DCHECK(is_data_reduction_proxy); On 2015/10/14 22:34:10, bengr wrote: > DCHECKing whether something is null isn't needed because the program will crash > anyway. Done. https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... File components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.h (right): https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.h:28: // a Data Reduction Proxy. |is_data_reduction_proxy| must not be null On 2015/10/14 22:34:10, bengr wrote: > net: Add a period. Done. https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:191: // state changes. |request| is used to get the Network Quality Estimator from On 2015/10/14 22:34:10, bengr wrote: > NetworkQualityEstimator or network quality estimator Done.
On 2015/10/15 03:44:06, megjablon wrote: > https://codereview.chromium.org/1363673004/diff/310001/chrome/renderer/page_l... > File chrome/renderer/page_load_histograms.cc (right): > > https://codereview.chromium.org/1363673004/diff/310001/chrome/renderer/page_l... > chrome/renderer/page_load_histograms.cc:487: PLT_HISTOGRAM( > On 2015/10/14 22:34:10, bengr wrote: > > On 2015/10/12 23:08:04, megjablon wrote: > > > On 2015/10/12 21:06:40, bengr wrote: > > > > These cases seem very repetitive. Can you put these in one or a small > number > > > of > > > > anonymous functions? > > > > > > That seems outside of the scope of this cl to fix. > > > > OK. Please add a TODO. > > Done. > > https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... > File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h > (right): > > https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... > components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:273: > // |network_quality_estimator| may be NULL. > On 2015/10/14 22:34:10, bengr wrote: > > On 2015/10/12 23:08:04, megjablon wrote: > > > On 2015/10/12 21:06:41, bengr wrote: > > > > Please say a little more about why this method needs a network quality > > > > estimator. > > > > > > Done. > > > > I couldn't quite parse the comment addition. Please rewrite. > > Done. > > https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... > File > components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc > (right): > > https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... > components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:28: > #include "net/base/load_flags.h" > On 2015/10/13 17:12:02, tbansal1 wrote: > > Is this include needed? > > Done. > > https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... > File > components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc > (right): > > https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... > components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:180: > LoFiDecider* lofi_decider = data_reduction_proxy_io_data_->lofi_decider(); > On 2015/10/14 22:34:10, bengr wrote: > > On 2015/10/12 23:08:05, megjablon wrote: > > > On 2015/10/12 21:06:41, bengr wrote: > > > > Can this return null? > > > > > > Yes, if the Lo-Fi decider isn't set. This is only currently set in Chrome. > It > > > can also be null in tests. > > > > Well then, you have the potential for a crash, right? > > I check data_reduction_proxy_io_data_->lofi_decider() > > https://codereview.chromium.org/1363673004/diff/310001/components/data_reduct... > components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:259: > data_reduction_proxy_io_data_->lofi_decider() && > On 2015/10/14 22:34:10, bengr wrote: > > On 2015/10/12 23:08:05, megjablon wrote: > > > On 2015/10/12 21:06:41, bengr wrote: > > > > Can this return null? > > > > > > Yes, if the Lo-Fi decider isn't set. This is only currently set in Chrome. > It > > > can also be null in tests. > > > > Again, you have to do something like: > > if (lofi_decider()) > > lofi_decider()->IsUsingLofiMode(...) > > I check data_reduction_proxy_io_data_->lofi_decider() > > https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... > File components/data_reduction_proxy/content/browser/content_lofi_decider.h > (right): > > https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... > components/data_reduction_proxy/content/browser/content_lofi_decider.h:18: // > fidelity (Lo-Fi) or not. Lives in content because the Lo-Fi mode state is > On 2015/10/14 22:34:10, bengr wrote: > > Instead of "Lives in content because the Lo-Fi mode state is" say "Relies on > the > > Lo-Fi mode state" > > Done. > > https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... > components/data_reduction_proxy/content/browser/content_lofi_decider.h:19: // > stored in the request's content::ResourceRequestInfo which must be fetched > On 2015/10/14 22:34:10, bengr wrote: > > nit: add a comma before which > > Done. > > https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... > components/data_reduction_proxy/content/browser/content_lofi_decider.h:20: // > using content::ResourceRequestInfo::ForRequest. Lives in > On 2015/10/14 22:34:10, bengr wrote: > > Is it owned by DRPIOData? If so, say "Owned by" instead of "Lives in" > > Done. > > https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... > File > components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.cc > (right): > > https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... > components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.cc:49: > DCHECK(is_data_reduction_proxy); > On 2015/10/14 22:34:10, bengr wrote: > > DCHECKing whether something is null isn't needed because the program will > crash > > anyway. > > Done. > > https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... > File > components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.h > (right): > > https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... > components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.h:28: > // a Data Reduction Proxy. |is_data_reduction_proxy| must not be null > On 2015/10/14 22:34:10, bengr wrote: > > net: Add a period. > > Done. > > https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... > File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h > (right): > > https://codereview.chromium.org/1363673004/diff/350001/components/data_reduct... > components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:191: > // state changes. |request| is used to get the Network Quality Estimator from > On 2015/10/14 22:34:10, bengr wrote: > > NetworkQualityEstimator or network quality estimator > > Done. ping!
https://codereview.chromium.org/1363673004/diff/430001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/1363673004/diff/430001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:181: is_using_lofi_mode = lofi_decider->IsUsingLoFiMode(*request); I don't understand how this is safe. What happens if data_reduction_proxy_io_data_->lofi_decider() returns nullptr? The is_using_lofi_mode = nullptr->IsUsingLoFiMode. Crash.
https://codereview.chromium.org/1363673004/diff/430001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/1363673004/diff/430001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:181: is_using_lofi_mode = lofi_decider->IsUsingLoFiMode(*request); On 2015/10/16 21:10:07, bengr wrote: > I don't understand how this is safe. What happens if > data_reduction_proxy_io_data_->lofi_decider() returns nullptr? The > is_using_lofi_mode = nullptr->IsUsingLoFiMode. Crash. On line 179 we check data_reduction_proxy_io_data_->lofi_decider()
lgtm https://codereview.chromium.org/1363673004/diff/430001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/1363673004/diff/430001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:181: is_using_lofi_mode = lofi_decider->IsUsingLoFiMode(*request); On 2015/10/16 22:06:40, megjablon wrote: > On 2015/10/16 21:10:07, bengr wrote: > > I don't understand how this is safe. What happens if > > data_reduction_proxy_io_data_->lofi_decider() returns nullptr? The > > is_using_lofi_mode = nullptr->IsUsingLoFiMode. Crash. > On line 179 we check data_reduction_proxy_io_data_->lofi_decider() Totally missed that. You could move assignment to lofi_decider before the "if" statement. Not required.
rs lgtm https://codereview.chromium.org/1363673004/diff/430001/chrome/browser/net/spd... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc (right): https://codereview.chromium.org/1363673004/diff/430001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:71: scoped_ptr<data_reduction_proxy::LoFiDecider>( you can probably use make_scoped_ptr here. https://codereview.chromium.org/1363673004/diff/430001/chrome/renderer/page_l... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/1363673004/diff/430001/chrome/renderer/page_l... chrome/renderer/page_load_histograms.cc:417: // TODO: Move these repetitive cases into an anonymous function. TODO(username) ?
https://chromiumcodereview.appspot.com/1363673004/diff/430001/chrome/browser/... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc (right): https://chromiumcodereview.appspot.com/1363673004/diff/430001/chrome/browser/... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:71: scoped_ptr<data_reduction_proxy::LoFiDecider>( On 2015/10/17 00:40:38, Lei Zhang wrote: > you can probably use make_scoped_ptr here. Done. https://chromiumcodereview.appspot.com/1363673004/diff/430001/chrome/renderer... File chrome/renderer/page_load_histograms.cc (right): https://chromiumcodereview.appspot.com/1363673004/diff/430001/chrome/renderer... chrome/renderer/page_load_histograms.cc:417: // TODO: Move these repetitive cases into an anonymous function. On 2015/10/17 00:40:38, Lei Zhang wrote: > TODO(username) ? Done. https://chromiumcodereview.appspot.com/1363673004/diff/430001/components/data... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://chromiumcodereview.appspot.com/1363673004/diff/430001/components/data... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:181: is_using_lofi_mode = lofi_decider->IsUsingLoFiMode(*request); On 2015/10/17 00:06:34, bengr wrote: > On 2015/10/16 22:06:40, megjablon wrote: > > On 2015/10/16 21:10:07, bengr wrote: > > > I don't understand how this is safe. What happens if > > > data_reduction_proxy_io_data_->lofi_decider() returns nullptr? The > > > is_using_lofi_mode = nullptr->IsUsingLoFiMode. Crash. > > On line 179 we check data_reduction_proxy_io_data_->lofi_decider() > > Totally missed that. You could move assignment to lofi_decider before the "if" > statement. Not required. Done.
The CQ bit was checked by megjablon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, bengr@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1363673004/#ps470001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1363673004/470001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1363673004/470001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1363673004/470001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1363673004/470001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by megjablon@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1363673004/470001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1363673004/470001
Patchset #17 (id:490001) 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 thestig@chromium.org, bengr@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1363673004/#ps510001 (title: "nullptr fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1363673004/510001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1363673004/510001
Message was sent while issue was closed.
Committed patchset #17 (id:510001)
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/61d87091c812e803e060854d24877c513814661b Cr-Commit-Position: refs/heads/master@{#355674}
Message was sent while issue was closed.
Patchset #16 (id:470001) has been deleted
Message was sent while issue was closed.
Patchset #5 (id:150001) has been deleted
Message was sent while issue was closed.
Patchset #12 (id:390001) has been deleted
Message was sent while issue was closed.
Patchset #4 (id:130001) has been deleted
Message was sent while issue was closed.
Patchset #3 (id:110001) has been deleted
Message was sent while issue was closed.
Patchset #11 (id:450001) has been deleted |
