Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(8)

Issue 2886933003: Use stricter type checking in UMA_HISTOGRAM_ENUMERATION (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 1 week ago by wychen
Modified:
2 months, 1 week ago
CC:
chromium-reviews, dominickn+watch_chromium.org, vmpstr+watch_chromium.org, tburkard+watch_chromium.org, dtapuska+chromiumwatch_chromium.org, cbentzel+watch_chromium.org, awdf+watch_chromium.org, jam, net-reviews_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mlamouri+watch-content_chromium.org, vabr+watchlistpasswordmanager_chromium.org, Peter Beverloo, speed-metrics-reviews_chromium.org, mlamouri+watch-notifications_chromium.org, csharrison+watch_chromium.org, asvitkine+watch_chromium.org, piman+watch_chromium.org, gcasto+watchlist_chromium.org, sync-reviews_chromium.org, David Trainor- moved to gerrit, gavinp+prer_chromium.org, tfarina, loading-reviews+metrics_chromium.org, danakj+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : fix android #

Patch Set 3 : chromeos #

Patch Set 4 : rebase #

Patch Set 5 : ios and mac #

Patch Set 6 : nocompile test #

Total comments: 8

Patch Set 7 : address comments #

Patch Set 8 : rebase #

Patch Set 9 : fix new errors #

Total comments: 5

Patch Set 10 : simplify type checking #

Unified diffs Side-by-side diffs Delta from patch set Stats (+221 lines, -170 lines) Patch
M ash/metrics/user_metrics_recorder.cc View 1 2 3 4 5 6 7 1 chunk +5 lines, -4 lines 0 comments Download
M ash/shelf/shelf_view.cc View 1 2 3 4 5 6 7 1 chunk +5 lines, -4 lines 0 comments Download
M base/android/library_loader/library_loader_hooks.cc View 1 2 3 4 5 6 7 1 chunk +4 lines, -3 lines 0 comments Download
M base/debug/activity_tracker.cc View 1 2 3 4 5 6 7 1 chunk +2 lines, -2 lines 0 comments Download
M base/metrics/histogram_functions_unittest.cc View 1 chunk +1 line, -1 line 0 comments Download
M base/metrics/histogram_macros_internal.h View 1 2 3 4 5 6 7 8 9 1 chunk +8 lines, -4 lines 0 comments Download
M base/metrics/histogram_unittest.nc View 1 2 3 4 5 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/browser/android/metrics/launch_metrics.cc View 1 1 chunk +4 lines, -3 lines 0 comments Download
M chrome/browser/captive_portal/captive_portal_service.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/chromeos/app_mode/kiosk_app_launch_error.cc View 1 2 1 chunk +6 lines, -4 lines 0 comments Download
M chrome/browser/chromeos/login/reauth_stats.cc View 1 2 1 chunk +6 lines, -3 lines 0 comments Download
M chrome/browser/chromeos/login/screens/reset_screen.cc View 1 2 4 chunks +10 lines, -8 lines 0 comments Download
M chrome/browser/chromeos/policy/upload_job_impl.cc View 1 2 3 4 5 6 7 1 chunk +3 lines, -2 lines 0 comments Download
M chrome/browser/diagnostics/diagnostics_test.cc View 1 2 1 chunk +5 lines, -6 lines 0 comments Download
M chrome/browser/engagement/important_sites_util.cc View 1 2 3 4 5 6 7 1 chunk +15 lines, -14 lines 0 comments Download
M chrome/browser/media/android/remote/record_cast_action.cc View 1 2 chunks +6 lines, -3 lines 0 comments Download
M chrome/browser/metrics/android_metrics_provider.cc View 1 1 chunk +5 lines, -4 lines 0 comments Download
M chrome/browser/notifications/message_center_stats_collector.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/policy/cloud/cloud_policy_invalidator.h View 2 chunks +4 lines, -2 lines 0 comments Download
M chrome/browser/policy/cloud/cloud_policy_invalidator.cc View 2 chunks +5 lines, -3 lines 0 comments Download
M chrome/browser/prerender/prerender_link_manager.cc View 1 chunk +4 lines, -2 lines 0 comments Download
M chrome/browser/ui/toolbar/app_menu_model.h View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/toolbar/app_menu_model.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/webui/app_launcher_login_handler.h View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/webui/app_launcher_login_handler.cc View 1 chunk +3 lines, -9 lines 0 comments Download
M chrome/browser/ui/webui/signin/inline_login_handler_impl.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M components/leveldb/public/cpp/util.h View 2 chunks +3 lines, -1 line 0 comments Download
M components/leveldb/public/cpp/util.cc View 1 chunk +2 lines, -1 line 0 comments Download
M components/metrics/stability_metrics_helper.cc View 1 2 3 4 5 6 7 2 chunks +2 lines, -2 lines 0 comments Download
M components/password_manager/core/browser/password_syncable_service.cc View 1 2 3 4 5 6 7 1 chunk +2 lines, -1 line 0 comments Download
M components/payments/core/journey_logger.cc View 1 2 3 4 5 6 7 8 2 chunks +6 lines, -3 lines 0 comments Download
M components/policy/core/common/cloud/device_management_service.cc View 1 2 3 4 5 6 7 1 chunk +3 lines, -3 lines 0 comments Download
M components/search_provider_logos/logo_tracker.cc View 1 2 3 4 5 6 7 1 chunk +2 lines, -1 line 0 comments Download
M components/signin/core/browser/signin_metrics.h View 1 2 3 4 5 6 7 3 chunks +3 lines, -3 lines 0 comments Download
M components/signin/core/browser/signin_metrics.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M components/sync/driver/async_directory_type_controller.cc View 1 chunk +3 lines, -1 line 0 comments Download
M components/sync/driver/data_type_manager_impl.cc View 1 2 3 4 5 6 7 1 chunk +2 lines, -1 line 0 comments Download
M components/sync/driver/frontend_data_type_controller.cc View 1 chunk +3 lines, -1 line 0 comments Download
M components/sync/driver/model_association_manager.cc View 1 chunk +2 lines, -1 line 0 comments Download
M components/sync/driver/model_type_controller.cc View 1 2 3 4 5 6 7 2 chunks +5 lines, -2 lines 0 comments Download
M components/sync/driver/startup_controller.cc View 1 chunk +3 lines, -1 line 0 comments Download
M components/sync/engine_impl/model_type_registry.cc View 1 chunk +4 lines, -2 lines 0 comments Download
M components/sync/model/data_type_error_handler_impl.cc View 1 chunk +2 lines, -1 line 0 comments Download
M components/sync_bookmarks/bookmark_model_associator.cc View 1 chunk +2 lines, -1 line 0 comments Download
M content/browser/browser_child_process_host_impl.h View 1 chunk +1 line, -1 line 0 comments Download
M content/browser/browser_child_process_host_impl.cc View 1 2 3 4 5 6 7 5 chunks +9 lines, -8 lines 0 comments Download
M content/browser/download/download_item_impl.cc View 1 2 3 4 5 6 7 1 chunk +2 lines, -1 line 0 comments Download
M content/browser/download/download_stats.h View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M content/browser/download/download_stats.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M content/browser/gpu/gpu_process_host.cc View 1 2 3 4 5 6 7 3 chunks +10 lines, -8 lines 0 comments Download
M content/renderer/input/render_widget_input_handler.cc View 1 2 3 4 5 6 7 2 chunks +2 lines, -2 lines 0 comments Download
M ios/chrome/browser/physical_web/physical_web_initial_state_recorder.mm View 1 2 3 4 5 6 7 1 chunk +4 lines, -2 lines 0 comments Download
M ios/chrome/browser/ui/browser_view_controller.mm View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M ios/chrome/browser/ui/contextual_search/contextual_search_metrics.mm View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M ios/chrome/browser/ui/sync/sync_util.mm View 1 2 3 4 5 6 7 2 chunks +2 lines, -2 lines 0 comments Download
M ios/chrome/today_extension/today_view_controller.mm View 1 2 3 4 5 6 7 1 chunk +2 lines, -1 line 0 comments Download
M media/audio/audio_output_resampler.cc View 1 2 3 2 chunks +12 lines, -20 lines 0 comments Download
M net/dns/host_resolver_impl.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M net/quic/chromium/quic_connection_logger.cc View 1 2 3 4 5 6 7 1 chunk +2 lines, -1 line 0 comments Download
M net/ssl/ssl_connection_status_flags.h View 2 chunks +5 lines, -4 lines 0 comments Download
Trybot results:
Commit queue not available (can’t edit this change).

Messages

Total messages: 31 (24 generated)
wychen
PTAL
3 months, 3 weeks ago (2017-05-31 06:42:46 UTC) #16
Alexei Svitkine (slow)
Some comments below. I suggest splitting all the simple cases (where you just add the ...
3 months, 3 weeks ago (2017-05-31 20:22:25 UTC) #17
wychen
https://codereview.chromium.org/2886933003/diff/100001/components/sync/driver/frontend_data_type_controller.cc File components/sync/driver/frontend_data_type_controller.cc (right): https://codereview.chromium.org/2886933003/diff/100001/components/sync/driver/frontend_data_type_controller.cc#newcode241 components/sync/driver/frontend_data_type_controller.cc:241: // TODO(wychen): enum uma should be strongly typed. crbug.com/661401 ...
2 months, 1 week ago (2017-07-14 23:18:45 UTC) #20
dcheng
(I mostly only looked at //base, let me know if I need to look at ...
2 months, 1 week ago (2017-07-17 09:14:00 UTC) #28
wychen
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_loader/library_loader_hooks.cc File base/android/library_loader/library_loader_hooks.cc (right): https://codereview.chromium.org/2886933003/diff/160001/base/android/library_loader/library_loader_hooks.cc#newcode130 base/android/library_loader/library_loader_hooks.cc:130: LIBRARY_LOAD_FROM_APK_STATUS_CODES_MAX); On ...
2 months, 1 week ago (2017-07-17 22:43:09 UTC) #29
dcheng
https://codereview.chromium.org/2886933003/diff/160001/base/android/library_loader/library_loader_hooks.cc File base/android/library_loader/library_loader_hooks.cc (right): https://codereview.chromium.org/2886933003/diff/160001/base/android/library_loader/library_loader_hooks.cc#newcode130 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 ...
2 months, 1 week ago (2017-07-18 10:28:07 UTC) #30
wychen
2 months, 1 week ago (2017-07-18 21:43:41 UTC) #31
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b