Add the JNI code in order to let Java use the InterestsFetcher.

Add the JNI code in order to let Java use the InterestsFetcher.

Depends on https://codereview.chromium.org/1317513004/

BUG=537145

[tache, 2015-09-11 13:33:51]

tache@google.com changed reviewers:
+ treib@chromium.org

[tache, 2015-09-11 13:33:51]

Added the JNI code in order to let Java use the InterestsFetcher.

[Marc Treib, 2015-09-11 14:54:26]

Please run "git cl format", there's a few formatting issues it should be able to
fix automatically (look at the diff though, sometimes it does wacky things).

Also, you don't want the blink trybots. linux_chromium_rel_ng is my "default"
one.

https://codereview.chromium.org/1334233003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java
(right):

https://codereview.chromium.org/1334233003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:18:
* POJO representing an interest.
POJO?

https://codereview.chromium.org/1334233003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:25:
public Interest(String mName, String mImageUrl, double mRelevance) {
Params shouldn't have the leading "m".

https://codereview.chromium.org/1334233003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:26:
super();
Unnecessary

https://codereview.chromium.org/1334233003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:36:
public void setName(String mName) {
Are the setters needed?

https://codereview.chromium.org/1334233003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:67:
public void onInterestsCallback(Interest[] interests);
onInterests what?
Maybe "onInterestsAvailable"?

https://codereview.chromium.org/1334233003/diff/1/chrome/browser/android/inte...
File chrome/browser/android/interests_service.cc (right):

https://codereview.chromium.org/1334233003/diff/1/chrome/browser/android/inte...
chrome/browser/android/interests_service.cc:12: #include "base/command_line.h"
Not needed, also the next three (and probably some more)

https://codereview.chromium.org/1334233003/diff/1/chrome/browser/android/inte...
chrome/browser/android/interests_service.cc:72: fetcher,
Can you use base::Owned here? Then |fetcher| should be deleted automatically
after the callback has run.
(Raw pointers are generally taken to mean "non-owning" in Chromium. For an
owning pointer, you should generally use scoped_ptr.)

https://codereview.chromium.org/1334233003/diff/1/chrome/browser/android/inte...
chrome/browser/android/interests_service.cc:92: for (uint i = 0; i !=
interests.size(); i++) {
nit: size_t

https://codereview.chromium.org/1334233003/diff/1/chrome/browser/android/inte...
File chrome/browser/android/interests_service.h (right):

https://codereview.chromium.org/1334233003/diff/1/chrome/browser/android/inte...
chrome/browser/android/interests_service.h:17: explicit InterestsService(Profile
*profile);
* goes with the type, also in a few other places

https://codereview.chromium.org/1334233003/diff/1/chrome/browser/android/inte...
chrome/browser/android/interests_service.h:19: void OnLoadingComplete(JNIEnv
*env, jobject obj);
This method doesn't actually exist, please remove the declaration.

https://codereview.chromium.org/1334233003/diff/1/chrome/browser/android/inte...
chrome/browser/android/interests_service.h:33:
DISALLOW_COPY_AND_ASSIGN(InterestsService);
By convention, this goes right at the end of the class.

[tache, 2015-09-11 15:45:09]

https://codereview.chromium.org/1334233003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java
(right):

https://codereview.chromium.org/1334233003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:18:
* POJO representing an interest.
On 2015/09/11 14:54:25, Marc Treib wrote:
> POJO?

Done.

https://codereview.chromium.org/1334233003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:25:
public Interest(String mName, String mImageUrl, double mRelevance) {
On 2015/09/11 14:54:25, Marc Treib wrote:
> Params shouldn't have the leading "m".

Done.

https://codereview.chromium.org/1334233003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:26:
super();
On 2015/09/11 14:54:25, Marc Treib wrote:
> Unnecessary

Done.

https://codereview.chromium.org/1334233003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:36:
public void setName(String mName) {
On 2015/09/11 14:54:25, Marc Treib wrote:
> Are the setters needed?

Done.

https://codereview.chromium.org/1334233003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:67:
public void onInterestsCallback(Interest[] interests);
On 2015/09/11 14:54:25, Marc Treib wrote:
> onInterests what?
> Maybe "onInterestsAvailable"?

Done.

https://codereview.chromium.org/1334233003/diff/1/chrome/browser/android/inte...
File chrome/browser/android/interests_service.cc (right):

https://codereview.chromium.org/1334233003/diff/1/chrome/browser/android/inte...
chrome/browser/android/interests_service.cc:12: #include "base/command_line.h"
On 2015/09/11 14:54:25, Marc Treib wrote:
> Not needed, also the next three (and probably some more)

Done.

https://codereview.chromium.org/1334233003/diff/1/chrome/browser/android/inte...
chrome/browser/android/interests_service.cc:72: fetcher,
On 2015/09/11 14:54:25, Marc Treib wrote:
> Can you use base::Owned here? Then |fetcher| should be deleted automatically
> after the callback has run.
> (Raw pointers are generally taken to mean "non-owning" in Chromium. For an
> owning pointer, you should generally use scoped_ptr.)

Done.

https://codereview.chromium.org/1334233003/diff/1/chrome/browser/android/inte...
chrome/browser/android/interests_service.cc:92: for (uint i = 0; i !=
interests.size(); i++) {
On 2015/09/11 14:54:25, Marc Treib wrote:
> nit: size_t

Done.

https://codereview.chromium.org/1334233003/diff/1/chrome/browser/android/inte...
File chrome/browser/android/interests_service.h (right):

https://codereview.chromium.org/1334233003/diff/1/chrome/browser/android/inte...
chrome/browser/android/interests_service.h:17: explicit InterestsService(Profile
*profile);
On 2015/09/11 14:54:25, Marc Treib wrote:
> * goes with the type, also in a few other places

Done.

https://codereview.chromium.org/1334233003/diff/1/chrome/browser/android/inte...
chrome/browser/android/interests_service.h:19: void OnLoadingComplete(JNIEnv
*env, jobject obj);
On 2015/09/11 14:54:25, Marc Treib wrote:
> This method doesn't actually exist, please remove the declaration.

Done.

https://codereview.chromium.org/1334233003/diff/1/chrome/browser/android/inte...
chrome/browser/android/interests_service.h:33:
DISALLOW_COPY_AND_ASSIGN(InterestsService);
On 2015/09/11 14:54:25, Marc Treib wrote:
> By convention, this goes right at the end of the class.

Done.

[Marc Treib, 2015-09-11 16:02:14]

https://codereview.chromium.org/1334233003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java
(right):

https://codereview.chromium.org/1334233003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:5:
package org.chromium.chrome.browser.ntp;
All the other code in that folder seems to be more or less UI stuff.
MostVisitedSites lives in chrome.browser.profiles, which doesn't seem much
better...hm.

https://codereview.chromium.org/1334233003/diff/40001/chrome/browser/android/...
File chrome/browser/android/interests_service.cc (right):

https://codereview.chromium.org/1334233003/diff/40001/chrome/browser/android/...
chrome/browser/android/interests_service.cc:94: j_interests);
nit: misaligned

https://codereview.chromium.org/1334233003/diff/40001/chrome/browser/android/...
File chrome/browser/android/interests_service_unittest.cc (right):

https://codereview.chromium.org/1334233003/diff/40001/chrome/browser/android/...
chrome/browser/android/interests_service_unittest.cc:38:
CHECK_EQ(env->GetArrayLength(j_interests), 0);
EXPECT_EQ (CHECK will make the test crash)

Also, the expected value goes first (the error message on failure is something
like "should be param_1 but is param_2").

[tache, 2015-09-18 09:41:21]

https://codereview.chromium.org/1334233003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java
(right):

https://codereview.chromium.org/1334233003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:5:
package org.chromium.chrome.browser.ntp;
On 2015/09/11 16:02:14, Marc Treib wrote:
> All the other code in that folder seems to be more or less UI stuff.
> MostVisitedSites lives in chrome.browser.profiles, which doesn't seem much
> better...hm.

Can't really think for a good place for it, either. Unless some other place is
found, I would leave it in the NTP since that's the only place it should be used
from.

I could move it to another folder inside the NTP so it would be separate from
the UI code.

https://codereview.chromium.org/1334233003/diff/40001/chrome/browser/android/...
File chrome/browser/android/interests_service.cc (right):

https://codereview.chromium.org/1334233003/diff/40001/chrome/browser/android/...
chrome/browser/android/interests_service.cc:94: j_interests);
On 2015/09/11 16:02:14, Marc Treib wrote:
> nit: misaligned

Done.

https://codereview.chromium.org/1334233003/diff/40001/chrome/browser/android/...
File chrome/browser/android/interests_service_unittest.cc (right):

https://codereview.chromium.org/1334233003/diff/40001/chrome/browser/android/...
chrome/browser/android/interests_service_unittest.cc:38:
CHECK_EQ(env->GetArrayLength(j_interests), 0);
On 2015/09/11 16:02:14, Marc Treib wrote:
> EXPECT_EQ (CHECK will make the test crash)
> 
> Also, the expected value goes first (the error message on failure is something
> like "should be param_1 but is param_2").

Done.

[tache, 2015-09-29 13:20:14]

tache@google.com changed reviewers:
+ bauerb@chromium.org

[Bernhard Bauer, 2015-09-29 14:04:05]

It might be helpful to mention that your CL is based on
https://codereview.chromium.org/1317513004/.

https://codereview.chromium.org/1334233003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java
(right):

https://codereview.chromium.org/1334233003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:22:
private double mRelevance;
Make these final?

https://codereview.chromium.org/1334233003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:39:
return this.mRelevance;
You can remove the `this.`.

https://codereview.chromium.org/1334233003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:48:
* In case of an error an empty array will be returned.
Generally, it's useful to be able to distinguish between an actual empty array
and an error condition. Would returning null work?

https://codereview.chromium.org/1334233003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:78:
// Don't notify callback if we've already been destroyed.
Is this actually going to be necessary? The native part uses weak pointers, so
we know that this callback will never be called when it's destroyed.

https://codereview.chromium.org/1334233003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:92:
public static Interest createInterest(String name, String imageUrl, double
relevance) {
If this method is only meant to be called from native, you can make it private
(JNI don't care about that).

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
File chrome/browser/android/interests_service.cc (right):

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
chrome/browser/android/interests_service.cc:24: const char kInterestJavaClass[]
=
Const variables actually get internal linkage automatically, so the anonymous
namespace isn't really necessary if there is nothing else in there.

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
chrome/browser/android/interests_service.cc:27: }  // end namespace
If you do keep the namespace, remove the "end ".

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
chrome/browser/android/interests_service.cc:41:
scoped_ptr<ScopedJavaGlobalRef<jobject>> j_callback(
You don't actually need a scoped_ptr for a ScopedJavaGlobalRef: It's totally
fine to copy it around.

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
chrome/browser/android/interests_service.cc:46: InterestsFetcher* fetcher =
I would make this a scoped_ptr and only release() it below, so it's clearer
you're not leaking the pointer.

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
chrome/browser/android/interests_service.cc:64: jclass interest_class =
env->FindClass(kInterestJavaClass);
It's not great to have to manually resolve this class (for example, it makes it
difficult to find references to the class when renaming it) -- Could you maybe
add a static Java method that will do this instead?

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
chrome/browser/android/interests_service.cc:67:
env->NewObjectArray(interests.size(), interest_class, 0);
I would use nullptr rather than 0.

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
chrome/browser/android/interests_service.cc:72: env,
env->NewStringUTF(interests[i].name.c_str()),
Please use base::android::ConvertUTF8ToJavaString() rather than doing your own
thing.

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
chrome/browser/android/interests_service.cc:89:
InterestsService::ConvertInterestsToJava(interests);
You could pass the JNIEnv to this method. Also, you are already inside the
InterestsService class, so you don't need the InterestsService:: prefix.

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
chrome/browser/android/interests_service.cc:99: new
InterestsService(ProfileAndroid::FromProfileAndroid(jprofile));
The current version of the CL that this is based on has a static Create() method
that takes a Profile*.

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
File chrome/browser/android/interests_service.h (right):

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
chrome/browser/android/interests_service.h:18: void Destroy(JNIEnv* env, jobject
obj);
I would group methods that are called from Java together and add a comment
before the group starts.

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
chrome/browser/android/interests_service.h:28: virtual ~InterestsService();
Destructors should come before other methods.

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
File chrome/browser/android/interests_service_unittest.cc (right):

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
chrome/browser/android/interests_service_unittest.cc:14:
std::vector<InterestsFetcher::Interest> GetEmptyResponse() {
Add an empty line before this one.

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
chrome/browser/android/interests_service_unittest.cc:31: InterestsServiceTest()
{}
This isn't actually necessary -- the class will get this constructor by default.

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
chrome/browser/android/interests_service_unittest.cc:34:
TEST_F(InterestsServiceTest, ConvertEmptyResponse) {
These tests don't seem to test much more than the fact that the number of
elements in the input to ConvertInterestsToJava() is the same as the number of
elements in the output. A more useful test might be to set up a fake server and
test the whole flow from Java.

[Marc Treib, 2015-10-06 13:33:57]

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
File chrome/browser/android/interests_service_unittest.cc (right):

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
chrome/browser/android/interests_service_unittest.cc:31: InterestsServiceTest()
{}
On 2015/09/29 14:04:05, Bernhard Bauer wrote:
> This isn't actually necessary -- the class will get this constructor by
default.

Actually, the whole class isn't necessary right now. You can just use TEST()
instead of TEST_F() and remove the class.

https://codereview.chromium.org/1334233003/diff/100001/chrome/browser/android...
File chrome/browser/android/interests_service.cc (right):

https://codereview.chromium.org/1334233003/diff/100001/chrome/browser/android...
chrome/browser/android/interests_service.cc:43:
InterestsFetcher::CreateFromProfile(profile_).release();
You should be able to actually keep the scoped_ptr, and use base::Passed instead
of base::Owned below. That makes it obvious you're not leaking anything.

[PEConn, 2015-10-07 13:55:20]

peconn@chromium.org changed reviewers:
+ peconn@chromium.org

[PEConn, 2015-10-07 13:55:21]

I've moved this CL over:
https://codereview.chromium.org/1396443002

https://codereview.chromium.org/1334233003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java
(right):

https://codereview.chromium.org/1334233003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:22:
private double mRelevance;
On 2015/09/29 14:04:04, Bernhard Bauer wrote:
> Make these final?

Done.

https://codereview.chromium.org/1334233003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:39:
return this.mRelevance;
On 2015/09/29 14:04:04, Bernhard Bauer wrote:
> You can remove the `this.`.

Done.

https://codereview.chromium.org/1334233003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:48:
* In case of an error an empty array will be returned.
On 2015/09/29 14:04:04, Bernhard Bauer wrote:
> Generally, it's useful to be able to distinguish between an actual empty array
> and an error condition. Would returning null work?

Done.

https://codereview.chromium.org/1334233003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:78:
// Don't notify callback if we've already been destroyed.
On 2015/09/29 14:04:04, Bernhard Bauer wrote:
> Is this actually going to be necessary? The native part uses weak pointers, so
> we know that this callback will never be called when it's destroyed.

Done.

https://codereview.chromium.org/1334233003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:92:
public static Interest createInterest(String name, String imageUrl, double
relevance) {
On 2015/09/29 14:04:04, Bernhard Bauer wrote:
> If this method is only meant to be called from native, you can make it private
> (JNI don't care about that).

Done.

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
File chrome/browser/android/interests_service.cc (right):

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
chrome/browser/android/interests_service.cc:24: const char kInterestJavaClass[]
=
On 2015/09/29 14:04:04, Bernhard Bauer wrote:
> Const variables actually get internal linkage automatically, so the anonymous
> namespace isn't really necessary if there is nothing else in there.

Done.

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
chrome/browser/android/interests_service.cc:27: }  // end namespace
On 2015/09/29 14:04:04, Bernhard Bauer wrote:
> If you do keep the namespace, remove the "end ".

Done.

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
chrome/browser/android/interests_service.cc:41:
scoped_ptr<ScopedJavaGlobalRef<jobject>> j_callback(
On 2015/09/29 14:04:05, Bernhard Bauer wrote:
> You don't actually need a scoped_ptr for a ScopedJavaGlobalRef: It's totally
> fine to copy it around.

Done.

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
chrome/browser/android/interests_service.cc:46: InterestsFetcher* fetcher =
On 2015/09/29 14:04:04, Bernhard Bauer wrote:
> I would make this a scoped_ptr and only release() it below, so it's clearer
> you're not leaking the pointer.

Done.

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
chrome/browser/android/interests_service.cc:64: jclass interest_class =
env->FindClass(kInterestJavaClass);
On 2015/09/29 14:04:05, Bernhard Bauer wrote:
> It's not great to have to manually resolve this class (for example, it makes
it
> difficult to find references to the class when renaming it) -- Could you maybe
> add a static Java method that will do this instead?

Done.

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
chrome/browser/android/interests_service.cc:67:
env->NewObjectArray(interests.size(), interest_class, 0);
On 2015/09/29 14:04:04, Bernhard Bauer wrote:
> I would use nullptr rather than 0.

Done.

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
chrome/browser/android/interests_service.cc:72: env,
env->NewStringUTF(interests[i].name.c_str()),
On 2015/09/29 14:04:05, Bernhard Bauer wrote:
> Please use base::android::ConvertUTF8ToJavaString() rather than doing your own
> thing.

Done.

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
chrome/browser/android/interests_service.cc:89:
InterestsService::ConvertInterestsToJava(interests);
On 2015/09/29 14:04:05, Bernhard Bauer wrote:
> You could pass the JNIEnv to this method. Also, you are already inside the
> InterestsService class, so you don't need the InterestsService:: prefix.

Not Done. This would require passing the JNIEnv through a callback.

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
File chrome/browser/android/interests_service.h (right):

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
chrome/browser/android/interests_service.h:18: void Destroy(JNIEnv* env, jobject
obj);
On 2015/09/29 14:04:05, Bernhard Bauer wrote:
> I would group methods that are called from Java together and add a comment
> before the group starts.

Done.

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
chrome/browser/android/interests_service.h:28: virtual ~InterestsService();
On 2015/09/29 14:04:05, Bernhard Bauer wrote:
> Destructors should come before other methods.

Done.

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
File chrome/browser/android/interests_service_unittest.cc (right):

https://codereview.chromium.org/1334233003/diff/80001/chrome/browser/android/...
chrome/browser/android/interests_service_unittest.cc:14:
std::vector<InterestsFetcher::Interest> GetEmptyResponse() {
On 2015/09/29 14:04:05, Bernhard Bauer wrote:
> Add an empty line before this one.

Done.