Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2850533003:
Log the creation of a dialog boxes using DialogDelegate. (Closed)
DescriptionLog the creation of a dialog boxes using DialogDelegate.
BUG=705331
Review-Url: https://codereview.chromium.org/2850533003
Cr-Commit-Position: refs/heads/master@{#469226}
Committed: https://chromium.googlesource.com/chromium/src/+/18c894ab917e7396857d10d247c278fc6f2e5c0f
Patch Set 1 #
Total comments: 5
Patch Set 2 : Use BooleanCreated enum for Dialog.Delegate.Creation #
Messages
Total messages: 24 (13 generated)
pdyson@chromium.org changed reviewers: + jwd@chromium.org, msw@chromium.org
The CQ bit was checked by pdyson@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.
Hopefully jwd@ can double check the histogram type. https://codereview.chromium.org/2850533003/diff/1/ui/views/window/dialog_dele... File ui/views/window/dialog_delegate.cc (right): https://codereview.chromium.org/2850533003/diff/1/ui/views/window/dialog_dele... ui/views/window/dialog_delegate.cc:36: UMA_HISTOGRAM_BOOLEAN("Dialog.Delegate.Creation", true); Why BOOLEAN? Shouldn't this be UMA_HISTOGRAM_COUNTS_*?
On 2017/05/01 at 21:51:30, msw wrote: > Hopefully jwd@ can double check the histogram type. > > https://codereview.chromium.org/2850533003/diff/1/ui/views/window/dialog_dele... > File ui/views/window/dialog_delegate.cc (right): > > https://codereview.chromium.org/2850533003/diff/1/ui/views/window/dialog_dele... > ui/views/window/dialog_delegate.cc:36: UMA_HISTOGRAM_BOOLEAN("Dialog.Delegate.Creation", true); > Why BOOLEAN? Shouldn't this be UMA_HISTOGRAM_COUNTS_*? This histogram counts a single thing, so I think boolean is appropriate. We'll see what Jesse thinks. I may in future also count the use of other dialog classes that do not inherit from DialogDelegate, such as WebDialogDelegate, but I will do that in a separate boolean UMA metric. So I do not need the ability to add other values to this metric and so boolean should be ok here.
On 2017/05/02 at 00:54:56, pdyson wrote: > Jesse, can you take a look at this?
LGTM with comment. https://codereview.chromium.org/2850533003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2850533003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:11791: +<histogram name="Dialog.Delegate.Creation"> Add an enum. Looks like we have a BooleanCreated enum already, so you can use that. https://codereview.chromium.org/2850533003/diff/1/ui/views/window/dialog_dele... File ui/views/window/dialog_delegate.cc (right): https://codereview.chromium.org/2850533003/diff/1/ui/views/window/dialog_dele... ui/views/window/dialog_delegate.cc:36: UMA_HISTOGRAM_BOOLEAN("Dialog.Delegate.Creation", true); On 2017/05/01 21:51:30, msw wrote: > Why BOOLEAN? Shouldn't this be UMA_HISTOGRAM_COUNTS_*? No, it's confusing naming. UMA_HISTOGRAM_COUNTS_* is for logging a count of something, like number of vowels on a page. It would let you answer questions like "how many pages were seen with >100 vowels" or more abstractly, "how many times a range of count values were logged". BOOLEAN is a special case of an enum histogram. It's for logging an event, like a vowel was seen. It will just let you answer "how many vowels were seen". In this case BOOLEAN is the correct histogram.
okay, lgtm with jwd@'s comments addressed. https://codereview.chromium.org/2850533003/diff/1/ui/views/window/dialog_dele... File ui/views/window/dialog_delegate.cc (right): https://codereview.chromium.org/2850533003/diff/1/ui/views/window/dialog_dele... ui/views/window/dialog_delegate.cc:36: UMA_HISTOGRAM_BOOLEAN("Dialog.Delegate.Creation", true); On 2017/05/03 20:14:40, jwd wrote: > On 2017/05/01 21:51:30, msw wrote: > > Why BOOLEAN? Shouldn't this be UMA_HISTOGRAM_COUNTS_*? > > No, it's confusing naming. UMA_HISTOGRAM_COUNTS_* is for logging a count of > something, like number of vowels on a page. It would let you answer questions > like "how many pages were seen with >100 vowels" or more abstractly, "how many > times a range of count values were logged". > > BOOLEAN is a special case of an enum histogram. It's for logging an event, like > a vowel was seen. It will just let you answer "how many vowels were seen". > > In this case BOOLEAN is the correct histogram. Acknowledged.
https://codereview.chromium.org/2850533003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2850533003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:11791: +<histogram name="Dialog.Delegate.Creation"> On 2017/05/03 at 20:14:40, jwd wrote: > Add an enum. Looks like we have a BooleanCreated enum already, so you can use that. Done.
The CQ bit was checked by pdyson@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 pdyson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, jwd@chromium.org Link to the patchset: https://codereview.chromium.org/2850533003/#ps20001 (title: "Use BooleanCreated enum for Dialog.Delegate.Creation")
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": 1493859936978350,
"parent_rev": "fd0cc89538d0cacf1d4f225e0e6077d1dbd4b966", "commit_rev":
"18c894ab917e7396857d10d247c278fc6f2e5c0f"}
Message was sent while issue was closed.
Description was changed from ========== Log the creation of a dialog boxes using DialogDelegate. BUG=705331 ========== to ========== Log the creation of a dialog boxes using DialogDelegate. BUG=705331 Review-Url: https://codereview.chromium.org/2850533003 Cr-Commit-Position: refs/heads/master@{#469226} Committed: https://chromium.googlesource.com/chromium/src/+/18c894ab917e7396857d10d247c2... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/18c894ab917e7396857d10d247c2... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
