Adds the renderer component of the Java Bridge

Adds the renderer component of the Java Bridge

This patch adds JavaBridgeDispatcher, which handles injecting Java objects into
the main frame of a RenderView. It also adds the IPC messages that are sent
from browser to renderer to manage the injection of these Java objects.

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

[Steve Block, 2011-10-18 18:11:19]

John, this isn't quite ready for a full review yet, but I wanted to ask you
about a checkdeps problem.

I want to include some stuff from content/common (child_process.h,
npobject_util.h and webkit_param_traits.h) from chrome/renderer. What's the
correct way to work around this? Do I need to move things to
content/public/common?

[jam, 2011-10-18 18:33:54]

On 2011/10/18 18:11:19, Steve Block wrote:
> John, this isn't quite ready for a full review yet, but I wanted to ask you
> about a checkdeps problem.
> 
> I want to include some stuff from content/common (child_process.h,
> npobject_util.h and webkit_param_traits.h) from chrome/renderer. What's the
> correct way to work around this? Do I need to move things to
> content/public/common?

sync to r105851, where I moved the java_bridge_dispatcher files to content :)

[Steve Block, 2011-10-18 23:33:48]

> sync to r105851, where I moved the java_bridge_dispatcher files to content :)
I have synced to that revision, so JavaBridgeChannel(Host) is in content/.

This patch adds JavaBridgeDispatcher(Host). I guess my question revolves around
whether we put this new code in content/ or chrome/. Originally we'd planned to
put all Java Bridge code in chrome/, but  JavaBridgeChannel(Host) is now in
content/ to avoid exposing too many internals.

The patch I've uploaded adds the new code to chrome/, as I assumed you'd want to
keep as much code there as possible. Is this correct, or would you rather keep
all the Java Bridge code together in content/?

If we put the new code in chrome/, the problem is that it requires a few headers
from content/ that aren't exposed in the API (I guess this is content/public/).

The new code doesn't use anything from chrome/, so could go in content/. If we
do this, I guess I'll eventually have to add to the content API, as the entry
point to the Java Bridge will be from TabContentsWrapper, which is in chrome/.

[Steve Block, 2011-10-19 15:41:54]

Uploaded a new patch which moves the new code to content/, as I'm starting to
think that's the way to go.

[jam, 2011-10-19 23:46:12]

just nits

On 2011/10/18 23:33:48, Steve Block wrote:
> > sync to r105851, where I moved the java_bridge_dispatcher files to content
:)
> I have synced to that revision, so JavaBridgeChannel(Host) is in content/.
> 
> This patch adds JavaBridgeDispatcher(Host). I guess my question revolves
around
> whether we put this new code in content/ or chrome/. Originally we'd planned
to
> put all Java Bridge code in chrome/, but  JavaBridgeChannel(Host) is now in
> content/ to avoid exposing too many internals.
> 
> The patch I've uploaded adds the new code to chrome/, as I assumed you'd want
to
> keep as much code there as possible. Is this correct, or would you rather keep
> all the Java Bridge code together in content/?

yes

> 
> If we put the new code in chrome/, the problem is that it requires a few
headers
> from content/ that aren't exposed in the API (I guess this is
content/public/).

note that the api isn't done yet, but also we don't want to expose a lot of this
stuff to all embedders just for this java use case, which is why we kept it in
content.

> 
> The new code doesn't use anything from chrome/, so could go in content/. If we
> do this, I guess I'll eventually have to add to the content API, as the entry
> point to the Java Bridge will be from TabContentsWrapper, which is in chrome/.

[jam, 2011-10-19 23:46:19]

http://codereview.chromium.org/8347005/diff/5001/content/common/java_bridge_m...
File content/common/java_bridge_messages.h (right):

http://codereview.chromium.org/8347005/diff/5001/content/common/java_bridge_m...
content/common/java_bridge_messages.h:18:
IPC_MESSAGE_ROUTED1(ViewMsg_JavaBridgeInit,
nit: I'd still call this JavaBridgeMsg. ViewMsg belong in view_messages.h, but I
agree this belongs in this file

http://codereview.chromium.org/8347005/diff/5001/content/renderer/java_bridge...
File content/renderer/java_bridge_dispatcher.cc (right):

http://codereview.chromium.org/8347005/diff/5001/content/renderer/java_bridge...
content/renderer/java_bridge_dispatcher.cc:44:
DCHECK_EQ(content::RenderThread::Get()->GetMessageLoop(),
(and below)

you don't need to test that RVO interface is called on the right thread. if you
really wanted to test that, then individual observers wouldn't be the best spot

http://codereview.chromium.org/8347005/diff/5001/content/renderer/java_bridge...
content/renderer/java_bridge_dispatcher.cc:86:
DCHECK_EQ(content::RenderThread::Get()->GetMessageLoop(),
ditto: this isn't needed

http://codereview.chromium.org/8347005/diff/5001/content/renderer/java_bridge...
File content/renderer/java_bridge_dispatcher.h (right):

http://codereview.chromium.org/8347005/diff/5001/content/renderer/java_bridge...
content/renderer/java_bridge_dispatcher.h:10: #include
"content/renderer/java_bridge_channel.h"
forward declare?

http://codereview.chromium.org/8347005/diff/5001/content/renderer/java_bridge...
content/renderer/java_bridge_dispatcher.h:20: {
nit: this should be on previous line

http://codereview.chromium.org/8347005/diff/5001/content/renderer/java_bridge...
content/renderer/java_bridge_dispatcher.h:32: void OnAddNamedObject(const
string16& name, const NPVariant_Param&);
nit: chrome style is to put the parameter names in the header

[Steve Block, 2011-10-20 09:43:08]

Thanks for the review John

http://codereview.chromium.org/8347005/diff/5001/content/common/java_bridge_m...
File content/common/java_bridge_messages.h (right):

http://codereview.chromium.org/8347005/diff/5001/content/common/java_bridge_m...
content/common/java_bridge_messages.h:18:
IPC_MESSAGE_ROUTED1(ViewMsg_JavaBridgeInit,
On 2011/10/19 23:46:19, John Abd-El-Malek wrote:
> nit: I'd still call this JavaBridgeMsg. ViewMsg belong in view_messages.h, but
I
> agree this belongs in this file

Done.

http://codereview.chromium.org/8347005/diff/5001/content/renderer/java_bridge...
File content/renderer/java_bridge_dispatcher.cc (right):

http://codereview.chromium.org/8347005/diff/5001/content/renderer/java_bridge...
content/renderer/java_bridge_dispatcher.cc:44:
DCHECK_EQ(content::RenderThread::Get()->GetMessageLoop(),
On 2011/10/19 23:46:19, John Abd-El-Malek wrote:
> (and below)
> 
> you don't need to test that RVO interface is called on the right thread. if
you
> really wanted to test that, then individual observers wouldn't be the best
spot

Done.

http://codereview.chromium.org/8347005/diff/5001/content/renderer/java_bridge...
content/renderer/java_bridge_dispatcher.cc:86:
DCHECK_EQ(content::RenderThread::Get()->GetMessageLoop(),
On 2011/10/19 23:46:19, John Abd-El-Malek wrote:
> ditto: this isn't needed

Done.

http://codereview.chromium.org/8347005/diff/5001/content/renderer/java_bridge...
File content/renderer/java_bridge_dispatcher.h (right):

http://codereview.chromium.org/8347005/diff/5001/content/renderer/java_bridge...
content/renderer/java_bridge_dispatcher.h:10: #include
"content/renderer/java_bridge_channel.h"
On 2011/10/19 23:46:19, John Abd-El-Malek wrote:
> forward declare?

Done.

http://codereview.chromium.org/8347005/diff/5001/content/renderer/java_bridge...
content/renderer/java_bridge_dispatcher.h:20: {
On 2011/10/19 23:46:19, John Abd-El-Malek wrote:
> nit: this should be on previous line

Done.

http://codereview.chromium.org/8347005/diff/5001/content/renderer/java_bridge...
content/renderer/java_bridge_dispatcher.h:32: void OnAddNamedObject(const
string16& name, const NPVariant_Param&);
On 2011/10/19 23:46:19, John Abd-El-Malek wrote:
> nit: chrome style is to put the parameter names in the header

Done.

[jam, 2011-10-20 22:11:38]

lgtm