Make UMA_HISTOGRAM_ENUMERATION work with scoped enums.

Make UMA_HISTOGRAM_ENUMERATION work with scoped enums.

Internally, the macro does some math on the macro arguments, which
doesn't interact well with scoped enums. To work around this, the
internal macro now explicitly casts the enum values to integral
types.

To ensure some modicum of safety, there are new static asserts to
verify that the enum values are in range of HistogramBase::Sample.
This breaks code that was using UMA_HISTOGRAM_ENUMERATION to record
things that aren't C++ enumerations, so switch those to use
UMA_HISTOGRAM_EXACT_LINEAR as well.

BUG=705171
R=rkaplow,jochen,mrefaat,sky,thestig,xhwang
TBR=mukai

Review-Url: https://codereview.chromium.org/2776853002
Cr-Commit-Position: refs/heads/master@{#460938}
Committed: https://chromium.googlesource.com/chromium/src/+/f6f4e494cd797690b41c4359c08bc33d49811759

[dcheng, 2017-03-25 06:48:17]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-25 06:48:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-25 07:00:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-25 07:00:47]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dcheng, 2017-03-26 08:26:03]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-26 08:26:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-26 08:33:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-26 08:33:58]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  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...)

[dcheng, 2017-03-26 08:36:27]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-26 08:36:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-26 08:44:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-26 08:44:28]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  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...)

[dcheng, 2017-03-27 09:07:10]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 09:07:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 09:16:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 09:16:31]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[dcheng, 2017-03-27 17:24:36]

Description was changed from

==========
Make UMA_HISTOGRAM_ENUMERATION work with scoped enums.

BUG=705171
==========

to

==========
Make UMA_HISTOGRAM_ENUMERATION work with scoped enums.

Internally, the macro does some math on the macro arguments, which
doesn't interact well with scoped enums. To work around this, the
internal macro now explicitly casts the enum values to integral
types.

To ensure some modicum of safety, there are new static asserts to
verify that the enum values are in range of HistogramBase::Sample.
This breaks code that was using UMA_HISTOGRAM_ENUMERATION to record
things that aren't C++ enumerations, so switch those to use
UMA_HISTOGRAM_EXACT_LINEAR as well.

BUG=705171
==========

[dcheng, 2017-03-27 17:24:36]

dcheng@chromium.org changed reviewers:
+ danakj@chromium.org, mpearson@chromium.org

[dcheng, 2017-03-27 17:26:57]

dcheng@chromium.org changed reviewers:
- danakj@chromium.org

[dcheng, 2017-03-27 17:28:49]

PTAL. There's still compile errors outside of Linux, but the fixes are
straightforward and can be updated if there's no objection to the macro
implementation.

[Mark P, 2017-03-27 17:44:41]

mpearson@chromium.org changed reviewers:
+ rkaplow@chromium.org
- mpearson@chromium.org

[Mark P, 2017-03-27 17:45:26]

mpearson@chromium.org changed reviewers:
+ mpearson@chromium.org

[Mark P, 2017-03-27 17:45:27]

Handing this review off to rkaplow@, who's done most of the heavy lifting with
histogram macros, their function and organization.

--mark

[dcheng, 2017-03-27 18:14:59]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 18:15:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mark P, 2017-03-27 18:21:09]

mpearson@chromium.org changed reviewers:
- mpearson@chromium.org

[wychen, 2017-03-27 19:44:00]

wychen@chromium.org changed reviewers:
+ wychen@chromium.org

[wychen, 2017-03-27 19:44:01]

https://codereview.chromium.org/2776853002/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/chromeos/change_picture_options_handler.cc
(right):

https://codereview.chromium.org/2776853002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/change_picture_options_handler.cc:355:
UMA_HISTOGRAM_EXACT_LINEAR("UserImage.ChangeChoice",
Do you know why these default_user_image:: constants have to be in "extern const
int" instead of simply "const int"? Avoid recompilation and binary bloat? Maybe
they can be converted to enum as well.

[wychen, 2017-03-27 20:08:56]

https://codereview.chromium.org/2776853002/diff/120001/base/metrics/histogram...
File base/metrics/histogram_macros_internal.h (right):

https://codereview.chromium.org/2776853002/diff/120001/base/metrics/histogram...
base/metrics/histogram_macros_internal.h:138: // TODO(dcheng): This should
assert that the passed in types are actually enum
Do we want to convert all UMA_HISTOGRAM_ENUMERATION() to
UMA_HISTOGRAM_EXACT_LINEAR() if either |sample| or |boundary| is not enum? Looks
like a good target for clang refactoring tools.

https://codereview.chromium.org/2776853002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/user_image_screen.cc (right):

https://codereview.chromium.org/2776853002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/user_image_screen.cc:149:
UMA_HISTOGRAM_EXACT_LINEAR("UserImage.FirstTimeChoice", uma_index,
If these constants are converted to enum, uma_index can be converted to enum as
well, and then we can keep using UMA_HISTOGRAM_ENUMERATION.

https://codereview.chromium.org/2776853002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc
(right):

https://codereview.chromium.org/2776853002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:651:
UMA_HISTOGRAM_EXACT_LINEAR("UserImage.LoggedIn",
Ditto.

[commit-bot: I haz the power, 2017-03-27 23:13:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 23:13:04]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[dcheng, 2017-03-28 07:16:45]

https://codereview.chromium.org/2776853002/diff/120001/base/metrics/histogram...
File base/metrics/histogram_macros_internal.h (right):

https://codereview.chromium.org/2776853002/diff/120001/base/metrics/histogram...
base/metrics/histogram_macros_internal.h:138: // TODO(dcheng): This should
assert that the passed in types are actually enum
On 2017/03/27 20:08:56, wychen wrote:
> Do we want to convert all UMA_HISTOGRAM_ENUMERATION() to
> UMA_HISTOGRAM_EXACT_LINEAR() if either |sample| or |boundary| is not enum?
Looks
> like a good target for clang refactoring tools.

Yes, that's the implication of this TODO. It's probably not even necessary for
tooling: we can just static assert that the input types are enum types, and feed
that to a simple script that changes the macro on that line.

https://codereview.chromium.org/2776853002/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/chromeos/change_picture_options_handler.cc
(right):

https://codereview.chromium.org/2776853002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/change_picture_options_handler.cc:355:
UMA_HISTOGRAM_EXACT_LINEAR("UserImage.ChangeChoice",
On 2017/03/27 19:44:01, wychen wrote:
> Do you know why these default_user_image:: constants have to be in "extern
const
> int" instead of simply "const int"? Avoid recompilation and binary bloat?
Maybe
> they can be converted to enum as well.

This "enum" has a pretty weird structure, so I opted not to touch it. If someone
from CrOS wants to fix it, they can =P

(See
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/users/defa...:
the gap at the beginning of the constants is intentional. Since it's sort of an
enum and sort of not, I'm opting to just leave it alone)

[dcheng, 2017-03-29 16:39:16]

ping

[rkaplow, 2017-03-29 18:02:51]

thanks for the fix and for fixing the clients. Mostly lg, just a few comments

https://codereview.chromium.org/2776853002/diff/120001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_view.h (right):

https://codereview.chromium.org/2776853002/diff/120001/ash/common/shelf/shelf...
ash/common/shelf/shelf_view.h:61: SHELF_ALIGNMENT_UMA_ENUM_VALUE_COUNT,
Seems a bit easy to miss that this last needs to be last, can you add a space
and something like // Must be last entry in enum.

https://codereview.chromium.org/2776853002/diff/120001/base/metrics/histogram...
File base/metrics/histogram_macros.h (right):

https://codereview.chromium.org/2776853002/diff/120001/base/metrics/histogram...
base/metrics/histogram_macros.h:44: #define UMA_HISTOGRAM_ENUMERATION(name,
sample, enum_max) \
did git cl format suggest this? otherwise I'd probably leave the trailing \s at
the end as in the rest of the file for readability

https://codereview.chromium.org/2776853002/diff/120001/base/metrics/histogram...
File base/metrics/histogram_macros_internal.h (right):

https://codereview.chromium.org/2776853002/diff/120001/base/metrics/histogram...
base/metrics/histogram_macros_internal.h:143:
!std::is_enum<decltype(sample)>::value ||                              \
since this macro should only be used for enums now (and not the linear model),
can't we now switch this check to verify both sample and boundary are enums
here? i.e. remove the first two !is_enum options.

https://codereview.chromium.org/2776853002/diff/120001/base/metrics/histogram...
base/metrics/histogram_macros_internal.h:153:
INTERNAL_HISTOGRAM_EXACT_LINEAR_WITH_FLAG(                                 \
From what I understand this macro forces the args to not be enums, which we're
doing by the casts here. Maybe worth adding a comment clarifying that?

https://codereview.chromium.org/2776853002/diff/120001/ios/chrome/browser/des...
File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.cc
(right):

https://codereview.chromium.org/2776853002/diff/120001/ios/chrome/browser/des...
ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.cc:78: if
(sms_entrypoint == i) {
linear looks like the wrong model here as this is supposed to be:

<enum name="DesktopIOSPromotionEntryPoint" type="int">
  <int value="1" label="Save Passwords new bubble."/>
  <int value="2" label="Bookmarks new bubble."/>
  <int value="3" label="Bookmarks footnote."/>
  <int value="4" label="HistoryPage."/>
</enum>

however I'm not seeing any data for this. maybe it's obsolete anyway? add a todo
to fix or clean it up for an owner?

https://codereview.chromium.org/2776853002/diff/120001/media/blink/resource_m...
File media/blink/resource_multibuffer_data_provider.cc (right):

https://codereview.chromium.org/2776853002/diff/120001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.cc:266:
UMA_HISTOGRAM_EXACT_LINEAR("Media.UncacheableReason", shift,
not sure why this can't be an enum either. Add a TODO to fix?

[dcheng, 2017-03-29 18:24:41]

https://codereview.chromium.org/2776853002/diff/120001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_view.h (right):

https://codereview.chromium.org/2776853002/diff/120001/ash/common/shelf/shelf...
ash/common/shelf/shelf_view.h:61: SHELF_ALIGNMENT_UMA_ENUM_VALUE_COUNT,
On 2017/03/29 18:02:50, rkaplow (slow) wrote:
> Seems a bit easy to miss that this last needs to be last, can you add a space
> and something like // Must be last entry in enum.

Done.

https://codereview.chromium.org/2776853002/diff/120001/base/metrics/histogram...
File base/metrics/histogram_macros.h (right):

https://codereview.chromium.org/2776853002/diff/120001/base/metrics/histogram...
base/metrics/histogram_macros.h:44: #define UMA_HISTOGRAM_ENUMERATION(name,
sample, enum_max) \
On 2017/03/29 18:02:50, rkaplow (slow) wrote:
> did git cl format suggest this? otherwise I'd probably leave the trailing \s
at
> the end as in the rest of the file for readability

Yes, this is git cl format.

https://codereview.chromium.org/2776853002/diff/120001/base/metrics/histogram...
File base/metrics/histogram_macros_internal.h (right):

https://codereview.chromium.org/2776853002/diff/120001/base/metrics/histogram...
base/metrics/histogram_macros_internal.h:143:
!std::is_enum<decltype(sample)>::value ||                              \
On 2017/03/29 18:02:50, rkaplow (slow) wrote:
> since this macro should only be used for enums now (and not the linear model),
> can't we now switch this check to verify both sample and boundary are enums
> here? i.e. remove the first two !is_enum options.

It should, but I'd prefer to do this in a followup (it's already a TODO on line
138): it's still possible to use non-enum things with this for the moment (if
you pass in a constexpr boundary).

https://codereview.chromium.org/2776853002/diff/120001/base/metrics/histogram...
base/metrics/histogram_macros_internal.h:153:
INTERNAL_HISTOGRAM_EXACT_LINEAR_WITH_FLAG(                                 \
On 2017/03/29 18:02:50, rkaplow (slow) wrote:
> From what I understand this macro forces the args to not be enums, which we're
> doing by the casts here. Maybe worth adding a comment clarifying that?

It's hard to add comments inside macros. Is there any particular comment you're
looking for other than the one already on line 130-132?

https://codereview.chromium.org/2776853002/diff/120001/ios/chrome/browser/des...
File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.cc
(right):

https://codereview.chromium.org/2776853002/diff/120001/ios/chrome/browser/des...
ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.cc:78: if
(sms_entrypoint == i) {
On 2017/03/29 18:02:51, rkaplow (slow) wrote:
> linear looks like the wrong model here as this is supposed to be:
> 
> <enum name="DesktopIOSPromotionEntryPoint" type="int">
>   <int value="1" label="Save Passwords new bubble."/>
>   <int value="2" label="Bookmarks new bubble."/>
>   <int value="3" label="Bookmarks footnote."/>
>   <int value="4" label="HistoryPage."/>
> </enum>
> 
> however I'm not seeing any data for this. maybe it's obsolete anyway? add a
todo
> to fix or clean it up for an owner?

To elaborate, are you saying that exact linear is the wrong model because the
UMA histogram is defined as an enum?

https://codereview.chromium.org/2776853002/diff/120001/media/blink/resource_m...
File media/blink/resource_multibuffer_data_provider.cc (right):

https://codereview.chromium.org/2776853002/diff/120001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.cc:266:
UMA_HISTOGRAM_EXACT_LINEAR("Media.UncacheableReason", shift,
On 2017/03/29 18:02:51, rkaplow (slow) wrote:
> not sure why this can't be an enum either. Add a TODO to fix?

I think it's because the enum itself is a bitmask style enum, so we'd have to
define two separate enums to make this work out nicely. Do you still want a
TODO?

[Mark P, 2017-03-29 19:03:36]

mpearson@chromium.org changed reviewers:
+ mpearson@chromium.org

[Mark P, 2017-03-29 19:03:36]

https://codereview.chromium.org/2776853002/diff/120001/ios/chrome/browser/des...
File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.cc
(right):

https://codereview.chromium.org/2776853002/diff/120001/ios/chrome/browser/des...
ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.cc:78: if
(sms_entrypoint == i) {
On 2017/03/29 18:24:40, dcheng wrote:
> On 2017/03/29 18:02:51, rkaplow (slow) wrote:
> > linear looks like the wrong model here as this is supposed to be:
> > 
> > <enum name="DesktopIOSPromotionEntryPoint" type="int">
> >   <int value="1" label="Save Passwords new bubble."/>
> >   <int value="2" label="Bookmarks new bubble."/>
> >   <int value="3" label="Bookmarks footnote."/>
> >   <int value="4" label="HistoryPage."/>
> > </enum>
> > 
> > however I'm not seeing any data for this. maybe it's obsolete anyway? add a
> todo
> > to fix or clean it up for an owner?
> 
> To elaborate, are you saying that exact linear is the wrong model because the
> UMA histogram is defined as an enum?

drive-by: it's not obsolete.  Instead, this feature hasn't been launched yet. 
(The iOS side to the feature is still under review.)

The original enum made sense.  0 is meant to be unused.  These values relate to
sync, and we're using the 0 value in sync to indicate "not set".  No 0 value is
expected to be in the UMA output, which is why when I revised the metrics for
this, I explicitly asked them to leave the 0 out of histograms.xml

[rkaplow, 2017-03-29 23:21:17]

lgtm

https://codereview.chromium.org/2776853002/diff/120001/base/metrics/histogram...
File base/metrics/histogram_macros_internal.h (right):

https://codereview.chromium.org/2776853002/diff/120001/base/metrics/histogram...
base/metrics/histogram_macros_internal.h:153:
INTERNAL_HISTOGRAM_EXACT_LINEAR_WITH_FLAG(                                 \
On 2017/03/29 18:24:40, dcheng wrote:
> On 2017/03/29 18:02:50, rkaplow (slow) wrote:
> > From what I understand this macro forces the args to not be enums, which
we're
> > doing by the casts here. Maybe worth adding a comment clarifying that?
> 
> It's hard to add comments inside macros. Is there any particular comment
you're
> looking for other than the one already on line 130-132?

Yeah, I would prefer it down here but since it's not really possible with macros
I guess it's fine.

https://codereview.chromium.org/2776853002/diff/120001/ios/chrome/browser/des...
File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.cc
(right):

https://codereview.chromium.org/2776853002/diff/120001/ios/chrome/browser/des...
ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.cc:78: if
(sms_entrypoint == i) {
On 2017/03/29 19:03:36, Mark P wrote:
> On 2017/03/29 18:24:40, dcheng wrote:
> > On 2017/03/29 18:02:51, rkaplow (slow) wrote:
> > > linear looks like the wrong model here as this is supposed to be:
> > > 
> > > <enum name="DesktopIOSPromotionEntryPoint" type="int">
> > >   <int value="1" label="Save Passwords new bubble."/>
> > >   <int value="2" label="Bookmarks new bubble."/>
> > >   <int value="3" label="Bookmarks footnote."/>
> > >   <int value="4" label="HistoryPage."/>
> > > </enum>
> > > 
> > > however I'm not seeing any data for this. maybe it's obsolete anyway? add
a
> > todo
> > > to fix or clean it up for an owner?
> > 
> > To elaborate, are you saying that exact linear is the wrong model because
the
> > UMA histogram is defined as an enum?
> 
> drive-by: it's not obsolete.  Instead, this feature hasn't been launched yet. 
> (The iOS side to the feature is still under review.)
> 
> The original enum made sense.  0 is meant to be unused.  These values relate
to
> sync, and we're using the 0 value in sync to indicate "not set".  No 0 value
is
> expected to be in the UMA output, which is why when I revised the metrics for
> this, I explicitly asked them to leave the 0 out of histograms.xml

I guess this is ok since it would be strange to have an intermediate enum.
Again, i wouldn't mind a comment explaining why we need LINEAR here.

Unless you think it's worth have an enum for this subset of the enum, it just
seems a bit overkill to me.

https://codereview.chromium.org/2776853002/diff/120001/media/blink/resource_m...
File media/blink/resource_multibuffer_data_provider.cc (right):

https://codereview.chromium.org/2776853002/diff/120001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.cc:266:
UMA_HISTOGRAM_EXACT_LINEAR("Media.UncacheableReason", shift,
On 2017/03/29 18:24:41, dcheng wrote:
> On 2017/03/29 18:02:51, rkaplow (slow) wrote:
> > not sure why this can't be an enum either. Add a TODO to fix?
> 
> I think it's because the enum itself is a bitmask style enum, so we'd have to
> define two separate enums to make this work out nicely. Do you still want a
> TODO?

Well, the team can evaluate if they want to change. LINEAR is intended for
capturing numeric data with linear sized buckets. It may be worth to have the
bitmask map to an underlying enum and they can use that.

If we want to keep it like this I'd at least have a comment explaining why we're
faking an enum with a linear macro

[Mark P, 2017-03-29 23:36:33]

https://codereview.chromium.org/2776853002/diff/120001/ios/chrome/browser/des...
File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.cc
(right):

https://codereview.chromium.org/2776853002/diff/120001/ios/chrome/browser/des...
ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.cc:78: if
(sms_entrypoint == i) {
On 2017/03/29 23:21:17, rkaplow (slow) wrote:
> On 2017/03/29 19:03:36, Mark P wrote:
> > On 2017/03/29 18:24:40, dcheng wrote:
> > > On 2017/03/29 18:02:51, rkaplow (slow) wrote:
> > > > linear looks like the wrong model here as this is supposed to be:
> > > > 
> > > > <enum name="DesktopIOSPromotionEntryPoint" type="int">
> > > >   <int value="1" label="Save Passwords new bubble."/>
> > > >   <int value="2" label="Bookmarks new bubble."/>
> > > >   <int value="3" label="Bookmarks footnote."/>
> > > >   <int value="4" label="HistoryPage."/>
> > > > </enum>
> > > > 
> > > > however I'm not seeing any data for this. maybe it's obsolete anyway?
add
> a
> > > todo
> > > > to fix or clean it up for an owner?
> > > 
> > > To elaborate, are you saying that exact linear is the wrong model because
> the
> > > UMA histogram is defined as an enum?
> > 
> > drive-by: it's not obsolete.  Instead, this feature hasn't been launched
yet. 
> > (The iOS side to the feature is still under review.)
> > 
> > The original enum made sense.  0 is meant to be unused.  These values relate
> to
> > sync, and we're using the 0 value in sync to indicate "not set".  No 0 value
> is
> > expected to be in the UMA output, which is why when I revised the metrics
for
> > this, I explicitly asked them to leave the 0 out of histograms.xml
> 
> I guess this is ok since it would be strange to have an intermediate enum.
> Again, i wouldn't mind a comment explaining why we need LINEAR here.
> 
> Unless you think it's worth have an enum for this subset of the enum, it just
> seems a bit overkill to me.

That would be overkill, especially because it would require keeping this enum in
sync between chromium code and server-side code.  Right now it only lives on
server-side code.

[dcheng, 2017-03-30 00:39:46]

dcheng@chromium.org changed reviewers:
+ sky@chromium.org

[dcheng, 2017-03-30 00:41:25]

dcheng@chromium.org changed reviewers:
+ jochen@chromium.org, sdefresne@chromium.org, xhwang@chromium.org

[dcheng, 2017-03-30 00:41:59]

+sky for //ash, //chrome, //ui
+jochen for //content, //components
+sdefresne for //ios
+xhwang for //media

https://codereview.chromium.org/2776853002/diff/120001/ios/chrome/browser/des...
File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.cc
(right):

https://codereview.chromium.org/2776853002/diff/120001/ios/chrome/browser/des...
ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.cc:78: if
(sms_entrypoint == i) {
On 2017/03/29 23:36:33, Mark P wrote:
> On 2017/03/29 23:21:17, rkaplow (slow) wrote:
> > On 2017/03/29 19:03:36, Mark P wrote:
> > > On 2017/03/29 18:24:40, dcheng wrote:
> > > > On 2017/03/29 18:02:51, rkaplow (slow) wrote:
> > > > > linear looks like the wrong model here as this is supposed to be:
> > > > > 
> > > > > <enum name="DesktopIOSPromotionEntryPoint" type="int">
> > > > >   <int value="1" label="Save Passwords new bubble."/>
> > > > >   <int value="2" label="Bookmarks new bubble."/>
> > > > >   <int value="3" label="Bookmarks footnote."/>
> > > > >   <int value="4" label="HistoryPage."/>
> > > > > </enum>
> > > > > 
> > > > > however I'm not seeing any data for this. maybe it's obsolete anyway?
> add
> > a
> > > > todo
> > > > > to fix or clean it up for an owner?
> > > > 
> > > > To elaborate, are you saying that exact linear is the wrong model
because
> > the
> > > > UMA histogram is defined as an enum?
> > > 
> > > drive-by: it's not obsolete.  Instead, this feature hasn't been launched
> yet. 
> > > (The iOS side to the feature is still under review.)
> > > 
> > > The original enum made sense.  0 is meant to be unused.  These values
relate
> > to
> > > sync, and we're using the 0 value in sync to indicate "not set".  No 0
value
> > is
> > > expected to be in the UMA output, which is why when I revised the metrics
> for
> > > this, I explicitly asked them to leave the 0 out of histograms.xml
> > 
> > I guess this is ok since it would be strange to have an intermediate enum.
> > Again, i wouldn't mind a comment explaining why we need LINEAR here.
> > 
> > Unless you think it's worth have an enum for this subset of the enum, it
just
> > seems a bit overkill to me.
> 
> That would be overkill, especially because it would require keeping this enum
in
> sync between chromium code and server-side code.  Right now it only lives on
> server-side code.

I added a comment... hopefully it makes sense.

https://codereview.chromium.org/2776853002/diff/120001/media/blink/resource_m...
File media/blink/resource_multibuffer_data_provider.cc (right):

https://codereview.chromium.org/2776853002/diff/120001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.cc:266:
UMA_HISTOGRAM_EXACT_LINEAR("Media.UncacheableReason", shift,
On 2017/03/29 23:21:17, rkaplow (slow) wrote:
> On 2017/03/29 18:24:41, dcheng wrote:
> > On 2017/03/29 18:02:51, rkaplow (slow) wrote:
> > > not sure why this can't be an enum either. Add a TODO to fix?
> > 
> > I think it's because the enum itself is a bitmask style enum, so we'd have
to
> > define two separate enums to make this work out nicely. Do you still want a
> > TODO?
> 
> Well, the team can evaluate if they want to change. LINEAR is intended for
> capturing numeric data with linear sized buckets. It may be worth to have the
> bitmask map to an underlying enum and they can use that.
> 
> If we want to keep it like this I'd at least have a comment explaining why
we're
> faking an enum with a linear macro

Done.

[dcheng, 2017-03-30 00:42:02]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 00:42:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2017-03-30 01:23:03]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 01:23:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2017-03-30 03:22:11]

LGTM

https://codereview.chromium.org/2776853002/diff/160001/ui/events/blink/input_...
File ui/events/blink/input_handler_proxy.cc (right):

https://codereview.chromium.org/2776853002/diff/160001/ui/events/blink/input_...
ui/events/blink/input_handler_proxy.cc:560: constexpr uint32_t
mainThreadScrollingReasonEnumMax =
kMainThread (or main_thread_sco...)

[commit-bot: I haz the power, 2017-03-30 04:23:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 04:23:58]

Dry run: This issue passed the CQ dry run.

[xhwang, 2017-03-30 04:45:10]

media/ lgtm

[sdefresne, 2017-03-30 09:14:05]

sdefresne@chromium.org changed reviewers:
+ mrefaat@chromium.org

[sdefresne, 2017-03-30 09:14:06]

lgtm

ios/ looks good to me but you may want to ask mrefaat@ as OWNERS of this code
for a review.

[jochen (gone - plz use gerrit), 2017-03-30 11:52:07]

lgtm

[dcheng, 2017-03-30 18:42:52]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 18:43:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 18:47:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 18:47:17]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[mrefaat, 2017-03-30 18:55:43]

lgtm

[dcheng, 2017-03-30 20:03:37]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 20:04:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2017-03-30 21:00:13]

Description was changed from

==========
Make UMA_HISTOGRAM_ENUMERATION work with scoped enums.

Internally, the macro does some math on the macro arguments, which
doesn't interact well with scoped enums. To work around this, the
internal macro now explicitly casts the enum values to integral
types.

To ensure some modicum of safety, there are new static asserts to
verify that the enum values are in range of HistogramBase::Sample.
This breaks code that was using UMA_HISTOGRAM_ENUMERATION to record
things that aren't C++ enumerations, so switch those to use
UMA_HISTOGRAM_EXACT_LINEAR as well.

BUG=705171
==========

to

==========
Make UMA_HISTOGRAM_ENUMERATION work with scoped enums.

Internally, the macro does some math on the macro arguments, which
doesn't interact well with scoped enums. To work around this, the
internal macro now explicitly casts the enum values to integral
types.

To ensure some modicum of safety, there are new static asserts to
verify that the enum values are in range of HistogramBase::Sample.
This breaks code that was using UMA_HISTOGRAM_ENUMERATION to record
things that aren't C++ enumerations, so switch those to use
UMA_HISTOGRAM_EXACT_LINEAR as well.

BUG=705171
R=rkaplow@chromium.org,jochen@chromium.org,mrefaat@chromium.org,sky@chromium....
TBR=mukai@chromium.org
==========

[dcheng, 2017-03-30 21:00:50]

Description was changed from

==========
Make UMA_HISTOGRAM_ENUMERATION work with scoped enums.

Internally, the macro does some math on the macro arguments, which
doesn't interact well with scoped enums. To work around this, the
internal macro now explicitly casts the enum values to integral
types.

To ensure some modicum of safety, there are new static asserts to
verify that the enum values are in range of HistogramBase::Sample.
This breaks code that was using UMA_HISTOGRAM_ENUMERATION to record
things that aren't C++ enumerations, so switch those to use
UMA_HISTOGRAM_EXACT_LINEAR as well.

BUG=705171
R=rkaplow@chromium.org,jochen@chromium.org,mrefaat@chromium.org,sky@chromium....
TBR=mukai@chromium.org
==========

to

==========
Make UMA_HISTOGRAM_ENUMERATION work with scoped enums.

Internally, the macro does some math on the macro arguments, which
doesn't interact well with scoped enums. To work around this, the
internal macro now explicitly casts the enum values to integral
types.

To ensure some modicum of safety, there are new static asserts to
verify that the enum values are in range of HistogramBase::Sample.
This breaks code that was using UMA_HISTOGRAM_ENUMERATION to record
things that aren't C++ enumerations, so switch those to use
UMA_HISTOGRAM_EXACT_LINEAR as well.

BUG=705171
R=rkaplow,jochen,mrefaat,sky,thestig,xhwang
TBR=mukai
==========

[dcheng, 2017-03-30 21:00:50]

dcheng@chromium.org changed reviewers:
+ mukai@chromium.org

[dcheng, 2017-03-30 21:01:16]

TBRing mukai for noparent webui changes

[dcheng, 2017-03-30 21:01:23]

The CQ bit was unchecked by dcheng@chromium.org

[dcheng, 2017-03-30 21:01:23]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2017-03-30 21:01:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
rkaplow@chromium.org, jochen@chromium.org, sky@chromium.org,
sdefresne@chromium.org, xhwang@chromium.org, mrefaat@chromium.org
Link to the patchset: https://codereview.chromium.org/2776853002/#ps200001
(title: "rebase")

[commit-bot: I haz the power, 2017-03-30 21:02:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 23:56:02]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1490907683875020,
"parent_rev": "27793e2b00cd4f06e24cde9599be5693006ed02b", "commit_rev":
"f6f4e494cd797690b41c4359c08bc33d49811759"}

[commit-bot: I haz the power, 2017-03-30 23:57:08]

Description was changed from

==========
Make UMA_HISTOGRAM_ENUMERATION work with scoped enums.

Internally, the macro does some math on the macro arguments, which
doesn't interact well with scoped enums. To work around this, the
internal macro now explicitly casts the enum values to integral
types.

To ensure some modicum of safety, there are new static asserts to
verify that the enum values are in range of HistogramBase::Sample.
This breaks code that was using UMA_HISTOGRAM_ENUMERATION to record
things that aren't C++ enumerations, so switch those to use
UMA_HISTOGRAM_EXACT_LINEAR as well.

BUG=705171
R=rkaplow,jochen,mrefaat,sky,thestig,xhwang
TBR=mukai
==========

to

==========
Make UMA_HISTOGRAM_ENUMERATION work with scoped enums.

Internally, the macro does some math on the macro arguments, which
doesn't interact well with scoped enums. To work around this, the
internal macro now explicitly casts the enum values to integral
types.

To ensure some modicum of safety, there are new static asserts to
verify that the enum values are in range of HistogramBase::Sample.
This breaks code that was using UMA_HISTOGRAM_ENUMERATION to record
things that aren't C++ enumerations, so switch those to use
UMA_HISTOGRAM_EXACT_LINEAR as well.

BUG=705171
R=rkaplow,jochen,mrefaat,sky,thestig,xhwang
TBR=mukai

Review-Url: https://codereview.chromium.org/2776853002
Cr-Commit-Position: refs/heads/master@{#460938}
Committed:
https://chromium.googlesource.com/chromium/src/+/f6f4e494cd797690b41c4359c08b...
==========

[commit-bot: I haz the power, 2017-03-30 23:57:10]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/f6f4e494cd797690b41c4359c08b...

[Ilya Sherman, 2017-03-31 00:28:13]

isherman@chromium.org changed reviewers:
+ isherman@chromium.org

[Ilya Sherman, 2017-03-31 00:28:15]

Thanks for working on this.

https://codereview.chromium.org/2776853002/diff/200001/base/metrics/histogram...
File base/metrics/histogram_macros_internal.h (right):

https://codereview.chromium.org/2776853002/diff/200001/base/metrics/histogram...
base/metrics/histogram_macros_internal.h:139: // types.
It looks to me like the current code allows, say, doubles to be passed in as
arguments to this macro.  Is that working as intended?

[dcheng, 2017-03-31 00:46:00]

https://codereview.chromium.org/2776853002/diff/200001/base/metrics/histogram...
File base/metrics/histogram_macros_internal.h (right):

https://codereview.chromium.org/2776853002/diff/200001/base/metrics/histogram...
base/metrics/histogram_macros_internal.h:139: // types.
On 2017/03/31 00:28:14, Ilya Sherman wrote:
> It looks to me like the current code allows, say, doubles to be passed in as
> arguments to this macro.  Is that working as intended?

I'll be addressing this TODO shortly. I expect to find interesting code in the
process, but the end state should be relatively type safe (well, as safe as a
macro is going to be =)

[Ilya Sherman, 2017-03-31 00:47:15]

https://codereview.chromium.org/2776853002/diff/200001/base/metrics/histogram...
File base/metrics/histogram_macros_internal.h (right):

https://codereview.chromium.org/2776853002/diff/200001/base/metrics/histogram...
base/metrics/histogram_macros_internal.h:139: // types.
On 2017/03/31 00:46:00, dcheng wrote:
> On 2017/03/31 00:28:14, Ilya Sherman wrote:
> > It looks to me like the current code allows, say, doubles to be passed in as
> > arguments to this macro.  Is that working as intended?
> 
> I'll be addressing this TODO shortly. I expect to find interesting code in the
> process, but the end state should be relatively type safe (well, as safe as a
> macro is going to be =)

Okay, lovely -- thanks!