Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2962523003:
Improve histogram and actions for CBD dialog (Closed)
DescriptionImprove histogram and actions for CBD dialog
Add histogram for time spent in Clear Browsing Data dialog.
Add actions for tab changes.
Fix incorrect logging of timeperiod changes by registering listeners
after time picker is fully initialized.
CBD metrics: https://docs.google.com/presentation/d/1uWaHywa3UVkXecsDdhL-24C5iyqAT5UWdoPh5PSwkPA/edit#slide=id.g1e32513c06_0_1
BUG=681523
Review-Url: https://codereview.chromium.org/2962523003
Cr-Commit-Position: refs/heads/master@{#482942}
Committed: https://chromium.googlesource.com/chromium/src/+/a82b3e3179a055144fab8894e03368700a7f153d
Patch Set 1 #Patch Set 2 : Record from java #
Total comments: 5
Patch Set 3 : Improve histogram summary #
Messages
Total messages: 28 (17 generated)
Description was changed from ========== Add histogram for time spent in CBD dialog BUG=681523 ========== to ========== Improve histogram and actions for CBD dialog Add histogram for time spent in Clear Browsing Data dialog. Add histogram for tab changes. Fix incorrect logging of timeperiod. BUG=681523 ==========
Description was changed from ========== Improve histogram and actions for CBD dialog Add histogram for time spent in Clear Browsing Data dialog. Add histogram for tab changes. Fix incorrect logging of timeperiod. BUG=681523 ========== to ========== Improve histogram and actions for CBD dialog Add histogram for time spent in Clear Browsing Data dialog. Add histogram for tab changes. Fix incorrect logging of timeperiod, due to change event on creating the dialog. BUG=681523 ==========
The CQ bit was checked by dullweber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Improve histogram and actions for CBD dialog Add histogram for time spent in Clear Browsing Data dialog. Add histogram for tab changes. Fix incorrect logging of timeperiod, due to change event on creating the dialog. BUG=681523 ========== to ========== Improve histogram and actions for CBD dialog Add histogram for time spent in Clear Browsing Data dialog. Add histogram for tab changes. Fix incorrect logging of timeperiod by registering listeners after time picker is fully initialized. BUG=681523 ==========
dullweber@chromium.org changed reviewers: + msramek@chromium.org
Hi Martin, please take a look at these histogram and action changes.
Description was changed from ========== Improve histogram and actions for CBD dialog Add histogram for time spent in Clear Browsing Data dialog. Add histogram for tab changes. Fix incorrect logging of timeperiod by registering listeners after time picker is fully initialized. BUG=681523 ========== to ========== Improve histogram and actions for CBD dialog Add histogram for time spent in Clear Browsing Data dialog. Add actions for tab changes. Fix incorrect logging of timeperiod changes by registering listeners after time picker is fully initialized. BUG=681523 ==========
Patchset #2 (id:20001) has been deleted
dullweber@chromium.org changed reviewers: + rkaplow@chromium.org, twellington@chromium.org
twellington@chromium.org: Please review changes in chrome/android
rkaplow@chromium.org: please review histograms.xml and actions.xml
Description was changed from ========== Improve histogram and actions for CBD dialog Add histogram for time spent in Clear Browsing Data dialog. Add actions for tab changes. Fix incorrect logging of timeperiod changes by registering listeners after time picker is fully initialized. BUG=681523 ========== to ========== Improve histogram and actions for CBD dialog Add histogram for time spent in Clear Browsing Data dialog. Add actions for tab changes. Fix incorrect logging of timeperiod changes by registering listeners after time picker is fully initialized. CBD metrics: https://docs.google.com/presentation/d/1uWaHywa3UVkXecsDdhL-24C5iyqAT5UWdoPh5... BUG=681523 ==========
https://codereview.chromium.org/2962523003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java (right): https://codereview.chromium.org/2962523003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java:145: RecordUserAction.record("ClearBrowsingData_SwitchTo_BasicTab"); Theresa: Do you know if there is any way to write tests in Java that can verify that the right actions are emitted from either Java or C++ code? This would help us to check that the right deletions are performed and that we don't make mistakes with the metrics code. On the C++ side there is UserActionTester but I couldn't find a bridge, that does something similar
https://codereview.chromium.org/2962523003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java (right): https://codereview.chromium.org/2962523003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java:145: RecordUserAction.record("ClearBrowsingData_SwitchTo_BasicTab"); On 2017/06/27 15:39:56, dullweber wrote: > Theresa: Do you know if there is any way to write tests in Java that can verify > that the right actions are emitted from either Java or C++ code? This would help > us to check that the right deletions are performed and that we don't make > mistakes with the metrics code. On the C++ side there is UserActionTester but I > couldn't find a bridge, that does something similar It looks like we a helper method in RecordHistogram -- #getHistogramValueCountForTesting() that's used in some tests. You could add something similar to RecordUserAction or otherwise create a bridge that exposes similar functionality to the C++ UserActionTester, but it doesn't appear there's anything Java-side yet.
lgtm
lgtm https://codereview.chromium.org/2962523003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2962523003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:24118: + Time spent in Clear Browsing Data dialog. From opening the dialog until data may want to clarify - this only records when data is cleared, but if data is not cleared it is not recorded?
Description was changed from ========== Improve histogram and actions for CBD dialog Add histogram for time spent in Clear Browsing Data dialog. Add actions for tab changes. Fix incorrect logging of timeperiod changes by registering listeners after time picker is fully initialized. CBD metrics: https://docs.google.com/presentation/d/1uWaHywa3UVkXecsDdhL-24C5iyqAT5UWdoPh5... BUG=681523 ========== to ========== Improve histogram and actions for CBD dialog Add histogram for time spent in Clear Browsing Data dialog. Add actions for tab changes. Fix incorrect logging of timeperiod changes by registering listeners after time picker is fully initialized. CBD metrics: https://docs.google.com/presentation/d/1uWaHywa3UVkXecsDdhL-24C5iyqAT5UWdoPh5... BUG=681523 ==========
https://codereview.chromium.org/2962523003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java (right): https://codereview.chromium.org/2962523003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java:145: RecordUserAction.record("ClearBrowsingData_SwitchTo_BasicTab"); On 2017/06/27 15:48:01, Theresa wrote: > On 2017/06/27 15:39:56, dullweber wrote: > > Theresa: Do you know if there is any way to write tests in Java that can > verify > > that the right actions are emitted from either Java or C++ code? This would > help > > us to check that the right deletions are performed and that we don't make > > mistakes with the metrics code. On the C++ side there is UserActionTester but > I > > couldn't find a bridge, that does something similar > > It looks like we a helper method in RecordHistogram -- > #getHistogramValueCountForTesting() that's used in some tests. You could add > something similar to RecordUserAction or otherwise create a bridge that exposes > similar functionality to the C++ UserActionTester, but it doesn't appear there's > anything Java-side yet. Thanks, getHistogramValueCountForTesting looks useful. I will see if it works for what I would like to do and create another CL with some tests for our actions. https://codereview.chromium.org/2962523003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2962523003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:24118: + Time spent in Clear Browsing Data dialog. From opening the dialog until data On 2017/06/27 21:40:33, rkaplow wrote: > may want to clarify - this only records when data is cleared, but if data is not > cleared it is not recorded? yes, that is right. I will look into recording aborts as well but only very few users exit the dialog without a deletion. I improved the summary
LGTM
The CQ bit was checked by dullweber@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2962523003/#ps60001 (title: "Improve histogram summary")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498642242784230,
"parent_rev": "0e922f67a9bcd8faae007264c08f58fa7d9dcae4", "commit_rev":
"a82b3e3179a055144fab8894e03368700a7f153d"}
Message was sent while issue was closed.
Description was changed from ========== Improve histogram and actions for CBD dialog Add histogram for time spent in Clear Browsing Data dialog. Add actions for tab changes. Fix incorrect logging of timeperiod changes by registering listeners after time picker is fully initialized. CBD metrics: https://docs.google.com/presentation/d/1uWaHywa3UVkXecsDdhL-24C5iyqAT5UWdoPh5... BUG=681523 ========== to ========== Improve histogram and actions for CBD dialog Add histogram for time spent in Clear Browsing Data dialog. Add actions for tab changes. Fix incorrect logging of timeperiod changes by registering listeners after time picker is fully initialized. CBD metrics: https://docs.google.com/presentation/d/1uWaHywa3UVkXecsDdhL-24C5iyqAT5UWdoPh5... BUG=681523 Review-Url: https://codereview.chromium.org/2962523003 Cr-Commit-Position: refs/heads/master@{#482942} Committed: https://chromium.googlesource.com/chromium/src/+/a82b3e3179a055144fab8894e033... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/a82b3e3179a055144fab8894e033... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
