Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2503273002:
Fetch a warmup URL if data reduction proxy is enabled (Closed)
DescriptionFetch a warmup URL if data reduction proxy is enabled
Fetch a warmup URL if data reduction proxy (DRP) is enabled. The fetching
of the warm up URL is disabled by default, and enabled only via the
field trial.
BUG=672334
Committed: https://crrev.com/3bc6d7167dc40c488fd948e8601584bcbcec3ce6
Cr-Commit-Position: refs/heads/master@{#437905}
Patch Set 1 : ps #
Total comments: 32
Patch Set 2 : ryansturm comments #Messages
Total messages: 61 (54 generated)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== DRP warm up URL BUG= ========== to ========== DRP fetch preconnect URL BUG= ==========
Description was changed from ========== DRP fetch preconnect URL BUG= ========== to ========== Fetch a warmup URL if data reduction proxy is enabled Fetch a warmup URL if data reduction proxy (DRP) is enabled. BUG= ==========
Patchset #1 (id:60001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fetch a warmup URL if data reduction proxy is enabled Fetch a warmup URL if data reduction proxy (DRP) is enabled. BUG= ========== to ========== Fetch a warmup URL if data reduction proxy is enabled Fetch a warmup URL if data reduction proxy (DRP) is enabled. The fetching of the warm up URL is disabled by default, and enabled only if enabled via the field trial. BUG= ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Fetch a warmup URL if data reduction proxy is enabled Fetch a warmup URL if data reduction proxy (DRP) is enabled. The fetching of the warm up URL is disabled by default, and enabled only if enabled via the field trial. BUG= ========== to ========== Fetch a warmup URL if data reduction proxy is enabled Fetch a warmup URL if data reduction proxy (DRP) is enabled. The fetching of the warm up URL is disabled by default, and enabled only if enabled via the field trial. BUG=672334 ==========
Patchset #1 (id:120001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #1 (id:140001) has been deleted
tbansal@chromium.org changed reviewers: + ryansturm@chromium.org
ryansturm: ptal. Thanks.
Description was changed from ========== Fetch a warmup URL if data reduction proxy is enabled Fetch a warmup URL if data reduction proxy (DRP) is enabled. The fetching of the warm up URL is disabled by default, and enabled only if enabled via the field trial. BUG=672334 ========== to ========== Fetch a warmup URL if data reduction proxy is enabled Fetch a warmup URL if data reduction proxy (DRP) is enabled. The fetching of the warm up URL is disabled by default, and enabled only if via the field trial. BUG=672334 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fetch a warmup URL if data reduction proxy is enabled Fetch a warmup URL if data reduction proxy (DRP) is enabled. The fetching of the warm up URL is disabled by default, and enabled only if via the field trial. BUG=672334 ========== to ========== Fetch a warmup URL if data reduction proxy is enabled Fetch a warmup URL if data reduction proxy (DRP) is enabled. The fetching of the warm up URL is disabled by default, and enabled only via the field trial. BUG=672334 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % a ton of nits https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:285: // Fetcher for fetching the warm up URL. "Fetcher for fetching" is redundant maybe "Class for fetching" https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:291: fetcher_callback_(nullptr) { I don't think you need to explicitly initialize this with nullptr. The default initialization of callbacks should set up a is_null callback. https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:302: std::unique_ptr<net::URLFetcher> fetcher(net::URLFetcher::Create( If fetcher is always std::move'd to fetcher_ can you just do: fetcher_ = ... and stop using fetcher at all? https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:307: fetcher->SetLoadFlags(net::LOAD_BYPASS_CACHE); consider LOAD_DISABLE_CACHE (I don't know which is better) https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:316: void SetWarmupURLFetcherCallbackForTesting( Pretty duplicative naming WarmupURLFetcher::SetWarmupURLFetcherCallbackForTesting. Maybe just SetFetchCompletionCallback. https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:337: // Called when the warm up URL has been fetched. May be null. Can you mention that this will be called even if the fetch has failed? https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:338: base::Callback<void()> fetcher_callback_; nit:s/fetcher_callback_/fetch_completion_callback_/? https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:384: net::URLRequestContextGetter* url_request_context_getter) { How come some places pass const& for the getters and some pass raw pointers? https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:108: // disables the use of alternative protocols. |url_request_context_getter| is nit: s/alternative protocols/alternative protocols and proxies/ https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:223: // Updates the callback called when the warm up URL has been fetched. s/callback called/callback that is called/ https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:327: std::unique_ptr<WarmupURLFetcher> warmup_url_fetcher_; member comment for warmup_url_fetcher_. And for secure_proxy_checker_ if you feel like it. https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:142: // Fetching of warm up URL can be enabled only for Enabled* and Control* s/of warm up/of the warm up/ https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h (right): https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:151: // Returns true if the fetching of warm up URL is enabled. s/the fetching of warm up/fetching of the warm up/ https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... File components/data_reduction_proxy/core/common/data_reduction_proxy_params_unittest.cc (right): https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_params_unittest.cc:382: bool set_warmup_url; can you get rid of set_warmup_url and do an empty string check for warm_up_url instead? https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_params_unittest.cc:383: std::string warm_up_url; s/warm_up_url/warmup_url/ or change the other variables above to *warm_up*. Make it consistent. https://codereview.chromium.org/2503273002/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2503273002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:9173: + <summary>Result of the fetching of the warm up URL.</summary> nit: s/Result of the fetching of the warm up URL/Whether the warm up URL was fetched succesfully/
Patchset #2 (id:180001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
tbansal@chromium.org changed reviewers: + rkaplow@chromium.org
rkaplow: ptal at histograms.xml. Thanks. https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:285: // Fetcher for fetching the warm up URL. On 2016/12/08 20:59:17, Ryan Sturm wrote: > "Fetcher for fetching" is redundant maybe "Class for fetching" Done. https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:291: fetcher_callback_(nullptr) { On 2016/12/08 20:59:17, Ryan Sturm wrote: > I don't think you need to explicitly initialize this with nullptr. The default > initialization of callbacks should set up a is_null callback. Done. https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:302: std::unique_ptr<net::URLFetcher> fetcher(net::URLFetcher::Create( On 2016/12/08 20:59:17, Ryan Sturm wrote: > If fetcher is always std::move'd to fetcher_ can you just do: > fetcher_ = ... > > and stop using fetcher at all? Done. https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:307: fetcher->SetLoadFlags(net::LOAD_BYPASS_CACHE); On 2016/12/08 20:59:17, Ryan Sturm wrote: > consider LOAD_DISABLE_CACHE (I don't know which is better) LOAD_DISABLE_CACHE disables writing to the local cache which seems unnecessary in this case. https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:316: void SetWarmupURLFetcherCallbackForTesting( On 2016/12/08 20:59:17, Ryan Sturm wrote: > Pretty duplicative naming > WarmupURLFetcher::SetWarmupURLFetcherCallbackForTesting. > > Maybe just SetFetchCompletionCallback. I think ForTesting suffix is helpful since a warning is generated if a method with ForTesting suffix is called in non-test code. Also, I would prefer to keep WarmipURL in the name since there are 2 fetchers in this file, and it makes it easier to distinguish. https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:337: // Called when the warm up URL has been fetched. May be null. On 2016/12/08 20:59:17, Ryan Sturm wrote: > Can you mention that this will be called even if the fetch has failed? Done. https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:338: base::Callback<void()> fetcher_callback_; On 2016/12/08 20:59:17, Ryan Sturm wrote: > nit:s/fetcher_callback_/fetch_completion_callback_/? Done. https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:384: net::URLRequestContextGetter* url_request_context_getter) { On 2016/12/08 20:59:17, Ryan Sturm wrote: > How come some places pass const& for the getters and some pass raw pointers? Done. https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:108: // disables the use of alternative protocols. |url_request_context_getter| is On 2016/12/08 20:59:17, Ryan Sturm wrote: > nit: s/alternative protocols/alternative protocols and proxies/ Done. https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:223: // Updates the callback called when the warm up URL has been fetched. On 2016/12/08 20:59:17, Ryan Sturm wrote: > s/callback called/callback that is called/ Done. https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:327: std::unique_ptr<WarmupURLFetcher> warmup_url_fetcher_; On 2016/12/08 20:59:17, Ryan Sturm wrote: > member comment for warmup_url_fetcher_. And for secure_proxy_checker_ if you > feel like it. Done. https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:142: // Fetching of warm up URL can be enabled only for Enabled* and Control* On 2016/12/08 20:59:17, Ryan Sturm wrote: > s/of warm up/of the warm up/ Done. https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h (right): https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:151: // Returns true if the fetching of warm up URL is enabled. On 2016/12/08 20:59:17, Ryan Sturm wrote: > s/the fetching of warm up/fetching of the warm up/ Done. https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... File components/data_reduction_proxy/core/common/data_reduction_proxy_params_unittest.cc (right): https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_params_unittest.cc:382: bool set_warmup_url; On 2016/12/08 20:59:17, Ryan Sturm wrote: > can you get rid of set_warmup_url and do an empty string check for warm_up_url > instead? Done. https://codereview.chromium.org/2503273002/diff/160001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_params_unittest.cc:383: std::string warm_up_url; On 2016/12/08 20:59:17, Ryan Sturm wrote: > s/warm_up_url/warmup_url/ or change the other variables above to *warm_up*. Make > it consistent. Done. https://codereview.chromium.org/2503273002/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2503273002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:9173: + <summary>Result of the fetching of the warm up URL.</summary> On 2016/12/08 20:59:17, Ryan Sturm wrote: > nit: s/Result of the fetching of the warm up URL/Whether the warm up URL was > fetched succesfully/ Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ryansturm@chromium.org Link to the patchset: https://codereview.chromium.org/2503273002/#ps200001 (title: "ryansturm comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1481562040419200,
"parent_rev": "291710d6f6243fd1abdc64f2a62323c3a11e3f97", "commit_rev":
"ece1a1ceebc0321d282e2861f40012b12f4563f4"}
Message was sent while issue was closed.
Description was changed from ========== Fetch a warmup URL if data reduction proxy is enabled Fetch a warmup URL if data reduction proxy (DRP) is enabled. The fetching of the warm up URL is disabled by default, and enabled only via the field trial. BUG=672334 ========== to ========== Fetch a warmup URL if data reduction proxy is enabled Fetch a warmup URL if data reduction proxy (DRP) is enabled. The fetching of the warm up URL is disabled by default, and enabled only via the field trial. BUG=672334 Review-Url: https://codereview.chromium.org/2503273002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Fetch a warmup URL if data reduction proxy is enabled Fetch a warmup URL if data reduction proxy (DRP) is enabled. The fetching of the warm up URL is disabled by default, and enabled only via the field trial. BUG=672334 Review-Url: https://codereview.chromium.org/2503273002 ========== to ========== Fetch a warmup URL if data reduction proxy is enabled Fetch a warmup URL if data reduction proxy (DRP) is enabled. The fetching of the warm up URL is disabled by default, and enabled only via the field trial. BUG=672334 Committed: https://crrev.com/3bc6d7167dc40c488fd948e8601584bcbcec3ce6 Cr-Commit-Position: refs/heads/master@{#437905} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/3bc6d7167dc40c488fd948e8601584bcbcec3ce6 Cr-Commit-Position: refs/heads/master@{#437905} |
