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

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

BUG=537145
Previous CL:https://codereview.chromium.org/1334233003

Committed: https://crrev.com/ff92275ee64400fa442e9ec8e3732eab42dda709
Cr-Commit-Position: refs/heads/master@{#353034}

[PEConn, 2015-10-07 13:54:56]

peconn@chromium.org changed reviewers:
+ bauerb@chromium.org, treib@chromium.org

[PEConn, 2015-10-07 13:56:25]

Continuing from https://codereview.chromium.org/1334233003.

The CL that depended on has now been committed, so this doesn't depend on
anything.

I am working on the integration tests for this.

[Marc Treib, 2015-10-07 15:03:01]

I think this needs a rebase, the changes from the already-landed CL are in here
again.

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

https://codereview.chromium.org/1396443002/diff/1/chrome/browser/android/inte...
chrome/browser/android/interests_service.cc:47: base::Owned(fetcher.get()),
Can you use base::Passed(fetcher), and remove the ugly explicit release() below?
(Get a raw pointer to fetcher first, and call FetchInterests using that.) As it
is, fetcher has two owners for a brief period.

https://codereview.chromium.org/1396443002/diff/1/chrome/browser/android/inte...
chrome/browser/android/interests_service.cc:90: 
nit: remove empty line

[Bernhard Bauer, 2015-10-07 15:26:37]

Pro-tip: Upload the previous version (before your changes) as a first patch set,
to make it easier to compare them.

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

https://codereview.chromium.org/1396443002/diff/1/chrome/browser/android/inte...
chrome/browser/android/interests_service.cc:23: const char kInterestJavaClass[]
=
This is now unused.

https://codereview.chromium.org/1396443002/diff/1/chrome/browser/android/inte...
chrome/browser/android/interests_service.cc:66: JNIEnv* env =
AttachCurrentThread();
What I meant in my previous comment was to pass |env| as a parameter into this
method. That does not require you to wrap it in a callback.

https://codereview.chromium.org/1396443002/diff/1/chrome/browser/android/inte...
chrome/browser/android/interests_service.cc:72: auto& interest =
interests->at(i);
Isn't this type just InterestsFetcher::Interest? I don't think using auto makes
things clearer here.

Also, use the [] operator instead of `at` (you might have to do it like this:
(*interests)[i]), as the latter throws an exception, but we don't use exceptions
in C++.

Oh hey, couldn't you just use a C++11-style for-loop? (But please still with an
explicit type.)

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

https://codereview.chromium.org/1396443002/diff/1/chrome/browser/android/inte...
chrome/browser/android/interests_service.h:5: #ifndef
CHROME_BROWSER_ANDROID_INTERESTS_SERVICE_H_
Can we move this file to chrome/browser/interests/android/?

https://codereview.chromium.org/1396443002/diff/1/chrome/browser/android/inte...
chrome/browser/android/interests_service.h:11: #include
"chrome/browser/interests/interests_fetcher.h"
You can forward-declare this and Profile.

OTOH, you might want to explicitly include weak_ptr.h and scoped_ptr.h (because
Include-What-You-Use).

https://codereview.chromium.org/1396443002/diff/1/chrome/browser/interests/in...
File chrome/browser/interests/interests_fetcher.h (right):

https://codereview.chromium.org/1396443002/diff/1/chrome/browser/interests/in...
chrome/browser/interests/interests_fetcher.h:22: // represented as a tuple of
its name, an url to a representative image, and a
Nit: URL

https://codereview.chromium.org/1396443002/diff/1/chrome/browser/interests/in...
chrome/browser/interests/interests_fetcher.h:48:
base::Callback<void(scoped_ptr<std::vector<Interest>>)>;
Indent four spaces if you are breaking a line.

[PEConn, 2015-10-07 18:27:20]

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

https://codereview.chromium.org/1396443002/diff/1/chrome/browser/android/inte...
chrome/browser/android/interests_service.cc:23: const char kInterestJavaClass[]
=
On 2015/10/07 15:26:37, Bernhard Bauer wrote:
> This is now unused.

Done.

https://codereview.chromium.org/1396443002/diff/1/chrome/browser/android/inte...
chrome/browser/android/interests_service.cc:47: base::Owned(fetcher.get()),
On 2015/10/07 15:03:01, Marc Treib wrote:
> Can you use base::Passed(fetcher), and remove the ugly explicit release()
below?
> (Get a raw pointer to fetcher first, and call FetchInterests using that.) As
it
> is, fetcher has two owners for a brief period.

Done.

https://codereview.chromium.org/1396443002/diff/1/chrome/browser/android/inte...
chrome/browser/android/interests_service.cc:66: JNIEnv* env =
AttachCurrentThread();
On 2015/10/07 15:26:37, Bernhard Bauer wrote:
> What I meant in my previous comment was to pass |env| as a parameter into this
> method. That does not require you to wrap it in a callback.

Done.

https://codereview.chromium.org/1396443002/diff/1/chrome/browser/android/inte...
chrome/browser/android/interests_service.cc:72: auto& interest =
interests->at(i);
On 2015/10/07 15:26:37, Bernhard Bauer wrote:
> Isn't this type just InterestsFetcher::Interest? I don't think using auto
makes
> things clearer here.
> 
> Also, use the [] operator instead of `at` (you might have to do it like this:
> (*interests)[i]), as the latter throws an exception, but we don't use
exceptions
> in C++.
> 
> Oh hey, couldn't you just use a C++11-style for-loop? (But please still with
an
> explicit type.)

Done. Good point about at throwing an exception. I didn't want to change this to
a for-each since we need the index (to insert into the java array at the correct
point).

https://codereview.chromium.org/1396443002/diff/1/chrome/browser/android/inte...
chrome/browser/android/interests_service.cc:90: 
On 2015/10/07 15:03:01, Marc Treib wrote:
> nit: remove empty line

Done.

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

https://codereview.chromium.org/1396443002/diff/1/chrome/browser/android/inte...
chrome/browser/android/interests_service.h:11: #include
"chrome/browser/interests/interests_fetcher.h"
On 2015/10/07 15:26:37, Bernhard Bauer wrote:
> You can forward-declare this and Profile.
> 
> OTOH, you might want to explicitly include weak_ptr.h and scoped_ptr.h
(because
> Include-What-You-Use).

Done. I forward declared Profile and ScopedJavaGlobalRef. I couldn't do that for
InterestsFetcher because we use Interest, its inner class.

https://codereview.chromium.org/1396443002/diff/1/chrome/browser/interests/in...
File chrome/browser/interests/interests_fetcher.h (right):

https://codereview.chromium.org/1396443002/diff/1/chrome/browser/interests/in...
chrome/browser/interests/interests_fetcher.h:22: // represented as a tuple of
its name, an url to a representative image, and a
On 2015/10/07 15:26:37, Bernhard Bauer wrote:
> Nit: URL

Done.

https://codereview.chromium.org/1396443002/diff/1/chrome/browser/interests/in...
chrome/browser/interests/interests_fetcher.h:48:
base::Callback<void(scoped_ptr<std::vector<Interest>>)>;
On 2015/10/07 15:26:37, Bernhard Bauer wrote:
> Indent four spaces if you are breaking a line.

Done.

[Marc Treib, 2015-10-08 08:26:51]

LGTM, thanks!

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
File chrome/browser/interests/android/interests_service.cc (right):

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
chrome/browser/interests/android/interests_service.cc:35: 
nit: no empty line here

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
File chrome/browser/interests/android/interests_service.h (right):

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
chrome/browser/interests/android/interests_service.h:17: template<typename T>
class ScopedJavaGlobalRef;
Ugh. TBH, I'd prefer to just include the header here. (But of course this is
okay too, especially since Bernhard suggested forward declaring)

[Bernhard Bauer, 2015-10-08 09:04:41]

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
File chrome/browser/interests/android/interests_service.cc (right):

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
chrome/browser/interests/android/interests_service.cc:17: using
base::android::ConvertJavaStringToUTF8;
This is unused.

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
chrome/browser/interests/android/interests_service.cc:21: using
base::android::ToJavaArrayOfStrings;
This is unused.

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
chrome/browser/interests/android/interests_service.cc:40: InterestsFetcher*
fetcherRawPtr = fetcher.get();
Nit: |fetcher_raw_ptr|. I know, mixing Java and C++ style is confusing ☺

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
chrome/browser/interests/android/interests_service.cc:56: jobjectArray
InterestsService::ConvertInterestsToJava(
I think it would make sense to use a ScopedJavaLocalRef for the local variable
and return value, just to make we sure don't accidentally call into a Java
method in between, the JVM decides to do a garbage collection, and throws away
the object.

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
chrome/browser/interests/android/interests_service.cc:66:
InterestsFetcher::Interest& interest = (*interests)[i];
Make this const?

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
File chrome/browser/interests/android/interests_service.h (right):

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
chrome/browser/interests/android/interests_service.h:14: class Profile;
Empty line afterwards.

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
chrome/browser/interests/android/interests_service.h:39: static jobjectArray
ConvertInterestsToJava(
Can you add a comment that explains why this is a static private method (instead
of being in an anonymous namespace)?

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
File chrome/browser/interests/interests_fetcher_unittest.cc (right):

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
chrome/browser/interests/interests_fetcher_unittest.cc:88: if (interests) {
Early-return if (!interests)?

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
chrome/browser/interests/interests_fetcher_unittest.cc:92:
OnSuccessfulResponse();
Add an additional else that fails the test, in case a different response is
received?

[PEConn, 2015-10-08 09:40:56]

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
File chrome/browser/interests/android/interests_service.cc (right):

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
chrome/browser/interests/android/interests_service.cc:17: using
base::android::ConvertJavaStringToUTF8;
On 2015/10/08 09:04:41, Bernhard Bauer wrote:
> This is unused.

Done.

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
chrome/browser/interests/android/interests_service.cc:21: using
base::android::ToJavaArrayOfStrings;
On 2015/10/08 09:04:41, Bernhard Bauer wrote:
> This is unused.

Done.

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
chrome/browser/interests/android/interests_service.cc:35: 
On 2015/10/08 08:26:51, Marc Treib wrote:
> nit: no empty line here

Done.

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
chrome/browser/interests/android/interests_service.cc:40: InterestsFetcher*
fetcherRawPtr = fetcher.get();
On 2015/10/08 09:04:41, Bernhard Bauer wrote:
> Nit: |fetcher_raw_ptr|. I know, mixing Java and C++ style is confusing ☺

Done.

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
chrome/browser/interests/android/interests_service.cc:56: jobjectArray
InterestsService::ConvertInterestsToJava(
On 2015/10/08 09:04:41, Bernhard Bauer wrote:
> I think it would make sense to use a ScopedJavaLocalRef for the local variable
> and return value, just to make we sure don't accidentally call into a Java
> method in between, the JVM decides to do a garbage collection, and throws away
> the object.

Done.

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
chrome/browser/interests/android/interests_service.cc:66:
InterestsFetcher::Interest& interest = (*interests)[i];
On 2015/10/08 09:04:41, Bernhard Bauer wrote:
> Make this const?

Done.

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
File chrome/browser/interests/android/interests_service.h (right):

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
chrome/browser/interests/android/interests_service.h:14: class Profile;
On 2015/10/08 09:04:41, Bernhard Bauer wrote:
> Empty line afterwards.

Done.

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
chrome/browser/interests/android/interests_service.h:17: template<typename T>
class ScopedJavaGlobalRef;
On 2015/10/08 08:26:51, Marc Treib wrote:
> Ugh. TBH, I'd prefer to just include the header here. (But of course this is
> okay too, especially since Bernhard suggested forward declaring)

Done.

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
chrome/browser/interests/android/interests_service.h:39: static jobjectArray
ConvertInterestsToJava(
On 2015/10/08 09:04:41, Bernhard Bauer wrote:
> Can you add a comment that explains why this is a static private method
(instead
> of being in an anonymous namespace)?

Moved to anonymous namespace.

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
File chrome/browser/interests/interests_fetcher_unittest.cc (right):

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
chrome/browser/interests/interests_fetcher_unittest.cc:88: if (interests) {
On 2015/10/08 09:04:41, Bernhard Bauer wrote:
> Early-return if (!interests)?

Done.

https://codereview.chromium.org/1396443002/diff/20001/chrome/browser/interest...
chrome/browser/interests/interests_fetcher_unittest.cc:92:
OnSuccessfulResponse();
On 2015/10/08 09:04:41, Bernhard Bauer wrote:
> Add an additional else that fails the test, in case a different response is
> received?

Done.

[Bernhard Bauer, 2015-10-08 09:50:03]

lgtm

[PEConn, 2015-10-08 09:50:50]

The CQ bit was checked by peconn@chromium.org

[PEConn, 2015-10-08 09:50:51]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org
Link to the patchset: https://codereview.chromium.org/1396443002/#ps40001
(title: " ")

[commit-bot: I haz the power, 2015-10-08 09:52:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1396443002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1396443002/40001

[commit-bot: I haz the power, 2015-10-08 11:42:10]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-08 11:43:50]

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