Pass method parameters as JavaParamRef in chrome/browser (part 2).

Pass method parameters as JavaParamRef in chrome/browser (part 2).

Pass all object parameters to JNI methods in JavaParamRef<> wrappers.
This matches previous changes made to do this for JNI non-method
functions.

BUG=506850
Committed: https://crrev.com/0a4a3ab13de0876e43ca92cb6e7b4fb66a5b70d1
Cr-Commit-Position: refs/heads/master@{#364435}

[Torne, 2015-12-09 18:51:26]

torne@chromium.org changed reviewers:
+ maxbogue@chromium.org, rogerta@chromium.org, treib@chromium.org,
vabr@chromium.org

[Torne, 2015-12-09 18:51:27]

All simple scripted changes to function prototypes. OWNERS for subdirs of
chrome/browser:

vabr: autofill, password_manager
maxbogue: sync
treib: interests
rogerta: signin

[Roger Tawa OOO till Jul 10th, 2015-12-09 19:19:31]

c/b/signin lgtm

[Marc Treib, 2015-12-10 08:37:17]

c/b/interests lgtm

https://codereview.chromium.org/1509373004/diff/1/chrome/browser/interests/an...
File chrome/browser/interests/android/interests_service.cc (right):

https://codereview.chromium.org/1509373004/diff/1/chrome/browser/interests/an...
chrome/browser/interests/android/interests_service.cc:54: void
InterestsService::Destroy(JNIEnv* env, const JavaParamRef<jobject>& obj) {
Don't you need a
using base::android::JavaParamRef;
at the top of this file?

[vabr (Chromium), 2015-12-10 08:43:13]

Rubberstamp LGTM for autofill and password_manager.
And by rubberstamp I mean that it looked sane to me, on the assumption that you
know what you are doing by replacing the jobject with const
base::android::JavaParamRef<jobject>&, and that the compiler is happy with the
new code.

Cheers,
Vaclav

[Torne, 2015-12-10 11:25:41]

On 2015/12/10 08:43:13, vabr (Chromium) wrote:
> And by rubberstamp I mean that it looked sane to me, on the assumption that
you
> know what you are doing by replacing the jobject with const
> base::android::JavaParamRef<jobject>&

I'm fairly confident I know what I'm doing; the plan to improve the JNI binding
system has been discussed with a number of people who work on Android stuff, and
there have been a number of changes like this landed already without issues. I
just want OWNERS to get a chance to see the changes so they are familiar with
what JNI code "should" look like in future, and because TBR'ing changing 1500+
functions across the tree is probably not okay ;)

> and that the compiler is happy with the
> new code.

Yeah, I've been building locally and will land through CQ; the main purpose of
these JNI changes is to allow the compiler to do a better job of enforcing
correct usage of Java object references, so the compiler's opinion of them is
key ;)

[Torne, 2015-12-10 11:26:01]

https://codereview.chromium.org/1509373004/diff/1/chrome/browser/interests/an...
File chrome/browser/interests/android/interests_service.cc (right):

https://codereview.chromium.org/1509373004/diff/1/chrome/browser/interests/an...
chrome/browser/interests/android/interests_service.cc:54: void
InterestsService::Destroy(JNIEnv* env, const JavaParamRef<jobject>& obj) {
On 2015/12/10 08:37:17, Marc Treib wrote:
> Don't you need a
> using base::android::JavaParamRef;
> at the top of this file?

All the generated jni/*_jni.h headers already use these types without the
namespace, and include jni_generator_helper.h, which has a using declaration for
ScopedJavaLocalRef and JavaParamRef (the types used in JNI bound functions).
This probably isn't the best style, but many .cc files throughout the codebase
already relied on it before I started this work, and I didn't think about it too
much at the time :/

If I changed the generator to output the full namespace I could remove the using
directive from the header file and go through and fix all the .cc files to do it
themselves - if people think this is worth doing I can file a bug to track doing
that later once I've been able to land all the parts of the migration I'm
already in the middle of.

[Marc Treib, 2015-12-10 11:46:02]

https://codereview.chromium.org/1509373004/diff/1/chrome/browser/interests/an...
File chrome/browser/interests/android/interests_service.cc (right):

https://codereview.chromium.org/1509373004/diff/1/chrome/browser/interests/an...
chrome/browser/interests/android/interests_service.cc:54: void
InterestsService::Destroy(JNIEnv* env, const JavaParamRef<jobject>& obj) {
On 2015/12/10 11:26:01, Torne wrote:
> On 2015/12/10 08:37:17, Marc Treib wrote:
> > Don't you need a
> > using base::android::JavaParamRef;
> > at the top of this file?
> 
> All the generated jni/*_jni.h headers already use these types without the
> namespace, and include jni_generator_helper.h, which has a using declaration
for
> ScopedJavaLocalRef and JavaParamRef (the types used in JNI bound functions).
> This probably isn't the best style, but many .cc files throughout the codebase
> already relied on it before I started this work, and I didn't think about it
too
> much at the time :/

Thanks for the explanation!
The generated *_jni.h headers should only ever get included in .cc files, so I
guess that's kinda sorta okay.

> If I changed the generator to output the full namespace I could remove the
using
> directive from the header file and go through and fix all the .cc files to do
it
> themselves - if people think this is worth doing I can file a bug to track
doing
> that later once I've been able to land all the parts of the migration I'm
> already in the middle of.

Hm, not sure if that's worth the hassle. I certainly won't pressure you to do
it. :)

[vabr (Chromium), 2015-12-10 11:48:44]

On 2015/12/10 11:25:41, Torne wrote:
> On 2015/12/10 08:43:13, vabr (Chromium) wrote:
> > And by rubberstamp I mean that it looked sane to me, on the assumption that
> you
> > know what you are doing by replacing the jobject with const
> > base::android::JavaParamRef<jobject>&
> 
> I'm fairly confident I know what I'm doing; the plan to improve the JNI
binding
> system has been discussed with a number of people who work on Android stuff,
and
> there have been a number of changes like this landed already without issues. I
> just want OWNERS to get a chance to see the changes so they are familiar with
> what JNI code "should" look like in future, and because TBR'ing changing 1500+
> functions across the tree is probably not okay ;)
> 
> > and that the compiler is happy with the
> > new code.
> 
> Yeah, I've been building locally and will land through CQ; the main purpose of
> these JNI changes is to allow the compiler to do a better job of enforcing
> correct usage of Java object references, so the compiler's opinion of them is
> key ;)

Acknowledged, thanks for elaborating. SGTM.
Vaclav

[maxbogue, 2015-12-10 18:25:01]

sync LGTM % one missed jobject.

https://codereview.chromium.org/1509373004/diff/1/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_android.h (right):

https://codereview.chromium.org/1509373004/diff/1/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_android.h:88: jboolean
IsPassphraseRequired(JNIEnv* env, jobject obj);
Missed one?

[Torne, 2015-12-10 18:29:51]

https://codereview.chromium.org/1509373004/diff/1/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_android.h (right):

https://codereview.chromium.org/1509373004/diff/1/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_android.h:88: jboolean
IsPassphraseRequired(JNIEnv* env, jobject obj);
On 2015/12/10 18:25:01, maxbogue wrote:
> Missed one?

This isn't actually a JNI method, it's not called from Java code.

I'm only trying to enforce that all the JNI method parameters are JavaParamRef
for now. In a future phase of this migration I'll be trying to get rid of all
bare jobjects. Sorry for the inconsistency, but if I try to change everything at
once it will be a nightmarishly large CL :)

[Torne, 2015-12-10 18:47:00]

The CQ bit was checked by torne@chromium.org

[commit-bot: I haz the power, 2015-12-10 18:49:39]

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

[commit-bot: I haz the power, 2015-12-10 19:46:01]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-12-10 19:47:50]

Description was changed from

==========
Pass method parameters as JavaParamRef in chrome/browser (part 2).

Pass all object parameters to JNI methods in JavaParamRef<> wrappers.
This matches previous changes made to do this for JNI non-method
functions.

BUG=506850
==========

to

==========
Pass method parameters as JavaParamRef in chrome/browser (part 2).

Pass all object parameters to JNI methods in JavaParamRef<> wrappers.
This matches previous changes made to do this for JNI non-method
functions.

BUG=506850

Committed: https://crrev.com/0a4a3ab13de0876e43ca92cb6e7b4fb66a5b70d1
Cr-Commit-Position: refs/heads/master@{#364435}
==========

[commit-bot: I haz the power, 2015-12-10 19:47:51]

Patchset 1 (id:??) landed as
https://crrev.com/0a4a3ab13de0876e43ca92cb6e7b4fb66a5b70d1
Cr-Commit-Position: refs/heads/master@{#364435}

[maxbogue, 2015-12-10 22:08:00]

https://codereview.chromium.org/1509373004/diff/1/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_android.h (right):

https://codereview.chromium.org/1509373004/diff/1/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_android.h:88: jboolean
IsPassphraseRequired(JNIEnv* env, jobject obj);
On 2015/12/10 18:29:51, Torne wrote:
> On 2015/12/10 18:25:01, maxbogue wrote:
> > Missed one?
> 
> This isn't actually a JNI method, it's not called from Java code.
> 
> I'm only trying to enforce that all the JNI method parameters are JavaParamRef
> for now. In a future phase of this migration I'll be trying to get rid of all
> bare jobjects. Sorry for the inconsistency, but if I try to change everything
at
> once it will be a nightmarishly large CL :)

Oh how interesting; this shouldn't actually be here if it's not used. I don't
know how you generated this CL, but however you did it might be useful for
finding other unused methods like this! I'll remove it in a followup.

[Torne, 2015-12-11 14:56:03]

On 2015/12/10 22:08:00, maxbogue wrote:
>
https://codereview.chromium.org/1509373004/diff/1/chrome/browser/sync/profile...
> File chrome/browser/sync/profile_sync_service_android.h (right):
> 
>
https://codereview.chromium.org/1509373004/diff/1/chrome/browser/sync/profile...
> chrome/browser/sync/profile_sync_service_android.h:88: jboolean
> IsPassphraseRequired(JNIEnv* env, jobject obj);
> On 2015/12/10 18:29:51, Torne wrote:
> > On 2015/12/10 18:25:01, maxbogue wrote:
> > > Missed one?
> > 
> > This isn't actually a JNI method, it's not called from Java code.
> > 
> > I'm only trying to enforce that all the JNI method parameters are
JavaParamRef
> > for now. In a future phase of this migration I'll be trying to get rid of
all
> > bare jobjects. Sorry for the inconsistency, but if I try to change
everything
> at
> > once it will be a nightmarishly large CL :)
> 
> Oh how interesting; this shouldn't actually be here if it's not used. I don't
> know how you generated this CL, but however you did it might be useful for
> finding other unused methods like this! I'll remove it in a followup.

I generated it by patching the JNI binding generator to spit out a list of all
the functions it generated bindings for as a side effect during the build, then
using nightmarish python scripts full of regex to find those functions and
update their prototypes :)

There isn't really any difference between a JNI-bound method and any other C++
method that just happens to take a JNIEnv* and a jobject/JavaRef<jobject> as its
first two parameters, so it's not really possible to assume across the board
that anything which looks like a JNI method but isn't used by the JNI generator
is unused - it might never have been intended to be a JNI method in the first
place and is called somewhere else. I'd suggest the answer here is to just use
the "normal" mechanisms of finding unused functions (e.g. seeing what the linker
discards at link time when using --gc-sections), though I'm not sure they're
particularly great ;)