Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2867103006:
[Offline pages] Rename REMOVED to USER_CANCELED and add better foot note descriptions for histogram… (Closed)
|
Created:
3 years, 7 months ago by chili Modified:
3 years, 7 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, asvitkine+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Offline pages] Rename REMOVED to USER_CANCELED and add better foot note descriptions for histogram enums.
BUG=
Review-Url: https://codereview.chromium.org/2867103006
Cr-Commit-Position: refs/heads/master@{#472576}
Committed: https://chromium.googlesource.com/chromium/src/+/ee0d78ca899ff20a65bb00b447a18f925df0ec6f
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix javatest reference to REMOVED, and old typo on retriable. #
Total comments: 1
Patch Set 3 : rebase #Patch Set 4 : rebase #
Total comments: 2
Patch Set 5 : rebase #Messages
Total messages: 47 (33 generated)
The CQ bit was checked by chili@chromium.org to run a CQ dry run
chili@chromium.org changed reviewers: + carlosk@chromium.org, petewil@chromium.org
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
This looks awesome! Thanks for taking care of this. lgtm.
LGTM with just a small nit. https://codereview.chromium.org/2867103006/diff/1/tools/metrics/histograms/en... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2867103006/diff/1/tools/metrics/histograms/en... tools/metrics/histograms/enums.xml:25504: + <int value="10" label="Loading failed (non-retryable)"> nit: /retryable/retriable/s
The CQ bit was checked by chili@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...
Thanks! https://codereview.chromium.org/2867103006/diff/1/tools/metrics/histograms/en... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2867103006/diff/1/tools/metrics/histograms/en... tools/metrics/histograms/enums.xml:25504: + <int value="10" label="Loading failed (non-retryable)"> On 2017/05/09 00:06:06, carlosk wrote: > nit: /retryable/retriable/s Done.
chili@chromium.org changed reviewers: + tedchoc@chromium.org
+tedchoc for histograms/enums.xml
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...)
On 2017/05/09 01:14:26, chili wrote: > +tedchoc for histograms/enums.xml Umm...I'm not an owner for enums.xml. Actually, it looks like it has * ownership, so maybe you don't need me at all (and I'm not the right person to rs it either as I've used enums very minimally so I can't comment too much on their correctness).
fgorski@chromium.org changed reviewers: + fgorski@chromium.org
https://codereview.chromium.org/2867103006/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2867103006/diff/20001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:25478: + <int value="3" label="RequestCoordinator canceled"> Would it not make sense to update it to user canceled as well, given that is the only way we get it (from description)?
The CQ bit was checked by chili@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_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_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 chili@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_asan_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 chili@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2017/05/09 16:03:37, fgorski wrote: > https://codereview.chromium.org/2867103006/diff/20001/tools/metrics/histogram... > File tools/metrics/histograms/enums.xml (right): > > https://codereview.chromium.org/2867103006/diff/20001/tools/metrics/histogram... > tools/metrics/histograms/enums.xml:25478: + <int value="3" > label="RequestCoordinator canceled"> > Would it not make sense to update it to user canceled as well, given that is the > only way we get it (from description)? I will update this in a different CL
lgtm and I double checked the enums for you. https://codereview.chromium.org/2867103006/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2867103006/diff/60001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:25625: + Loading was canceled by various pre-render monitoring. These include when Is this never happening in BLO?
chili@chromium.org changed reviewers: - tedchoc@chromium.org
-TedC oops, seems like I don't need any owners approvals for the enum changes anymore! Thanks! https://codereview.chromium.org/2867103006/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2867103006/diff/60001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:25625: + Loading was canceled by various pre-render monitoring. These include when On 2017/05/16 20:10:50, fgorski wrote: > Is this never happening in BLO? Nope, the various monitored errors either manifest as Loading failed (retriable) or are handled through other means (i.e. mute audio stream rather than fail, prevent new windows from opening rather than fail)
The CQ bit was checked by chili@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org, carlosk@chromium.org Link to the patchset: https://codereview.chromium.org/2867103006/#ps60001 (title: "rebase")
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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) 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_...)
The CQ bit was checked by chili@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by chili@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org, carlosk@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2867103006/#ps80001 (title: "rebase")
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": 80001, "attempt_start_ts": 1495051343113440,
"parent_rev": "68b3fab8a62e3e060690a7a0ea89fa6564b2f49f", "commit_rev":
"ee0d78ca899ff20a65bb00b447a18f925df0ec6f"}
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Rename REMOVED to USER_CANCELED and add better foot note descriptions for histogram enums. BUG= ========== to ========== [Offline pages] Rename REMOVED to USER_CANCELED and add better foot note descriptions for histogram enums. BUG= Review-Url: https://codereview.chromium.org/2867103006 Cr-Commit-Position: refs/heads/master@{#472576} Committed: https://chromium.googlesource.com/chromium/src/+/ee0d78ca899ff20a65bb00b447a1... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/ee0d78ca899ff20a65bb00b447a1... |
