Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13981002:
Added UMA_HISTOGRAM_ENUMERATION for RenderViewContextMenu. (Closed)
|
Created:
7 years, 8 months ago by Vitaly Buka (NO REVIEWS) Modified:
7 years, 8 months ago CC:
chromium-reviews, Avi (use Gerrit), creis+watch_chromium.org, ajwong+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdded UMA_HISTOGRAM_ENUMERATION for RenderViewContextMenu.
BUG=229708
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193357
Patch Set 1 #Patch Set 2 : #
Total comments: 9
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Messages
Total messages: 13 (0 generated)
Ilya, please review as UMA code. Lei, please review as owner.
UMA usage LGTM https://codereview.chromium.org/13981002/diff/3001/chrome/browser/tab_content... File chrome/browser/tab_contents/render_view_context_menu.cc (right): https://codereview.chromium.org/13981002/diff/3001/chrome/browser/tab_content... chrome/browser/tab_contents/render_view_context_menu.cc:122: static const struct { nit: No need for static. You're already in an anonymous namespace. https://codereview.chromium.org/13981002/diff/3001/chrome/browser/tab_content... chrome/browser/tab_contents/render_view_context_menu.cc:179: int GetEnumerationBoundaryValue() { nit: Docs. https://codereview.chromium.org/13981002/diff/3001/chrome/browser/tab_content... chrome/browser/tab_contents/render_view_context_menu.cc:187: void RecordExecutedContextItem(int id) { nit: Docs.
https://codereview.chromium.org/13981002/diff/3001/chrome/browser/tab_content... File chrome/browser/tab_contents/render_view_context_menu.cc (right): https://codereview.chromium.org/13981002/diff/3001/chrome/browser/tab_content... chrome/browser/tab_contents/render_view_context_menu.cc:179: int GetEnumerationBoundaryValue() { If you never delete from |kUmaEnumToCommand|, then you can just return arraysize() here. If you keep the code as is, make boundary_value static and initialize it to, say -1. If it's not -1, then just return the value instead of recalculating every time. Also, bounadary_value is misspelled.
https://codereview.chromium.org/13981002/diff/3001/chrome/browser/tab_content... File chrome/browser/tab_contents/render_view_context_menu.cc (right): https://codereview.chromium.org/13981002/diff/3001/chrome/browser/tab_content... chrome/browser/tab_contents/render_view_context_menu.cc:166: { 40, IDC_CONTENT_CONTEXT_PASTE }, Ilya, I assume it's OK to extend enum in new versions?. We don't need to reserve some large boundary value?
https://codereview.chromium.org/13981002/diff/3001/chrome/browser/tab_content... File chrome/browser/tab_contents/render_view_context_menu.cc (right): https://codereview.chromium.org/13981002/diff/3001/chrome/browser/tab_content... chrome/browser/tab_contents/render_view_context_menu.cc:166: { 40, IDC_CONTENT_CONTEXT_PASTE }, On 2013/04/10 02:50:00, Vitaly Buka wrote: > Ilya, I assume it's OK to extend enum in new versions?. We don't need to reserve > some large boundary value? Yes, it's safe to extend the enum in new versions as long as you ensure that the histogram's boundary value is one more than any legal value.
https://codereview.chromium.org/13981002/diff/3001/chrome/browser/tab_content... File chrome/browser/tab_contents/render_view_context_menu.cc (right): https://codereview.chromium.org/13981002/diff/3001/chrome/browser/tab_content... chrome/browser/tab_contents/render_view_context_menu.cc:122: static const struct { On 2013/04/10 02:22:42, Ilya Sherman wrote: > nit: No need for static. You're already in an anonymous namespace. Done. https://codereview.chromium.org/13981002/diff/3001/chrome/browser/tab_content... chrome/browser/tab_contents/render_view_context_menu.cc:179: int GetEnumerationBoundaryValue() { On 2013/04/10 02:49:57, Lei Zhang wrote: > If you never delete from |kUmaEnumToCommand|, then you can just return > arraysize() here. IDC_ constants can be deleted in future. Otherwise I'd be using array of int, not structures. > > If you keep the code as is, make boundary_value static and initialize it to, say > -1. If it's not -1, then just return the value instead of recalculating every That's not necessary. UMA_HISTOGRAM_ENUMERATION implemented such way, so it calls GetEnumerationBoundaryValue twice (I still expected once :-) ) > time. > > Also, bounadary_value is misspelled. GetEnumerationBoundaryValue is just protection from cases when someone puts largest value in the middle. Probably to protective. Comment should be enough. https://codereview.chromium.org/13981002/diff/3001/chrome/browser/tab_content... chrome/browser/tab_contents/render_view_context_menu.cc:187: void RecordExecutedContextItem(int id) { On 2013/04/10 02:22:42, Ilya Sherman wrote: > nit: Docs. Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/13981002/23001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/13981002/23001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_aura. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/13981002/49001
Message was sent while issue was closed.
Change committed as 193357 |
