Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2320013003:
[Extensions] Add UMA for the proxy override bubble (Closed)
Description[Extensions] Add UMA for the proxy override bubble
The proxy override bubble had some UMA entries, but they weren't present
in histograms.xml. Add an entry and for the user action metric and
add an obsolete entry (for historical reasons) for the extension count
metric.
BUG=643531
Committed: https://crrev.com/8d7f62dfb5542efbd4ffeac6e7022a7d0bfb19dc
Cr-Commit-Position: refs/heads/master@{#417588}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Mark's #
Messages
Total messages: 18 (10 generated)
The CQ bit was checked by rdevlin.cronin@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...
Description was changed from ========== foo BUG= ========== to ========== [Extensions] Add UMA for the proxy override bubble The proxy override bubble had some UMA entries, but they weren't present in histograms.xml. Add an entry and update the title of the histogram to be consistent with other extension message bubble histograms. BUG=643531 ==========
rdevlin.cronin@chromium.org changed reviewers: + finnur@chromium.org, mpearson@chromium.org
Finnur and Mark, mind taking a look? https://codereview.chromium.org/2320013003/diff/1/chrome/browser/extensions/p... File chrome/browser/extensions/proxy_overridden_bubble_delegate.cc (left): https://codereview.chromium.org/2320013003/diff/1/chrome/browser/extensions/p... chrome/browser/extensions/proxy_overridden_bubble_delegate.cc:136: UMA_HISTOGRAM_COUNTS_100("ProxyOverriddenBubble.ExtensionCount", count); This bubble is limited to a single extension, so this histogram is unneeded.
While I appreciate the cleanups, I'd like things to be tidier. :-) --mark https://codereview.chromium.org/2320013003/diff/1/chrome/browser/extensions/p... File chrome/browser/extensions/proxy_overridden_bubble_delegate.cc (left): https://codereview.chromium.org/2320013003/diff/1/chrome/browser/extensions/p... chrome/browser/extensions/proxy_overridden_bubble_delegate.cc:136: UMA_HISTOGRAM_COUNTS_100("ProxyOverriddenBubble.ExtensionCount", count); On 2016/09/08 22:09:32, Devlin wrote: > This bubble is limited to a single extension, so this histogram is unneeded. Please add this histogram with an appropriate description (describing its problems) and an obsolete tag, otherwise perhaps someone hunting for histograms to investigate something in the future will be misled. https://codereview.chromium.org/2320013003/diff/1/chrome/browser/extensions/p... chrome/browser/extensions/proxy_overridden_bubble_delegate.cc:141: UMA_HISTOGRAM_ENUMERATION("ProxyOverriddenBubble.UserSelection", Please add this old histogram, optionally with an obsolete tag pointing to the new one. It's possible a user of the new one will wish they had more history accessible, which is my the old one should be in histograms.xml.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [Extensions] Add UMA for the proxy override bubble The proxy override bubble had some UMA entries, but they weren't present in histograms.xml. Add an entry and update the title of the histogram to be consistent with other extension message bubble histograms. BUG=643531 ========== to ========== [Extensions] Add UMA for the proxy override bubble The proxy override bubble had some UMA entries, but they weren't present in histograms.xml. Add an entry and for the user action metric and add an obsolete entry (for historical reasons) for the extension count metric. BUG=643531 ==========
https://codereview.chromium.org/2320013003/diff/1/chrome/browser/extensions/p... File chrome/browser/extensions/proxy_overridden_bubble_delegate.cc (left): https://codereview.chromium.org/2320013003/diff/1/chrome/browser/extensions/p... chrome/browser/extensions/proxy_overridden_bubble_delegate.cc:136: UMA_HISTOGRAM_COUNTS_100("ProxyOverriddenBubble.ExtensionCount", count); On 2016/09/08 22:55:25, Mark P wrote: > On 2016/09/08 22:09:32, Devlin wrote: > > This bubble is limited to a single extension, so this histogram is unneeded. > > Please add this histogram with an appropriate description (describing its > problems) and an obsolete tag, otherwise perhaps someone hunting for histograms > to investigate something in the future will be misled. Done. https://codereview.chromium.org/2320013003/diff/1/chrome/browser/extensions/p... chrome/browser/extensions/proxy_overridden_bubble_delegate.cc:141: UMA_HISTOGRAM_ENUMERATION("ProxyOverriddenBubble.UserSelection", On 2016/09/08 22:55:25, Mark P wrote: > Please add this old histogram, optionally with an obsolete tag pointing to the > new one. It's possible a user of the new one will wish they had more history > accessible, which is my the old one should be in histograms.xml. As discussed offline, since retroactively adding the entry to histograms.xml will still have the data from before, added this histogram and still using this name.
lgtm
LGTM
The CQ bit was checked by rdevlin.cronin@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 ========== [Extensions] Add UMA for the proxy override bubble The proxy override bubble had some UMA entries, but they weren't present in histograms.xml. Add an entry and for the user action metric and add an obsolete entry (for historical reasons) for the extension count metric. BUG=643531 ========== to ========== [Extensions] Add UMA for the proxy override bubble The proxy override bubble had some UMA entries, but they weren't present in histograms.xml. Add an entry and for the user action metric and add an obsolete entry (for historical reasons) for the extension count metric. BUG=643531 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [Extensions] Add UMA for the proxy override bubble The proxy override bubble had some UMA entries, but they weren't present in histograms.xml. Add an entry and for the user action metric and add an obsolete entry (for historical reasons) for the extension count metric. BUG=643531 ========== to ========== [Extensions] Add UMA for the proxy override bubble The proxy override bubble had some UMA entries, but they weren't present in histograms.xml. Add an entry and for the user action metric and add an obsolete entry (for historical reasons) for the extension count metric. BUG=643531 Committed: https://crrev.com/8d7f62dfb5542efbd4ffeac6e7022a7d0bfb19dc Cr-Commit-Position: refs/heads/master@{#417588} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/8d7f62dfb5542efbd4ffeac6e7022a7d0bfb19dc Cr-Commit-Position: refs/heads/master@{#417588} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
