[Android] Java Bridge with Gin: implement Java methods invocation

[Android] Java Bridge with Gin: implement Java methods invocation

This patch adds GinJavaMethodInvocationHelper class which serves
for coercion of arguments and return values of Java methods to / from
base::Value and GinJavaBridgeValue.

The coercion code is taken from the existing implementation
(JavaBoundObject) with required conversions to use base::Value and
GinJavaBridgeValue instead of NPVARIANT and friends.

There are extensive Java tests for coercion, so here we only add some
unit tests for edge cases.

This patch also adds a trivial EventLog class for writing into Android
Event Log from C++ code.

BUG=355644
R=bulach@chromium.org, torne@chromium.org
TBR=brettw@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276586

[mnaganov (inactive), 2014-06-02 11:05:50]

Hi Torne,

Please help with pushing forward the new Java Bridge implementation.

GinJavaBridgeInvocation helper class looks big, but in fact all the coercion
code is taken from the current implementation.

[Torne, 2014-06-06 13:49:54]

LGTM. I don't see any problems in here, and it's not hooked up to anything yet
so let's go for it :)

[mnaganov (inactive), 2014-06-06 13:51:00]

The CQ bit was checked by mnaganov@chromium.org

[commit-bot: I haz the power, 2014-06-06 13:52:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mnaganov@chromium.org/302173006/1

[commit-bot: I haz the power, 2014-06-06 16:37:01]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-06-06 16:40:05]

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

[commit-bot: I haz the power, 2014-06-06 16:40:06]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[mnaganov (inactive), 2014-06-09 08:28:47]

OWNERS review needed, Marcus , can you please take a look at base/android?

base/ itself only has trivial changes to build files, so I think I will just TBR
them.

[bulach, 2014-06-09 11:44:34]

lgtm, mostly nits and some suggestions below.

https://codereview.chromium.org/302173006/diff/1/base/android/event_log.h
File base/android/event_log.h (right):

https://codereview.chromium.org/302173006/diff/1/base/android/event_log.h#new...
base/android/event_log.h:15: void BASE_EXPORT eventLogWriteInt(int tag, int
value);
not: s/event/Event/

https://codereview.chromium.org/302173006/diff/1/base/android/jni_weak_ref.h
File base/android/jni_weak_ref.h (right):

https://codereview.chromium.org/302173006/diff/1/base/android/jni_weak_ref.h#...
base/android/jni_weak_ref.h:28: bool is_null() const { return obj_ == NULL; }
hmm... :)

this can be misleading, it'd be possible to is_null() == false, and yet, get()
can return a "real object" that can be NULL...

how about "is_initialized()", or "has_reference()" ?

https://codereview.chromium.org/302173006/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/java/gin_java_method_invocation_helper.cc
(right):

https://codereview.chromium.org/302173006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/java/gin_java_method_invocation_helper.cc:137:
base::android::eventLogWriteInt(70151, getuid());
any chance we could have those defined here?!

https://codereview.chromium.org/302173006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/java/gin_java_method_invocation_helper.cc:318:
const JavaType& type) {
nit: left align, or keep all indented by +4 like the function below

https://codereview.chromium.org/302173006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/java/gin_java_method_invocation_helper.cc:337:
coerce_to_string);
nit: align

https://codereview.chromium.org/302173006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/java/gin_java_method_invocation_helper.cc:406:
const int64 limit = (GG_INT64_C(1) << 63) - static_cast<uint64>(1 << 10);
nit: kLimit

https://codereview.chromium.org/302173006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/java/gin_java_method_invocation_helper.cc:898:
const JavaType& type,
nit: align

https://codereview.chromium.org/302173006/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/java/gin_java_method_invocation_helper.h
(right):

https://codereview.chromium.org/302173006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/java/gin_java_method_invocation_helper.h:31:
GinJavaBoundObject::ObjectID object_id) = 0;
nit: private: DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/302173006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/java/gin_java_method_invocation_helper.h:43:
virtual const base::android::JavaRef<jclass>& GetSafeAnnotationClass() = 0;
nit: I think we normally return by value rather than by const ref.

https://codereview.chromium.org/302173006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/java/gin_java_method_invocation_helper.h:44: };
nit: private DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/302173006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/java/gin_java_method_invocation_helper.h:102:
jobject CreateJavaArray(const JavaType& type, jsize length);
question: could all these Coerce* methods go into their own
gin_java_type_coercion.h|.cc as a collection of static methods?
I understand there's already a bunch of tests in the java side, but I suppose we
could make this file simpler by splitting it, and then the new tests would only
exercise the behavior added here... wdyt?

[mnaganov (inactive), 2014-06-11 13:50:45]

Thanks, Marcus! Super helpful comments.

https://codereview.chromium.org/302173006/diff/1/base/android/event_log.h
File base/android/event_log.h (right):

https://codereview.chromium.org/302173006/diff/1/base/android/event_log.h#new...
base/android/event_log.h:15: void BASE_EXPORT eventLogWriteInt(int tag, int
value);
On 2014/06/09 11:44:33, bulach wrote:
> not: s/event/Event/

Done.

https://codereview.chromium.org/302173006/diff/1/base/android/jni_weak_ref.h
File base/android/jni_weak_ref.h (right):

https://codereview.chromium.org/302173006/diff/1/base/android/jni_weak_ref.h#...
base/android/jni_weak_ref.h:28: bool is_null() const { return obj_ == NULL; }
On 2014/06/09 11:44:33, bulach wrote:
> hmm... :)
> 
> this can be misleading, it'd be possible to is_null() == false, and yet, get()
> can return a "real object" that can be NULL...
> 
> how about "is_initialized()", or "has_reference()" ?

Thanks for raising this! Indeed, is_null() seems to be misleading.

My preference will be is_empty() -- also looks informative(and is similar to the
terminology used in variant types) and is short.

https://codereview.chromium.org/302173006/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/java/gin_java_method_invocation_helper.cc
(right):

https://codereview.chromium.org/302173006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/java/gin_java_method_invocation_helper.cc:137:
base::android::eventLogWriteInt(70151, getuid());
On 2014/06/09 11:44:33, bulach wrote:
> any chance we could have those defined here?!

I have moved the value into a named constant. There is in fact a method in Java
EventLog class for obtaining a tag code by its name, but then we have to define
tag names here :) Since it's us who actually define the mapping, using the int
code directly is just simpler and faster.

https://codereview.chromium.org/302173006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/java/gin_java_method_invocation_helper.cc:318:
const JavaType& type) {
On 2014/06/09 11:44:33, bulach wrote:
> nit: left align, or keep all indented by +4 like the function below

Done.

https://codereview.chromium.org/302173006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/java/gin_java_method_invocation_helper.cc:337:
coerce_to_string);
On 2014/06/09 11:44:33, bulach wrote:
> nit: align

Done, thanks!

https://codereview.chromium.org/302173006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/java/gin_java_method_invocation_helper.cc:406:
const int64 limit = (GG_INT64_C(1) << 63) - static_cast<uint64>(1 << 10);
On 2014/06/09 11:44:33, bulach wrote:
> nit: kLimit

Done.

https://codereview.chromium.org/302173006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/java/gin_java_method_invocation_helper.cc:898:
const JavaType& type,
On 2014/06/09 11:44:33, bulach wrote:
> nit: align

Done.

https://codereview.chromium.org/302173006/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/java/gin_java_method_invocation_helper.h
(right):

https://codereview.chromium.org/302173006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/java/gin_java_method_invocation_helper.h:31:
GinJavaBoundObject::ObjectID object_id) = 0;
On 2014/06/09 11:44:33, bulach wrote:
> nit: private: DISALLOW_COPY_AND_ASSIGN?

A good idea! Done.

https://codereview.chromium.org/302173006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/java/gin_java_method_invocation_helper.h:43:
virtual const base::android::JavaRef<jclass>& GetSafeAnnotationClass() = 0;
On 2014/06/09 11:44:33, bulach wrote:
> nit: I think we normally return by value rather than by const ref.

It's not possible to return a JavaRef by value because it's non-copyable, you
can only return ScopedJavaLocal / GlobalRef this way.

https://codereview.chromium.org/302173006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/java/gin_java_method_invocation_helper.h:44: };
On 2014/06/09 11:44:33, bulach wrote:
> nit: private DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/302173006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/java/gin_java_method_invocation_helper.h:102:
jobject CreateJavaArray(const JavaType& type, jsize length);
On 2014/06/09 11:44:34, bulach wrote:
> question: could all these Coerce* methods go into their own
> gin_java_type_coercion.h|.cc as a collection of static methods?
> I understand there's already a bunch of tests in the java side, but I suppose
we
> could make this file simpler by splitting it, and then the new tests would
only
> exercise the behavior added here... wdyt?

This makes sense, extracted.

We don't add any new behaviour in our Coerce* methods, I have only converted
them from using NPAPI types to using base::Value and GinJavaBridgeValue.

I have already added tests for GinJavaMethodInvocationHelper itself.

[mnaganov (inactive), 2014-06-11 13:58:06]

TBR=brettw for trivial changes to build files in base/

[mnaganov (inactive), 2014-06-11 13:58:13]

The CQ bit was checked by mnaganov@chromium.org

[commit-bot: I haz the power, 2014-06-11 13:59:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mnaganov@chromium.org/302173006/40001

[mnaganov (inactive), 2014-06-11 14:33:03]

The CQ bit was unchecked by mnaganov@chromium.org

[mnaganov (inactive), 2014-06-11 14:40:46]

The CQ bit was checked by mnaganov@chromium.org

[commit-bot: I haz the power, 2014-06-11 14:42:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mnaganov@chromium.org/302173006/60001

[commit-bot: I haz the power, 2014-06-11 22:22:32]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-11 22:27:07]

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

[commit-bot: I haz the power, 2014-06-11 22:27:08]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[mnaganov (inactive), 2014-06-12 08:36:15]

Committed patchset #4 manually as r276586 (presubmit successful).