Expose the Site Engagement Service to Java.

Expose the Site Engagement Service to Java.

This CL adds a Java-side interface to the Site Engagement Service, as
well as a basic integration test for the interface. This allows Java
code (particularly tests) to programatically adjust and retrieve the
score.

BUG=671100
Committed: https://crrev.com/b8723b73ef4c0e4c7b59aa655f7a5d0cccbd79f2
Cr-Commit-Position: refs/heads/master@{#437825}

[dominickn, 2016-12-06 07:12:36]

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

[commit-bot: I haz the power, 2016-12-06 07:12:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2016-12-06 07:14:54]

Description was changed from

==========
Expose the Site Engagement Service to Java.

This CL adds a Java-side interface to the Site Engagement Service, as
well as a basic integration test for the interface. This allows Java
code (particularly tests) to programatically adjust and retrieve the
score.

BUG=671100
==========

to

==========
Expose the Site Engagement Service to Java.

This CL adds a Java-side interface to the Site Engagement Service, as
well as a basic integration test for the interface. This allows Java
code (particularly tests) to programatically adjust and retrieve the
score.

BUG=671100
==========

[dominickn, 2016-12-06 07:14:54]

dominickn@chromium.org changed reviewers:
+ benwells@chromium.org, dfalcantara@chromium.org

[commit-bot: I haz the power, 2016-12-06 07:25:22]

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

[commit-bot: I haz the power, 2016-12-06 07:25:23]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[dominickn, 2016-12-06 07:42:05]

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

[commit-bot: I haz the power, 2016-12-06 07:42:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2016-12-06 08:24:59]

PTAL, thanks!

[commit-bot: I haz the power, 2016-12-06 08:32:26]

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

[commit-bot: I haz the power, 2016-12-06 08:32:27]

Dry run: This issue passed the CQ dry run.

[gone, 2016-12-07 21:50:26]

https://codereview.chromium.org/2553013002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/engagement/SiteEngagementService.java
(right):

https://codereview.chromium.org/2553013002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/engagement/SiteEngagementService.java:27:
public static SiteEngagementService getForProfile(Profile profile) {
Does this always create a new SiteEngagementService that wraps a potentially
existing native one?

https://codereview.chromium.org/2553013002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/engagement/SiteEngagementService.java:37:
assert mNativePointer != 0;
Not sure how useful this assert is; if the pointer is null then the native call
will crash anyway.  It might make more sense to just return early if the pointer
is 0 (for whatever potentially asynchronous reason).

https://codereview.chromium.org/2553013002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/engagement/SiteEngagementService.java:47:
assert mNativePointer != 0;
Ditto.

https://codereview.chromium.org/2553013002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/engagement/SiteEngagementServiceTest.java
(right):

https://codereview.chromium.org/2553013002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/engagement/SiteEngagementServiceTest.java:25:
@MediumTest
Don't think MediumTest is required nowadays; it's deprecated AFAICT.

https://codereview.chromium.org/2553013002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/engagement/SiteEngagementServiceTest.java:29:
runTestOnUiThread(new Runnable() {
Can you just use the @UiThreadTest annotation?

https://codereview.chromium.org/2553013002/diff/20001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service.cc (right):

https://codereview.chromium.org/2553013002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service.cc:248: }
nit: newline here

https://codereview.chromium.org/2553013002/diff/20001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service_android.h (right):

https://codereview.chromium.org/2553013002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service_android.h:14: // owned by the
|service_| which it wraps, and is lazily created when
when...?  Intrigued minds want to know.

https://codereview.chromium.org/2553013002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service_android.h:27:
SiteEngagementService* service);
nit: newline here

https://codereview.chromium.org/2553013002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service_android.h:40:
base::android::ScopedJavaGlobalRef<jobject> obj_;
Use something more descriptive than obj?

https://codereview.chromium.org/2553013002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service_android.h:41:
SiteEngagementService* service_;
DISALLOW_COPY_ETC_ETC?

[dominickn, 2016-12-07 23:27:17]

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

[commit-bot: I haz the power, 2016-12-07 23:27:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2016-12-08 00:24:40]

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

[commit-bot: I haz the power, 2016-12-08 00:25:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2016-12-08 01:03:36]

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

[dominickn, 2016-12-08 01:03:54]

Thanks!

https://codereview.chromium.org/2553013002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/engagement/SiteEngagementService.java
(right):

https://codereview.chromium.org/2553013002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/engagement/SiteEngagementService.java:27:
public static SiteEngagementService getForProfile(Profile profile) {
On 2016/12/07 21:50:25, dfalcantara (check my queue) wrote:
> Does this always create a new SiteEngagementService that wraps a potentially
> existing native one?

Hmm, there's an issue here with multiple ones on the Java side wrapping one on
the native side. Fixed.

https://codereview.chromium.org/2553013002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/engagement/SiteEngagementService.java:37:
assert mNativePointer != 0;
On 2016/12/07 21:50:25, dfalcantara (check my queue) wrote:
> Not sure how useful this assert is; if the pointer is null then the native
call
> will crash anyway.  It might make more sense to just return early if the
pointer
> is 0 (for whatever potentially asynchronous reason).

Done.

https://codereview.chromium.org/2553013002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/engagement/SiteEngagementService.java:47:
assert mNativePointer != 0;
On 2016/12/07 21:50:25, dfalcantara (check my queue) wrote:
> Ditto.

Done.

https://codereview.chromium.org/2553013002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/engagement/SiteEngagementServiceTest.java
(right):

https://codereview.chromium.org/2553013002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/engagement/SiteEngagementServiceTest.java:25:
@MediumTest
On 2016/12/07 21:50:26, dfalcantara (check my queue) wrote:
> Don't think MediumTest is required nowadays; it's deprecated AFAICT.

The test runner errors out without the annotation:

MissingSizeAnnotationError:
org.chromium.chrome.browser.engagement.SiteEngagementServiceTest#testSettingAndRetrievingScore:
Test method is missing required size annotation. Add one of: @LargeTest,
@PerfTest, @Manual, @FlakyTest, @DisabledTest, @MediumTest, @Smoke, @SmallTest,
@IntegrationTest, @EnormousTest

https://codereview.chromium.org/2553013002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/engagement/SiteEngagementServiceTest.java:29:
runTestOnUiThread(new Runnable() {
On 2016/12/07 21:50:26, dfalcantara (check my queue) wrote:
> Can you just use the @UiThreadTest annotation?

New annotation learned! \o/

https://codereview.chromium.org/2553013002/diff/20001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service.cc (right):

https://codereview.chromium.org/2553013002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service.cc:248: }
On 2016/12/07 21:50:26, dfalcantara (check my queue) wrote:
> nit: newline here

Done.

https://codereview.chromium.org/2553013002/diff/20001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service_android.h (right):

https://codereview.chromium.org/2553013002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service_android.h:14: // owned by the
|service_| which it wraps, and is lazily created when
On 2016/12/07 21:50:26, dfalcantara (check my queue) wrote:
> when...?  Intrigued minds want to know.

Oops.

https://codereview.chromium.org/2553013002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service_android.h:27:
SiteEngagementService* service);
On 2016/12/07 21:50:26, dfalcantara (check my queue) wrote:
> nit: newline here

Done.

https://codereview.chromium.org/2553013002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service_android.h:40:
base::android::ScopedJavaGlobalRef<jobject> obj_;
On 2016/12/07 21:50:26, dfalcantara (check my queue) wrote:
> Use something more descriptive than obj?

Done.

https://codereview.chromium.org/2553013002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service_android.h:41:
SiteEngagementService* service_;
On 2016/12/07 21:50:26, dfalcantara (check my queue) wrote:
> DISALLOW_COPY_ETC_ETC?

Derp.

[commit-bot: I haz the power, 2016-12-08 01:04:09]

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, 2016-12-08 02:03:36]

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

[commit-bot: I haz the power, 2016-12-08 02:03:38]

Dry run: This issue passed the CQ dry run.

[gone, 2016-12-08 19:35:25]

lgtm on my end

[benwells, 2016-12-09 04:31:32]

https://codereview.chromium.org/2553013002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/engagement/SiteEngagementService.java
(right):

https://codereview.chromium.org/2553013002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/engagement/SiteEngagementService.java:46:
public void resetScoreForUrl(String url, double score) {
Nit: can you put ..ForTest on this, and on the corresponding native function? I
assume we'd only ever want to do this in a test.

https://codereview.chromium.org/2553013002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/engagement/SiteEngagementService.java:57:
/** This object may only be created via the static getForProfile method. */
Is this used? getForProfile doesn't call this.

https://codereview.chromium.org/2553013002/diff/80001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service_android.cc (right):

https://codereview.chromium.org/2553013002/diff/80001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service_android.cc:71:
base::android::ScopedJavaLocalRef<jobject> SiteEngagementServiceForProfile(
Out of interest, how does this get hooked up to
nativeSiteEngagementServiceForProfile? Is it pure magic?

[dominickn, 2016-12-09 04:44:18]

https://codereview.chromium.org/2553013002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/engagement/SiteEngagementService.java
(right):

https://codereview.chromium.org/2553013002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/engagement/SiteEngagementService.java:46:
public void resetScoreForUrl(String url, double score) {
On 2016/12/09 04:31:32, benwells wrote:
> Nit: can you put ..ForTest on this, and on the corresponding native function?
I
> assume we'd only ever want to do this in a test.

No, this native method is used by the chrome://site-engagement page handler to
reset scores when edited by users. It's not just used for tests.

https://codereview.chromium.org/2553013002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/engagement/SiteEngagementService.java:57:
/** This object may only be created via the static getForProfile method. */
On 2016/12/09 04:31:32, benwells wrote:
> Is this used? getForProfile doesn't call this.

This method is called by create(), which is called across the JNI by
SiteEngagementServiceAndroid to make one of these things.

https://codereview.chromium.org/2553013002/diff/80001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service_android.cc (right):

https://codereview.chromium.org/2553013002/diff/80001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service_android.cc:71:
base::android::ScopedJavaLocalRef<jobject> SiteEngagementServiceForProfile(
On 2016/12/09 04:31:32, benwells wrote:
> Out of interest, how does this get hooked up to
> nativeSiteEngagementServiceForProfile? Is it pure magic?

The JNI bindings generator generates the declaration for this method (by slicing
native off nativeSiteEngagementServiceForProfile and interpolating the arguments
from Java, e.g. a class static method or an object non-static method) and calls
it, sending the LocalRef it returns to Java as a SiteEngagementService Java-side
object.

[benwells, 2016-12-09 04:57:36]

lgtm

https://codereview.chromium.org/2553013002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/engagement/SiteEngagementService.java
(right):

https://codereview.chromium.org/2553013002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/engagement/SiteEngagementService.java:46:
public void resetScoreForUrl(String url, double score) {
On 2016/12/09 04:44:17, dominickn wrote:
> On 2016/12/09 04:31:32, benwells wrote:
> > Nit: can you put ..ForTest on this, and on the corresponding native
function?
> I
> > assume we'd only ever want to do this in a test.
> 
> No, this native method is used by the chrome://site-engagement page handler to
> reset scores when edited by users. It's not just used for tests.

Acknowledged.

https://codereview.chromium.org/2553013002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/engagement/SiteEngagementService.java:57:
/** This object may only be created via the static getForProfile method. */
On 2016/12/09 04:44:17, dominickn wrote:
> On 2016/12/09 04:31:32, benwells wrote:
> > Is this used? getForProfile doesn't call this.
> 
> This method is called by create(), which is called across the JNI by
> SiteEngagementServiceAndroid to make one of these things.

Acknowledged.

https://codereview.chromium.org/2553013002/diff/80001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service_android.cc (right):

https://codereview.chromium.org/2553013002/diff/80001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service_android.cc:71:
base::android::ScopedJavaLocalRef<jobject> SiteEngagementServiceForProfile(
On 2016/12/09 04:44:17, dominickn wrote:
> On 2016/12/09 04:31:32, benwells wrote:
> > Out of interest, how does this get hooked up to
> > nativeSiteEngagementServiceForProfile? Is it pure magic?
> 
> The JNI bindings generator generates the declaration for this method (by
slicing
> native off nativeSiteEngagementServiceForProfile and interpolating the
arguments
> from Java, e.g. a class static method or an object non-static method) and
calls
> it, sending the LocalRef it returns to Java as a SiteEngagementService
Java-side
> object.

Cool, thanks for the explanation!

[dominickn, 2016-12-12 02:07:09]

The CQ bit was checked by dominickn@chromium.org

[commit-bot: I haz the power, 2016-12-12 02:07:20]

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, 2016-12-12 04:01:03]

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

[commit-bot: I haz the power, 2016-12-12 04:01:04]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dominickn, 2016-12-12 05:05:02]

The CQ bit was checked by dominickn@chromium.org

[commit-bot: I haz the power, 2016-12-12 05:05:19]

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, 2016-12-12 06:42:52]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481519102858450,
"parent_rev": "5da84172ba29f1802af75647261121ca55058642", "commit_rev":
"8ec69b8877948ec8dc4e8ad667090d91ae97ef01"}

[commit-bot: I haz the power, 2016-12-12 06:43:29]

Description was changed from

==========
Expose the Site Engagement Service to Java.

This CL adds a Java-side interface to the Site Engagement Service, as
well as a basic integration test for the interface. This allows Java
code (particularly tests) to programatically adjust and retrieve the
score.

BUG=671100
==========

to

==========
Expose the Site Engagement Service to Java.

This CL adds a Java-side interface to the Site Engagement Service, as
well as a basic integration test for the interface. This allows Java
code (particularly tests) to programatically adjust and retrieve the
score.

BUG=671100

Review-Url: https://codereview.chromium.org/2553013002
==========

[commit-bot: I haz the power, 2016-12-12 06:43:30]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-12 15:10:45]

Description was changed from

==========
Expose the Site Engagement Service to Java.

This CL adds a Java-side interface to the Site Engagement Service, as
well as a basic integration test for the interface. This allows Java
code (particularly tests) to programatically adjust and retrieve the
score.

BUG=671100

Review-Url: https://codereview.chromium.org/2553013002
==========

to

==========
Expose the Site Engagement Service to Java.

This CL adds a Java-side interface to the Site Engagement Service, as
well as a basic integration test for the interface. This allows Java
code (particularly tests) to programatically adjust and retrieve the
score.

BUG=671100

Committed: https://crrev.com/b8723b73ef4c0e4c7b59aa655f7a5d0cccbd79f2
Cr-Commit-Position: refs/heads/master@{#437825}
==========

[commit-bot: I haz the power, 2016-12-12 15:10:46]

Patchset 5 (id:??) landed as
https://crrev.com/b8723b73ef4c0e4c7b59aa655f7a5d0cccbd79f2
Cr-Commit-Position: refs/heads/master@{#437825}