Add support for calling static methods in the Java Bridge

Add support for calling static methods in the Java Bridge

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

[joth, 2011-11-29 11:24:36]

LGTM with a couple nits, and a hang-over comment from the previous CL!

http://codereview.chromium.org/8659007/diff/1/content/browser/renderer_host/j...
File content/browser/renderer_host/java/java_method.cc (right):

http://codereview.chromium.org/8659007/diff/1/content/browser/renderer_host/j...
content/browser/renderer_host/java/java_method.cc:84: class
MethodGetParameterTypesID : public MethodID {
aside: I never noticed MethodID get added in the later patch of the prior CL.

I don't think it really belongs in base/... as (a) it's only useful if derived
into a class hierarchy - seems nicer to keep base primitives concrete where they
can be, and (b) the comment on it suggests it's not as good to use as the plain
GetMethodId method (that avoids the class name lookup)

more aside: as a general pattern I think the static jmethodId is preferable to
these Signletons: as well as needing less boiler plate, they also don't have the
heap overhead. (no difference for 1 or 2, but over hundreds of these things it
could add up?)

http://codereview.chromium.org/8659007/diff/1/content/browser/renderer_host/j...
content/browser/renderer_host/java/java_method.cc:140: class
MethodGetModifiersID : public MethodID {
nit: I'd find all these easier to read if named like GetModifiersMethodId

http://codereview.chromium.org/8659007/diff/1/content/browser/renderer_host/j...
content/browser/renderer_host/java/java_method.cc:145: private:
nit: we normally have a blank line before private: and another before
DISALLOW.... (applies to whole file)

http://codereview.chromium.org/8659007/diff/1/content/browser/renderer_host/j...
content/browser/renderer_host/java/java_method.cc:154: class ClassGetNameID :
public MethodID {
add 'Method' in the name for consistency with the rest

[Steve Block, 2011-11-29 16:54:00]

http://codereview.chromium.org/8659007/diff/1/content/browser/renderer_host/j...
File content/browser/renderer_host/java/java_method.cc (right):

http://codereview.chromium.org/8659007/diff/1/content/browser/renderer_host/j...
content/browser/renderer_host/java/java_method.cc:84: class
MethodGetParameterTypesID : public MethodID {
Personally I prefer a simple static too, but I think that statics are
discouraged. It was suggested that I use a Singleton -
http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho...

http://codereview.chromium.org/8659007/diff/1/content/browser/renderer_host/j...
content/browser/renderer_host/java/java_method.cc:140: class
MethodGetModifiersID : public MethodID {
All of these are named <class><method>ID. I don't think we need an extra 'Get'?

http://codereview.chromium.org/8659007/diff/1/content/browser/renderer_host/j...
content/browser/renderer_host/java/java_method.cc:145: private:
On 2011/11/29 11:24:36, joth wrote:
> nit: we normally have a blank line before private: and another before
> DISALLOW.... (applies to whole file)

Done.

http://codereview.chromium.org/8659007/diff/1/content/browser/renderer_host/j...
content/browser/renderer_host/java/java_method.cc:154: class ClassGetNameID :
public MethodID {
See above

[joth, 2011-11-29 18:13:12]

On 29 November 2011 16:54, <steveblock@chromium.org> wrote:

>
> http://codereview.chromium.**org/8659007/diff/1/content/**
>
browser/renderer_host/java/**java_method.cc<http://codereview.chromium.org/8659007/diff/1/content/browser/renderer_host/java/java_method.cc>
> File content/browser/renderer_host/**java/java_method.cc (right):
>
> http://codereview.chromium.**org/8659007/diff/1/content/**
>
browser/renderer_host/java/**java_method.cc#newcode84<http://codereview.chromium.org/8659007/diff/1/content/browser/renderer_host/java/java_method.cc#newcode84>
> content/browser/renderer_host/**java/java_method.cc:84: class
> MethodGetParameterTypesID : public MethodID {
> Personally I prefer a simple static too, but I think that statics are
> discouraged. It was suggested that I use a Singleton -
> http://codereview.chromium.**org/8509019/diff/12003/**
>
content/browser/renderer_host/**java/java_bound_object.cc#**newcode489<http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_host/java/java_bound_object.cc#newcode489>
>
> ah yes. I guess I missed that as it was tacked on the bottom of the file
ordering comment.

these don't feel /much/ like  a Singleton<> as
- they don't need thread safe c'tor
- they don't need onExit cleanup
- they don't have a distinct 'singleton' class to differentiate all the
instances (you've had to invent such classes.)
- they are only one word in size to start with

possibly could be LazyInstance, although you'd still need lots of
boiler-plate derived classes as you can't pass params to a lazy instance
c'tor. we could make a very nice customization based on those if needed
(i.e. lazy class ID / lazy method ID that do thread safe lock free init)
but as you don't need thread safety here, it really could be as simple as:

jmethodID GetObjectGetClassID() {
  static jmethodID g_object_get_class_id = GetMethodIDFromClassName(...);
  return g_object_get_class_id;
}






>
> http://codereview.chromium.**org/8659007/diff/1/content/**
>
browser/renderer_host/java/**java_method.cc#newcode140<http://codereview.chromium.org/8659007/diff/1/content/browser/renderer_host/java/java_method.cc#newcode140>
> content/browser/renderer_host/**java/java_method.cc:140: class
> MethodGetModifiersID : public MethodID {
> All of these are named <class><method>ID. I don't think we need an extra
> 'Get'?
>
>
> http://codereview.chromium.**org/8659007/diff/1/content/**
>
browser/renderer_host/java/**java_method.cc#newcode145<http://codereview.chromium.org/8659007/diff/1/content/browser/renderer_host/java/java_method.cc#newcode145>
> content/browser/renderer_host/**java/java_method.cc:145: private:
> On 2011/11/29 11:24:36, joth wrote:
>
>> nit: we normally have a blank line before private: and another before
>> DISALLOW.... (applies to whole file)
>>
>
> Done.
>
>
> http://codereview.chromium.**org/8659007/diff/1/content/**
>
browser/renderer_host/java/**java_method.cc#newcode154<http://codereview.chromium.org/8659007/diff/1/content/browser/renderer_host/java/java_method.cc#newcode154>
> content/browser/renderer_host/**java/java_method.cc:154: class
> ClassGetNameID : public MethodID {
> See above
>
>
http://codereview.chromium.**org/8659007/<http://codereview.chromium.org/8659...
>

[Steve Block, 2011-12-05 15:20:31]

Ready for another look, now http://codereview.chromium.org/8769005 has landed

[joth, 2011-12-05 15:53:23]

http://codereview.chromium.org/8659007/diff/9001/content/browser/renderer_hos...
File content/browser/renderer_host/java/java_method.cc (right):

http://codereview.chromium.org/8659007/diff/9001/content/browser/renderer_hos...
content/browser/renderer_host/java/java_method.cc:43: // The parenthesis is very
important here to force POD type initialization.
this sentence not needed here. (we already know ScopedJavaGlobalRef<> is not
POD)

http://codereview.chromium.org/8659007/diff/9001/content/browser/renderer_hos...
content/browser/renderer_host/java/java_method.cc:212:
g_java_lang_reflect_modifier_class.Pointer()->obj(),
nit: g_java_lang_reflect_modifier_class.Get().obj() is slightly easier to read.

[Avi (use Gerrit), 2011-12-05 16:37:32]

owner rubberstamp lgtm