Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2529263003:
predictors: Ignore repeating subresources while checking. (Closed)
Descriptionpredictors: Ignore repeating subresources while checking.
This eliminates one of the sources of flakiness in
ResourcePrefetchPredictorBrowserTest.
It seems impossible to avoid an emergence of several requests belonging to the
same subresource but we could just ignore them as real code does.
The real code also ignores repeating entries of the same subresource but doesn't
modify collection passing to test observer.
BUG=650253
Committed: https://crrev.com/3fbb26424d252cec6fb1a20f94f47a6211dcf80e
Cr-Commit-Position: refs/heads/master@{#434951}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Make sort stable + add comment. #
Total comments: 2
Patch Set 3 : . #Patch Set 4 : Add anonymous namespace. #Messages
Total messages: 21 (12 generated)
Description was changed from ========== predictors: Ignore repeating subresources while checking. This eliminates one of the sources of flakiness in ResourcePrefetchPredictorBrowserTest. The real code also ignores repeating entries of the same subresource but doesn't modify collection passing to test observer. BUG=650253 ========== to ========== predictors: Ignore repeating subresources while checking. This eliminates one of the sources of flakiness in ResourcePrefetchPredictorBrowserTest. It seems impossible to avoid an emergence of several requests belonging to the same subresource but we could just ignore them as real code does. The real code also ignores repeating entries of the same subresource but doesn't modify collection passing to test observer. BUG=650253 ==========
alexilin@chromium.org changed reviewers: + lizeb@chromium.org, pasko@chromium.org
Description was changed from ========== predictors: Ignore repeating subresources while checking. This eliminates one of the sources of flakiness in ResourcePrefetchPredictorBrowserTest. It seems impossible to avoid an emergence of several requests belonging to the same subresource but we could just ignore them as real code does. The real code also ignores repeating entries of the same subresource but doesn't modify collection passing to test observer. BUG=650253 ========== to ========== predictors: Ignore repeating subresources while checking. This eliminates one of the sources of flakiness in ResourcePrefetchPredictorBrowserTest. It seems impossible to avoid an emergence of several requests belonging to the same subresource but we could just ignore them as real code does. The real code also ignores repeating entries of the same subresource but doesn't modify collection passing to test observer. BUG=650253 ==========
Hi, I've noticed that the real code actually expects that there could be repeated occurrence of the same subresource and successfully ignores them. We just could do the same in browser tests.
https://codereview.chromium.org/2529263003/diff/1/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2529263003/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:69: std::vector<URLRequestSummary> subresources( nit: why not just a copy? https://codereview.chromium.org/2529263003/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:71: std::sort(subresources.begin(), subresources.end(), Do you need stable_sort here? https://codereview.chromium.org/2529263003/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:104: EXPECT_THAT( Can you add a comment to explain that we have duplicate resources, and that only care about the (first?) occurrence of each?
Thanks! https://codereview.chromium.org/2529263003/diff/1/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2529263003/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:69: std::vector<URLRequestSummary> subresources( On 2016/11/28 14:58:04, Benoit L wrote: > nit: why not just a copy? Done. https://codereview.chromium.org/2529263003/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:71: std::sort(subresources.begin(), subresources.end(), On 2016/11/28 14:58:04, Benoit L wrote: > Do you need stable_sort here? Thanks! Indeed we need first occurrences of each subresource. Done. https://codereview.chromium.org/2529263003/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:104: EXPECT_THAT( On 2016/11/28 14:58:04, Benoit L wrote: > Can you add a comment to explain that we have duplicate resources, and that only > care about the (first?) occurrence of each? Done.
lgtm https://codereview.chromium.org/2529263003/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2529263003/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:67: std::vector<URLRequestSummary> GetUniqueSubresources( nit: anonymous namespace for everything between kImagePath..GetUniqueSubresources The predictors namespace is not big, but it is good to be explicit about these things not being declared outside this file.
On 2016/11/28 15:41:28, pasko wrote: > lgtm > > https://codereview.chromium.org/2529263003/diff/20001/chrome/browser/predicto... > File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc > (right): > > https://codereview.chromium.org/2529263003/diff/20001/chrome/browser/predicto... > chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:67: > std::vector<URLRequestSummary> GetUniqueSubresources( > nit: anonymous namespace for everything between > kImagePath..GetUniqueSubresources > > The predictors namespace is not big, but it is good to be explicit about these > things not being declared outside this file. lgtm
The CQ bit was checked by alexilin@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.
Very important draft need to be published. https://codereview.chromium.org/2529263003/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2529263003/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:67: std::vector<URLRequestSummary> GetUniqueSubresources( On 2016/11/28 15:41:28, pasko wrote: > nit: anonymous namespace for everything between > kImagePath..GetUniqueSubresources > > The predictors namespace is not big, but it is good to be explicit about these > things not being declared outside this file. Done.
The CQ bit was checked by alexilin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org, lizeb@chromium.org Link to the patchset: https://codereview.chromium.org/2529263003/#ps60001 (title: "Add anonymous namespace.")
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": 60001, "attempt_start_ts": 1480415530614650,
"parent_rev": "285937af69d17502ecfa24fb5b24694e464be1c4", "commit_rev":
"b0f96ee3006230800fa9fdf88f4b0cacc50559f7"}
Message was sent while issue was closed.
Description was changed from ========== predictors: Ignore repeating subresources while checking. This eliminates one of the sources of flakiness in ResourcePrefetchPredictorBrowserTest. It seems impossible to avoid an emergence of several requests belonging to the same subresource but we could just ignore them as real code does. The real code also ignores repeating entries of the same subresource but doesn't modify collection passing to test observer. BUG=650253 ========== to ========== predictors: Ignore repeating subresources while checking. This eliminates one of the sources of flakiness in ResourcePrefetchPredictorBrowserTest. It seems impossible to avoid an emergence of several requests belonging to the same subresource but we could just ignore them as real code does. The real code also ignores repeating entries of the same subresource but doesn't modify collection passing to test observer. BUG=650253 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== predictors: Ignore repeating subresources while checking. This eliminates one of the sources of flakiness in ResourcePrefetchPredictorBrowserTest. It seems impossible to avoid an emergence of several requests belonging to the same subresource but we could just ignore them as real code does. The real code also ignores repeating entries of the same subresource but doesn't modify collection passing to test observer. BUG=650253 ========== to ========== predictors: Ignore repeating subresources while checking. This eliminates one of the sources of flakiness in ResourcePrefetchPredictorBrowserTest. It seems impossible to avoid an emergence of several requests belonging to the same subresource but we could just ignore them as real code does. The real code also ignores repeating entries of the same subresource but doesn't modify collection passing to test observer. BUG=650253 Committed: https://crrev.com/3fbb26424d252cec6fb1a20f94f47a6211dcf80e Cr-Commit-Position: refs/heads/master@{#434951} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3fbb26424d252cec6fb1a20f94f47a6211dcf80e Cr-Commit-Position: refs/heads/master@{#434951} |
