Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2956283002:
Expose Add/RemoveActionCallback to java (Closed)
DescriptionExpose Add/RemoveActionCallback to java
Add support for testing user actions from Java by exposing
addActionCallback and removeActionCallback.
Add a UserActionTester class for java tests.
Add some tests for BrowsingDataBridge as an example.
There is already a similar approach for histogram tests using
getHistogramValueCountForTesting().
BUG=
Review-Url: https://codereview.chromium.org/2956283002
Cr-Commit-Position: refs/heads/master@{#485616}
Committed: https://chromium.googlesource.com/chromium/src/+/4f2ac33787e3661d3ce05d7aeb2429f300e08b82
Patch Set 1 #Patch Set 2 : add Test #Patch Set 3 : Add UserActionTester.java and convert to Junit4 #
Total comments: 4
Patch Set 4 : Use CallbackHelper #
Total comments: 1
Patch Set 5 : notify -> notifies #Patch Set 6 : add simple test case for incorrect logging #Patch Set 7 : rebase and update test #
Total comments: 2
Patch Set 8 : Use assert instead of an exception #
Messages
Total messages: 46 (29 generated)
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...
Patchset #2 (id:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Expose Add/RemoveActionCallback to java BUG= ========== to ========== Expose Add/RemoveActionCallback to java Add support for testing user actions from Java by exposing addActionCallback and removeActionCallback. As a first example I added some tests for BrowsingDataBridge. There is already a similar approach for histogram tests using getHistogramValueCountForTesting(). BUG= ==========
Description was changed from ========== Expose Add/RemoveActionCallback to java Add support for testing user actions from Java by exposing addActionCallback and removeActionCallback. As a first example I added some tests for BrowsingDataBridge. There is already a similar approach for histogram tests using getHistogramValueCountForTesting(). BUG= ========== to ========== Expose Add/RemoveActionCallback to java Add support for testing user actions from Java by exposing addActionCallback and removeActionCallback. As a first example I added some tests for BrowsingDataBridge. There is already a similar approach for histogram tests using getHistogramValueCountForTesting(). BUG= ==========
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...
dullweber@chromium.org changed reviewers: + twellington@chromium.org
Hello Theresa, I created a UserActionTester for Java and wrote a first test for it. I already discovered an action that was logged incorrectly. (http://crrev.com/2963973002)
dullweber@chromium.org changed reviewers: + msramek@chromium.org
Hi Martin, I added a test for recording of UserActions. Could you take a look at BrowsingDataBridgeTest.java and check if these are the actions that should actually be logged. Especially I'm not sure about MaskContainsUnProtectedWeb and whether it should be logged twice.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Expose Add/RemoveActionCallback to java Add support for testing user actions from Java by exposing addActionCallback and removeActionCallback. As a first example I added some tests for BrowsingDataBridge. There is already a similar approach for histogram tests using getHistogramValueCountForTesting(). BUG= ========== to ========== Expose Add/RemoveActionCallback to java Add support for testing user actions from Java by exposing addActionCallback and removeActionCallback. Add a UserActionTester class for java tests. Add some tests for BrowsingDataBridge as an example. There is already a similar approach for histogram tests using getHistogramValueCountForTesting(). BUG= ==========
https://codereview.chromium.org/2956283002/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataBridgeTest.java (right): https://codereview.chromium.org/2956283002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataBridgeTest.java:51: mSemaphore.release(); We typically use CallbackHelpers to wait for asynchronous operations to complete in test code. https://cs.chromium.org/chromium/src/base/test/android/javatests/src/org/chro... https://codereview.chromium.org/2956283002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataBridgeTest.java:146: // This is logged twice because of a separate call for important storage nit: Add a period at the end of this comment. The double logging seems erroneous to me. Will you please check with dmurph@ to see if he is aware of that behavior?
dullweber@chromium.org changed reviewers: + dmurph@chromium.org
dmurph: by writing a test for UserActions in ClearBrowsingData, I discovered that ClearBrowsingData_MaskContainsUnprotectedWeb is logged twice on some deletions. Could you take a look at BrowsingDataBridgeTest::testClearingAll and tell me whether this behavior is right or should be fixed.
https://codereview.chromium.org/2956283002/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataBridgeTest.java (right): https://codereview.chromium.org/2956283002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataBridgeTest.java:51: mSemaphore.release(); On 2017/07/05 20:55:29, Theresa wrote: > We typically use CallbackHelpers to wait for asynchronous operations to complete > in test code. > > https://cs.chromium.org/chromium/src/base/test/android/javatests/src/org/chro... Thanks for the link, fixed https://codereview.chromium.org/2956283002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataBridgeTest.java:146: // This is logged twice because of a separate call for important storage On 2017/07/05 20:55:29, Theresa wrote: > nit: Add a period at the end of this comment. > > > The double logging seems erroneous to me. Will you please check with dmurph@ to > see if he is aware of that behavior? I added him as reviewer
It looks like the code from this change got merged in: https://chromium.googlesource.com/chromium/src/+/72ce5d481f11212f8cbb99f13058... Was that intentional? https://codereview.chromium.org/2956283002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/2956283002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:309: * Notify subclasses that browsing data is about to be cleared. nit: s/Notify/Notifies
On 2017/07/06 23:06:35, Theresa wrote: > It looks like the code from this change got merged in: > https://chromium.googlesource.com/chromium/src/+/72ce5d481f11212f8cbb99f13058... > > Was that intentional? > > https://codereview.chromium.org/2956283002/diff/80001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java > (right): > > https://codereview.chromium.org/2956283002/diff/80001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:309: > * Notify subclasses that browsing data is about to be cleared. > nit: s/Notify/Notifies Oh, I think this happened the following way: I created a CL to fix the incorrect logging, rebased this CL on top and did set-upstream. Then I submitted the other CL and did set-upstream origin/master but I didn't rebase on master again. I rebased and uploaded another patchset. Sorry about this.
On 2017/07/07 08:17:30, dullweber wrote: > On 2017/07/06 23:06:35, Theresa wrote: > > It looks like the code from this change got merged in: > > > https://chromium.googlesource.com/chromium/src/+/72ce5d481f11212f8cbb99f13058... > > > > Was that intentional? > > > > > https://codereview.chromium.org/2956283002/diff/80001/chrome/android/java/src... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java > > (right): > > > > > https://codereview.chromium.org/2956283002/diff/80001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:309: > > * Notify subclasses that browsing data is about to be cleared. > > nit: s/Notify/Notifies > > Oh, I think this happened the following way: I created a CL to fix the incorrect > logging, rebased this CL on top and did set-upstream. Then I submitted the other > CL and did set-upstream origin/master but I didn't rebase on master again. I > rebased and uploaded another patchset. Sorry about this. Do you still need me to review? That double logging shouldn't happen, and it looks like it's only recorded when we specifically clear data?
On 2017/07/07 18:33:34, dmurph wrote: > On 2017/07/07 08:17:30, dullweber wrote: > > On 2017/07/06 23:06:35, Theresa wrote: > > > It looks like the code from this change got merged in: > > > > > > https://chromium.googlesource.com/chromium/src/+/72ce5d481f11212f8cbb99f13058... > > > > > > Was that intentional? > > > > > > > > > https://codereview.chromium.org/2956283002/diff/80001/chrome/android/java/src... > > > File > > > > > > chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2956283002/diff/80001/chrome/android/java/src... > > > > > > chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:309: > > > * Notify subclasses that browsing data is about to be cleared. > > > nit: s/Notify/Notifies > > > > Oh, I think this happened the following way: I created a CL to fix the > incorrect > > logging, rebased this CL on top and did set-upstream. Then I submitted the > other > > CL and did set-upstream origin/master but I didn't rebase on master again. I > > rebased and uploaded another patchset. Sorry about this. > > Do you still need me to review? That double logging shouldn't happen, and it > looks like it's only recorded when we specifically clear data? I created a bug and to fix it separately.
dullweber@chromium.org changed reviewers: - dmurph@chromium.org
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.
chrome/android/.../preferences lgtm
On 2017/07/07 18:33:34, dmurph wrote: > On 2017/07/07 08:17:30, dullweber wrote: > > On 2017/07/06 23:06:35, Theresa wrote: > > > It looks like the code from this change got merged in: > > > > > > https://chromium.googlesource.com/chromium/src/+/72ce5d481f11212f8cbb99f13058... > > > > > > Was that intentional? > > > > > > > > > https://codereview.chromium.org/2956283002/diff/80001/chrome/android/java/src... > > > File > > > > > > chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2956283002/diff/80001/chrome/android/java/src... > > > > > > chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:309: > > > * Notify subclasses that browsing data is about to be cleared. > > > nit: s/Notify/Notifies > > > > Oh, I think this happened the following way: I created a CL to fix the > incorrect > > logging, rebased this CL on top and did set-upstream. Then I submitted the > other > > CL and did set-upstream origin/master but I didn't rebase on master again. I > > rebased and uploaded another patchset. Sorry about this. > > Do you still need me to review? That double logging shouldn't happen, and it > looks like it's only recorded when we specifically clear data? I fixed that we perform separate calls to the BrowsingDataRemover if no important sites are specified. If there are ImportantSites and there is some data type that is filtered and some other data type that is not filtered, we have to do separate calls and this will log the action twice. I don't think there is a better solution to this because the logging should also happen if the BrowsingDataRemover is called from other places. (cl http://crrev.com/c/565138)
dullweber@chromium.org changed reviewers: + yfriedman@chromium.org
yfriedman@chromium.org: Please review changes in base/android and base/test/android
https://codereview.chromium.org/2956283002/diff/140001/base/android/java/src/... File base/android/java/src/org/chromium/base/metrics/RecordUserAction.java (right): https://codereview.chromium.org/2956283002/diff/140001/base/android/java/src/... base/android/java/src/org/chromium/base/metrics/RecordUserAction.java:69: if (sNativeActionCallback != 0) { How about you use an assert so that it doesn't add code to release builds?
I replaced the exceptions with an assert, thanks https://codereview.chromium.org/2956283002/diff/140001/base/android/java/src/... File base/android/java/src/org/chromium/base/metrics/RecordUserAction.java (right): https://codereview.chromium.org/2956283002/diff/140001/base/android/java/src/... base/android/java/src/org/chromium/base/metrics/RecordUserAction.java:69: if (sNativeActionCallback != 0) { On 2017/07/10 18:14:25, Yaron wrote: > How about you use an assert so that it doesn't add code to release builds? Done.
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.
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 Link to the patchset: https://codereview.chromium.org/2956283002/#ps160001 (title: "Use assert instead of an exception")
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": 160001, "attempt_start_ts": 1499779701158740,
"parent_rev": "72fd03ef3ba18912fad65a49778564392622c2a5", "commit_rev":
"4f2ac33787e3661d3ce05d7aeb2429f300e08b82"}
Message was sent while issue was closed.
Description was changed from ========== Expose Add/RemoveActionCallback to java Add support for testing user actions from Java by exposing addActionCallback and removeActionCallback. Add a UserActionTester class for java tests. Add some tests for BrowsingDataBridge as an example. There is already a similar approach for histogram tests using getHistogramValueCountForTesting(). BUG= ========== to ========== Expose Add/RemoveActionCallback to java Add support for testing user actions from Java by exposing addActionCallback and removeActionCallback. Add a UserActionTester class for java tests. Add some tests for BrowsingDataBridge as an example. There is already a similar approach for histogram tests using getHistogramValueCountForTesting(). BUG= Review-Url: https://codereview.chromium.org/2956283002 Cr-Commit-Position: refs/heads/master@{#485616} Committed: https://chromium.googlesource.com/chromium/src/+/4f2ac33787e3661d3ce05d7aeb24... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001) as https://chromium.googlesource.com/chromium/src/+/4f2ac33787e3661d3ce05d7aeb24... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
