Move ContentWindow from BrowserPlugin To GuestView

Move ContentWindow from BrowserPlugin To GuestView

This CL plumbs out the swapped out RenderView's routing ID out to the content embedder which then uses it to expose its contentWindow to an optional callback in the AttachGuest API Method.

This patch also makes it fairly trivial to support a contentWindow in other GuestViews.

BUG=330264
Committed: https://crrev.com/212c6daa34e6a5fd1a2754b08973d18d6d09246b
Cr-Commit-Position: refs/heads/master@{#295477}

[Fady Samuel, 2014-09-16 23:37:44]

fsamuel@chromium.org changed reviewers:
+ lazyboy@chromium.org

[lazyboy, 2014-09-17 18:24:01]

Sending comments, I'd defer to someone familiar with v8:: stuff in extensions
for the relevant parts.

https://chromiumcodereview.appspot.com/564973004/diff/1/extensions/renderer/g...
File extensions/renderer/guest_view/guest_view_container.cc (right):

https://chromiumcodereview.appspot.com/564973004/diff/1/extensions/renderer/g...
extensions/renderer/guest_view/guest_view_container.cc:61: if (attach_pending_)
What happens if attached_ = true?

https://chromiumcodereview.appspot.com/564973004/diff/1/extensions/renderer/g...
extensions/renderer/guest_view/guest_view_container.cc:152:
v8::Handle<v8::Function> callback = callback_.NewHandle(isolate_);
Can you ask someone else familiar with this code to look at this one?

https://chromiumcodereview.appspot.com/564973004/diff/1/extensions/renderer/g...
extensions/renderer/guest_view/guest_view_container.cc:166:
callback->Call(context->Global(), argc, argv);
reset callback_ here

https://chromiumcodereview.appspot.com/564973004/diff/1/extensions/renderer/g...
File extensions/renderer/guest_view/guest_view_container.h (right):

https://chromiumcodereview.appspot.com/564973004/diff/1/extensions/renderer/g...
extensions/renderer/guest_view/guest_view_container.h:55: v8::Isolate* isolate_;
What about the lifetime of this?
I don't know much about Isolate-s, can you ask someone familiar to take a look
at this one?

https://chromiumcodereview.appspot.com/564973004/diff/1/extensions/renderer/g...
File extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc
(right):

https://chromiumcodereview.appspot.com/564973004/diff/1/extensions/renderer/g...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:32:
CHECK(args.Length() >= 3);
I'd document the args here a bit as they are growing.

https://chromiumcodereview.appspot.com/564973004/diff/1/extensions/renderer/r...
File extensions/renderer/resources/web_view.js (right):

https://chromiumcodereview.appspot.com/564973004/diff/1/extensions/renderer/r...
extensions/renderer/resources/web_view.js:169: this.contentWindow = null;
This should be done in the constructor as well.

https://chromiumcodereview.appspot.com/564973004/diff/1/extensions/renderer/r...
extensions/renderer/resources/web_view.js:582: //window.console.log(w);
Remove

[Fady Samuel, 2014-09-17 20:25:46]

fsamuel@chromium.org changed reviewers:
+ creis@chromium.org, rockot@chromium.org

[Fady Samuel, 2014-09-17 20:25:47]

+rockot@: Would you be comfortable reviewing the GuestViewContainer::AttachGuest
and GuestViewContainer::OnGuestAttached code?

+creis@ could you please take a look at the content changes?

https://codereview.chromium.org/564973004/diff/1/extensions/renderer/guest_vi...
File extensions/renderer/guest_view/guest_view_container.cc (right):

https://codereview.chromium.org/564973004/diff/1/extensions/renderer/guest_vi...
extensions/renderer/guest_view/guest_view_container.cc:61: if (attach_pending_)
On 2014/09/17 18:24:00, lazyboy wrote:
> What happens if attached_ = true?

Then we reattach. <appview> supports reattachment, it can jump from one app to
another. I added a comment.

https://codereview.chromium.org/564973004/diff/1/extensions/renderer/guest_vi...
extensions/renderer/guest_view/guest_view_container.cc:152:
v8::Handle<v8::Function> callback = callback_.NewHandle(isolate_);
On 2014/09/17 18:24:00, lazyboy wrote:
> Can you ask someone else familiar with this code to look at this one?

Will do. Please note that this code is modeled after this:
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere...

https://codereview.chromium.org/564973004/diff/1/extensions/renderer/guest_vi...
extensions/renderer/guest_view/guest_view_container.cc:166:
callback->Call(context->Global(), argc, argv);
On 2014/09/17 18:24:00, lazyboy wrote:
> reset callback_ here

Done.

https://codereview.chromium.org/564973004/diff/1/extensions/renderer/guest_vi...
File extensions/renderer/guest_view/guest_view_container.h (right):

https://codereview.chromium.org/564973004/diff/1/extensions/renderer/guest_vi...
extensions/renderer/guest_view/guest_view_container.h:55: v8::Isolate* isolate_;
On 2014/09/17 18:24:00, lazyboy wrote:
> What about the lifetime of this?
> I don't know much about Isolate-s, can you ask someone familiar to take a look
> at this one?

Acknowledged.

https://codereview.chromium.org/564973004/diff/1/extensions/renderer/guest_vi...
File extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc
(right):

https://codereview.chromium.org/564973004/diff/1/extensions/renderer/guest_vi...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:32:
CHECK(args.Length() >= 3);
On 2014/09/17 18:24:01, lazyboy wrote:
> I'd document the args here a bit as they are growing.

Done.

https://codereview.chromium.org/564973004/diff/1/extensions/renderer/resource...
File extensions/renderer/resources/web_view.js (right):

https://codereview.chromium.org/564973004/diff/1/extensions/renderer/resource...
extensions/renderer/resources/web_view.js:169: this.contentWindow = null;
On 2014/09/17 18:24:01, lazyboy wrote:
> This should be done in the constructor as well.

Done.

https://codereview.chromium.org/564973004/diff/1/extensions/renderer/resource...
extensions/renderer/resources/web_view.js:582: //window.console.log(w);
On 2014/09/17 18:24:01, lazyboy wrote:
> Remove

Done.

[Ken Rockot(use gerrit already), 2014-09-17 21:30:37]

lgtm

https://codereview.chromium.org/564973004/diff/40001/extensions/renderer/reso...
File extensions/renderer/resources/web_view.js (right):

https://codereview.chromium.org/564973004/diff/40001/extensions/renderer/reso...
extensions/renderer/resources/web_view.js:583: this.contentWindow = w;
nit: This indentation seems a little funky to me; but if it's normal, please
ignore.

[Charlie Reis, 2014-09-17 21:46:43]

Great, glad you're able to eliminate the NPObject.  What implications are there
for interacting with the swapped out RenderView outside the content module?

https://codereview.chromium.org/564973004/diff/40001/content/public/browser/b...
File content/public/browser/browser_plugin_guest_delegate.h (right):

https://codereview.chromium.org/564973004/diff/40001/content/public/browser/b...
content/public/browser/browser_plugin_guest_delegate.h:32: int
element_instance_id) {}
Please document these new parameters, since this is the public .h file.  The
meaning of |guest_routing_id| in particular isn't obvious.

[Fady Samuel, 2014-09-17 22:05:46]

PTAL Charlie, Istiaque.

https://codereview.chromium.org/564973004/diff/40001/content/public/browser/b...
File content/public/browser/browser_plugin_guest_delegate.h (right):

https://codereview.chromium.org/564973004/diff/40001/content/public/browser/b...
content/public/browser/browser_plugin_guest_delegate.h:32: int
element_instance_id) {}
On 2014/09/17 21:46:43, Charlie Reis wrote:
> Please document these new parameters, since this is the public .h file.  The
> meaning of |guest_routing_id| in particular isn't obvious.

Done.

https://codereview.chromium.org/564973004/diff/40001/extensions/renderer/reso...
File extensions/renderer/resources/web_view.js (right):

https://codereview.chromium.org/564973004/diff/40001/extensions/renderer/reso...
extensions/renderer/resources/web_view.js:583: this.contentWindow = w;
On 2014/09/17 21:30:37, Ken Rockot wrote:
> nit: This indentation seems a little funky to me; but if it's normal, please
> ignore.

Done.

[Charlie Reis, 2014-09-17 22:13:21]

Thanks.  Did you see my other question?
(What implications are there for interacting with the swapped out RenderView
outside the content module?)

I'm asking because we wouldn't want callers to look up the RenderViewHost by ID
and assume it's a normal one.  We normally try to hide the swapped out RVH as an
implementation detail, and in the near future this will be a routing ID of a
RenderFrameProxyHost (which has no public API).  In that sense, this ID would
probably need to be treated as somewhat opaque.

[Fady Samuel, 2014-09-17 23:08:33]

On 2014/09/17 22:13:21, Charlie Reis wrote:
> Thanks.  Did you see my other question?
> (What implications are there for interacting with the swapped out RenderView
> outside the content module?)
> 
> I'm asking because we wouldn't want callers to look up the RenderViewHost by
ID
> and assume it's a normal one.  We normally try to hide the swapped out RVH as
an
> implementation detail, and in the near future this will be a routing ID of a
> RenderFrameProxyHost (which has no public API).  In that sense, this ID would
> probably need to be treated as somewhat opaque.

Basically we pass this ID up to the embedder renderer and then we grab the
contentWindow from the RenderView in the embedder in
GuestViewContainer::OnGuestAttached.

In the future, we would have to grab the contentWindow from the
RenderFrameProxy?

[Charlie Reis, 2014-09-17 23:38:59]

On 2014/09/17 23:08:33, Fady Samuel wrote:
> On 2014/09/17 22:13:21, Charlie Reis wrote:
> > Thanks.  Did you see my other question?
> > (What implications are there for interacting with the swapped out RenderView
> > outside the content module?)
> > 
> > I'm asking because we wouldn't want callers to look up the RenderViewHost by
> ID
> > and assume it's a normal one.  We normally try to hide the swapped out RVH
as
> an
> > implementation detail, and in the near future this will be a routing ID of a
> > RenderFrameProxyHost (which has no public API).  In that sense, this ID
would
> > probably need to be treated as somewhat opaque.
> 
> Basically we pass this ID up to the embedder renderer and then we grab the
> contentWindow from the RenderView in the embedder in
> GuestViewContainer::OnGuestAttached.
> 
> In the future, we would have to grab the contentWindow from the
> RenderFrameProxy?

Yes, this will be a problem when swapped out RenderViews go away and we only
have RenderFrameProxy objects, which won't be exposed in the public API.  Since
all you need out of the RenderFrameProxy is its V8 window reference, we might be
able to have a function that returns that directly given a frame or proxy
routing ID.  We can debate that API later, though, when it comes time to move to
RenderFrameProxy.

In the interest of removing the NPAPI usage, this LGTM.

[Fady Samuel, 2014-09-17 23:40:40]

fsamuel@chromium.org changed reviewers:
+ kenrb@chromium.org

[Fady Samuel, 2014-09-17 23:40:40]

PTAL Istiaque
+kenrb@ for *_messages.h

[lazyboy, 2014-09-17 23:56:32]

lgtm

[kenrb, 2014-09-18 15:15:48]

ipc lgtm

[Fady Samuel, 2014-09-18 15:16:47]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-09-18 15:17:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/564973004/100001

[commit-bot: I haz the power, 2014-09-18 16:16:24]

Committed patchset #6 (id:100001) as 26c714cbf3bc8d40d5806e060acae8f2a8251b5a

[commit-bot: I haz the power, 2014-09-18 16:17:10]

Patchset 6 (id:??) landed as
https://crrev.com/212c6daa34e6a5fd1a2754b08973d18d6d09246b
Cr-Commit-Position: refs/heads/master@{#295477}