Adds support for calling postMessage on a frame living in a different renderer.

Adds support for calling postMessage on a frame living in a different renderer. 

Patch partly from Karl Koscher <supersat@chromium.org>;
BUG=99202
TEST=PostMessage calls work after navigating a window to a different process.

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

[Charlie Reis, 2012-05-11 19:21:13]

Hey Matt--
  Here's the end result for supporting cross-process postMessage.  It doesn't
yet support sending to/from iframes (which wouldn't have worked in Karl's
original patch), but it does work for top level pages in different processes.

  I was hoping to remove the hosted app process model workaround that keeps
non-app pages in the app's process after this, but it looks like Facebook OAuth
has changed to use communication between an iframe and the popup.  I'll have to
investigate that part more closely.

Thanks,
Charlie

[Matt Perry, 2012-05-11 21:59:34]

lgtm

http://codereview.chromium.org/9108001/diff/64018/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

http://codereview.chromium.org/9108001/diff/64018/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:2438: RenderViewHostDelegate*
source_delegate = source_rvh->GetDelegate();
Is it possible for source_rvh to be NULL?

http://codereview.chromium.org/9108001/diff/64018/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/9108001/diff/64018/content/renderer/render_vie...
content/renderer/render_view_impl.cc:4239:
ChildThread::current()->ResolveRoute(params.source_routing_id));
Maybe move this to a RenderViewImpl::FromRoutingID() to centralize the
conversion casting.

[Charlie Reis, 2012-05-11 22:36:02]

Thanks, fixed.

http://codereview.chromium.org/9108001/diff/64018/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

http://codereview.chromium.org/9108001/diff/64018/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:2438: RenderViewHostDelegate*
source_delegate = source_rvh->GetDelegate();
On 2012/05/11 21:59:34, Matt Perry wrote:
> Is it possible for source_rvh to be NULL?

Good call.  Can't trust the renderer to pass a legit value.  Fixed.

http://codereview.chromium.org/9108001/diff/64018/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/9108001/diff/64018/content/renderer/render_vie...
content/renderer/render_view_impl.cc:4239:
ChildThread::current()->ResolveRoute(params.source_routing_id));
On 2012/05/11 21:59:34, Matt Perry wrote:
> Maybe move this to a RenderViewImpl::FromRoutingID() to centralize the
> conversion casting.

Done.

[Charlie Reis, 2012-05-11 22:36:52]

John, can you review for OWNERs when you're back?

[Matt Perry, 2012-05-11 22:37:23]

still lgtm

http://codereview.chromium.org/9108001/diff/72019/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/9108001/diff/72019/content/renderer/render_vie...
content/renderer/render_view_impl.cc:583:
ChildThread::current()->ResolveRoute(opener_id));
This can use FromRouteID too.

[Charlie Reis, 2012-05-11 22:43:53]

http://codereview.chromium.org/9108001/diff/72019/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/9108001/diff/72019/content/renderer/render_vie...
content/renderer/render_view_impl.cc:583:
ChildThread::current()->ResolveRoute(opener_id));
On 2012/05/11 22:37:23, Matt Perry wrote:
> This can use FromRouteID too.

Oh, nice catch.  Done.

[jam, 2012-05-14 06:41:45]

http://codereview.chromium.org/9108001/diff/58030/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_impl.cc (right):

http://codereview.chromium.org/9108001/diff/58030/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_impl.cc:1270:
delegate_->RouteMessageEvent(this, params);
a while back, I had gone through and removed all the message handlers in RVH
that just call WebContents. just dispatch this message there directly

http://codereview.chromium.org/9108001/diff/58030/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/9108001/diff/58030/content/common/view_message...
content/common/view_messages.h:527:
IPC_STRUCT_BEGIN(ViewHostMsg_PostMessage_Params)
why isn't this VieMsg_PostMessage_Params to match ViewMsg_PostMessageEvent?

http://codereview.chromium.org/9108001/diff/58030/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/9108001/diff/58030/content/renderer/render_vie...
content/renderer/render_view_impl.cc:133: #include
"third_party/WebKit/Source/WebKit/chromium/public/platform/WebSerializedScriptValue.h"
nit: order

http://codereview.chromium.org/9108001/diff/58030/content/renderer/render_vie...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/9108001/diff/58030/content/renderer/render_vie...
content/renderer/render_view_impl.h:210: static RenderViewImpl*
FromRoutingID(int32 routing_id);
nit: it seems best to avoid exposing this outside of RVImpl since it's not
needed there

http://codereview.chromium.org/9108001/diff/58030/content/renderer/render_vie...
content/renderer/render_view_impl.h:604: 
nit: no blank line

[Charlie Reis, 2012-05-14 17:38:42]

Thanks, new patch set uploaded.

http://codereview.chromium.org/9108001/diff/58030/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_impl.cc (right):

http://codereview.chromium.org/9108001/diff/58030/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_impl.cc:1270:
delegate_->RouteMessageEvent(this, params);
On 2012/05/14 06:41:45, John Abd-El-Malek wrote:
> a while back, I had gone through and removed all the message handlers in RVH
> that just call WebContents. just dispatch this message there directly 

It's unsafe to dispatch messages like this directly to WebContents, because we
need to know exactly which RenderViewHost sent the message (hence passing
"this").  Otherwise we end up with bugs like http://crbug.com/126160.

http://codereview.chromium.org/9108001/diff/58030/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/9108001/diff/58030/content/common/view_message...
content/common/view_messages.h:527:
IPC_STRUCT_BEGIN(ViewHostMsg_PostMessage_Params)
On 2012/05/14 06:41:45, John Abd-El-Malek wrote:
> why isn't this VieMsg_PostMessage_Params to match ViewMsg_PostMessageEvent?

It's an identical struct passed in both directions, and matches
ViewHostMsg_RouteMessageEvent.  (Didn't think we'd want a duplicate struct
definition.)  I suppose I renamed that from ViewHostMsg_PostMessageEvent,
though, so this doesn't quite match either case anymore.

Ok, I've renamed it to ViewMsg_PostMessage_Params so it at least matches one of
them.  :)

http://codereview.chromium.org/9108001/diff/58030/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/9108001/diff/58030/content/renderer/render_vie...
content/renderer/render_view_impl.cc:133: #include
"third_party/WebKit/Source/WebKit/chromium/public/platform/WebSerializedScriptValue.h"
On 2012/05/14 06:41:45, John Abd-El-Malek wrote:
> nit: order

Done.

http://codereview.chromium.org/9108001/diff/58030/content/renderer/render_vie...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/9108001/diff/58030/content/renderer/render_vie...
content/renderer/render_view_impl.h:210: static RenderViewImpl*
FromRoutingID(int32 routing_id);
On 2012/05/14 06:41:45, John Abd-El-Malek wrote:
> nit: it seems best to avoid exposing this outside of RVImpl since it's not
> needed there

Made it a static helper function in render_view_impl.cc.

http://codereview.chromium.org/9108001/diff/58030/content/renderer/render_vie...
content/renderer/render_view_impl.h:604: 
On 2012/05/14 06:41:45, John Abd-El-Malek wrote:
> nit: no blank line

Done.

[Aaron Boodman, 2012-05-14 21:27:40]

https://chromiumcodereview.appspot.com/9108001/diff/77019/content/browser/web...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/9108001/diff/77019/content/browser/web...
content/browser/web_contents/web_contents_impl.cc:2434: if
(rvh->GetSiteInstance()->IsRelatedSiteInstance(GetSiteInstance())) {
Nit: invert check and return early.

[Charlie Reis, 2012-05-14 23:20:36]

http://codereview.chromium.org/9108001/diff/77019/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

http://codereview.chromium.org/9108001/diff/77019/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:2434: if
(rvh->GetSiteInstance()->IsRelatedSiteInstance(GetSiteInstance())) {
On 2012/05/14 21:27:40, Aaron Boodman wrote:
> Nit: invert check and return early.

Done.

[jam, 2012-05-14 23:58:30]

lgtm