Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(53)

Issue 2248793002: Remove OnRecordingDisabled() metrics client interface. (Closed)

Created:
4 years, 4 months ago by Alexei Svitkine (slow)
Modified:
4 years, 4 months ago
CC:
chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, halliwell+watch_chromium.org, lcwu+watch_chromium.org, gcasto+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, asvitkine+watch_chromium.org, lethalantidote+watch-blimp_chromium.org, alokp+watch_chromium.org, dtrainor+watch-blimp_chromium.org, android-webview-reviews_chromium.org, khushalsagar+watch-blimp_chromium.org, sdefresne+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Remove OnRecordingDisabled() metrics client interface. This was there so that crash client id could be cleared. However, this was being called out of MetricsService::Stop(), so it needed a special provision to not clear it when shutting down, for example. This change instead removes all of that plumbing and makes the id clearing happen in the same place we clear the UMA client id, when the user actually opts-out (as opposed to just metrics service stopping). Unifies the prefs manipulation that happens when the user changes the setting into a helper function. As a follow up change after this lands, we should also make that function be called on iOS. BUG=635669 TBR=sgurun@chromium.org,nyquist@chromium.org,noyau@chromium.org Committed: https://crrev.com/12aec333ac6595deb79bc1ea6a59d933e3c11b9a Cr-Commit-Position: refs/heads/master@{#412568}

Patch Set 1 #

Patch Set 2 : add helper function #

Total comments: 4

Patch Set 3 : Remove crash_keys import. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+30 lines, -67 lines) Patch
M android_webview/browser/aw_metrics_service_client.h View 1 chunk +0 lines, -1 line 0 comments Download
M android_webview/browser/aw_metrics_service_client.cc View 1 chunk +2 lines, -4 lines 0 comments Download
M blimp/engine/app/blimp_metrics_service_client.h View 1 chunk +0 lines, -1 line 0 comments Download
M blimp/engine/app/blimp_metrics_service_client.cc View 1 chunk +1 line, -4 lines 0 comments Download
M chrome/browser/android/metrics/uma_session_stats.cc View 1 2 2 chunks +2 lines, -11 lines 0 comments Download
M chrome/browser/metrics/chrome_metrics_service_client.h View 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/metrics/chrome_metrics_service_client.cc View 1 chunk +0 lines, -7 lines 0 comments Download
M chrome/browser/metrics/metrics_reporting_state.h View 1 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/metrics/metrics_reporting_state.cc View 1 3 chunks +20 lines, -19 lines 0 comments Download
M chromecast/browser/metrics/cast_metrics_service_client.h View 1 chunk +0 lines, -1 line 0 comments Download
M chromecast/browser/metrics/cast_metrics_service_client.cc View 1 chunk +0 lines, -3 lines 0 comments Download
M components/metrics/metrics_service.cc View 1 chunk +0 lines, -2 lines 0 comments Download
M components/metrics/metrics_service_client.h View 1 chunk +0 lines, -4 lines 0 comments Download
M components/metrics/test_metrics_service_client.h View 1 chunk +0 lines, -1 line 0 comments Download
M components/metrics/test_metrics_service_client.cc View 1 chunk +0 lines, -3 lines 0 comments Download
M ios/chrome/browser/metrics/ios_chrome_metrics_service_client.h View 1 chunk +0 lines, -1 line 0 comments Download
M ios/chrome/browser/metrics/ios_chrome_metrics_service_client.cc View 1 chunk +0 lines, -4 lines 0 comments Download

Messages

Total messages: 39 (26 generated)
Alexei Svitkine (slow)
4 years, 4 months ago (2016-08-15 22:34:37 UTC) #17
Alexei Svitkine (slow)
Friendly ping
4 years, 4 months ago (2016-08-16 16:59:16 UTC) #20
jwd
https://codereview.chromium.org/2248793002/diff/60001/chrome/browser/metrics/metrics_reporting_state.cc File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/2248793002/diff/60001/chrome/browser/metrics/metrics_reporting_state.cc#newcode124 chrome/browser/metrics/metrics_reporting_state.cc:124: void UpdateMetricsPrefsOnPermissionChange(bool metrics_enabled) { Didn't think to ask this ...
4 years, 4 months ago (2016-08-16 20:30:05 UTC) #21
Alexei Svitkine (slow)
https://codereview.chromium.org/2248793002/diff/60001/chrome/browser/metrics/metrics_reporting_state.cc File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/2248793002/diff/60001/chrome/browser/metrics/metrics_reporting_state.cc#newcode124 chrome/browser/metrics/metrics_reporting_state.cc:124: void UpdateMetricsPrefsOnPermissionChange(bool metrics_enabled) { On 2016/08/16 20:30:04, jwd wrote: ...
4 years, 4 months ago (2016-08-16 21:07:50 UTC) #22
gayane -on leave until 09-2017
LGTM https://codereview.chromium.org/2248793002/diff/60001/chrome/browser/android/metrics/uma_session_stats.cc File chrome/browser/android/metrics/uma_session_stats.cc (right): https://codereview.chromium.org/2248793002/diff/60001/chrome/browser/android/metrics/uma_session_stats.cc#newcode16 chrome/browser/android/metrics/uma_session_stats.cc:16: #include "chrome/common/crash_keys.h" need this?
4 years, 4 months ago (2016-08-17 15:57:54 UTC) #23
jwd
lgtm
4 years, 4 months ago (2016-08-17 16:23:33 UTC) #24
Alexei Svitkine (slow)
https://codereview.chromium.org/2248793002/diff/60001/chrome/browser/android/metrics/uma_session_stats.cc File chrome/browser/android/metrics/uma_session_stats.cc (right): https://codereview.chromium.org/2248793002/diff/60001/chrome/browser/android/metrics/uma_session_stats.cc#newcode16 chrome/browser/android/metrics/uma_session_stats.cc:16: #include "chrome/common/crash_keys.h" On 2016/08/17 15:57:54, gayane wrote: > need ...
4 years, 4 months ago (2016-08-17 16:26:38 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2248793002/80001
4 years, 4 months ago (2016-08-17 16:27:38 UTC) #28
commit-bot: I haz the power
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_presubmit/builds/239943)
4 years, 4 months ago (2016-08-17 16:35:52 UTC) #30
Alexei Svitkine (slow)
TBR'ing some owners of MetricsServiceClient implementations since I'm changing the interface. TBR noyau for ios/chrome/browser/metrics/ios_chrome_metrics_service_client.cc ...
4 years, 4 months ago (2016-08-17 16:43:07 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2248793002/80001
4 years, 4 months ago (2016-08-17 16:43:49 UTC) #35
commit-bot: I haz the power
Committed patchset #3 (id:80001)
4 years, 4 months ago (2016-08-17 17:09:53 UTC) #37
commit-bot: I haz the power
4 years, 4 months ago (2016-08-17 17:12:42 UTC) #39
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/12aec333ac6595deb79bc1ea6a59d933e3c11b9a
Cr-Commit-Position: refs/heads/master@{#412568}

Powered by Google App Engine
This is Rietveld 408576698