JavaBridge should use Annotation

JavaBridge should use Annotation

Use an annotation to check whether or not a method should be exposed to JavaScript.  This is better
than relying on inheritence to determine whether or not to expose a method.

BUG=http://b/6910450


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150406

[David Trainor- moved to gerrit, 2012-08-04 00:17:13]

ptal

[gone, 2012-08-04 01:21:27]

LGTM, but there's some syntax I'm not super-familiar with.

[joth, 2012-08-04 20:38:28]

+palmer

Thanks for adding this, most just some readability suggestions from me.

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/render...
File content/browser/renderer_host/java/java_bound_object.cc (right):

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/render...
content/browser/renderer_host/java/java_bound_object.cc:41:
"org/chromium/content/common/JavaScriptInterface";
could be in content/browser, as it's only relevant in browser process?

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/render...
content/browser/renderer_host/java/java_bound_object.cc:50: jclass
g_JavaScriptInterface_clazz = NULL;
nit: in the context of this file 'JavaScriptInterface' is pretty ambiguous. Also
naming for non-generated code should be hacker_case.
Maybe g_safe_annotation_clazz

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/render...
content/browser/renderer_host/java/java_bound_object.cc:843: DCHECK(num_methods)
<< "Java objects always have public methods";
nit: I think chromium style is to put purely static message as a comment
instead, to keep bin size down. (sending runtime valies into the check is ok)

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/render...
content/browser/renderer_host/java/java_bound_object.cc:860: is_method_allowed
&= safe;
&= is bitwise operator, and it's not obvious if jboolean is actually type bool,
so could in theory do the wrong thing.
I'd be tempted to say s_method_allowed = s_method_allowed && safe, or maybe
just:
if (!safe)
  continue;
(and remove is_method_allowed)

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/render...
File content/browser/renderer_host/java/java_bound_object.h (right):

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/render...
content/browser/renderer_host/java/java_bound_object.h:28: // annotation to be
exposed.  This property propagates to all Objects that
takes a boolean that determines whether or not methods require an explicit
annotation marking them as safe to be exposed to JavaScript.

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/render...
content/browser/renderer_host/java/java_bound_object.h:63: bool
require_safe_annotation_;
const bool

https://chromiumcodereview.appspot.com/10830173/diff/1/content/public/android...
File
content/public/android/java/src/org/chromium/content/common/JavaScriptInterface.java
(right):

https://chromiumcodereview.appspot.com/10830173/diff/1/content/public/android...
content/public/android/java/src/org/chromium/content/common/JavaScriptInterface.java:7:
import org.chromium.content.browser.ContentViewCore;
maybe rather than import, just use the fully qualified name in the javadoc? (As
is, sets up a circular import)

https://chromiumcodereview.appspot.com/10830173/diff/1/content/public/android...
content/public/android/java/src/org/chromium/content/common/JavaScriptInterface.java:15:
* Marks a method as being able to be run from JavaScript.
nit: Marks a method as being safe to be exposed to JavaScript.

[Steve Block, 2012-08-06 12:12:30]

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/androi...
File content/browser/android/content_view_core_impl.h (right):

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/androi...
content/browser/android/content_view_core_impl.h:120: jboolean
require_safe_annotation);
Bit of a nit, but referring to this as the 'safe annotation' seems odd, as the
annotation's name doesn't include the word 'safe'. Also, we don't know whether
the method is safe: we simply expose it if it's annotated. Maybe
'require_(method_)annotations' or 'expose_all_methods' to keep the current sense
of the boolean?

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/render...
File content/browser/renderer_host/java/java_bound_object.cc (right):

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/render...
content/browser/renderer_host/java/java_bound_object.cc:39: // use different
annotations.
What's the motivation for this?

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/render...
content/browser/renderer_host/java/java_bound_object.cc:737: }
Do we usually use browser_jni_registrar.cc to trigger the  the JNI lookup for
these kind of internals? We don't need to do it at startup, so why not do it
lazily when first required? Also, the name of the method suggests that we're
registering JavaBoundObject for use from Java, which isn't the case. Is this a
standard pattern?

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/render...
content/browser/renderer_host/java/java_bound_object.cc:858:
g_JavaScriptInterface_clazz);
wrong indentation

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/render...
File content/browser/renderer_host/java/java_bound_object.h (right):

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/render...
content/browser/renderer_host/java/java_bound_object.h:28: // annotation to be
exposed.  This property propagates to all Objects that
Or 'The require_safe_annotation flag specifies whether only methods with the
JavaScriptInterface annotation are exposed to JavaScript.'

https://chromiumcodereview.appspot.com/10830173/diff/1/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentView.java
(left):

https://chromiumcodereview.appspot.com/10830173/diff/1/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentView.java:94:
* @hide Only used by the platform browser.
Is this relevant to this change?

https://chromiumcodereview.appspot.com/10830173/diff/1/content/public/android...
File
content/public/android/java/src/org/chromium/content/common/JavaScriptInterface.java
(right):

https://chromiumcodereview.appspot.com/10830173/diff/1/content/public/android...
content/public/android/java/src/org/chromium/content/common/JavaScriptInterface.java:15:
* Marks a method as being able to be run from JavaScript.
Same comment about using the word 'safe' - I think we should either use 'safe'
in the annotation name, or not mention it in the description of what the
annotation does. Mentioning safety as an example of why you might use the
annotation would make sense of course.

https://chromiumcodereview.appspot.com/10830173/diff/1/content/public/android...
content/public/android/java/src/org/chromium/content/common/JavaScriptInterface.java:21:
public @interface JavaScriptInterface {
Hmm, 'JavaScriptInterface' is the correct camel-case, but the WebView methods
from which the Chromium methods take their names use JavascriptInterface. We
plan to add this annotation functionality to the WebView API, right? If so, we
should probably be consistent with the API naming.

[joth, 2012-08-06 15:59:31]

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/render...
File content/browser/renderer_host/java/java_bound_object.cc (right):

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/render...
content/browser/renderer_host/java/java_bound_object.cc:39: // use different
annotations.
On 2012/08/06 12:12:30, Steve Block wrote:
> What's the motivation for this?

WebView would need to use one defined in the SDK, but chrome app can't, as that
wouldn't be present all platforms targeted by the app.

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/render...
content/browser/renderer_host/java/java_bound_object.cc:737: }
On 2012/08/06 12:12:30, Steve Block wrote:
> Do we usually use browser_jni_registrar.cc to trigger the  the JNI lookup for
> these kind of internals? We don't need to do it at startup, so why not do it
> lazily when first required? Also, the name of the method suggests that we're
> registering JavaBoundObject for use from Java, which isn't the case. Is this a
> standard pattern?

It needs to be done in a thread that originated in Java, the easiest way to
ensure it (consistent with our other JNI using code) is to setup at startup.
(see email thread on Friday on this)

It's not usual to have a RegisterFoo method that only does hand-coded lookups
like this, but I'm sure that some other methods do add some manual registartions
along side the script generated ones. I suggested naming it this way, as from
the caller's point of view (i.e. the content registrar) they don't really care
what registration is happening inside this file, the important thing is that
RegisterFoo lives in foo.cc, and that it must be called.

[palmer, 2012-08-06 17:31:26]

lgtm

LGTM

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/androi...
File content/browser/android/content_view_core_impl.h (right):

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/androi...
content/browser/android/content_view_core_impl.h:120: jboolean
require_safe_annotation);
+1; I'd call it |require_annotations|.

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/render...
File content/browser/renderer_host/java/java_bound_object.h (right):

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/render...
content/browser/renderer_host/java/java_bound_object.h:28: // annotation to be
exposed.  This property propagates to all Objects that
> Or 'The require_safe_annotation flag specifies whether only methods with the
> JavaScriptInterface annotation are exposed to JavaScript.'

+1 to this version of the explanation.

[David Trainor- moved to gerrit, 2012-08-06 23:42:32]

Let me know if this addresses your comments/concerns.  Thanks!

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/androi...
File content/browser/android/content_view_core_impl.h (right):

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/androi...
content/browser/android/content_view_core_impl.h:120: jboolean
require_safe_annotation);
On 2012/08/06 17:31:26, Chris P. wrote:
> +1; I'd call it |require_annotations|.

Done.

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/render...
File content/browser/renderer_host/java/java_bound_object.cc (right):

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/render...
content/browser/renderer_host/java/java_bound_object.cc:41:
"org/chromium/content/common/JavaScriptInterface";
Originally I thought we were just going to move the current one to WebView and
in the SDK, so I thought common kind of made sense.  But if we're going to have
a different one for Chrome and for WebView yeah I'll move it into
content/browser :).

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/render...
content/browser/renderer_host/java/java_bound_object.cc:50: jclass
g_JavaScriptInterface_clazz = NULL;
On 2012/08/04 20:38:28, joth wrote:
> nit: in the context of this file 'JavaScriptInterface' is pretty ambiguous.
Also
> naming for non-generated code should be hacker_case.
> Maybe g_safe_annotation_clazz 

Done.

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/render...
content/browser/renderer_host/java/java_bound_object.cc:737: }
On 2012/08/06 15:59:31, joth wrote:
> On 2012/08/06 12:12:30, Steve Block wrote:
> > Do we usually use browser_jni_registrar.cc to trigger the  the JNI lookup
for
> > these kind of internals? We don't need to do it at startup, so why not do it
> > lazily when first required? Also, the name of the method suggests that we're
> > registering JavaBoundObject for use from Java, which isn't the case. Is this
a
> > standard pattern?
> 
> It needs to be done in a thread that originated in Java, the easiest way to
> ensure it (consistent with our other JNI using code) is to setup at startup.
> (see email thread on Friday on this)
> 
> It's not usual to have a RegisterFoo method that only does hand-coded lookups
> like this, but I'm sure that some other methods do add some manual
registartions
> along side the script generated ones. I suggested naming it this way, as from
> the caller's point of view (i.e. the content registrar) they don't really care
> what registration is happening inside this file, the important thing is that
> RegisterFoo lives in foo.cc, and that it must be called. 

If we want to change the name, I can pull this out to a new file, but since it's
basically contained to java_bound_object and solely for it's use, this seemed to
make sense.

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/render...
content/browser/renderer_host/java/java_bound_object.cc:843: DCHECK(num_methods)
<< "Java objects always have public methods";
On 2012/08/04 20:38:28, joth wrote:
> nit: I think chromium style is to put purely static message as a comment
> instead, to keep bin size down. (sending runtime valies into the check is ok)

Done.

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/render...
content/browser/renderer_host/java/java_bound_object.cc:858:
g_JavaScriptInterface_clazz);
On 2012/08/06 12:12:30, Steve Block wrote:
> wrong indentation

Ah moved these in by 2.  Not sure if anything else is incorrect with this.  The
last parameter isn't a member of the GetMethodIDFromClassName so not sure if it
should stay at the same indent level as the parameter above.

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/render...
content/browser/renderer_host/java/java_bound_object.cc:860: is_method_allowed
&= safe;
On 2012/08/04 20:38:28, joth wrote:
> &= is bitwise operator, and it's not obvious if jboolean is actually type
bool,
> so could in theory do the wrong thing.
> I'd be tempted to say s_method_allowed = s_method_allowed && safe, or maybe
> just:
> if (!safe)
>   continue;
> (and remove is_method_allowed)

Ah good catch.  Done.

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/render...
File content/browser/renderer_host/java/java_bound_object.h (right):

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/render...
content/browser/renderer_host/java/java_bound_object.h:28: // annotation to be
exposed.  This property propagates to all Objects that
On 2012/08/06 17:31:26, Chris P. wrote:
> > Or 'The require_safe_annotation flag specifies whether only methods with the
> > JavaScriptInterface annotation are exposed to JavaScript.'
> 
> +1 to this version of the explanation.

Done.

https://chromiumcodereview.appspot.com/10830173/diff/1/content/browser/render...
content/browser/renderer_host/java/java_bound_object.h:63: bool
require_safe_annotation_;
On 2012/08/04 20:38:28, joth wrote:
> const bool

Done.

https://chromiumcodereview.appspot.com/10830173/diff/1/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentView.java
(left):

https://chromiumcodereview.appspot.com/10830173/diff/1/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentView.java:94:
* @hide Only used by the platform browser.
On 2012/08/06 12:12:30, Steve Block wrote:
> Is this relevant to this change?

No I can pull this out to a separate change.

https://chromiumcodereview.appspot.com/10830173/diff/1/content/public/android...
File
content/public/android/java/src/org/chromium/content/common/JavaScriptInterface.java
(right):

https://chromiumcodereview.appspot.com/10830173/diff/1/content/public/android...
content/public/android/java/src/org/chromium/content/common/JavaScriptInterface.java:7:
import org.chromium.content.browser.ContentViewCore;
On 2012/08/04 20:38:28, joth wrote:
> maybe rather than import, just use the fully qualified name in the javadoc?
(As
> is, sets up a circular import)

Done.

https://chromiumcodereview.appspot.com/10830173/diff/1/content/public/android...
content/public/android/java/src/org/chromium/content/common/JavaScriptInterface.java:15:
* Marks a method as being able to be run from JavaScript.
On 2012/08/06 12:12:30, Steve Block wrote:
> Same comment about using the word 'safe' - I think we should either use 'safe'
> in the annotation name, or not mention it in the description of what the
> annotation does. Mentioning safety as an example of why you might use the
> annotation would make sense of course.
> 

Done.

https://chromiumcodereview.appspot.com/10830173/diff/1/content/public/android...
content/public/android/java/src/org/chromium/content/common/JavaScriptInterface.java:21:
public @interface JavaScriptInterface {
On 2012/08/06 12:12:30, Steve Block wrote:
> Hmm, 'JavaScriptInterface' is the correct camel-case, but the WebView methods
> from which the Chromium methods take their names use JavascriptInterface. We
> plan to add this annotation functionality to the WebView API, right? If so, we
> should probably be consistent with the API naming.

Will change thanks!

[joth, 2012-08-06 23:54:16]

lgtm

https://chromiumcodereview.appspot.com/10830173/diff/9001/content/browser/ren...
File content/browser/renderer_host/java/java_bound_object.h (right):

https://chromiumcodereview.appspot.com/10830173/diff/9001/content/browser/ren...
content/browser/renderer_host/java/java_bound_object.h:27: // require_annotation
flag specifies whether or not only methods with the
nit: we use pipes around param names:  |require_annotation|

[Steve Block, 2012-08-07 10:22:50]

lgtm

thanks!

[commit-bot: I haz the power, 2012-08-07 17:58:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtrainor@chromium.org/10830173/12001

[commit-bot: I haz the power, 2012-08-07 17:58:06]

Presubmit check for 10830173-12001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/public/android

Presubmit checks took 2.6s to calculate.

[David Trainor- moved to gerrit, 2012-08-07 18:01:49]

Yaron: Can I get an owners LGTM?  Thanks!

[Yaron, 2012-08-07 18:12:35]

lgtm

https://chromiumcodereview.appspot.com/10830173/diff/12001/content/browser/re...
File content/browser/renderer_host/java/java_bound_object.h (right):

https://chromiumcodereview.appspot.com/10830173/diff/12001/content/browser/re...
content/browser/renderer_host/java/java_bound_object.h:69: bool
RegisterJavaBoundObject(JNIEnv* env);
Nit: make this a static member function of above class

[commit-bot: I haz the power, 2012-08-07 18:42:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtrainor@chromium.org/10830173/5007

[commit-bot: I haz the power, 2012-08-07 21:12:03]

Change committed as 150406