Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2675883002:
Fixing mismatch between expected and declared minimum values. (Closed)
DescriptionFixing mismatch between expected and declared minimum values.
All the declared minimum values defined in Java histograms will be silently
changed to 1 if it was 0 for CustomTimesHistogram. Adding the same check
used in c++ histograms to change the value before calling CheckHistogramArgs.
This would avoid a DCHECK described in the bug.
BUG=681180
Review-Url: https://codereview.chromium.org/2675883002
Cr-Commit-Position: refs/heads/master@{#448705}
Committed: https://chromium.googlesource.com/chromium/src/+/0e1108687b5138bd546120a861d730b8b5268342
Patch Set 1 #
Total comments: 1
Patch Set 2 : comments. #Messages
Total messages: 26 (17 generated)
The CQ bit was checked by romax@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...
romax@chromium.org changed reviewers: + yfriedman@chromium.org
Doing this since I'm not sure if putting 0 as minimum value is reasonable. There is a comment for argument above 'recordLinearCountHistogram()' which is a similar method. PTAL, thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
romax@chromium.org changed reviewers: + nyquist@chromium.org - yfriedman@chromium.org
Seems yfriedman@ would be OOO for a while. nyquist@ can you please take a look? Thanks!
nyquist@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine: PTAL from a histogram perspective
lgtm % comment https://codereview.chromium.org/2675883002/diff/1/base/android/record_histogr... File base/android/record_histogram.cc (right): https://codereview.chromium.org/2675883002/diff/1/base/android/record_histogr... base/android/record_histogram.cc:59: Histogram::InspectConstructionArguments( Can you assign the return value to a var and DCHECK on it - same as is done in base/metrics/ code? e.g. from base/metrics: bool valid_arguments = Histogram::InspectConstructionArguments( name, &minimum, &maximum, &bucket_count); DCHECK(valid_arguments);
The CQ bit was checked by romax@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 romax@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2675883002/#ps20001 (title: "comments.")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
rubberstamp lgtm
The CQ bit was checked by romax@chromium.org
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": 1486496911286810,
"parent_rev": "2f6c5e252867ccacdfd9be8c3c70ee229e7cc12d", "commit_rev":
"0e1108687b5138bd546120a861d730b8b5268342"}
Message was sent while issue was closed.
Description was changed from ========== Fixing mismatch between expected and declared minimum values. All the declared minimum values defined in Java histograms will be silently changed to 1 if it was 0 for CustomTimesHistogram. Adding the same check used in c++ histograms to change the value before calling CheckHistogramArgs. This would avoid a DCHECK described in the bug. BUG=681180 ========== to ========== Fixing mismatch between expected and declared minimum values. All the declared minimum values defined in Java histograms will be silently changed to 1 if it was 0 for CustomTimesHistogram. Adding the same check used in c++ histograms to change the value before calling CheckHistogramArgs. This would avoid a DCHECK described in the bug. BUG=681180 Review-Url: https://codereview.chromium.org/2675883002 Cr-Commit-Position: refs/heads/master@{#448705} Committed: https://chromium.googlesource.com/chromium/src/+/0e1108687b5138bd546120a861d7... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/0e1108687b5138bd546120a861d7... |
