Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2790723004:
Function to count dialog box creations. (Closed)
DescriptionFunction to count dialog box creations.
Create a function to count dialog box creations and use it to log translate.
Future CLs will add this logging to other dialog boxes.
BUG=705331
Review-Url: https://codereview.chromium.org/2790723004
Cr-Commit-Position: refs/heads/master@{#462260}
Committed: https://chromium.googlesource.com/chromium/src/+/7872d6c00ef5493bb8abdbf5c33a62d82d2fde45
Patch Set 1 #Patch Set 2 : Use function instead of class for logging. #
Total comments: 8
Patch Set 3 : Address comments. #Patch Set 4 : Change location of files in BUILD.gn #
Total comments: 2
Patch Set 5 : format #
Total comments: 3
Patch Set 6 : Move function to browser_dialogs.h #
Messages
Total messages: 46 (26 generated)
Description was changed from ========== Class to count dialog box impressions. BUG= ========== to ========== Function to count dialog box creations. Create a function to count dialog box creations and use it to log translate. Future CLs will add this logging to other dialog boxes. BUG=705331 ==========
pdyson@chromium.org changed reviewers: + jwd@chromium.org, tapted@chromium.org
https://codereview.chromium.org/2790723004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/dialog_uma/dialog_uma.cc (right): https://codereview.chromium.org/2790723004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/dialog_uma/dialog_uma.cc:10: UMA_HISTOGRAM_SPARSE_SLOWLY("Dialog.Creation", identifier); I'm not really a UMA expert, but I think UMA_HISTOGRAM_ENUMERATION("Dialog.Creation", static_cast<int>(identifier), static_cast<int>(DialogBoxIdentifier::MAX_VALUE)); is the best way to log this https://codereview.chromium.org/2790723004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/dialog_uma/dialog_uma.h (right): https://codereview.chromium.org/2790723004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/dialog_uma/dialog_uma.h:12: enum DialogBoxIdentifier { enum class DialogIdentifier (currently this pollutes the global namespace with the identifier `UNKNOWN`) (also I'd drop the "box" part of the enum type) https://codereview.chromium.org/2790723004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/dialog_uma/dialog_uma.h:17: void ReportDialogCreation(DialogBoxIdentifier identifier); nit: needs a comment also other UMA stuff (e.g. touch_uma) tends towards "Record" rather than "Report"
LGTM with tapted@'s comments. https://codereview.chromium.org/2790723004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/dialog_uma/dialog_uma.cc (right): https://codereview.chromium.org/2790723004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/dialog_uma/dialog_uma.cc:10: UMA_HISTOGRAM_SPARSE_SLOWLY("Dialog.Creation", identifier); On 2017/04/03 09:31:30, tapted wrote: > I'm not really a UMA expert, but I think > > UMA_HISTOGRAM_ENUMERATION("Dialog.Creation", static_cast<int>(identifier), > static_cast<int>(DialogBoxIdentifier::MAX_VALUE)); > > is the best way to log this Yeah, I'd agree. If we move towards having non-contiguous enum values with big gaps between values, it may make sense to move to the sparse though. We can always make the switch to sparse later.
Addressed comments. https://codereview.chromium.org/2790723004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/dialog_uma/dialog_uma.cc (right): https://codereview.chromium.org/2790723004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/dialog_uma/dialog_uma.cc:10: UMA_HISTOGRAM_SPARSE_SLOWLY("Dialog.Creation", identifier); On 2017/04/03 at 23:02:04, jwd wrote: > On 2017/04/03 09:31:30, tapted wrote: > > I'm not really a UMA expert, but I think > > > > UMA_HISTOGRAM_ENUMERATION("Dialog.Creation", static_cast<int>(identifier), > > static_cast<int>(DialogBoxIdentifier::MAX_VALUE)); > > > > is the best way to log this > > Yeah, I'd agree. If we move towards having non-contiguous enum values with big gaps between values, it may make sense to move to the sparse though. We can always make the switch to sparse later. Oh, yes. An earlier version of this had sparse enums, but now they are contiguous. Changed. https://codereview.chromium.org/2790723004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/dialog_uma/dialog_uma.h (right): https://codereview.chromium.org/2790723004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/dialog_uma/dialog_uma.h:12: enum DialogBoxIdentifier { On 2017/04/03 at 09:31:30, tapted wrote: > enum class DialogIdentifier > > (currently this pollutes the global namespace with the identifier `UNKNOWN`) > > (also I'd drop the "box" part of the enum type) Done and Done. https://codereview.chromium.org/2790723004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/dialog_uma/dialog_uma.h:17: void ReportDialogCreation(DialogBoxIdentifier identifier); On 2017/04/03 at 09:31:30, tapted wrote: > nit: needs a comment > > also other UMA stuff (e.g. touch_uma) tends towards "Record" rather than "Report" Translate used Report, but I see Record elsewhere. Changed to Record. Added comment.
Don't forget to upload a new CL :) (also running `git cl try` is a good strategy before sending out for review so you can ensure all the trybots are happy with the change) https://codereview.chromium.org/2790723004/diff/20001/chrome/browser/ui/BUILD.gn File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2790723004/diff/20001/chrome/browser/ui/BUILD... chrome/browser/ui/BUILD.gn:1483: "views/download/download_danger_prompt_views.cc", ooh - actually views/dialog_uma/ should be added up here (this batch is for "secondary UI" which we're compiling on Mac whereas the `(!is_mac || mac_views_browser)` block is for "primary UI" i.e. the browser window). It's not currently a compile error since the translate UI is still an infobar inside the browser window on Mac, but other dialogs already in this `if (toolkit_views)` section won't have access to dialog_uma if it's in the other block.
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 pdyson@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.
On 2017/04/04 at 01:11:59, tapted wrote: > Don't forget to upload a new CL :) > > (also running `git cl try` is a good strategy before sending out for review so you can ensure all the trybots are happy with the change) > > https://codereview.chromium.org/2790723004/diff/20001/chrome/browser/ui/BUILD.gn > File chrome/browser/ui/BUILD.gn (right): > > https://codereview.chromium.org/2790723004/diff/20001/chrome/browser/ui/BUILD... > chrome/browser/ui/BUILD.gn:1483: "views/download/download_danger_prompt_views.cc", > ooh - actually views/dialog_uma/ should be added up here (this batch is for "secondary UI" which we're compiling on Mac whereas the `(!is_mac || mac_views_browser)` block is for "primary UI" i.e. the browser window). It's not currently a compile error since the translate UI is still an infobar inside the browser window on Mac, but other dialogs already in this `if (toolkit_views)` section won't have access to dialog_uma if it's in the other block. I thought I had uploaded it. I'd typed "git cl upload" but it was stuck waiting for me to enter a title for the patch. :( I've moved the files in the BUILD.gn file and done a QC dry run now.
lgtm with the following https://codereview.chromium.org/2790723004/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/dialog_uma/dialog_uma.h (right): https://codereview.chromium.org/2790723004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/dialog_uma/dialog_uma.h:15: MAX_VALUE = 2, This should drop the `= 2` and the trailing comma. The goal of trailing commas is to avoid a `diff` on that line when someone appends something, but here anything added should go _above_ this line rather than below it. So we actually want to highlight this line if that's being violated (which we can do by removing the trailing comma). And removing the =2 ensures it gets incremented automatically by the compiler
https://codereview.chromium.org/2790723004/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/dialog_uma/dialog_uma.h (right): https://codereview.chromium.org/2790723004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/dialog_uma/dialog_uma.h:15: MAX_VALUE = 2, On 2017/04/04 at 04:18:06, tapted wrote: > This should drop the `= 2` and the trailing comma. The goal of trailing commas is to avoid a `diff` on that line when someone appends something, but here anything added should go _above_ this line rather than below it. So we actually want to highlight this line if that's being violated (which we can do by removing the trailing comma). And removing the =2 ensures it gets incremented automatically by the compiler I see. Done. With those changes "git cl format" put all of the enums on the same line. It will go back to separate lines when I add more enums in the next cl.
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...
pdyson@chromium.org changed reviewers: + hajimehoshi@chromium.org
translate lgtm
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 pdyson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jwd@chromium.org, tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2790723004/#ps80001 (title: "format")
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...)
pdyson@chromium.org changed reviewers: + msw@chromium.org
On 2017/04/05 at 00:05:23, commit-bot wrote: > 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...) Adding msw@ as OWNER of chrome/browser/ui/views/ to review adding the directory chrome/browser/ui/views/dialog_uma and the files it contains.
https://codereview.chromium.org/2790723004/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/dialog_uma/dialog_uma.h (right): https://codereview.chromium.org/2790723004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/dialog_uma/dialog_uma.h:5: #ifndef CHROME_BROWSER_UI_VIEWS_DIALOG_UMA_DIALOG_UMA_H_ A couple questions: (1) Why is this specific to views? (seems viable for Mac/Android/iOS too) (2) Why create the dialog_uma subdir? (seems unnecessary for two files)
https://codereview.chromium.org/2790723004/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/dialog_uma/dialog_uma.h (right): https://codereview.chromium.org/2790723004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/dialog_uma/dialog_uma.h:5: #ifndef CHROME_BROWSER_UI_VIEWS_DIALOG_UMA_DIALOG_UMA_H_ On 2017/04/05 at 00:26:54, msw wrote: > A couple questions: > (1) Why is this specific to views? (seems viable for Mac/Android/iOS too) > (2) Why create the dialog_uma subdir? (seems unnecessary for two files) (1) This is to enable DPM (decisions per thousand) to be measured. DPM already has good coverage for InfoBars on Android and iOS using UMA metric InfoBar.Shown. This CL is to start adding coverage for Dialog Boxes on Windows/Linux (and Mac when it moves to views). So I think that makes Views the appropriate place, but I'm quite new to the Chrome code base. (2) I'm open to ideas on where this should go. How about chrome/browser/ui/views/dialog_uma.h?
https://codereview.chromium.org/2790723004/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/dialog_uma/dialog_uma.h (right): https://codereview.chromium.org/2790723004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/dialog_uma/dialog_uma.h:5: #ifndef CHROME_BROWSER_UI_VIEWS_DIALOG_UMA_DIALOG_UMA_H_ On 2017/04/05 03:43:24, pdyson wrote: > On 2017/04/05 at 00:26:54, msw wrote: > > A couple questions: > > (1) Why is this specific to views? (seems viable for Mac/Android/iOS too) > > (2) Why create the dialog_uma subdir? (seems unnecessary for two files) > > (1) This is to enable DPM (decisions per thousand) to be measured. DPM > already has good coverage for InfoBars on Android and iOS using UMA > metric InfoBar.Shown. This CL is to start adding coverage for Dialog > Boxes on Windows/Linux (and Mac when it moves to views). So I think > that makes Views the appropriate place, but I'm quite new to the Chrome > code base. Is there a reason that this shouldn't support Cocoa in the interim on Mac? > (2) I'm open to ideas on where this should go. How about > chrome/browser/ui/views/dialog_uma.h? That would be better, if it should be views-specific. Or I'd suggest: chrome/browser/ui/browser_dialogs.h (and a new *.cc)
On 2017/04/05 at 04:32:12, msw wrote: > https://codereview.chromium.org/2790723004/diff/80001/chrome/browser/ui/views... > File chrome/browser/ui/views/dialog_uma/dialog_uma.h (right): > > https://codereview.chromium.org/2790723004/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/dialog_uma/dialog_uma.h:5: #ifndef CHROME_BROWSER_UI_VIEWS_DIALOG_UMA_DIALOG_UMA_H_ > On 2017/04/05 03:43:24, pdyson wrote: > > On 2017/04/05 at 00:26:54, msw wrote: > > > A couple questions: > > > (1) Why is this specific to views? (seems viable for Mac/Android/iOS too) > > > (2) Why create the dialog_uma subdir? (seems unnecessary for two files) > > > > (1) This is to enable DPM (decisions per thousand) to be measured. DPM > > already has good coverage for InfoBars on Android and iOS using UMA > > metric InfoBar.Shown. This CL is to start adding coverage for Dialog > > Boxes on Windows/Linux (and Mac when it moves to views). So I think > > that makes Views the appropriate place, but I'm quite new to the Chrome > > code base. > > Is there a reason that this shouldn't support Cocoa in the interim on Mac? > > > (2) I'm open to ideas on where this should go. How about > > chrome/browser/ui/views/dialog_uma.h? > > That would be better, if it should be views-specific. Or I'd suggest: > chrome/browser/ui/browser_dialogs.h (and a new *.cc) I have no problem with this being able to support Cocoa on the Mac. My current plans are not to do that, but I can leave that possibility open. So I have moved this to browser_dialogs.h as you suggested.
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.
lgtm, thanks
The CQ bit was checked by pdyson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hajimehoshi@chromium.org, jwd@chromium.org, tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2790723004/#ps100001 (title: "Move function to browser_dialogs.h")
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": 100001, "attempt_start_ts": 1491433472755220,
"parent_rev": "ba45071136a37deb60e5ce17d25432a7f2e9aeae", "commit_rev":
"7872d6c00ef5493bb8abdbf5c33a62d82d2fde45"}
Message was sent while issue was closed.
Description was changed from ========== Function to count dialog box creations. Create a function to count dialog box creations and use it to log translate. Future CLs will add this logging to other dialog boxes. BUG=705331 ========== to ========== Function to count dialog box creations. Create a function to count dialog box creations and use it to log translate. Future CLs will add this logging to other dialog boxes. BUG=705331 Review-Url: https://codereview.chromium.org/2790723004 Cr-Commit-Position: refs/heads/master@{#462260} Committed: https://chromium.googlesource.com/chromium/src/+/7872d6c00ef5493bb8abdbf5c33a... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/7872d6c00ef5493bb8abdbf5c33a... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
