Use stricter type checking in UMA_HISTOGRAM_ENUMERATION

Use stricter type checking in UMA_HISTOGRAM_ENUMERATION

Add one more type checking: if |boundary| is enum, |sample| must be
an enum as well.

Common violations:
1) Unnecessary casting to int: fixed by retaining types.
2) Casting to int for convenience or bitfield: fixed by static_cast'ing.
3) Ranges: use UMA_HISTOGRAM_EXACT_LINEAR instead.

BUG=661401
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

[wychen, 2017-05-17 07:30:58]

Description was changed from

==========
Use stricter type checking in UMA_HISTOGRAM_ENUMERATION

BUG=661401
==========

to

==========
Use stricter type checking in UMA_HISTOGRAM_ENUMERATION

BUG=661401
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[wychen, 2017-05-17 07:32:44]

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

[commit-bot: I haz the power, 2017-05-17 07:33:25]

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-05-17 07:44:20]

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

[commit-bot: I haz the power, 2017-05-17 07:44:21]

Dry run: Try jobs failed on following builders:
  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...)

[wychen, 2017-05-31 00:32:52]

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

[commit-bot: I haz the power, 2017-05-31 00:33:30]

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-05-31 00:48:31]

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

[commit-bot: I haz the power, 2017-05-31 00:48:32]

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

[wychen, 2017-05-31 01:08:13]

Description was changed from

==========
Use stricter type checking in UMA_HISTOGRAM_ENUMERATION

BUG=661401
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Use stricter type checking in UMA_HISTOGRAM_ENUMERATION

Add one more type checking: if |boundary| is enum, |sample|
must be the same type of enum.

Common violations:
1) Unnecessary casting to int: fixed by retaining types.
2) Casting to int for convenience: fixed by static_cast'ing.
2) Bit-field or ranges: use UMA_HISTOGRAM_EXACT_LINEAR instead.

BUG=661401
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[wychen, 2017-05-31 01:09:39]

Description was changed from

==========
Use stricter type checking in UMA_HISTOGRAM_ENUMERATION

Add one more type checking: if |boundary| is enum, |sample|
must be the same type of enum.

Common violations:
1) Unnecessary casting to int: fixed by retaining types.
2) Casting to int for convenience: fixed by static_cast'ing.
2) Bit-field or ranges: use UMA_HISTOGRAM_EXACT_LINEAR instead.

BUG=661401
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Use stricter type checking in UMA_HISTOGRAM_ENUMERATION

Add one more type checking: if |boundary| is enum, |sample| must be
the same type of enum.

Common violations:
1) Unnecessary casting to int: fixed by retaining types.
2) Casting to int for convenience: fixed by static_cast'ing.
3) Bit-field or ranges: use UMA_HISTOGRAM_EXACT_LINEAR instead.

BUG=661401
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[wychen, 2017-05-31 05:27:41]

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

[commit-bot: I haz the power, 2017-05-31 05:28:00]

Dry run: CQ is trying da patch.

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

[wychen, 2017-05-31 05:28:21]

Description was changed from

==========
Use stricter type checking in UMA_HISTOGRAM_ENUMERATION

Add one more type checking: if |boundary| is enum, |sample| must be
the same type of enum.

Common violations:
1) Unnecessary casting to int: fixed by retaining types.
2) Casting to int for convenience: fixed by static_cast'ing.
3) Bit-field or ranges: use UMA_HISTOGRAM_EXACT_LINEAR instead.

BUG=661401
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Use stricter type checking in UMA_HISTOGRAM_ENUMERATION

Add one more type checking: if |boundary| is enum, |sample| must be
an enum as well.

Common violations:
1) Unnecessary casting to int: fixed by retaining types.
2) Casting to int for convenience: fixed by static_cast'ing.
3) Bit-field or ranges: use UMA_HISTOGRAM_EXACT_LINEAR instead.

BUG=661401
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[wychen, 2017-05-31 06:42:45]

wychen@chromium.org changed reviewers:
+ asvitkine@chromium.org, dcheng@chromium.org

[wychen, 2017-05-31 06:42:46]

PTAL

[Alexei Svitkine (slow), 2017-05-31 20:22:25]

Some comments below.

I suggest splitting all the simple cases (where you just add the correct types
to function params, etc) into a separate CL - which can be landed first to keep
this CL to the more tricky bits.

https://codereview.chromium.org/2886933003/diff/100001/components/sync/driver...
File components/sync/driver/frontend_data_type_controller.cc (right):

https://codereview.chromium.org/2886933003/diff/100001/components/sync/driver...
components/sync/driver/frontend_data_type_controller.cc:241: // TODO(wychen):
enum uma should be strongly typed. crbug.com/661401
Can't all of these sync ones just use the ModelType enum?

https://cs.chromium.org/chromium/src/components/sync/base/model_type.h?rcl=2f...

https://codereview.chromium.org/2886933003/diff/100001/content/browser/downlo...
File content/browser/download/download_stats.cc (right):

https://codereview.chromium.org/2886933003/diff/100001/content/browser/downlo...
content/browser/download/download_stats.cc:919:
static_cast<int>(ORIGIN_STATE_ON_RESUMPTION_MAX));
But these are actual enums, as defined in XML.

I don't think we should be changing to use EXACT_LINEAR macro when logically
these are enums. Can the types of the function params be updated instead?

https://codereview.chromium.org/2886933003/diff/100001/ios/chrome/browser/phy...
File ios/chrome/browser/physical_web/physical_web_initial_state_recorder.mm
(right):

https://codereview.chromium.org/2886933003/diff/100001/ios/chrome/browser/phy...
ios/chrome/browser/physical_web/physical_web_initial_state_recorder.mm:168:
static_cast<int>(PHYSICAL_WEB_INITIAL_STATE_COUNT));
Can this use PhysicalWebInitialState instead? Since there is an enum for it.

https://codereview.chromium.org/2886933003/diff/100001/ios/chrome/today_exten...
File ios/chrome/today_extension/today_view_controller.mm (right):

https://codereview.chromium.org/2886933003/diff/100001/ios/chrome/today_exten...
ios/chrome/today_extension/today_view_controller.mm:930:
UMA_HISTOGRAM_EXACT_LINEAR(
Same comment as physical_web_initial_state_recorder.mm.

[wychen, 2017-07-14 23:17:10]

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

[commit-bot: I haz the power, 2017-07-14 23:17:23]

Dry run: CQ is trying da patch.

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

[wychen, 2017-07-14 23:18:45]

https://codereview.chromium.org/2886933003/diff/100001/components/sync/driver...
File components/sync/driver/frontend_data_type_controller.cc (right):

https://codereview.chromium.org/2886933003/diff/100001/components/sync/driver...
components/sync/driver/frontend_data_type_controller.cc:241: // TODO(wychen):
enum uma should be strongly typed. crbug.com/661401
On 2017/05/31 20:22:24, Alexei Svitkine (slow) wrote:
> Can't all of these sync ones just use the ModelType enum?
> 
>
https://cs.chromium.org/chromium/src/components/sync/base/model_type.h?rcl=2f...

It takes a much larger CL to do the types correctly.
There's a lookup table for it, and directly using ModelType would break backward
compatibility.

https://cs.chromium.org/chromium/src/components/sync/syncable/model_type.cc?l...

https://codereview.chromium.org/2886933003/diff/100001/content/browser/downlo...
File content/browser/download/download_stats.cc (right):

https://codereview.chromium.org/2886933003/diff/100001/content/browser/downlo...
content/browser/download/download_stats.cc:919:
static_cast<int>(ORIGIN_STATE_ON_RESUMPTION_MAX));
On 2017/05/31 20:22:24, Alexei Svitkine (slow) wrote:
> But these are actual enums, as defined in XML.
> 
> I don't think we should be changing to use EXACT_LINEAR macro when logically
> these are enums. Can the types of the function params be updated instead?

Fixed. Some static casts are still needed because this enum is inherently a
bitmask.

https://codereview.chromium.org/2886933003/diff/100001/ios/chrome/browser/phy...
File ios/chrome/browser/physical_web/physical_web_initial_state_recorder.mm
(right):

https://codereview.chromium.org/2886933003/diff/100001/ios/chrome/browser/phy...
ios/chrome/browser/physical_web/physical_web_initial_state_recorder.mm:168:
static_cast<int>(PHYSICAL_WEB_INITIAL_STATE_COUNT));
On 2017/05/31 20:22:24, Alexei Svitkine (slow) wrote:
> Can this use PhysicalWebInitialState instead? Since there is an enum for it.

Used PhysicalWebInitialStateIosChrome.

https://codereview.chromium.org/2886933003/diff/100001/ios/chrome/today_exten...
File ios/chrome/today_extension/today_view_controller.mm (right):

https://codereview.chromium.org/2886933003/diff/100001/ios/chrome/today_exten...
ios/chrome/today_extension/today_view_controller.mm:930:
UMA_HISTOGRAM_EXACT_LINEAR(
On 2017/05/31 20:22:24, Alexei Svitkine (slow) wrote:
> Same comment as physical_web_initial_state_recorder.mm.

Used PhysicalWebInitialStateIosChrome.

[commit-bot: I haz the power, 2017-07-14 23:42:17]

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

[commit-bot: I haz the power, 2017-07-14 23:42:18]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[wychen, 2017-07-15 08:42:59]

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

[commit-bot: I haz the power, 2017-07-15 08:43:06]

Dry run: CQ is trying da patch.

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

[wychen, 2017-07-15 08:59:06]

Description was changed from

==========
Use stricter type checking in UMA_HISTOGRAM_ENUMERATION

Add one more type checking: if |boundary| is enum, |sample| must be
an enum as well.

Common violations:
1) Unnecessary casting to int: fixed by retaining types.
2) Casting to int for convenience: fixed by static_cast'ing.
3) Bit-field or ranges: use UMA_HISTOGRAM_EXACT_LINEAR instead.

BUG=661401
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Use stricter type checking in UMA_HISTOGRAM_ENUMERATION

Add one more type checking: if |boundary| is enum, |sample| must be
an enum as well.

Common violations:
1) Unnecessary casting to int: fixed by retaining types.
2) Casting to int for convenience or bitfield: fixed by static_cast'ing.
3) Ranges: use UMA_HISTOGRAM_EXACT_LINEAR instead.

BUG=661401
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2017-07-15 11:28:37]

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

[commit-bot: I haz the power, 2017-07-15 11:28:38]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-07-17 09:14:00]

(I mostly only looked at //base, let me know if I need to look at other things
too)

https://codereview.chromium.org/2886933003/diff/160001/base/android/library_l...
File base/android/library_loader/library_loader_hooks.cc (right):

https://codereview.chromium.org/2886933003/diff/160001/base/android/library_l...
base/android/library_loader/library_loader_hooks.cc:130:
LIBRARY_LOAD_FROM_APK_STATUS_CODES_MAX);
Hmm a little sad. I would personally if we did the static cast closer to the
point where we got it back from Java, but that's just a nit

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

https://codereview.chromium.org/2886933003/diff/160001/base/metrics/histogram...
base/metrics/histogram_macros_internal.h:143:
!std::is_enum<std::remove_const<std::remove_reference<decltype(        \
I think std::decay can replace the use of
std::remove_const<std::remove_reference<...>> here and below.

[wychen, 2017-07-17 22:43:09]

https://codereview.chromium.org/2886933003/#ps180001 is split to
https://chromium-review.googlesource.com/c/575380/2 and
https://chromium-review.googlesource.com/c/575428/2.

https://codereview.chromium.org/2886933003/diff/160001/base/android/library_l...
File base/android/library_loader/library_loader_hooks.cc (right):

https://codereview.chromium.org/2886933003/diff/160001/base/android/library_l...
base/android/library_loader/library_loader_hooks.cc:130:
LIBRARY_LOAD_FROM_APK_STATUS_CODES_MAX);
On 2017/07/17 09:13:59, dcheng wrote:
> Hmm a little sad. I would personally if we did the static cast closer to the
> point where we got it back from Java, but that's just a nit

This function is the immediate JNI function, and this variable is only used
here. Does it look better this way?

  LibraryLoadFromApkStatusCodes library_load_from_apk_status_code =
      library_load_from_apk_status;

  UMA_HISTOGRAM_ENUMERATION(
      "ChromiumAndroidLinker.LibraryLoadFromApkStatus",
      library_load_from_apk_status_code,
      LIBRARY_LOAD_FROM_APK_STATUS_CODES_MAX);

[dcheng, 2017-07-18 10:28:07]

https://codereview.chromium.org/2886933003/diff/160001/base/android/library_l...
File base/android/library_loader/library_loader_hooks.cc (right):

https://codereview.chromium.org/2886933003/diff/160001/base/android/library_l...
base/android/library_loader/library_loader_hooks.cc:130:
LIBRARY_LOAD_FROM_APK_STATUS_CODES_MAX);
On 2017/07/17 22:43:09, wychen wrote:
> On 2017/07/17 09:13:59, dcheng wrote:
> > Hmm a little sad. I would personally if we did the static cast closer to the
> > point where we got it back from Java, but that's just a nit
> 
> This function is the immediate JNI function, and this variable is only used
> here. Does it look better this way?
> 
>   LibraryLoadFromApkStatusCodes library_load_from_apk_status_code =
>       library_load_from_apk_status;
> 
>   UMA_HISTOGRAM_ENUMERATION(
>       "ChromiumAndroidLinker.LibraryLoadFromApkStatus",
>       library_load_from_apk_status_code,
>       LIBRARY_LOAD_FROM_APK_STATUS_CODES_MAX);

Ah... I thought this was called as a helper. I didn't realize that JNI would
work for functions with internal linkage. I guess the existing code is fine =)

[wychen, 2017-07-18 21:43:41]

https://codereview.chromium.org/2886933003/diff/160001/base/android/library_l...
File base/android/library_loader/library_loader_hooks.cc (right):

https://codereview.chromium.org/2886933003/diff/160001/base/android/library_l...
base/android/library_loader/library_loader_hooks.cc:130:
LIBRARY_LOAD_FROM_APK_STATUS_CODES_MAX);
On 2017/07/18 10:28:07, dcheng wrote:
> On 2017/07/17 22:43:09, wychen wrote:
> > On 2017/07/17 09:13:59, dcheng wrote:
> > > Hmm a little sad. I would personally if we did the static cast closer to
the
> > > point where we got it back from Java, but that's just a nit
> > 
> > This function is the immediate JNI function, and this variable is only used
> > here. Does it look better this way?
> > 
> >   LibraryLoadFromApkStatusCodes library_load_from_apk_status_code =
> >       library_load_from_apk_status;
> > 
> >   UMA_HISTOGRAM_ENUMERATION(
> >       "ChromiumAndroidLinker.LibraryLoadFromApkStatus",
> >       library_load_from_apk_status_code,
> >       LIBRARY_LOAD_FROM_APK_STATUS_CODES_MAX);
> 
> Ah... I thought this was called as a helper. I didn't realize that JNI would
> work for functions with internal linkage. I guess the existing code is fine =)

Got it. Let's move on to the split CLs then.