Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2820393002:
predictors: Use several hosts in resource_prefetch_predictor tests. (Closed)
Descriptionpredictors: Use several hosts in resource_prefetch_predictor tests.
All resources were served either out of the main resource domain. Adding
diversity is required to test the origin prediction mode of the
predictor.
BUG=699080
Review-Url: https://codereview.chromium.org/2820393002
Cr-Commit-Position: refs/heads/master@{#465568}
Committed: https://chromium.googlesource.com/chromium/src/+/839dfcedb91dcfc8c5baded9be2855e11ad24bac
Patch Set 1 #
Total comments: 10
Patch Set 2 : Address comments.' #
Messages
Total messages: 18 (13 generated)
The CQ bit was checked by lizeb@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...
lizeb@chromium.org changed reviewers: + alexilin@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lgtm with a few comments https://codereview.chromium.org/2820393002/diff/1/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2820393002/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:79: const std::pair<std::string, std::string> kStyle = {kBazHost, kStylePath}; The compilation error (warning, actually) is a MSVC bug :( http://stackoverflow.com/questions/34013930/error-c4592-symbol-will-be-dynami... https://codereview.chromium.org/2820393002/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:523: bool https) const { unused parameter Probably you wanted to use it as auto* server = https ? https_server() : embedded_test_server(); std::string port = base::StringPrintf("%d", server->port()); https://codereview.chromium.org/2820393002/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:531: GURL GetPageURLWithReplacements(const std::string& host, Add a warning comment that the result URL is only usable with a URL served from file. https://codereview.chromium.org/2820393002/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:534: std::string path_with_replacements = GetPathWithReplacements(path, https); just nit: Scheme also could be set with replacements. https://codereview.chromium.org/2820393002/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:965: GetPageURLWithReplacements(kFooHost, kHtmlSubresourcesPath); s/kFooHost/kBarHost to preserve cross-host redirect
The CQ bit was checked by lizeb@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.
https://codereview.chromium.org/2820393002/diff/1/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2820393002/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:79: const std::pair<std::string, std::string> kStyle = {kBazHost, kStylePath}; On 2017/04/19 09:31:06, alexilin wrote: > The compilation error (warning, actually) is a MSVC bug :( > http://stackoverflow.com/questions/34013930/error-c4592-symbol-will-be-dynami... Done. https://codereview.chromium.org/2820393002/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:523: bool https) const { On 2017/04/19 09:31:07, alexilin wrote: > unused parameter > Probably you wanted to use it as > auto* server = https ? https_server() : embedded_test_server(); > std::string port = base::StringPrintf("%d", server->port()); That was the plan, yes, you guessed what was the previous version of this code. Turns out that hosts + HTTPS = boom. ... https://codereview.chromium.org/2820393002/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:531: GURL GetPageURLWithReplacements(const std::string& host, On 2017/04/19 09:31:07, alexilin wrote: > Add a warning comment that the result URL is only usable with a URL served from > file. Done. https://codereview.chromium.org/2820393002/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:534: std::string path_with_replacements = GetPathWithReplacements(path, https); On 2017/04/19 09:31:07, alexilin wrote: > just nit: > Scheme also could be set with replacements. As above... https://codereview.chromium.org/2820393002/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:965: GetPageURLWithReplacements(kFooHost, kHtmlSubresourcesPath); On 2017/04/19 09:31:06, alexilin wrote: > s/kFooHost/kBarHost to preserve cross-host redirect Oops. Done.
The CQ bit was checked by lizeb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alexilin@chromium.org Link to the patchset: https://codereview.chromium.org/2820393002/#ps20001 (title: "Address 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": 20001, "attempt_start_ts": 1492606770412080,
"parent_rev": "a07a8f7a373cc13ee2163341a1b9c8c813c49a1f", "commit_rev":
"839dfcedb91dcfc8c5baded9be2855e11ad24bac"}
Message was sent while issue was closed.
Description was changed from ========== predictors: Use several hosts in resource_prefetch_predictor tests. All resources were served either out of the main resource domain. Adding diversity is required to test the origin prediction mode of the predictor. BUG=699080 ========== to ========== predictors: Use several hosts in resource_prefetch_predictor tests. All resources were served either out of the main resource domain. Adding diversity is required to test the origin prediction mode of the predictor. BUG=699080 Review-Url: https://codereview.chromium.org/2820393002 Cr-Commit-Position: refs/heads/master@{#465568} Committed: https://chromium.googlesource.com/chromium/src/+/839dfcedb91dcfc8c5baded9be28... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/839dfcedb91dcfc8c5baded9be28... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
