Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2227723002:
predictors: Also track "no-cache" and "must-revalidate" resources. (Closed)
Descriptionpredictors: Also track "no-cache" and "must-revalidate" resources.
TBR=isherman # Marking a histogram as deprecated.
BUG=631966
Committed: https://crrev.com/cf76d6c6e1ef57c91951028b68735de3beac9d7b
Cr-Commit-Position: refs/heads/master@{#410691}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address comments. #
Messages
Total messages: 21 (12 generated)
lizeb@chromium.org changed reviewers: + pasko@chromium.org
good catch! https://codereview.chromium.org/2227723002/diff/1/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2227723002/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor.cc:66: RESOURCE_STATUS_NO_STORE = 32, this enum is used in a histogram, hence it should not be updated, fields can only be added, otherwise it is not possible to make a histograms.xml that serves all datapoints. I found that this histogram has data collected on 2015-06-09. https://codereview.chromium.org/2227723002/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor.cc:286: UMA_HISTOGRAM_ENUMERATION("ResourcePrefetchPredictor.ResourceStatus", do we actually need all of this histogram? Alternatively we could record counts for each resource status. We could have interesting combinations as well, like 3, 7, 35, 36 and 39. They would be more readable this way on the dashboard. https://codereview.chromium.org/2227723002/diff/1/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2227723002/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor.h:69: // TODO(shishir): Do speculative prefetching for https resources and/or https this is done
Thanks! All done. https://codereview.chromium.org/2227723002/diff/1/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2227723002/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor.cc:66: RESOURCE_STATUS_NO_STORE = 32, On 2016/08/09 09:31:11, pasko wrote: > this enum is used in a histogram, hence it should not be updated, fields can > only be added, otherwise it is not possible to make a histograms.xml that serves > all datapoints. > > I found that this histogram has data collected on 2015-06-09. Acknowledged. https://codereview.chromium.org/2227723002/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor.cc:286: UMA_HISTOGRAM_ENUMERATION("ResourcePrefetchPredictor.ResourceStatus", On 2016/08/09 09:31:11, pasko wrote: > do we actually need all of this histogram? Alternatively we could record counts > for each resource status. We could have interesting combinations as well, like > 3, 7, 35, 36 and 39. They would be more readable this way on the dashboard. Per offline discussion, removed the histogram (but kept the enum for now, used for debugging). Done. https://codereview.chromium.org/2227723002/diff/1/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2227723002/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor.h:69: // TODO(shishir): Do speculative prefetching for https resources and/or https On 2016/08/09 09:31:11, pasko wrote: > this is done Done.
Description was changed from ========== predictors: Also track "no-cache" and "must-revalidate" resources. BUG=631966 ========== to ========== predictors: Also track "no-cache" and "must-revalidate" resources. TBR=isherman BUG=631966 ==========
lgtm, thank you
Description was changed from ========== predictors: Also track "no-cache" and "must-revalidate" resources. TBR=isherman BUG=631966 ========== to ========== predictors: Also track "no-cache" and "must-revalidate" resources. TBR=isherman # Marking a histogram as deprecated. BUG=631966 ==========
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: 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 lizeb@chromium.org
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
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 lizeb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== predictors: Also track "no-cache" and "must-revalidate" resources. TBR=isherman # Marking a histogram as deprecated. BUG=631966 ========== to ========== predictors: Also track "no-cache" and "must-revalidate" resources. TBR=isherman # Marking a histogram as deprecated. BUG=631966 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== predictors: Also track "no-cache" and "must-revalidate" resources. TBR=isherman # Marking a histogram as deprecated. BUG=631966 ========== to ========== predictors: Also track "no-cache" and "must-revalidate" resources. TBR=isherman # Marking a histogram as deprecated. BUG=631966 Committed: https://crrev.com/cf76d6c6e1ef57c91951028b68735de3beac9d7b Cr-Commit-Position: refs/heads/master@{#410691} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/cf76d6c6e1ef57c91951028b68735de3beac9d7b Cr-Commit-Position: refs/heads/master@{#410691} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
