Make <webview> use out-of-process iframe architecture.

Make <webview> use out-of-process iframe architecture.

This is behind --site-per-process flag.

This is the first step to make <webview> --site-per-process
compatible, only basic features work: rendering, input,
navigation, executeScript and postMessage.

This CL makes <webview> BrowserPlugin free, so <webview>
is not a plugin to blink anymore, rather it's similar to iframe.
Therefore, this CL makes <webview> work without BrowserPlugin
or BrowserPluginGuest/BrowserPluginEmbedder.
We still need to use the old browser/ counterparts (BPG and BPE)
because we use it for 1) Guest related decision making
in WebContents, so WebContentsImpl::browser_plugin_guest() is
still used and useful, 2) Initializing and attaching logic
inside content/, e.g. BPG::InitInternal(), BPG::OnWillAttachComplete.
In subsequent CLs, these dependencies will be either removed if not
required, or extracted to a separate class.

TBR=jochen@chromium.org for testing/buildbot/chromium.fyi.json
BUG=330264
Committed: https://crrev.com/6ec48b2a6529b753f28340fd6741908086224bee
Cr-Commit-Position: refs/heads/master@{#336565}

[Fady Samuel, 2015-03-04 20:44:45]

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

[Fady Samuel, 2015-03-04 20:44:46]

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

[Fady Samuel, 2015-03-04 20:44:47]

https://chromiumcodereview.appspot.com/972313002/diff/20001/content/browser/b...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/20001/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest.cc:5: // Note that all IPC
sent out from this class will be dropped on the
I'd like to better understand what we do and don't need here.

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/browse...
File extensions/browser/guest_view/guest_view_base.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/browse...
extensions/browser/guest_view/guest_view_base.cc:228: //if
(CanRunInDetachedState())
This makes me sad.

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/browse...
File extensions/browser/guest_view/guest_view_message_filter.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/browse...
extensions/browser/guest_view/guest_view_message_filter.cc:126:
guest_manager->GetGuestByInstanceIDHack(guest_instance_id);
Why are you exposing this? This is a security issue. This means that an embedder
can lie about what guest it owns and get access to that guest.

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/browse...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/browse...
extensions/browser/guest_view/web_view/web_view_guest.cc:301:
//PushWebViewStateToIOThread();
This makes me sad.

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/browse...
extensions/browser/guest_view/web_view/web_view_guest.cc:480:
//ReportFrameNameChange(name);
This makes me sad.

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/browse...
extensions/browser/guest_view/web_view/web_view_guest.cc:864:
//PushWebViewStateToIOThread();
This makes me sad.

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/browse...
extensions/browser/guest_view/web_view/web_view_guest.cc:1129: printf("source:
[%p], web_contents(): [%p]\n", source, web_contents());
This basically breaks the new window API. Why is this necessary?

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/render...
File extensions/renderer/guest_view/extensions_guest_view_container.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/render...
extensions/renderer/guest_view/extensions_guest_view_container.cc:215: ready_ =
true;
Really? This is weird. Have we determined a size?

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/render...
File extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc
(right):

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/render...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:178: bool
GetWebNodeFromValue(v8::Handle<v8::Value> value, blink::WebNode* node) {
This can't use NPAPI. That code is deprecated. Is there another way to do this?

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/render...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:236:
guest_view_container = new extensions::ExtensionsGuestViewContainer(
The container should self-destruct.

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/render...
File extensions/renderer/resources/guest_view/web_view.js (left):

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/render...
extensions/renderer/resources/guest_view/web_view.js:66:
this.attributes[WebViewConstants.ATTRIBUTE_SRC].parse();
This code is old. What's the problem here?

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/render...
File extensions/renderer/resources/guest_view/web_view.js (right):

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/render...
extensions/renderer/resources/guest_view/web_view.js:266: this.guest.getId(),
src);
This totally breaks the new window API.

[Fady Samuel, 2015-03-04 20:44:48]

https://chromiumcodereview.appspot.com/972313002/diff/20001/content/browser/b...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/20001/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest.cc:5: // Note that all IPC
sent out from this class will be dropped on the
I'd like to better understand what we do and don't need here.

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/browse...
File extensions/browser/guest_view/guest_view_base.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/browse...
extensions/browser/guest_view/guest_view_base.cc:228: //if
(CanRunInDetachedState())
This makes me sad.

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/browse...
File extensions/browser/guest_view/guest_view_message_filter.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/browse...
extensions/browser/guest_view/guest_view_message_filter.cc:126:
guest_manager->GetGuestByInstanceIDHack(guest_instance_id);
Why are you exposing this? This is a security issue. This means that an embedder
can lie about what guest it owns and get access to that guest.

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/browse...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/browse...
extensions/browser/guest_view/web_view/web_view_guest.cc:301:
//PushWebViewStateToIOThread();
This makes me sad.

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/browse...
extensions/browser/guest_view/web_view/web_view_guest.cc:480:
//ReportFrameNameChange(name);
This makes me sad.

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/browse...
extensions/browser/guest_view/web_view/web_view_guest.cc:864:
//PushWebViewStateToIOThread();
This makes me sad.

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/browse...
extensions/browser/guest_view/web_view/web_view_guest.cc:1129: printf("source:
[%p], web_contents(): [%p]\n", source, web_contents());
This basically breaks the new window API. Why is this necessary?

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/render...
File extensions/renderer/guest_view/extensions_guest_view_container.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/render...
extensions/renderer/guest_view/extensions_guest_view_container.cc:215: ready_ =
true;
Really? This is weird. Have we determined a size?

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/render...
File extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc
(right):

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/render...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:178: bool
GetWebNodeFromValue(v8::Handle<v8::Value> value, blink::WebNode* node) {
This can't use NPAPI. That code is deprecated. Is there another way to do this?

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/render...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:236:
guest_view_container = new extensions::ExtensionsGuestViewContainer(
The container should self-destruct.

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/render...
File extensions/renderer/resources/guest_view/web_view.js (left):

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/render...
extensions/renderer/resources/guest_view/web_view.js:66:
this.attributes[WebViewConstants.ATTRIBUTE_SRC].parse();
This code is old. What's the problem here?

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/render...
File extensions/renderer/resources/guest_view/web_view.js (right):

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/render...
extensions/renderer/resources/guest_view/web_view.js:266: this.guest.getId(),
src);
This totally breaks the new window API.

[Fady Samuel, 2015-03-04 22:21:06]

More comments. Generally I'm OK if the new code paths starts out with broken
functionality (along with TODOs) as long as we don't break the current
<webview>.

There are a bunch of bad assumptions surrounding chrome-guest scheme though.

https://codereview.chromium.org/972313002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/972313002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:447: if
(render_frame_host->GetSiteInstance()->GetSiteURL().SchemeIs(
This doesn't work for other guest types such as <appview>.

https://codereview.chromium.org/972313002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:454:
current_host()->GetView()->Show();
This breaks visibility I think.

https://codereview.chromium.org/972313002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/972313002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.cc:60:
->GetSiteInstance()->GetSiteURL().SchemeIs("chrome-guest");
This is not feasible for guests that are not isolated such as
<extensionoptions>, <extensionview>, <appview>, and MimeHandlerView.

https://codereview.chromium.org/972313002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (left):

https://codereview.chromium.org/972313002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_widget_host_view_child_frame.cc:126:
NOTREACHED();
Does this mean IME is broken?

https://codereview.chromium.org/972313002/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/972313002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:4315: bool is_guest =
GetSiteInstance()->GetSiteURL().SchemeIs("chrome-guest");
Guests are guests if they have a BrowserPluginGuest... Once we call this thing a
GuestProxyHostImpl, then it becomes less scary I think.

https://codereview.chromium.org/972313002/diff/20001/content/renderer/render_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/972313002/diff/20001/content/renderer/render_...
content/renderer/render_view_impl.cc:757:
webview()->setMainFrame(main_render_frame_->GetWebFrame());
What does this do?

[Charlie Reis, 2015-03-10 04:09:43]

creis@chromium.org changed reviewers:
+ creis@chromium.org

[Charlie Reis, 2015-03-10 04:09:45]

Thanks for putting this (and the accompanying doc) together.  I'm sure I'm still
missing a lot, but here's some thoughts on a few places that might be cleaned up
so far.

https://codereview.chromium.org/972313002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/972313002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:447: if
(render_frame_host->GetSiteInstance()->GetSiteURL().SchemeIs(
On 2015/03/04 22:21:06, Fady Samuel wrote:
> This doesn't work for other guest types such as <appview>.

Is RenderProcessHostImpl::IsIsolatedGuest() still valid?

https://codereview.chromium.org/972313002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:451:
proxy_to_embedder->SetChildRWHView(current_host()->GetView());
This doesn't seem like the right place for these calls.  Should it be within
CommitPending?

https://codereview.chromium.org/972313002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/972313002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.h:624: int64
pseudo_parent_frame_tree_node_id_;
Pseudo is not very descriptive.  Do you mean embedder?

(I think it's worth documenting these things now to help us read the CL and
understand it.)

https://codereview.chromium.org/972313002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/972313002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.cc:60:
->GetSiteInstance()->GetSiteURL().SchemeIs("chrome-guest");
On 2015/03/04 22:21:06, Fady Samuel wrote:
> This is not feasible for guests that are not isolated such as
> <extensionoptions>, <extensionview>, <appview>, and MimeHandlerView.

Tangent: what does "not isolated" mean here?

https://codereview.chromium.org/972313002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.cc:100:
pseudo_parent_frame_tree_node_id_ =
embedder_frame_tree_node_id_

https://codereview.chromium.org/972313002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.cc:104: RenderViewHostImpl*
RenderFrameProxyHost::GetRenderViewHost() {
Note: We could probably just cache a pointer to the RVH in this class (passed in
via the constructor), as we do in RenderFrameHostImpl.  That might be cleaner
than tracking the embedder's FrameTreeNode ID, etc.

https://codereview.chromium.org/972313002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.cc:105: FrameTree* ft = NULL;
embedder_frame_tree = nullptr;

https://codereview.chromium.org/972313002/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/972313002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:9: #include
"content/browser/frame_host/render_frame_proxy_host.h"
Not allowed by DEPS.  web_contents/ can't depend on frame_host/.

https://codereview.chromium.org/972313002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:901:
CHECK(!browser_plugin_guest_);
This doesn't belong in an accessor for the SiteInstance.

https://codereview.chromium.org/972313002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1273: int
WebContentsImpl::AttachLocalFrameToGuest(WebContents* owner_web_contents,
It looks like most of the operations in here probably belong in
RenderFrameHostManager.

https://codereview.chromium.org/972313002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1282:
embedder_local_render_frame_id);
embedder_frame_routing_id?

https://codereview.chromium.org/972313002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1299: int rv_of_embedder_id;
Let's start moving to names that might use in practice, which will make it
easier to follow.

https://codereview.chromium.org/972313002/diff/20001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/972313002/diff/20001/content/common/frame_mes...
content/common/frame_messages.h:549: // Replace this frame with a
RenderFrameProxy whose routing ID is
Isn't this essentially what swap does?

[lazyboy, 2015-04-01 21:34:24]

Patchset #6 (id:100001) has been deleted

[lazyboy, 2015-04-01 21:47:58]

Patch Set #6 is updated with the recent changes.
I've updated the doc to reflect the changes:
https://docs.google.com/a/chromium.org/document/d/1LqR-hsI0d2fDUjS8Xu7KplD2k7...

Addressed few comments, but not complete.

Charlie, can you take a look? Thanks.

https://chromiumcodereview.appspot.com/972313002/diff/20001/content/browser/f...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/20001/content/browser/f...
content/browser/frame_host/render_frame_host_manager.cc:447: if
(render_frame_host->GetSiteInstance()->GetSiteURL().SchemeIs(
On 2015/03/10 04:09:44, Charlie Reis wrote:
> On 2015/03/04 22:21:06, Fady Samuel wrote:
> > This doesn't work for other guest types such as <appview>.
> 
> Is RenderProcessHostImpl::IsIsolatedGuest() still valid?

I'm using GetProxyToEmbedder() instead, if we have a proxy, then we need the
Child RWHV.

https://chromiumcodereview.appspot.com/972313002/diff/20001/content/browser/f...
content/browser/frame_host/render_frame_host_manager.cc:451:
proxy_to_embedder->SetChildRWHView(current_host()->GetView());
On 2015/03/10 04:09:44, Charlie Reis wrote:
> This doesn't seem like the right place for these calls.  Should it be within
> CommitPending?

Technically we don't have pending and don't commit pending.
However, with recent changes, this code has moved from ::DidNavigateFrame() to
::CommitPendingIfNecessary().

https://chromiumcodereview.appspot.com/972313002/diff/20001/content/browser/f...
content/browser/frame_host/render_frame_host_manager.cc:454:
current_host()->GetView()->Show();
On 2015/03/04 22:21:06, Fady Samuel wrote:
> This breaks visibility I think.

Added TODO to investigate.

https://chromiumcodereview.appspot.com/972313002/diff/20001/content/browser/f...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/20001/content/browser/f...
content/browser/frame_host/render_frame_proxy_host.cc:100:
pseudo_parent_frame_tree_node_id_ =
On 2015/03/10 04:09:44, Charlie Reis wrote:
> embedder_frame_tree_node_id_

Obsolete now.

https://chromiumcodereview.appspot.com/972313002/diff/20001/content/browser/f...
content/browser/frame_host/render_frame_proxy_host.cc:104: RenderViewHostImpl*
RenderFrameProxyHost::GetRenderViewHost() {
On 2015/03/10 04:09:44, Charlie Reis wrote:
> Note: We could probably just cache a pointer to the RVH in this class (passed
in
> via the constructor), as we do in RenderFrameHostImpl.  That might be cleaner
> than tracking the embedder's FrameTreeNode ID, etc.

Added a TODO since we're not tracking embedder's FTN ID here anymore.

https://chromiumcodereview.appspot.com/972313002/diff/20001/content/browser/w...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/20001/content/browser/w...
content/browser/web_contents/web_contents_impl.cc:9: #include
"content/browser/frame_host/render_frame_proxy_host.h"
On 2015/03/10 04:09:44, Charlie Reis wrote:
> Not allowed by DEPS.  web_contents/ can't depend on frame_host/.

Not required anymore since I moved codes requiring RFPH into RFHM.

https://chromiumcodereview.appspot.com/972313002/diff/20001/content/browser/w...
content/browser/web_contents/web_contents_impl.cc:901:
CHECK(!browser_plugin_guest_);
On 2015/03/10 04:09:44, Charlie Reis wrote:
> This doesn't belong in an accessor for the SiteInstance.

Removed.

https://chromiumcodereview.appspot.com/972313002/diff/20001/content/browser/w...
content/browser/web_contents/web_contents_impl.cc:1273: int
WebContentsImpl::AttachLocalFrameToGuest(WebContents* owner_web_contents,
On 2015/03/10 04:09:44, Charlie Reis wrote:
> It looks like most of the operations in here probably belong in
> RenderFrameHostManager.

Moved bulk of these to RFHM.

https://chromiumcodereview.appspot.com/972313002/diff/20001/content/browser/w...
content/browser/web_contents/web_contents_impl.cc:1282:
embedder_local_render_frame_id);
On 2015/03/10 04:09:44, Charlie Reis wrote:
> embedder_frame_routing_id?

Done.

https://chromiumcodereview.appspot.com/972313002/diff/20001/content/browser/w...
content/browser/web_contents/web_contents_impl.cc:1299: int rv_of_embedder_id;
On 2015/03/10 04:09:44, Charlie Reis wrote:
> Let's start moving to names that might use in practice, which will make it
> easier to follow.

OK, I've made some changes.

https://chromiumcodereview.appspot.com/972313002/diff/20001/content/browser/w...
content/browser/web_contents/web_contents_impl.cc:4315: bool is_guest =
GetSiteInstance()->GetSiteURL().SchemeIs("chrome-guest");
On 2015/03/04 22:21:06, Fady Samuel wrote:
> Guests are guests if they have a BrowserPluginGuest... Once we call this thing
a
> GuestProxyHostImpl, then it becomes less scary I think.

Changed to looking at browser_plugin_guest_ for now.
Done.

https://chromiumcodereview.appspot.com/972313002/diff/20001/content/common/fr...
File content/common/frame_messages.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/20001/content/common/fr...
content/common/frame_messages.h:549: // Replace this frame with a
RenderFrameProxy whose routing ID is
On 2015/03/10 04:09:45, Charlie Reis wrote:
> Isn't this essentially what swap does?

More or less, it does subset of swap.
Swap disconnects the local frame, but I'm keeping it around.
Haven't got to try out disconnecting the frame yet.

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/browse...
File extensions/browser/guest_view/guest_view_message_filter.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/browse...
extensions/browser/guest_view/guest_view_message_filter.cc:126:
guest_manager->GetGuestByInstanceIDHack(guest_instance_id);
On 2015/03/04 20:44:47, Fady Samuel wrote:
> Why are you exposing this? This is a security issue. This means that an
embedder
> can lie about what guest it owns and get access to that guest.

This is temporary and will be fixed.

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/browse...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/20001/extensions/browse...
extensions/browser/guest_view/web_view/web_view_guest.cc:301:
//PushWebViewStateToIOThread();
On 2015/03/04 20:44:47, Fady Samuel wrote:
> This makes me sad.

These changes here are temporary, I'll fix these once content/ changes get
settled.

[Charlie Reis, 2015-04-08 23:42:12]

Sorry for the delay; this got buried among other tasks.

I've left some comments as I went through the content/ stuff (minus
BrowserPluginGuest), though it seems like there's still a lot of temporary code
here.

Are there other particular things you're looking for feedback on at this stage?

https://codereview.chromium.org/972313002/diff/120001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/972313002/diff/120001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:170:
RenderFrameProxyHostMap::iterator iter =
Just use RenderFrameHostManager::GetRenderFrameProxyHost.

https://codereview.chromium.org/972313002/diff/120001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:171:
proxy_hosts_.find(embedder_frame_tree_node->render_manager()
Use FrameTreeNode::current_frame_host(), skipping the render_manager() call.

https://codereview.chromium.org/972313002/diff/120001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:477:
proxy_to_embedder->SetChildRWHView(current_host()->GetView());
Do you have to do this for every commit in the guest?

https://codereview.chromium.org/972313002/diff/120001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/972313002/diff/120001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:176: virtual int64
GetParentFrameTreeNodeID() = 0;
This needs a better name.  A RFHM belongs to a FrameTreeNode, which may have a
parent FrameTreeNode in the current WebContents.  In contrast, this method
normally returns -1 unless the WebContents itself has a parent WebContents.

Maybe something like GetEmbedderFrameTreeNodeID()?

https://codereview.chromium.org/972313002/diff/120001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:421: // Creates a proxy
pointing to embedder process inside a guest RFHM.
Terminology: "proxy for frame X in process Y"
Pointing to a process doesn't really make sense; instead, the proxy lives in the
process and represents a frame rendered elsewhere.

https://codereview.chromium.org/972313002/diff/120001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:425: int*
swapped_out_render_view_routing_id);
Let's avoid having a mix of return parameters and out parameters if we can.

https://codereview.chromium.org/972313002/diff/120001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/972313002/diff/120001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1288:
embedder_web_contents->GetSiteInstance(),
What if the embedder WebContents has the <webview> tag in an OOPIF?  We should
call GetSiteInstance on the RenderFrameHost that has the tag, not on the
WebContents itself (which corresponds to the main frame).

https://codereview.chromium.org/972313002/diff/120001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1297:
embedder_web_contents->GetRenderProcessHost()->GetID(),
Same.  We have to pass in the process ID, because not all frames in the embedder
are in the same process.

https://codereview.chromium.org/972313002/diff/120001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/972313002/diff/120001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:128: int
AttachLocalFrameToGuest(WebContents* embedder_web_contents,
This belongs with the WebContents overrides below.

https://codereview.chromium.org/972313002/diff/120001/content/browser/web_con...
File content/browser/web_contents/web_contents_node.h (right):

https://codereview.chromium.org/972313002/diff/120001/content/browser/web_con...
content/browser/web_contents/web_contents_node.h:8: class WebContentsNode {
No one outside WebContentsImpl seems to need this.  Can we make it a private
struct within WebContentsImpl instead?

https://codereview.chromium.org/972313002/diff/120001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/972313002/diff/120001/content/common/frame_me...
content/common/frame_messages.h:510: // The proxy points to a guest process.
Please rephrase (same "proxy points" issue).

https://codereview.chromium.org/972313002/diff/120001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/972313002/diff/120001/content/public/browser/...
content/public/browser/web_contents.h:163: int embedder_frame_routing_id) = 0;
Always pass routing IDs and process IDs together.  A routing ID is ambiguous on
its own.

https://codereview.chromium.org/972313002/diff/120001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/972313002/diff/120001/content/renderer/render...
content/renderer/render_frame_impl.cc:4096:
render_view_->RegisterRenderFrameProxy(proxy_to_replace_frame);
Please check with Lucas about this, since he and Daniel have a better sense for
the swap semantics.

https://codereview.chromium.org/972313002/diff/120001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/972313002/diff/120001/content/renderer/render...
content/renderer/render_frame_impl.h:615: void OnReplaceLocalFrameWithProxy(int
proxy_routing_id);
This probably belongs closer to OnSwapOut, since it's not related to
find/replace.  (These aren't in alphabetical order.)

[lazyboy, 2015-04-14 01:38:04]

(Comments addressed in Patch Set #8)

I've left some comments as I went through the content/ stuff (minus
BrowserPluginGuest), though it seems like there's still a lot of temporary code
here.
re: temporary code, I'll work on polishing them: making the new code path
trigger by --site-per-process flag.

Are there other particular things you're looking for feedback on at this stage?
re: High level approach mostly, in case we find any showstopper sooner rather
than later. (That's why I've been holding off polishing the entire CL).

https://chromiumcodereview.appspot.com/972313002/diff/120001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/120001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:170:
RenderFrameProxyHostMap::iterator iter =
On 2015/04/08 23:42:11, Charlie Reis (slow until 4-15) wrote:
> Just use RenderFrameHostManager::GetRenderFrameProxyHost.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/120001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:171:
proxy_hosts_.find(embedder_frame_tree_node->render_manager()
On 2015/04/08 23:42:10, Charlie Reis (slow until 4-15) wrote:
> Use FrameTreeNode::current_frame_host(), skipping the render_manager() call.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/120001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:477:
proxy_to_embedder->SetChildRWHView(current_host()->GetView());
On 2015/04/08 23:42:10, Charlie Reis (slow until 4-15) wrote:
> Do you have to do this for every commit in the guest?

No, only the first time.
I've moved this to WebContentsImpl::CreateRenderViewForRenderManager()

However, the next GetView()->Show() needs to stay here.
Calling it too early in CreateRenderViewFor... doesn't work.
As Fady pointed out, this needs more work, I can investigate this separately.

https://chromiumcodereview.appspot.com/972313002/diff/120001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/120001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:176: virtual int64
GetParentFrameTreeNodeID() = 0;
On 2015/04/08 23:42:11, Charlie Reis (slow until 4-15) wrote:
> This needs a better name.  A RFHM belongs to a FrameTreeNode, which may have a
> parent FrameTreeNode in the current WebContents.  In contrast, this method
> normally returns -1 unless the WebContents itself has a parent WebContents.
> 
> Maybe something like GetEmbedderFrameTreeNodeID()?

Changed to GetEmbedderFrameTreeNodeID(), added doc.

https://chromiumcodereview.appspot.com/972313002/diff/120001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:421: // Creates a proxy
pointing to embedder process inside a guest RFHM.
On 2015/04/08 23:42:11, Charlie Reis (slow until 4-15) wrote:
> Terminology: "proxy for frame X in process Y"
> Pointing to a process doesn't really make sense; instead, the proxy lives in
the
> process and represents a frame rendered elsewhere.

Thanks for the terminology.
Changed.

https://chromiumcodereview.appspot.com/972313002/diff/120001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:425: int*
swapped_out_render_view_routing_id);
On 2015/04/08 23:42:11, Charlie Reis (slow until 4-15) wrote:
> Let's avoid having a mix of return parameters and out parameters if we can.

I've changed both to out parameters.

https://chromiumcodereview.appspot.com/972313002/diff/120001/content/browser/...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/120001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:1288:
embedder_web_contents->GetSiteInstance(),
On 2015/04/08 23:42:11, Charlie Reis (slow until 4-15) wrote:
> What if the embedder WebContents has the <webview> tag in an OOPIF?  We should
> call GetSiteInstance on the RenderFrameHost that has the tag, not on the
> WebContents itself (which corresponds to the main frame).

Done.

https://chromiumcodereview.appspot.com/972313002/diff/120001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:1297:
embedder_web_contents->GetRenderProcessHost()->GetID(),
On 2015/04/08 23:42:11, Charlie Reis (slow until 4-15) wrote:
> Same.  We have to pass in the process ID, because not all frames in the
embedder
> are in the same process.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/120001/content/browser/...
File content/browser/web_contents/web_contents_impl.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/120001/content/browser/...
content/browser/web_contents/web_contents_impl.h:128: int
AttachLocalFrameToGuest(WebContents* embedder_web_contents,
On 2015/04/08 23:42:11, Charlie Reis (slow until 4-15) wrote:
> This belongs with the WebContents overrides below.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/120001/content/browser/...
File content/browser/web_contents/web_contents_node.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/120001/content/browser/...
content/browser/web_contents/web_contents_node.h:8: class WebContentsNode {
On 2015/04/08 23:42:11, Charlie Reis (slow until 4-15) wrote:
> No one outside WebContentsImpl seems to need this.  Can we make it a private
> struct within WebContentsImpl instead?

Done.

https://chromiumcodereview.appspot.com/972313002/diff/120001/content/common/f...
File content/common/frame_messages.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/120001/content/common/f...
content/common/frame_messages.h:510: // The proxy points to a guest process.
On 2015/04/08 23:42:11, Charlie Reis (slow until 4-15) wrote:
> Please rephrase (same "proxy points" issue).

Done.

https://chromiumcodereview.appspot.com/972313002/diff/120001/content/public/b...
File content/public/browser/web_contents.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/120001/content/public/b...
content/public/browser/web_contents.h:163: int embedder_frame_routing_id) = 0;
On 2015/04/08 23:42:12, Charlie Reis (slow until 4-15) wrote:
> Always pass routing IDs and process IDs together.  A routing ID is ambiguous
on
> its own.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/120001/content/renderer...
File content/renderer/render_frame_impl.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/120001/content/renderer...
content/renderer/render_frame_impl.cc:4096:
render_view_->RegisterRenderFrameProxy(proxy_to_replace_frame);
On 2015/04/08 23:42:12, Charlie Reis (slow until 4-15) wrote:
> Please check with Lucas about this, since he and Daniel have a better sense
for
> the swap semantics.

Acknowledged.
Lucas and I discussed about this initially, will check back again.

https://chromiumcodereview.appspot.com/972313002/diff/120001/content/renderer...
File content/renderer/render_frame_impl.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/120001/content/renderer...
content/renderer/render_frame_impl.h:615: void OnReplaceLocalFrameWithProxy(int
proxy_routing_id);
On 2015/04/08 23:42:12, Charlie Reis (slow until 4-15) wrote:
> This probably belongs closer to OnSwapOut, since it's not related to
> find/replace.  (These aren't in alphabetical order.)

Done.

[lazyboy, 2015-04-28 17:17:52]

Hi Charlie,
I've updated the CL to get <webview> working in both
site-per-process and non site-per-process code path, you
can review this now.

[Fady Samuel, 2015-04-28 17:56:11]

I like where this is going. We need to avoid using NPAPI though.

I hope we can also get to a point where we minimize SitePerProcess checks. I'd
rather get rid of BrowserPluginGuest, and replace it with a GuestHostImpl, along
with a delegate of some kind for BrowserPlugin: BrowserPluginHostDelegate. This
delegate can be nullptr for the SitePerProcess Code path.

https://chromiumcodereview.appspot.com/972313002/diff/220001/extensions/rende...
File extensions/renderer/dispatcher.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/220001/extensions/rende...
extensions/renderer/dispatcher.cc:1042:
ExtensionsGuestViewMsg_ContentWindowReady::Param param;
This should be in ExtensionsGuestViewContainer...

https://chromiumcodereview.appspot.com/972313002/diff/220001/extensions/rende...
File extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc
(right):

https://chromiumcodereview.appspot.com/972313002/diff/220001/extensions/rende...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:232: bool
GetWebNodeFromValue(v8::Handle<v8::Value> value, blink::WebNode* node) {
This needs to change. This uses NPAPI code.

https://chromiumcodereview.appspot.com/972313002/diff/220001/extensions/rende...
File extensions/renderer/resources/guest_view/web_view/web_view.js (right):

https://chromiumcodereview.appspot.com/972313002/diff/220001/extensions/rende...
extensions/renderer/resources/guest_view/web_view/web_view.js:228:
WebViewInternal.navigate(
This breaks webview expectations now. What happened to createGuest?

[Charlie Reis, 2015-04-30 23:06:44]

creis@chromium.org changed reviewers:
+ nasko@chromium.org

[Charlie Reis, 2015-04-30 23:06:48]

Thanks for getting it to work both with --site-per-process and without.  Some
feedback and things I noticed below.  (I've only looked at content/, not
extensions/.)

I'll be OOO next week, so I'm adding Nasko to continue the review.  Alex can
help with the postMessage questions, and Daniel or Lucas might have ideas about
ReplaceLocalFrameWithProxy vs SwapOut.

Hope that helps!

https://codereview.chromium.org/972313002/diff/220001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/972313002/diff/220001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:6: // floor b/c we don't
have a BrowserPlugin anymore.
Class-level comments belong on the class declaration in the .h file, rather than
under the copyright notice.

https://codereview.chromium.org/972313002/diff/220001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:112: // Don't create the
proxy here, we'll create it later.
nit: Semi-colon or period.  (In general, two complete sentences cannot be joined
by a comma.)

Also, it's worth clarifying when it will be created, rather than just saying
later.

https://codereview.chromium.org/972313002/diff/220001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:279:
static_cast<WebContentsViewGuest*>(GetWebContents()->GetView());
Wrong indent?  git cl format can help.

https://codereview.chromium.org/972313002/diff/220001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/972313002/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:168: return NULL;
nullptr

https://codereview.chromium.org/972313002/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:479:
current_host()->GetView()->Show();
This is not the right place to call Show.  Please put it with the other Show()
call in CommitPending.

https://codereview.chromium.org/972313002/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1622:
CREATE_RF_FOR_MAIN_FRAME_NAVIGATION;  // TODO(lazyboy): Do we need this?
This affects |for_main_frame_navigation| in
WebContentsImpl::CreateRenderViewForRenderManager, making it decide between
RWH::CreateViewForWidget (for main frames) and creating a new
RenderWidgetHostViewChildFrame (for subframes).

I'm not sure which approach you need.  This is the guest's RVH, so I'm guessing
it will need the main frame case (even though it will never get swapped in).

https://codereview.chromium.org/972313002/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1623:
scoped_ptr<RenderFrameHostImpl> so_rfh_in_embedder =
swapped_out_embedder_rfh

https://codereview.chromium.org/972313002/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1640: new
FrameMsg_ReplaceLocalFrameWithProxy(
(I'm wondering if we should find a way to unify this with SwapOut.  See my
comments in render_frame_impl.cc.)

https://codereview.chromium.org/972313002/diff/220001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/972313002/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:176: // Returns the FTN
id of the parent delegate (WebContents) that is hosting
s/parent/embedder/

https://codereview.chromium.org/972313002/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:177: // this delegate
(WebContents), -1 if this WebContents does not have a
Period.  Then "Returns -1 if"

https://codereview.chromium.org/972313002/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:178: // parent delegate.
s/parent/embedder/

https://codereview.chromium.org/972313002/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:430: // Creates a proxy
for guest frame in embedder's process.
Called on the guest RFHM to create a proxy for this guest frame in its
embedder's process.

https://codereview.chromium.org/972313002/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:431: // This proxy is
used to swap out the initial guest frame in embedder.
in the embedder.

https://codereview.chromium.org/972313002/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:436: // Replaces initial
guest frame in embedder with the proxy that has routing
Called on the embedder RFHM to replace the initial guest frame in the embedder
with...

https://codereview.chromium.org/972313002/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:441: void
UpdateGuestRWHView(RenderWidgetHostView* child_rwhv);
SetGuestRenderWidgetHostView

https://codereview.chromium.org/972313002/diff/220001/content/browser/frame_h...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/972313002/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:59: // If this is a proxy
to embedder created in the guest's FTN, then we
If this is a proxy for the guest's FTN in its embedder's process,

https://codereview.chromium.org/972313002/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:61: bool
is_proxy_to_embedder_for_guest =
is_proxy_for_guest_in_embedder

https://codereview.chromium.org/972313002/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:64:
->GetSiteInstance()->GetSiteURL().SchemeIs("chrome-guest");
Seems like there would be better ways to tell whether it's a guest, like a
RFHM::IsGuest() (which might use something like GetEmbedderFrameTreeNodeID()
under the hood).  The SiteInstance site URL feels a bit fragile as the uses of
<webview> evolve.

https://codereview.chromium.org/972313002/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:109: FrameTree* frame_tree
= proxy_to_embedder ?
This looks buggy.  It uses the embedder's frame tree for any RFPH in the guest,
not just the RFPH in the embedder's SiteInstance.

https://codereview.chromium.org/972313002/diff/220001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/972313002/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:126: // 
NOTREACHED();
What do we need to do to resolve this?

https://codereview.chromium.org/972313002/diff/220001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/972313002/diff/220001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:291: :
parent_web_contents_(NULL) {
nullptr

https://codereview.chromium.org/972313002/diff/220001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:294:
WebContentsImpl::WebContentsNode::~WebContentsNode() {
Is there any way for us to enforce that a child WebContentsNode gets deleted
before its parent?  Otherwise the child could have a stale pointer to its parent
WebContents, risking a use-after-free.

https://codereview.chromium.org/972313002/diff/220001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:633:
DCHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
CHECK

https://codereview.chromium.org/972313002/diff/220001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:645: // to embedder site
instance. The swapped out RVH will be used to send
the embedder's SiteInstance.

https://codereview.chromium.org/972313002/diff/220001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:646: // postMessage to guest.
Alex pointed out that postMessage doesn't go through RenderViewHost anymore.  We
may still need the swapped out RVH (not sure), but you might want to check with
him about routing postMessage through the RFPH instead.

https://codereview.chromium.org/972313002/diff/220001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:654: // 3. Swap the embedder's
initial frame pointing to guest with the proxy
initial frame for the guest

https://codereview.chromium.org/972313002/diff/220001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:663: return
swapped_out_render_view_routing_id;
Is this needed, given that postMessage should go through RFPH?

https://codereview.chromium.org/972313002/diff/220001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1557: DCHECK(0);
This needs updating to work in non-site-per-process mode.

https://codereview.chromium.org/972313002/diff/220001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:4227: if
(!for_main_frame_navigation || is_guest_in_site_per_process) {
Sounds like you might not need this if you don't pass
CREATE_RF_FOR_MAIN_FRAME_NAVIGATION?

https://codereview.chromium.org/972313002/diff/220001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/972313002/diff/220001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:221: int
AttachLocalFrameToGuest(WebContents* embedder_web_contents,
Move lower, as mentioned in web_contents.h.

https://codereview.chromium.org/972313002/diff/220001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:739: // Used to create tree of
WebContents.
Please clarify what this is used for, since it's not obvious when reading
WebContentsImpl code.  It's especially important to distinguish it from
FrameTree and FrameTreeNodes.

https://codereview.chromium.org/972313002/diff/220001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:740: class WebContentsNode {
Perhaps this should just be a struct?

WebContentsTreeNode may also make the uses clearer.

https://codereview.chromium.org/972313002/diff/220001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:1283: WebContentsNode node_;
This needs a comment and should probably be up higher, perhaps above
frame_tree_.

https://codereview.chromium.org/972313002/diff/220001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/972313002/diff/220001/content/common/frame_me...
content/common/frame_messages.h:578: // Replace this frame with a
RenderFrameProxy whose routing ID is
nit: Blank line before.

https://codereview.chromium.org/972313002/diff/220001/content/common/frame_me...
content/common/frame_messages.h:580: // The proxy is for a frame in guest that
lives in embedder process.
nit: Move to previous line.
Also rephrase:
The proxy is a placeholder in the embedder's process for a guest's main frame.

https://codereview.chromium.org/972313002/diff/220001/content/common/frame_me...
content/common/frame_messages.h:581:
IPC_MESSAGE_ROUTED1(FrameMsg_ReplaceLocalFrameWithProxy,
This should probably be listed next to SwapOut.

https://codereview.chromium.org/972313002/diff/220001/content/common/view_mes...
File content/common/view_messages.h (right):

https://codereview.chromium.org/972313002/diff/220001/content/common/view_mes...
content/common/view_messages.h:540: // TODO(lazyboy): Remove once we can use RFP
to route postMessage.
I think that's already possible.  Check with alexmos@.

https://codereview.chromium.org/972313002/diff/220001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/972313002/diff/220001/content/public/browser/...
content/public/browser/web_contents.h:159: // Attaches or associates this
WebContents to a frame in
this guest WebContents to its embedder frame in...

https://codereview.chromium.org/972313002/diff/220001/content/public/browser/...
content/public/browser/web_contents.h:162: // {embedder_frame_process_id,
embedder_frame_routing_id} pair.
Also say what this returns.

https://codereview.chromium.org/972313002/diff/220001/content/public/browser/...
content/public/browser/web_contents.h:163: virtual int
AttachLocalFrameToGuest(WebContents* embedder_web_contents,
This name doesn't make sense to me, if we're calling it on the guest.  What
about AttachToEmbedderFrame?

Also, this belongs lower in the class.  Maybe in "Internal state?"

https://codereview.chromium.org/972313002/diff/220001/content/public/browser/...
content/public/browser/web_contents.h:165: int embedder_frame_routing_id) = 0;
I think it might be clearer to pass in the the RenderFrameHost instead of the ID
pair.  The caller should be able to get it.

https://codereview.chromium.org/972313002/diff/220001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/972313002/diff/220001/content/renderer/render...
content/renderer/render_frame_impl.cc:1361:
blink::WebRemoteFrame::create(proxy_to_replace_frame);
It's not obvious why the existing proxy and swap codepaths aren't sufficient for
this, and it makes me nervous to add another path that we have to reason about.

Can you check with Daniel and Lucas about this?  They've been working on the
swap semantics and would know whether there's a better way to handle this.  (If
there is a good reason for the separate path, we should have comments explaining
it.)

https://codereview.chromium.org/972313002/diff/220001/content/renderer/render...
File content/renderer/render_frame_proxy.h (right):

https://codereview.chromium.org/972313002/diff/220001/content/renderer/render...
content/renderer/render_frame_proxy.h:130: void
SetWebRemoteFrame(blink::WebRemoteFrame* remote_frame);
I don't yet understand why this needs to be exposed, since it should be handled
in a consistent way by Init.  See my comment in render_frame_impl.cc.

https://codereview.chromium.org/972313002/diff/220001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/972313002/diff/220001/content/renderer/render...
content/renderer/render_view_impl.cc:743: // break;
nit: Period, not semicolon.  :)

But it also sounds like this might not be necessary, given alexmos's postMessage
work.

[lazyboy, 2015-05-05 07:28:15]

I've address content/ comments for now
(except one comment in render_widget_host_view_child_frame.cc)

Major changes:
1) use the <iframe>.contentWindow to directly send postMessage to <webview>
instead of sending swapped out RV's routing ID to renderer and then grabbing
contentWindow out of the RenderView.
2) My "almost swap" step using a new IPC is removed. I'm using the existing
FrameMsg_SwapOut now.

List of comments not addressed:
1) Couple of commented out NOTREACHED() RenderWidgetHostViewChildFrame, I didn't
spend much time on this yet.
2) WebContentsTreeNode can hold stale pointer to its parent if the parent
WebContents goes away before child. I've added a TODO to address this.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/browser_plugin/browser_plugin_guest.cc:6: // floor b/c we don't
have a BrowserPlugin anymore.
On 2015/04/30 23:06:45, Charlie Reis (OOO until 5-11) wrote:
> Class-level comments belong on the class declaration in the .h file, rather
than
> under the copyright notice.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/browser_plugin/browser_plugin_guest.cc:112: // Don't create the
proxy here, we'll create it later.
On 2015/04/30 23:06:45, Charlie Reis (OOO until 5-11) wrote:
> nit: Semi-colon or period.  (In general, two complete sentences cannot be
joined
> by a comma.)
> 
> Also, it's worth clarifying when it will be created, rather than just saying
> later.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/browser_plugin/browser_plugin_guest.cc:279:
static_cast<WebContentsViewGuest*>(GetWebContents()->GetView());
On 2015/04/30 23:06:45, Charlie Reis (OOO until 5-11) wrote:
> Wrong indent?  git cl format can help.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:168: return NULL;
On 2015/04/30 23:06:46, Charlie Reis (OOO until 5-11) wrote:
> nullptr

Done.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:479:
current_host()->GetView()->Show();
On 2015/04/30 23:06:46, Charlie Reis (OOO until 5-11) wrote:
> This is not the right place to call Show.  Please put it with the other Show()
> call in CommitPending.

There's no CommitPending() pending call as swapping is done manually w/o
CreatePending/CommitPending.

However the TODO in line above still needs to be addressed, I'll revisit once I
have a better solution to calling Show() altogether, by then it might be a non
issue.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:1622:
CREATE_RF_FOR_MAIN_FRAME_NAVIGATION;  // TODO(lazyboy): Do we need this?
On 2015/04/30 23:06:46, Charlie Reis (OOO until 5-11) wrote:
> This affects |for_main_frame_navigation| in
> WebContentsImpl::CreateRenderViewForRenderManager, making it decide between
> RWH::CreateViewForWidget (for main frames) and creating a new
> RenderWidgetHostViewChildFrame (for subframes).
> 
> I'm not sure which approach you need.  This is the guest's RVH, so I'm
guessing
> it will need the main frame case (even though it will never get swapped in).

Keeping FOR_MAIN_FRAME_NAVIGATION.
(there's also another comment in WebContentsImpl related to this).

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:1623:
scoped_ptr<RenderFrameHostImpl> so_rfh_in_embedder =
On 2015/04/30 23:06:45, Charlie Reis (OOO until 5-11) wrote:
> swapped_out_embedder_rfh

Done.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:1640: new
FrameMsg_ReplaceLocalFrameWithProxy(
On 2015/04/30 23:06:45, Charlie Reis (OOO until 5-11) wrote:
> (I'm wondering if we should find a way to unify this with SwapOut.  See my
> comments in render_frame_impl.cc.)

Done as I got rid of the new IPC, using SwapOut IPC instead.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:176: // Returns the FTN
id of the parent delegate (WebContents) that is hosting
On 2015/04/30 23:06:46, Charlie Reis (OOO until 5-11) wrote:
> s/parent/embedder/

Done for now.
Although it was suggested by nasko to use parent/child terminology
in content/ instead of embedder/guest.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:177: // this delegate
(WebContents), -1 if this WebContents does not have a
On 2015/04/30 23:06:46, Charlie Reis (OOO until 5-11) wrote:
> Period.  Then "Returns -1 if"

Done.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:178: // parent delegate.
On 2015/04/30 23:06:46, Charlie Reis (OOO until 5-11) wrote:
> s/parent/embedder/

Done.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:430: // Creates a proxy
for guest frame in embedder's process.
On 2015/04/30 23:06:46, Charlie Reis (OOO until 5-11) wrote:
> Called on the guest RFHM to create a proxy for this guest frame in its
> embedder's process.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:431: // This proxy is
used to swap out the initial guest frame in embedder.
On 2015/04/30 23:06:46, Charlie Reis (OOO until 5-11) wrote:
> in the embedder.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:436: // Replaces initial
guest frame in embedder with the proxy that has routing
On 2015/04/30 23:06:46, Charlie Reis (OOO until 5-11) wrote:
> Called on the embedder RFHM to replace the initial guest frame in the embedder
> with...

Done.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:441: void
UpdateGuestRWHView(RenderWidgetHostView* child_rwhv);
On 2015/04/30 23:06:46, Charlie Reis (OOO until 5-11) wrote:
> SetGuestRenderWidgetHostView

Done.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/frame_host/render_frame_proxy_host.cc:59: // If this is a proxy
to embedder created in the guest's FTN, then we
On 2015/04/30 23:06:46, Charlie Reis (OOO until 5-11) wrote:
> If this is a proxy for the guest's FTN in its embedder's process,

Done.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/frame_host/render_frame_proxy_host.cc:61: bool
is_proxy_to_embedder_for_guest =
On 2015/04/30 23:06:46, Charlie Reis (OOO until 5-11) wrote:
> is_proxy_for_guest_in_embedder

Obsolete.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/frame_host/render_frame_proxy_host.cc:64:
->GetSiteInstance()->GetSiteURL().SchemeIs("chrome-guest");
On 2015/04/30 23:06:46, Charlie Reis (OOO until 5-11) wrote:
> Seems like there would be better ways to tell whether it's a guest, like a
> RFHM::IsGuest() (which might use something like GetEmbedderFrameTreeNodeID()
> under the hood).  The SiteInstance site URL feels a bit fragile as the uses of
> <webview> evolve.

I've changed this to use RFHM::IsGuest() that I've added.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/frame_host/render_frame_proxy_host.cc:109: FrameTree* frame_tree
= proxy_to_embedder ?
On 2015/04/30 23:06:46, Charlie Reis (OOO until 5-11) wrote:
> This looks buggy.  It uses the embedder's frame tree for any RFPH in the
guest,
> not just the RFPH in the embedder's SiteInstance.

Added to check whether this proxy's site instance matches with the one of
|proxy_to_embedder|.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:291: :
parent_web_contents_(NULL) {
On 2015/04/30 23:06:46, Charlie Reis (OOO until 5-11) wrote:
> nullptr

Done.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:294:
WebContentsImpl::WebContentsNode::~WebContentsNode() {
On 2015/04/30 23:06:47, Charlie Reis (OOO until 5-11) wrote:
> Is there any way for us to enforce that a child WebContentsNode gets deleted
> before its parent?  Otherwise the child could have a stale pointer to its
parent
> WebContents, risking a use-after-free.

Should we keep vector<WebContents*> children_ inside WebContentsNode?
That way we can invalidate the pointers when ~WebContents() is called.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:633:
DCHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2015/04/30 23:06:46, Charlie Reis (OOO until 5-11) wrote:
> CHECK

Done.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:645: // to embedder site
instance. The swapped out RVH will be used to send
On 2015/04/30 23:06:47, Charlie Reis (OOO until 5-11) wrote:
> the embedder's SiteInstance.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:646: // postMessage to guest.
On 2015/04/30 23:06:46, Charlie Reis (OOO until 5-11) wrote:
> Alex pointed out that postMessage doesn't go through RenderViewHost anymore. 
We
> may still need the swapped out RVH (not sure), but you might want to check
with
> him about routing postMessage through the RFPH instead.

I've changed the CL to use <iframe>.contentWindow directly to send postMessage.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:654: // 3. Swap the embedder's
initial frame pointing to guest with the proxy
On 2015/04/30 23:06:47, Charlie Reis (OOO until 5-11) wrote:
> initial frame for the guest

Done.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:663: return
swapped_out_render_view_routing_id;
On 2015/04/30 23:06:46, Charlie Reis (OOO until 5-11) wrote:
> Is this needed, given that postMessage should go through RFPH?

Not anymore. this function returns void now.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:1557: DCHECK(0);
On 2015/04/30 23:06:47, Charlie Reis (OOO until 5-11) wrote:
> This needs updating to work in non-site-per-process mode.

This doesn't break until we use new window from <webview>,
so I've put a NOTREACHED() here.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:4227: if
(!for_main_frame_navigation || is_guest_in_site_per_process) {
On 2015/04/30 23:06:46, Charlie Reis (OOO until 5-11) wrote:
> Sounds like you might not need this if you don't pass
> CREATE_RF_FOR_MAIN_FRAME_NAVIGATION?

Not passing CREATE_RF_FOR_MAIN_FRAME_NAVIGATION still sends
for_main_frame_navigation = true as RFHM::Navigate uses
frame_tree_node_->IsMainFrame() to set the param through RFHM::InitRenderView.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
File content/browser/web_contents/web_contents_impl.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/web_contents/web_contents_impl.h:221: int
AttachLocalFrameToGuest(WebContents* embedder_web_contents,
On 2015/04/30 23:06:47, Charlie Reis (OOO until 5-11) wrote:
> Move lower, as mentioned in web_contents.h.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/web_contents/web_contents_impl.h:739: // Used to create tree of
WebContents.
On 2015/04/30 23:06:47, Charlie Reis (OOO until 5-11) wrote:
> Please clarify what this is used for, since it's not obvious when reading
> WebContentsImpl code.  It's especially important to distinguish it from
> FrameTree and FrameTreeNodes.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/web_contents/web_contents_impl.h:740: class WebContentsNode {
On 2015/04/30 23:06:47, Charlie Reis (OOO until 5-11) wrote:
> Perhaps this should just be a struct?
> 
Done.
> WebContentsTreeNode may also make the uses clearer.
Done.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/browser/...
content/browser/web_contents/web_contents_impl.h:1283: WebContentsNode node_;
On 2015/04/30 23:06:47, Charlie Reis (OOO until 5-11) wrote:
> This needs a comment and should probably be up higher, perhaps above
> frame_tree_.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/common/f...
File content/common/frame_messages.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/common/f...
content/common/frame_messages.h:578: // Replace this frame with a
RenderFrameProxy whose routing ID is
On 2015/04/30 23:06:47, Charlie Reis (OOO until 5-11) wrote:
> nit: Blank line before.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/common/f...
content/common/frame_messages.h:580: // The proxy is for a frame in guest that
lives in embedder process.
On 2015/04/30 23:06:47, Charlie Reis (OOO until 5-11) wrote:
> nit: Move to previous line.
> Also rephrase:
> The proxy is a placeholder in the embedder's process for a guest's main frame.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/common/f...
content/common/frame_messages.h:581:
IPC_MESSAGE_ROUTED1(FrameMsg_ReplaceLocalFrameWithProxy,
On 2015/04/30 23:06:47, Charlie Reis (OOO until 5-11) wrote:
> This should probably be listed next to SwapOut.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/common/v...
File content/common/view_messages.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/common/v...
content/common/view_messages.h:540: // TODO(lazyboy): Remove once we can use RFP
to route postMessage.
On 2015/04/30 23:06:47, Charlie Reis (OOO until 5-11) wrote:
> I think that's already possible.  Check with alexmos@.

The comment was a bit misleading, this is required until we can create a RFP
without
a swapped out RV, fixed comment.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/public/b...
File content/public/browser/web_contents.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/public/b...
content/public/browser/web_contents.h:159: // Attaches or associates this
WebContents to a frame in
On 2015/04/30 23:06:47, Charlie Reis (OOO until 5-11) wrote:
> this guest WebContents to its embedder frame in...

Done.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/public/b...
content/public/browser/web_contents.h:162: // {embedder_frame_process_id,
embedder_frame_routing_id} pair.
On 2015/04/30 23:06:47, Charlie Reis (OOO until 5-11) wrote:
> Also say what this returns.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/public/b...
content/public/browser/web_contents.h:163: virtual int
AttachLocalFrameToGuest(WebContents* embedder_web_contents,
On 2015/04/30 23:06:47, Charlie Reis (OOO until 5-11) wrote:
> This name doesn't make sense to me, if we're calling it on the guest.  What
> about AttachToEmbedderFrame?
Ok.
> 
> Also, this belongs lower in the class.  Maybe in "Internal state?"
Done.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/public/b...
content/public/browser/web_contents.h:165: int embedder_frame_routing_id) = 0;
On 2015/04/30 23:06:47, Charlie Reis (OOO until 5-11) wrote:
> I think it might be clearer to pass in the the RenderFrameHost instead of the
ID
> pair.  The caller should be able to get it.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/renderer...
File content/renderer/render_frame_impl.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/renderer...
content/renderer/render_frame_impl.cc:1361:
blink::WebRemoteFrame::create(proxy_to_replace_frame);
On 2015/04/30 23:06:47, Charlie Reis (OOO until 5-11) wrote:
> It's not obvious why the existing proxy and swap codepaths aren't sufficient
for
> this, and it makes me nervous to add another path that we have to reason
about.
> 
> Can you check with Daniel and Lucas about this?  They've been working on the
> swap semantics and would know whether there's a better way to handle this. 
(If
> there is a good reason for the separate path, we should have comments
explaining
> it.)

Using OnSwapOut() now.
Lucas and I checked this bit, so this function is removed.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/renderer...
File content/renderer/render_frame_proxy.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/renderer...
content/renderer/render_frame_proxy.h:130: void
SetWebRemoteFrame(blink::WebRemoteFrame* remote_frame);
On 2015/04/30 23:06:47, Charlie Reis (OOO until 5-11) wrote:
> I don't yet understand why this needs to be exposed, since it should be
handled
> in a consistent way by Init.  See my comment in render_frame_impl.cc.

As I'm using SwapOut, this isn't needed anymore, reverted.

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/renderer...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/220001/content/renderer...
content/renderer/render_view_impl.cc:743: // break;
On 2015/04/30 23:06:47, Charlie Reis (OOO until 5-11) wrote:
> nit: Period, not semicolon.  :)
> 
> But it also sounds like this might not be necessary, given alexmos's
postMessage
> work.

This is reverted as I'm not creating the proxy on RenderView creation anymore,
Since we're now sending a FrameMsg_SwapOut, the proxy is created on that
message's
handler.

[nasko, 2015-05-05 21:34:50]

Small wave of mostly nits, since I reviewed only code in content/.

https://codereview.chromium.org/972313002/diff/280001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/972313002/diff/280001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1631:
DCHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
nit: CHECK is fine too : )

https://codereview.chromium.org/972313002/diff/280001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1643:
DCHECK(rfph_in_embedder);
nit: CHECK

https://codereview.chromium.org/972313002/diff/280001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/972313002/diff/280001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:231: bool IsGuest();
nit: I would prefer if we can come up with a more generic terminology than
Embedder/Guest, as we are going for more generic method of linking WebContents
to each other.

I noticed that Charlie went the other direction, so let's sync on that the next
time we have a meeting.

https://codereview.chromium.org/972313002/diff/280001/content/browser/frame_h...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/972313002/diff/280001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:101: // RVH of embedder
site instance does not live in the guest RFHM, we need
nit: s/site instance/SiteInstance/

https://codereview.chromium.org/972313002/diff/280001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/972313002/diff/280001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1151: // 1. Create a link to
our parent WebContents.
nit: I don't know if the numbering of the comments brings any value to them.

https://codereview.chromium.org/972313002/diff/280001/content/renderer/render...
File content/renderer/render_frame_proxy.h (right):

https://codereview.chromium.org/972313002/diff/280001/content/renderer/render...
content/renderer/render_frame_proxy.h:131: void
SetWebRemoteFrame(blink::WebRemoteFrame* remote_frame);
Why is this needed? I don't see any callers of this method in this CL.

[lazyboy, 2015-05-05 22:33:54]

https://chromiumcodereview.appspot.com/972313002/diff/280001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/280001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:1631:
DCHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2015/05/05 21:34:49, nasko wrote:
> nit: CHECK is fine too : )

Done.

https://chromiumcodereview.appspot.com/972313002/diff/280001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:1643:
DCHECK(rfph_in_embedder);
On 2015/05/05 21:34:49, nasko wrote:
> nit: CHECK

Done.

https://chromiumcodereview.appspot.com/972313002/diff/280001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/280001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:231: bool IsGuest();
On 2015/05/05 21:34:50, nasko wrote:
> nit: I would prefer if we can come up with a more generic terminology than
> Embedder/Guest, as we are going for more generic method of linking WebContents
> to each other.
> 
> I noticed that Charlie went the other direction, so let's sync on that the
next
> time we have a meeting.

Acknowledged.

https://chromiumcodereview.appspot.com/972313002/diff/280001/content/browser/...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/280001/content/browser/...
content/browser/frame_host/render_frame_proxy_host.cc:101: // RVH of embedder
site instance does not live in the guest RFHM, we need
On 2015/05/05 21:34:50, nasko wrote:
> nit: s/site instance/SiteInstance/

Done.

https://chromiumcodereview.appspot.com/972313002/diff/280001/content/browser/...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/280001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:1151: // 1. Create a link to
our parent WebContents.
On 2015/05/05 21:34:50, nasko wrote:
> nit: I don't know if the numbering of the comments brings any value to them.

Removed numbers.

https://chromiumcodereview.appspot.com/972313002/diff/280001/content/renderer...
File content/renderer/render_frame_proxy.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/280001/content/renderer...
content/renderer/render_frame_proxy.h:131: void
SetWebRemoteFrame(blink::WebRemoteFrame* remote_frame);
On 2015/05/05 21:34:50, nasko wrote:
> Why is this needed? I don't see any callers of this method in this CL.

Forgot to revert this one.
Done.

[Charlie Reis, 2015-05-19 07:12:30]

Glad to see you've started running some tests!

Let's add a bug number and update CL description.

Will all of this need to land at once, or can parts of it land first?

There's also hacks and commented out code in components/, so presumably there's
more work to do before this can land.

https://codereview.chromium.org/972313002/diff/400001/chrome/browser/apps/gue...
File chrome/browser/apps/guest_view/web_view_browsertest.cc (right):

https://codereview.chromium.org/972313002/diff/400001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_browsertest.cc:850:
command_line->AppendSwitch(switches::kSitePerProcess);
If you're doing this, you can use any try bot.  You only need to use the
linux_site_isolation try bot to run normal tests in --site-per-process mode.

https://codereview.chromium.org/972313002/diff/400001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/972313002/diff/400001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:111: //
--site-per-process, we use the contentWindow directly from
nit: , since

https://codereview.chromium.org/972313002/diff/400001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:279: :
static_cast<WebContentsViewGuest*>(GetWebContents()->GetView());
Style nit: This is a bit awkward to read.  Maybe it would be cleaner to write it
as:

WebContentsViewGuest* new_view = nullptr;
if
(!base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kSitePerProcess))
  new_view = static_cast<WebContentsViewGuest*>(GetWebContents()->GetView());

https://codereview.chromium.org/972313002/diff/400001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/972313002/diff/400001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.h:82: // floor b/c we don't
have a BrowserPlugin anymore.
This comment needs updating, assuming it only applies in --site-per-process
mode.

https://codereview.chromium.org/972313002/diff/400001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/972313002/diff/400001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:164: int64
embedder_frame_tree_node_id = delegate_->GetEmbedderFrameTreeNodeID();
nit: We've recently changed FTN IDs to be ints rather than int64.  (Check for
this throughout the CL.)

https://codereview.chromium.org/972313002/diff/400001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:175: return
frame_tree_node_->IsMainFrame() &&
Why is this line needed?  Is it important not to treat subframes of the guest
(inner WebContents) as guests?  The comment for this method should probably
clarify the reason for that.

https://codereview.chromium.org/972313002/diff/400001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:484:
current_host()->GetView()->Show();
Any thoughts yet on where this belongs?  I don't think it should stay here.

https://codereview.chromium.org/972313002/diff/400001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1664:
CHECK(rfph_in_embedder);
DCHECK is sufficient if you want to document the invariant.  We'll crash on the
next line anyway in release builds.

https://codereview.chromium.org/972313002/diff/400001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/972313002/diff/400001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:178: // Returns the FTN
id of the embedder delegate (WebContents) that is hosting
Let's update all the embedder/guest references to outer/inner WebContents.

https://codereview.chromium.org/972313002/diff/400001/content/browser/frame_h...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/972313002/diff/400001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:65: // need a
CrossProcessFrameConnector.
This is getting a bit too complicated, given that there's comments within the
conditional itself.  Maybe we should split out the first half into a
is_proxy_to_parent bool and then move the comment before the conditional.

https://codereview.chromium.org/972313002/diff/400001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/972313002/diff/400001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1563: // TODO(lazyboy):
CreateNewWindow doesn't work for <webview> yet.
for OOPIF-based <webview> yet

https://codereview.chromium.org/972313002/diff/400001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/972313002/diff/400001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:744: // parent-child
relationship using this class. Note that their FrameTrees
outer/inner

https://codereview.chromium.org/972313002/diff/400001/content/common/view_mes...
File content/common/view_messages.h (right):

https://codereview.chromium.org/972313002/diff/400001/content/common/view_mes...
content/common/view_messages.h:545: // Whether the RenderView is for a guest.
nit: Remove embedder/guest references.

https://codereview.chromium.org/972313002/diff/400001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/972313002/diff/400001/content/renderer/render...
content/renderer/render_view_impl.cc:736: // a guest. The proxy will be created
in RenderFrameImpl::OnSwapOut().
nit: Please put comments before (not in) the conditional.

[lazyboy, 2015-05-21 23:23:48]

Patch set #27

Glad to see you've started running some tests!
-> I've separated the test out to another CL to avoid churn here,
once that lands, I'll just remove the exclusion in this CL.

Let's add a bug number and update CL description.
-> Done.

Will all of this need to land at once, or can parts of it land first?
-> As we discussed, this is sort of the minimum set.

There's also hacks and commented out code in components/, so presumably there's
more work to do before this can land.
-> This is now fixed except some logs.


Overall, I've also did {outer,inner} WebContents rename, some notes;
- The rename is only done inside content/ except in BrowserPluginGuest
- I'm keeping RFHM::IsGuest(), but not sure if I should rename that too
- The public functions in RFHM refer to WebContents as just "Contents", since it
should not refer to WebContents directly?

https://chromiumcodereview.appspot.com/972313002/diff/400001/chrome/browser/a...
File chrome/browser/apps/guest_view/web_view_browsertest.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/400001/chrome/browser/a...
chrome/browser/apps/guest_view/web_view_browsertest.cc:850:
command_line->AppendSwitch(switches::kSitePerProcess);
On 2015/05/19 07:12:30, Charlie Reis wrote:
> If you're doing this, you can use any try bot.  You only need to use the
> linux_site_isolation try bot to run normal tests in --site-per-process mode.

I've extracted the test to another CL:
https://chromiumcodereview.appspot.com/1148903003/
I'll submit that one disabled in site_isolation bot and once
that lands I'll just enable the test in this CL.

https://chromiumcodereview.appspot.com/972313002/diff/400001/content/browser/...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/400001/content/browser/...
content/browser/browser_plugin/browser_plugin_guest.cc:111: //
--site-per-process, we use the contentWindow directly from
On 2015/05/19 07:12:30, Charlie Reis wrote:
> nit: , since

Done.

https://chromiumcodereview.appspot.com/972313002/diff/400001/content/browser/...
content/browser/browser_plugin/browser_plugin_guest.cc:279: :
static_cast<WebContentsViewGuest*>(GetWebContents()->GetView());
On 2015/05/19 07:12:30, Charlie Reis wrote:
> Style nit: This is a bit awkward to read.  Maybe it would be cleaner to write
it
> as:
> 
> WebContentsViewGuest* new_view = nullptr;
> if
>
(!base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kSitePerProcess))
>   new_view = static_cast<WebContentsViewGuest*>(GetWebContents()->GetView());

Done.

https://chromiumcodereview.appspot.com/972313002/diff/400001/content/browser/...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/400001/content/browser/...
content/browser/browser_plugin/browser_plugin_guest.h:82: // floor b/c we don't
have a BrowserPlugin anymore.
On 2015/05/19 07:12:31, Charlie Reis wrote:
> This comment needs updating, assuming it only applies in --site-per-process
> mode.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/400001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/400001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:164: int64
embedder_frame_tree_node_id = delegate_->GetEmbedderFrameTreeNodeID();
On 2015/05/19 07:12:31, Charlie Reis wrote:
> nit: We've recently changed FTN IDs to be ints rather than int64.  (Check for
> this throughout the CL.)

Done.

https://chromiumcodereview.appspot.com/972313002/diff/400001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:175: return
frame_tree_node_->IsMainFrame() &&
On 2015/05/19 07:12:31, Charlie Reis wrote:
> Why is this line needed?  Is it important not to treat subframes of the guest
> (inner WebContents) as guests?  The comment for this method should probably
> clarify the reason for that.

I haven't started running subframes inside guest yet.
That's why I narrowed this to main frames only.
Added a TODO in description to follow up with subframes inside guest.

https://chromiumcodereview.appspot.com/972313002/diff/400001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:484:
current_host()->GetView()->Show();
On 2015/05/19 07:12:31, Charlie Reis wrote:
> Any thoughts yet on where this belongs?  I don't think it should stay here.

Not entirely yet, I've moved this outside of content/, to
GuestViewBase(WCO)::DidNavigateMainFrame()

There used to be guest visibility issue with plugin implementation and we
needed to send extra IPC (BrowserPluginHostMsg_SetVisibility), I am yet
to investigate that for out-of-process implementation, but we should be
good for now.

https://chromiumcodereview.appspot.com/972313002/diff/400001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:1664:
CHECK(rfph_in_embedder);
On 2015/05/19 07:12:31, Charlie Reis wrote:
> DCHECK is sufficient if you want to document the invariant.  We'll crash on
the
> next line anyway in release builds.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/400001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/400001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:178: // Returns the FTN
id of the embedder delegate (WebContents) that is hosting
On 2015/05/19 07:12:31, Charlie Reis wrote:
> Let's update all the embedder/guest references to outer/inner WebContents.

See overall CL comment.
Done.

https://chromiumcodereview.appspot.com/972313002/diff/400001/content/browser/...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/400001/content/browser/...
content/browser/frame_host/render_frame_proxy_host.cc:65: // need a
CrossProcessFrameConnector.
On 2015/05/19 07:12:31, Charlie Reis wrote:
> This is getting a bit too complicated, given that there's comments within the
> conditional itself.  Maybe we should split out the first half into a
> is_proxy_to_parent bool and then move the comment before the conditional.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/400001/content/browser/...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/400001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:1563: // TODO(lazyboy):
CreateNewWindow doesn't work for <webview> yet.
On 2015/05/19 07:12:31, Charlie Reis wrote:
> for OOPIF-based <webview> yet

Done.

https://chromiumcodereview.appspot.com/972313002/diff/400001/content/browser/...
File content/browser/web_contents/web_contents_impl.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/400001/content/browser/...
content/browser/web_contents/web_contents_impl.h:744: // parent-child
relationship using this class. Note that their FrameTrees
On 2015/05/19 07:12:31, Charlie Reis wrote:
> outer/inner

Done, please check.

https://chromiumcodereview.appspot.com/972313002/diff/400001/content/common/v...
File content/common/view_messages.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/400001/content/common/v...
content/common/view_messages.h:545: // Whether the RenderView is for a guest.
On 2015/05/19 07:12:31, Charlie Reis wrote:
> nit: Remove embedder/guest references.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/400001/content/renderer...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/400001/content/renderer...
content/renderer/render_view_impl.cc:736: // a guest. The proxy will be created
in RenderFrameImpl::OnSwapOut().
On 2015/05/19 07:12:31, Charlie Reis wrote:
> nit: Please put comments before (not in) the conditional.

Done.

[nasko, 2015-05-22 16:32:28]

Thanks for keeping at this! It is getting very close. Most of the comments are
nits and polish, but there is one bigger issue about bridging FrameTrees that we
need to resolve before landing it.

https://codereview.chromium.org/972313002/diff/540001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/972313002/diff/540001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1627:
CreateRenderFrame(outer_contents_site_instance, nullptr,
Ah, this is unfortunate : (. I'm working on a CL that removes swapped out in
--site-per-process, which means you will be able to just call
RFHM::CreateRenderFrameProxy :).

https://codereview.chromium.org/972313002/diff/540001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1642: false /*
is_loading */, content::FrameReplicationState()));
nit: No need to use "content::" prefix, this code is already in content/ :)

https://codereview.chromium.org/972313002/diff/540001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/972313002/diff/540001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:178: // Returns the FTN
id of the outer WebContents that is hosting this
nit: s/FTN/FrameTreeNode/

https://codereview.chromium.org/972313002/diff/540001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:181: virtual int
GetOuterWebContentsFrameTreeNodeID() = 0;
At this layer (content/browser/frame_host) we don't know of WebContents, which
is why we have this delegate. I don't know if it is a good idea to put that
object type in the name of the method. Unfortunately, I don't have a good
suggestion on what will work better : (. Maybe GetContainerFrameTreeNodeID,
since you used that terminology in the WebContents layer?

https://codereview.chromium.org/972313002/diff/540001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:229:
RenderFrameProxyHost* GetProxyToOuterContents();
nit: Similar issue with Contents as above. This layer doesn't know what embeds
it.

https://codereview.chromium.org/972313002/diff/540001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:231: // Returns whether
this manager belongs to a FTN that is a main frame in
nit: s/FTN/FrameTreeNode/

https://codereview.chromium.org/972313002/diff/540001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:433: int
CreateOuterContentsProxy(SiteInstance* outer_contents_site_instance);
Can we avoid using "Contents" here? The method seems to just create a proxy
based on a SiteInstance. Does it do anything more special than
CreateRenderFrameProxy?

https://codereview.chromium.org/972313002/diff/540001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:438: void
ReplaceWithInnerContentsProxy(int proxy_to_outer_contents_routing_id);
Maybe named "SwapFrameWithProxy"? Those are all basic operations that we can
perform even without inner/outer WebContents, so having those be more generic
names allows us to reuse them if needed. Also, the comments about how
WebContents are linked to each other belong in the WebContents code that invokes
those methods.

https://codereview.chromium.org/972313002/diff/540001/content/browser/frame_h...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/972313002/diff/540001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:100: // as we do in
RenderFrameHostImpl.
I have a CL that does just that, but got reverted. Let's make sure we stay in
sync on that.

https://codereview.chromium.org/972313002/diff/540001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:102: // RVH of outer
WebContents' SiteInstance does not live in the RFHM of the
Hmmm, this strikes me as odd and possibly wrong. A proxy is associated with a
RVH in the same FrameTree. Each FrameTree should be self contained and we
shouldn't be bridging them here. The only true link between the two should be
the CrossProcessFrameConnector.

https://codereview.chromium.org/972313002/diff/540001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/972313002/diff/540001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:329:
params.is_inner_web_contents_in_site_per_process =
Why not use this conditional and set the proxy_routing_id to MSG_ROUTING_NONE?
Then the boolean isn't used at all.

https://codereview.chromium.org/972313002/diff/540001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/972313002/diff/540001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:733: // still remain
disjoint/separate.
nit: Drop the "/separate", one word should be enough.

https://codereview.chromium.org/972313002/diff/540001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:734: // The root is referred to
as "outer WebContents" and the descendents are
Where is "root" used?

https://codereview.chromium.org/972313002/diff/540001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:741: void
set_parent_web_contents(WebContentsImpl* parent_web_contents) {
nit: s/parent/outer/ in this entire class.

https://codereview.chromium.org/972313002/diff/540001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:1064: // If this WebContents is
part of a "tree of WebContents" structure, then
nit: Drop one of the usages of "structure. It seems better if only the latter
stays.

https://codereview.chromium.org/972313002/diff/540001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:1065: // this contain
information about the structure.
nit: s/contain/contains/

https://codereview.chromium.org/972313002/diff/540001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/972313002/diff/540001/content/public/browser/...
content/public/browser/web_contents.h:379: // Attaches or associates this inner
WebContents to its container frame
nit: What is the difference between "attach" and "associate"?

https://codereview.chromium.org/972313002/diff/540001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/972313002/diff/540001/content/renderer/render...
content/renderer/render_view_impl.cc:736: // The proxy will be created in
RenderFrameImpl::OnSwapOut().
Is it not possible to just pass MSG_ROUTING_NONE from the browser process in
this case? Then it will make this code independent of the boolean.

[Charlie Reis, 2015-05-22 23:44:29]

Thanks; mostly nits below.

Please list some of the high level changes happening in the CL description.  For
example:
 - BrowserPlugins aren't used.
 - BrowserPluginGuest doesn't need to do as much (why?)
 - etc

https://codereview.chromium.org/972313002/diff/540001/components/guest_view/b...
File components/guest_view/browser/guest_view_base.cc (right):

https://codereview.chromium.org/972313002/diff/540001/components/guest_view/b...
components/guest_view/browser/guest_view_base.cc:564: // TODO(lazyboy): This
breaks guest visibility in --site-per-process.
Can you explain a little more about the case that's broken in the TODO, to make
it easier to revisit and fix?

https://codereview.chromium.org/972313002/diff/540001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/972313002/diff/540001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.h:81: // Note: in
--site-per-process, all IPC sent out from this class will be
nit: IPCs

https://codereview.chromium.org/972313002/diff/540001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/972313002/diff/540001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:176: return
frame_tree_node_->IsMainFrame() &&
Thanks for the TODO in the .h file.  Can you also put it above this line, to
make it clear to readers that this IsMainFrame check is temporary?

https://codereview.chromium.org/972313002/diff/540001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1640:
current_frame_host()->Send(new FrameMsg_SwapOut(
Is it correct to pass false for is_loading?  I was under the impression that we
call this when we start the load in the guest.  Maybe we should pass the loading
state from frame_tree_node_?

https://codereview.chromium.org/972313002/diff/540001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/972313002/diff/540001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:181: virtual int
GetOuterWebContentsFrameTreeNodeID() = 0;
On 2015/05/22 16:32:27, nasko wrote:
> At this layer (content/browser/frame_host) we don't know of WebContents, which
> is why we have this delegate. I don't know if it is a good idea to put that
> object type in the name of the method. Unfortunately, I don't have a good
> suggestion on what will work better : (. Maybe GetContainerFrameTreeNodeID,
> since you used that terminology in the WebContents layer?

In a sense, "delegate" refers to WebContents, so maybe
GetOuterDelegateFrameTreeNodeID?

https://codereview.chromium.org/972313002/diff/540001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:229:
RenderFrameProxyHost* GetProxyToOuterContents();
On 2015/05/22 16:32:27, nasko wrote:
> nit: Similar issue with Contents as above. This layer doesn't know what embeds
> it.

GetProxyToOuterDelegate?

https://codereview.chromium.org/972313002/diff/540001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:236: bool IsGuest();
ForInnerDelegate?

I'm ok with mentioning "inner WebContents" in the comment, just not the method
name.  We should also avoid guest/embedder.

https://codereview.chromium.org/972313002/diff/540001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:431: // inner
WebContents' frame in its outer WebContents' process.
nit: WebContents's

(Always use 's when the noun is singular, even when it ends in s.  In this case,
WebContents is singular.)

https://codereview.chromium.org/972313002/diff/540001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:433: int
CreateOuterContentsProxy(SiteInstance* outer_contents_site_instance);
On 2015/05/22 16:32:27, nasko wrote:
> Can we avoid using "Contents" here? 

CreateOuterDelegateProxy(SiteInstance* outer_site_instance)?

> The method seems to just create a proxy
> based on a SiteInstance. Does it do anything more special than
> CreateRenderFrameProxy?

https://codereview.chromium.org/972313002/diff/540001/content/browser/frame_h...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/972313002/diff/540001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:65: // WebContents' FTN in
outer WebContents' process, then we need a
nit: WebContents's 
nit: FrameTreeNode

https://codereview.chromium.org/972313002/diff/540001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:102: // RVH of outer
WebContents' SiteInstance does not live in the RFHM of the
On 2015/05/22 16:32:27, nasko wrote:
> Hmmm, this strikes me as odd and possibly wrong. A proxy is associated with a
> RVH in the same FrameTree. Each FrameTree should be self contained and we
> shouldn't be bridging them here. The only true link between the two should be
> the CrossProcessFrameConnector.

+1.  The RVH for a proxy should be in the same tree.

If you need the RVH from the outer WebContents, that should be a different
method.

https://codereview.chromium.org/972313002/diff/540001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/972313002/diff/540001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:139: if
(!BrowserPluginGuest::IsGuest(static_cast<RenderViewHostImpl*>(
This needs a comment.  I'm also confused, since it seems backwards.  Won't
IsGuest be true in the default <webview> flow, causing us to skip the
NOTREACHED?  And is it false in --site-per-process, in which case we hit the
NOTREACHED?  What was the intention?

https://codereview.chromium.org/972313002/diff/540001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/972313002/diff/540001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:329:
params.is_inner_web_contents_in_site_per_process =
On 2015/05/22 16:32:27, nasko wrote:
> Why not use this conditional and set the proxy_routing_id to MSG_ROUTING_NONE?
> Then the boolean isn't used at all.

+1

https://codereview.chromium.org/972313002/diff/540001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/972313002/diff/540001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:297: // parent WebContents,
risking a use-after-free.
This must be fixed before committing.

https://codereview.chromium.org/972313002/diff/540001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1151: // Create a link to our
parent WebContents.
outer

https://codereview.chromium.org/972313002/diff/540001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1164: // Swap the outer
WebContents' initial frame for the inner WebContents with
WebContents's

https://codereview.chromium.org/972313002/diff/540001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/972313002/diff/540001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:731: // Two WebContents with
separate FrameTree can be connected by
FrameTrees

https://codereview.chromium.org/972313002/diff/540001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/972313002/diff/540001/content/public/browser/...
content/public/browser/web_contents.h:83: class WebContents : public
PageNavigator,
We may want to add a small paragraph about inner/outer WebContents, now that
we're adding it as a first class concept.  Something like:

One WebContents can be nested inside another for the purpose of embedding
out-of-process top-level pages inside a page.  The outer WebContents can host a
tree of inner WebContentses using |AttachToOuterWebContentsFrame|.

[lazyboy, 2015-05-26 16:32:55]

Most comments are address, some pending issues:

1) is_loading in SwapOut still needs to be fixed
2) Keyboard events regressed as CL progressed, need to add test for that.
3) Known features that work still requires explicit tests: a) postMessage test
already exists b) executeScript requires test.

https://chromiumcodereview.appspot.com/972313002/diff/540001/components/guest...
File components/guest_view/browser/guest_view_base.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/540001/components/guest...
components/guest_view/browser/guest_view_base.cc:564: // TODO(lazyboy): This
breaks guest visibility in --site-per-process.
On 2015/05/22 at 23:44:30, Charlie Reis wrote:
> Can you explain a little more about the case that's broken in the TODO, to
make it easier to revisit and fix?

Done.

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
content/browser/browser_plugin/browser_plugin_guest.h:81: // Note: in
--site-per-process, all IPC sent out from this class will be
On 2015/05/22 at 23:44:30, Charlie Reis wrote:
> nit: IPCs

Done.

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:176: return
frame_tree_node_->IsMainFrame() &&
On 2015/05/22 at 23:44:30, Charlie Reis wrote:
> Thanks for the TODO in the .h file.  Can you also put it above this line, to
make it clear to readers that this IsMainFrame check is temporary?

Done.

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:1627:
CreateRenderFrame(outer_contents_site_instance, nullptr,
On 2015/05/22 at 16:32:27, nasko wrote:
> Ah, this is unfortunate : (. I'm working on a CL that removes swapped out in
--site-per-process, which means you will be able to just call
RFHM::CreateRenderFrameProxy :).

Bug#?

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:1642: false /*
is_loading */, content::FrameReplicationState()));
On 2015/05/22 at 16:32:27, nasko wrote:
> nit: No need to use "content::" prefix, this code is already in content/ :)

Done, thanks.

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:178: // Returns the FTN
id of the outer WebContents that is hosting this
On 2015/05/22 at 16:32:27, nasko wrote:
> nit: s/FTN/FrameTreeNode/

Done.

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:181: virtual int
GetOuterWebContentsFrameTreeNodeID() = 0;
On 2015/05/22 at 23:44:31, Charlie Reis wrote:
> On 2015/05/22 16:32:27, nasko wrote:
> > At this layer (content/browser/frame_host) we don't know of WebContents,
which
> > is why we have this delegate. I don't know if it is a good idea to put that
> > object type in the name of the method. Unfortunately, I don't have a good
> > suggestion on what will work better : (. Maybe GetContainerFrameTreeNodeID,
> > since you used that terminology in the WebContents layer?
> 
> In a sense, "delegate" refers to WebContents, so maybe
GetOuterDelegateFrameTreeNodeID?

Using GetOuterDelegateFrameTreeNodeID.

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:229:
RenderFrameProxyHost* GetProxyToOuterContents();
On 2015/05/22 at 23:44:30, Charlie Reis wrote:
> On 2015/05/22 16:32:27, nasko wrote:
> > nit: Similar issue with Contents as above. This layer doesn't know what
embeds
> > it.
> 
> GetProxyToOuterDelegate?

Using GetProxyToOuterDelegate().
Done.

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:231: // Returns whether
this manager belongs to a FTN that is a main frame in
On 2015/05/22 at 16:32:27, nasko wrote:
> nit: s/FTN/FrameTreeNode/

Done.

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:236: bool IsGuest();
On 2015/05/22 at 23:44:30, Charlie Reis wrote:
> ForInnerDelegate?
> 
> I'm ok with mentioning "inner WebContents" in the comment, just not the method
name.  We should also avoid guest/embedder.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:431: // inner
WebContents' frame in its outer WebContents' process.
On 2015/05/22 at 23:44:30, Charlie Reis wrote:
> nit: WebContents's
> 
> (Always use 's when the noun is singular, even when it ends in s.  In this
case, WebContents is singular.)

Done.

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:433: int
CreateOuterContentsProxy(SiteInstance* outer_contents_site_instance);
On 2015/05/22 at 16:32:27, nasko wrote:
> Can we avoid using "Contents" here?
Done.
 The method seems to just create a proxy based on a SiteInstance. Does it do
anything more special than CreateRenderFrameProxy?

As you've noted in the .cc file, we need to create swapped
out RV and RF as well to create the proxy.
I've added a todo on the caller to use CreateRenderFrameProxy
once we can create proxies w/o swapped out. Bug #? I can add a reference to
that.

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:438: void
ReplaceWithInnerContentsProxy(int proxy_to_outer_contents_routing_id);
On 2015/05/22 at 16:32:27, nasko wrote:
> Maybe named "SwapFrameWithProxy"? Those are all basic operations that we can
perform even without inner/outer WebContents, so having those be more generic
names allows us to reuse them if needed. 
Done.
Also, the comments about how WebContents are linked to each other belong in the
WebContents code that invokes those methods.

Done, the caller already has comments.
We did this because we wanted to be explicit about how to use
this method, now that we've renamed to SwapFrameWithProxy,
and this is more generic to be used in other places, I think
removing the comment here makes sense.

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
content/browser/frame_host/render_frame_proxy_host.cc:65: // WebContents' FTN in
outer WebContents' process, then we need a
On 2015/05/22 at 23:44:31, Charlie Reis wrote:
> nit: WebContents's 
> nit: FrameTreeNode

Done.

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
content/browser/frame_host/render_frame_proxy_host.cc:100: // as we do in
RenderFrameHostImpl.
On 2015/05/22 at 16:32:27, nasko wrote:
> I have a CL that does just that, but got reverted. Let's make sure we stay in
sync on that.

This function has been moved now.

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
content/browser/frame_host/render_frame_proxy_host.cc:102: // RVH of outer
WebContents' SiteInstance does not live in the RFHM of the
On 2015/05/22 at 23:44:31, Charlie Reis wrote:
> On 2015/05/22 16:32:27, nasko wrote:
> > Hmmm, this strikes me as odd and possibly wrong. A proxy is associated with
a
> > RVH in the same FrameTree. Each FrameTree should be self contained and we
> > shouldn't be bridging them here. The only true link between the two should
be
> > the CrossProcessFrameConnector.
> 
> +1.  The RVH for a proxy should be in the same tree.
> 
> If you need the RVH from the outer WebContents, that should be a different
method.

This is required because RWHVChildFrame needed to
access the parent widget to read the last keyboard event.
See CPFC::OnForwardInputEvent().
However, during review cycle I broke keyboard events in this
CL and this was basically returning not the RVH I meant.
I've fixed keyboard events now, will add a test for that.
Since this is only used for keyboard, I've created a new
function that does that and made it obvious that it's only
used for keyboard events.

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
content/browser/frame_host/render_widget_host_view_child_frame.cc:139: if
(!BrowserPluginGuest::IsGuest(static_cast<RenderViewHostImpl*>(
On 2015/05/22 at 23:44:31, Charlie Reis wrote:
> This needs a comment.  I'm also confused, since it seems backwards.  Won't
IsGuest be true in the default <webview> flow, causing us to skip the
NOTREACHED?  And is it false in --site-per-process, in which case we hit the
NOTREACHED?  What was the intention?

default webview does not hit this function.
I think is_loading is broken in this CL as you've pointed
out in the Swap IPC.

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
content/browser/renderer_host/render_view_host_impl.cc:329:
params.is_inner_web_contents_in_site_per_process =
On 2015/05/22 at 23:44:31, Charlie Reis wrote:
> On 2015/05/22 16:32:27, nasko wrote:
> > Why not use this conditional and set the proxy_routing_id to
MSG_ROUTING_NONE?
> > Then the boolean isn't used at all.
> 
> +1

OK.
Guess that's OK, we wanted to be explicit about this step
so that's the code is easy to reason about/read. However, now
that I think about it, adding a new bool isn't super either.
Removed.

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:1151: // Create a link to our
parent WebContents.
On 2015/05/22 at 23:44:31, Charlie Reis wrote:
> outer

Done.

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:1164: // Swap the outer
WebContents' initial frame for the inner WebContents with
On 2015/05/22 at 23:44:31, Charlie Reis wrote:
> WebContents's

Done.

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
File content/browser/web_contents/web_contents_impl.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
content/browser/web_contents/web_contents_impl.h:731: // Two WebContents with
separate FrameTree can be connected by
On 2015/05/22 at 23:44:31, Charlie Reis wrote:
> FrameTrees

Done.

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
content/browser/web_contents/web_contents_impl.h:733: // still remain
disjoint/separate.
On 2015/05/22 at 16:32:28, nasko wrote:
> nit: Drop the "/separate", one word should be enough.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
content/browser/web_contents/web_contents_impl.h:734: // The root is referred to
as "outer WebContents" and the descendents are
On 2015/05/22 at 16:32:27, nasko wrote:
> Where is "root" used?

I meant the root of this tree structure, not in code.
How do I improve this comment?

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
content/browser/web_contents/web_contents_impl.h:741: void
set_parent_web_contents(WebContentsImpl* parent_web_contents) {
On 2015/05/22 at 16:32:28, nasko wrote:
> nit: s/parent/outer/ in this entire class.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
content/browser/web_contents/web_contents_impl.h:1064: // If this WebContents is
part of a "tree of WebContents" structure, then
On 2015/05/22 at 16:32:28, nasko wrote:
> nit: Drop one of the usages of "structure. It seems better if only the latter
stays.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
content/browser/web_contents/web_contents_impl.h:1065: // this contain
information about the structure.
On 2015/05/22 at 16:32:27, nasko wrote:
> nit: s/contain/contains/

Done.

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/public/b...
File content/public/browser/web_contents.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/public/b...
content/public/browser/web_contents.h:379: // Attaches or associates this inner
WebContents to its container frame
On 2015/05/22 at 16:32:28, nasko wrote:
> nit: What is the difference between "attach" and "associate"?

None, removed "associates".

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/renderer...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/renderer...
content/renderer/render_view_impl.cc:736: // The proxy will be created in
RenderFrameImpl::OnSwapOut().
On 2015/05/22 at 16:32:28, nasko wrote:
> Is it not possible to just pass MSG_ROUTING_NONE from the browser process in
this case? Then it will make this code independent of the boolean.

Yes, done.

[Fady Samuel, 2015-05-26 16:42:15]

https://chromiumcodereview.appspot.com/972313002/diff/600001/extensions/rende...
File extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc
(right):

https://chromiumcodereview.appspot.com/972313002/diff/600001/extensions/rende...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:298: new
extensions::ExtensionsGuestViewContainer(embedder_parent_frame);
Fix this.

https://chromiumcodereview.appspot.com/972313002/diff/600001/extensions/rende...
File extensions/renderer/guest_view/guest_view_request.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/600001/extensions/rende...
extensions/renderer/guest_view/guest_view_request.cc:156:
ExtensionsGuestViewMsg_GuestReady::Read(&message, &param);
Why is this an ExtensionsGuestViewMsg?

https://chromiumcodereview.appspot.com/972313002/diff/600001/extensions/rende...
File extensions/renderer/resources/guest_view/guest_view.js (right):

https://chromiumcodereview.appspot.com/972313002/diff/600001/extensions/rende...
extensions/renderer/resources/guest_view/guest_view.js:149: contentWindow =
this.guestView.browserPluginElement.contentWindow;
Why are you using browserPluginElement for sitePerProcess? This isn't the right
name.

https://chromiumcodereview.appspot.com/972313002/diff/600001/extensions/rende...
File extensions/renderer/resources/guest_view/guest_view_container.js (right):

https://chromiumcodereview.appspot.com/972313002/diff/600001/extensions/rende...
extensions/renderer/resources/guest_view/guest_view_container.js:26:
this.guest.setBrowserPluginElement(privates(this).browserPluginElement);
Why is this necessary? Also, guests can change their container.

https://chromiumcodereview.appspot.com/972313002/diff/600001/extensions/rende...
extensions/renderer/resources/guest_view/guest_view_container.js:199: function
registerBrowserPluginElement(viewType) {
Is this at all necessary in sitePerProcess?

[Fady Samuel, 2015-05-26 19:02:28]

https://codereview.chromium.org/972313002/diff/600001/extensions/renderer/gue...
File extensions/renderer/guest_view/extensions_guest_view_container.cc (right):

https://codereview.chromium.org/972313002/diff/600001/extensions/renderer/gue...
extensions/renderer/guest_view/extensions_guest_view_container.cc:53: bool
ExtensionsGuestViewContainer::OnMessage(const IPC::Message& message) {
Move this code to GuestViewContainer.

[Fady Samuel, 2015-05-26 19:08:45]

My general concerns with this CL are as follows:

1. Too many if
(base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kSitePerProcess)) {
This makes it very hard to reason about the behavior of the code. Please find a
way to minimize this. I'm concerned about letting this patch go through as it
would slow down all subsequent code review, and potentially introduce subtle new
bugs as you clean up.

2. Minimize extension specific code. Also, how does all this interact with
MIMEHandlerView, appview and so on?

3. Please find a way to manage the lifetime of the GuestViewContainer.

4. Do not expose site-per-process above the guestView layer. Really this is a
content layer concept, and anything outside of content should behave in the
correct way regardless. guestView is effectively a components concept now, and
so I'm OK with exposing that detail there, for now. GuestView may move back into
content in the future, I have no idea at this point.

[lfg, 2015-05-26 19:18:13]

lfg@chromium.org changed reviewers:
+ lfg@chromium.org

[lfg, 2015-05-26 19:18:15]

Just a couple of comments.

https://codereview.chromium.org/972313002/diff/600001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/972313002/diff/600001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:158:
->GetSiteInstance());
Can this be done with ftn->current_frame_host()->render_view_host()?

https://codereview.chromium.org/972313002/diff/600001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/972313002/diff/600001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:295: // Remove child pointers
from our parent.
Maybe we should have the outer WebContents own the inner WebContents through
this node?

Consider adding a TODO to clean this up later, so that the inner/outer lifetime
management can be managed by the WebContentsTree instead of managing it directly
in the GuestView.

[nasko, 2015-05-28 22:13:46]

Some more comments, though most are nits.

https://codereview.chromium.org/972313002/diff/600001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/972313002/diff/600001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:158:
->GetSiteInstance());
On 2015/05/26 19:18:15, lfg wrote:
> Can this be done with ftn->current_frame_host()->render_view_host()?

I think it can. Is there a reason why we don't use this simpler version?

https://codereview.chromium.org/972313002/diff/600001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:190: // TODO(lazyboy):
We should remove IsMainFrame() check below since any frames
What prevents us from removing it now?

https://codereview.chromium.org/972313002/diff/600001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/972313002/diff/600001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:228: // Returns the RVH
of outer WebContents that can used to fetch the last
nit: RenderViewHost of the outer

https://codereview.chromium.org/972313002/diff/600001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:230: // TODO(lazyboy):
This can be removed once we input events are sent directly
nit: Extra "we"?
nit: Input events won't be sent to remote frames, would they? It is my
understanding that we will be sending input events directly to the frame that it
is targeting.

https://codereview.chromium.org/972313002/diff/600001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/972313002/diff/600001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:145:
static_cast<RenderViewHostImpl*>(RenderViewHost::From(host_)))) {
Why static cast? Just call RenderViewHostImpl::FromID, which will give you back
an Impl type.

https://codereview.chromium.org/972313002/diff/600001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/972313002/diff/600001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:306: // Do not instruct
the view to create a proxy if this is a swapped out
nit: Either an empty line above the comment or move the variable after the
comment.

https://codereview.chromium.org/972313002/diff/600001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/972313002/diff/600001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:295: // Remove child pointers
from our parent.
On 2015/05/26 19:18:15, lfg wrote:
> Maybe we should have the outer WebContents own the inner WebContents through
> this node?
> 
> Consider adding a TODO to clean this up later, so that the inner/outer
lifetime
> management can be managed by the WebContentsTree instead of managing it
directly
> in the GuestView.

This does seem a bit hacky. Can't we manage the lifetimes directly from
WebContents?

https://codereview.chromium.org/972313002/diff/600001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:297:
std::set<WebContentsTreeNode*>& child_ptrs_in_parent =
nit: Why not typedef std::set<WebContentsTreeNode*> to something shorter?

https://codereview.chromium.org/972313002/diff/600001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1180: // we can a proxy
without swapped out RV and RF.
nit: RenderView will stick around a bit longer, but we will be able to avoid
creating the RF.

https://codereview.chromium.org/972313002/diff/600001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/972313002/diff/600001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:747:
std::set<WebContentsTreeNode*> inner_web_contents_tree_nodes_;
Why does the outer WebContents need to know about the inner ones? It doesn't
seem we use this anywhere but to clear pointers during destruction.

[dcheng, 2015-05-28 22:23:37]

Random drive-by.

https://codereview.chromium.org/972313002/diff/600001/extensions/browser/gues...
File extensions/browser/guest_view/extensions_guest_view_message_filter.cc
(right):

https://codereview.chromium.org/972313002/diff/600001/extensions/browser/gues...
extensions/browser/guest_view/extensions_guest_view_message_filter.cc:92: auto
guest = WebViewGuest::FromWebContents(guest_web_contents);
Warning: personal opinion ahead.

I think it's nice to qualify auto with * or & when possible, to make it clear
that it's not an expensive copy (and to remind people that it is, in fact, a
pointer)

[lazyboy, 2015-05-29 00:02:25]

I'm still juggling ideas to fix higher level extensions/ and
components/ issues Fady raised, will address them gradually.

content/ comments addressed in Patchset #31

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/540001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:1640:
current_frame_host()->Send(new FrameMsg_SwapOut(
On 2015/05/22 23:44:30, Charlie Reis wrote:
> Is it correct to pass false for is_loading?  I was under the impression that
we
> call this when we start the load in the guest.  Maybe we should pass the
loading
> state from frame_tree_node_?

We are in the outer WebContents's FTN and this FTN or the outer
WebContents's FrameTree never sees a load start.
Eventually, that also makes it never see a load stop.
Think that's the right behavior?
I've added DCHECK because frame tree node's is_loading should
always be false here.

https://chromiumcodereview.appspot.com/972313002/diff/600001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/600001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:158:
->GetSiteInstance());
On 2015/05/28 22:13:46, nasko wrote:
> On 2015/05/26 19:18:15, lfg wrote:
> > Can this be done with ftn->current_frame_host()->render_view_host()?
> 
Done.
> I think it can. Is there a reason why we don't use this simpler version?
No reason.

https://chromiumcodereview.appspot.com/972313002/diff/600001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:190: // TODO(lazyboy):
We should remove IsMainFrame() check below since any frames
On 2015/05/28 22:13:46, nasko wrote:
> What prevents us from removing it now?

I haven't started testing frame inside <webview> yet, and I wanted to
start testing that afterwards, not sure that works as is, so didn't
want to expose that yet.

https://chromiumcodereview.appspot.com/972313002/diff/600001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/600001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:228: // Returns the RVH
of outer WebContents that can used to fetch the last
On 2015/05/28 22:13:46, nasko wrote:
> nit: RenderViewHost of the outer

Done.

https://chromiumcodereview.appspot.com/972313002/diff/600001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:230: // TODO(lazyboy):
This can be removed once we input events are sent directly
On 2015/05/28 22:13:46, nasko wrote:
> nit: Extra "we"?
> nit: Input events won't be sent to remote frames, would they? It is my
> understanding that we will be sending input events directly to the frame that
it
> is targeting.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/600001/content/browser/...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/600001/content/browser/...
content/browser/frame_host/render_widget_host_view_child_frame.cc:145:
static_cast<RenderViewHostImpl*>(RenderViewHost::From(host_)))) {
On 2015/05/28 22:13:46, nasko wrote:
> Why static cast? Just call RenderViewHostImpl::FromID, which will give you
back
> an Impl type.
I need From() not FromID(), did you want me to do
RenderViewHostImpl::FromID(host_->GetProcess()->GetID(), host_->GetRoutingID())
?

https://chromiumcodereview.appspot.com/972313002/diff/600001/content/browser/...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/600001/content/browser/...
content/browser/renderer_host/render_view_host_impl.cc:306: // Do not instruct
the view to create a proxy if this is a swapped out
On 2015/05/28 22:13:46, nasko wrote:
> nit: Either an empty line above the comment or move the variable after the
> comment.

Done (first one).

https://chromiumcodereview.appspot.com/972313002/diff/600001/content/browser/...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/600001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:295: // Remove child pointers
from our parent.
On 2015/05/28 22:13:46, nasko wrote:
> On 2015/05/26 19:18:15, lfg wrote:
> > Maybe we should have the outer WebContents own the inner WebContents through
> > this node?
> > 
> > Consider adding a TODO to clean this up later, so that the inner/outer
> lifetime
> > management can be managed by the WebContentsTree instead of managing it
> directly
> > in the GuestView.
> 
> This does seem a bit hacky. Can't we manage the lifetimes directly from
> WebContents?

Right now <webview>s WebContents's lifetime depends of app/outer WebContents's
lifetime, which is done outside of content/ via
WebContentsObserver of the app.
The suggestion Lucas made is to destroy the inner WebContents here
when outer contents is destroyed. Is that what seems hacky or sth
else?

https://chromiumcodereview.appspot.com/972313002/diff/600001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:297:
std::set<WebContentsTreeNode*>& child_ptrs_in_parent =
On 2015/05/28 22:13:46, nasko wrote:
> nit: Why not typedef std::set<WebContentsTreeNode*> to something shorter?

Done.

https://chromiumcodereview.appspot.com/972313002/diff/600001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:1180: // we can a proxy
without swapped out RV and RF.
On 2015/05/28 22:13:46, nasko wrote:
> nit: RenderView will stick around a bit longer, but we will be able to avoid
> creating the RF.

So remove the RV from the comment?
Done.

https://chromiumcodereview.appspot.com/972313002/diff/600001/content/browser/...
File content/browser/web_contents/web_contents_impl.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/600001/content/browser/...
content/browser/web_contents/web_contents_impl.h:747:
std::set<WebContentsTreeNode*> inner_web_contents_tree_nodes_;
On 2015/05/28 22:13:46, nasko wrote:
> Why does the outer WebContents need to know about the inner ones? It doesn't
> seem we use this anywhere but to clear pointers during destruction.

The outer WebContents(TreeNode) may be destroyed before its children,
so this allows us to remove stale pointers to outer from those children.

https://chromiumcodereview.appspot.com/972313002/diff/600001/extensions/brows...
File extensions/browser/guest_view/extensions_guest_view_message_filter.cc
(right):

https://chromiumcodereview.appspot.com/972313002/diff/600001/extensions/brows...
extensions/browser/guest_view/extensions_guest_view_message_filter.cc:92: auto
guest = WebViewGuest::FromWebContents(guest_web_contents);
On 2015/05/28 22:23:37, dcheng wrote:
> Warning: personal opinion ahead.
> 
> I think it's nice to qualify auto with * or & when possible, to make it clear
> that it's not an expensive copy (and to remind people that it is, in fact, a
> pointer)

OK.
Done.

https://chromiumcodereview.appspot.com/972313002/diff/600001/extensions/rende...
File extensions/renderer/resources/guest_view/guest_view_container.js (right):

https://chromiumcodereview.appspot.com/972313002/diff/600001/extensions/rende...
extensions/renderer/resources/guest_view/guest_view_container.js:199: function
registerBrowserPluginElement(viewType) {
On 2015/05/26 16:42:15, Fady Samuel wrote:
> Is this at all necessary in sitePerProcess?

This function would register the custom element inside the shadow
to be an iframe type. So we still need it.

[Charlie Reis, 2015-06-02 18:19:15]

Mostly nits for content/.  There's still a lot here, but glad to see it has
simplified a bit, and I think it'll give us a decent base for moving <webview>
to OOPIF.

Note that Ken recently moved OOPIFs to use Surfaces on Friday.  You might check
with him whether any changes will be required here.

https://codereview.chromium.org/972313002/diff/640001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/972313002/diff/640001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:228: // Returns the
RenderWidgetHost of outer WebContents that can used to fetch
nit: can be

https://codereview.chromium.org/972313002/diff/640001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:444: // Sets the child
RWHV for this frame that is part of an inner WebContents.
nit: for this frame, which must be part of an

https://codereview.chromium.org/972313002/diff/640001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/972313002/diff/640001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:295: // Remove child pointers
from our parent.
nit: pointer

https://codereview.chromium.org/972313002/diff/640001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:304: // Remove parent pointers
from our children.
Wouldn't this leak all the children?  Seems like we should be destroying them if
their parent goes away.

https://codereview.chromium.org/972313002/diff/640001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1200: // we can a proxy
without swapped out RF.
nit: can what?

https://codereview.chromium.org/972313002/diff/640001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/972313002/diff/640001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:748: typedef
std::set<WebContentsTreeNode*> ChildrenList;
set != list

Will we ever care about the order of the children?

https://codereview.chromium.org/972313002/diff/640001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:1076: WebContentsTreeNode
node_;
Should this be a pointer instead of inline?  The vast majority of WebContents
objects will not have one.

[lazyboy, 2015-06-02 20:15:21]

I've talked to Ken and verified that surfaces are working with this CL without
any additional change.

https://chromiumcodereview.appspot.com/972313002/diff/640001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/640001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:228: // Returns the
RenderWidgetHost of outer WebContents that can used to fetch
On 2015/06/02 18:19:14, Charlie Reis wrote:
> nit: can be

Done.

https://chromiumcodereview.appspot.com/972313002/diff/640001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:444: // Sets the child
RWHV for this frame that is part of an inner WebContents.
On 2015/06/02 18:19:14, Charlie Reis wrote:
> nit: for this frame, which must be part of an

Done.

https://chromiumcodereview.appspot.com/972313002/diff/640001/content/browser/...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/640001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:295: // Remove child pointers
from our parent.
On 2015/06/02 18:19:14, Charlie Reis wrote:
> nit: pointer

Done.

https://chromiumcodereview.appspot.com/972313002/diff/640001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:304: // Remove parent pointers
from our children.
On 2015/06/02 18:19:14, Charlie Reis wrote:
> Wouldn't this leak all the children?  Seems like we should be destroying them
if
> their parent goes away.

Right now in <webview> use case, children WebContentses manage their own
lifetime so
we do not leak in that case. Agreed we should be also deleting the children WCs.

https://chromiumcodereview.appspot.com/972313002/diff/640001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:1200: // we can a proxy
without swapped out RF.
On 2015/06/02 18:19:14, Charlie Reis wrote:
> nit: can what?

Create.
Done.

https://chromiumcodereview.appspot.com/972313002/diff/640001/content/browser/...
File content/browser/web_contents/web_contents_impl.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/640001/content/browser/...
content/browser/web_contents/web_contents_impl.h:748: typedef
std::set<WebContentsTreeNode*> ChildrenList;
On 2015/06/02 18:19:14, Charlie Reis wrote:
> set != list
Changed the typedef to ChildrenSet.
> 
> Will we ever care about the order of the children?

Don't think so. Do we want to change this to vector? The reason I made it a set
is to quickly find() and erase() in the destructor.

https://chromiumcodereview.appspot.com/972313002/diff/640001/content/browser/...
content/browser/web_contents/web_contents_impl.h:1076: WebContentsTreeNode
node_;
On 2015/06/02 18:19:14, Charlie Reis wrote:
> Should this be a pointer instead of inline?  The vast majority of WebContents
> objects will not have one.

Changed to scoped_ptr.

[Charlie Reis, 2015-06-02 23:55:44]

On 2015/06/02 20:15:21, lazyboy wrote:
> I've talked to Ken and verified that surfaces are working with this CL without
> any additional change.

Great!

I'm ok with the content/ changes, once you take care of the detach issue we
discussed in the meeting.

How is it doing on try jobs?  It's great to hear so many more webview tests will
pass in --site-per-process.  Is there anything regressing at the moment?

https://codereview.chromium.org/972313002/diff/640001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/972313002/diff/640001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:748: typedef
std::set<WebContentsTreeNode*> ChildrenList;
On 2015/06/02 20:15:21, lazyboy wrote:
> On 2015/06/02 18:19:14, Charlie Reis wrote:
> > set != list
> Changed the typedef to ChildrenSet.
> > 
> > Will we ever care about the order of the children?
> 
> Don't think so. Do we want to change this to vector? The reason I made it a
set
> is to quickly find() and erase() in the destructor.

No, a set is fine if we don't care about the order of the children.  We can
always change it if it's needed later.

https://codereview.chromium.org/972313002/diff/680001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/972313002/diff/680001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:305: for (WebContentsTreeNode*
child : inner_web_contents_tree_nodes_)
Please add a TODO to delete the child WCs if you don't do it in this CL.

[lazyboy, 2015-06-03 01:37:03]

How is it doing on try jobs?  It's great to hear so many more webview tests will
pass in --site-per-process.  Is there anything regressing at the moment?

Today's sync regressed one new issue and as we discussed in the mtg,
RFPH::OnDetach() is another known issue, I've uploaded a test CL 1168443003
with temporary fixes to those and the trybots seem to more or less agree with
my local run:
27 tests failed out of ~160 (I was seeing 28 failures locally)
Here's the run:
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...

[lazyboy, 2015-06-03 22:32:27]

Fixed the RenderFrameProxyHost::OnDetach() issue in
patchset #36.

Sent patchset #35 to site isolation bots. (#35 is the same as #36 except one
comment).

https://chromiumcodereview.appspot.com/972313002/diff/680001/content/browser/...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/680001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:305: for (WebContentsTreeNode*
child : inner_web_contents_tree_nodes_)
On 2015/06/02 23:55:43, Charlie Reis wrote:
> Please add a TODO to delete the child WCs if you don't do it in this CL.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/700001/content/browser/...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/700001/content/browser/...
content/browser/frame_host/render_frame_proxy_host.cc:189: if
(frame_tree_node_->render_manager()->ForInnerDelegate()) {
I've fixed the proxy detached issue.

The scenario is:

Let, the outer WebContents's FT:
  A
 /
B
And the inner one's FT:
C

C.proxy_hosts_[A] is the proxy we create in outer process.
This lives in guest FTN, i.e. C.

When we do removeChild(B) from JavaScript, this sends
C.proxy_hosts_[A].OnDetach()

Now since this proxy host lives in main frame, the following
check will fail. The FrameTreeNode we want to delete in this
case is B. I've done this by asking C's RFHM to delete the outer
FTN. This is RFHM::RemoveOuterDelegateFrame().

Note that before this patchset, C's OuterDelegateFrameTreeNodeID()
was pointing to A.frame_tree_node_id(), now this points to
B.frame_tree_node_id() instead so that we can find out B.

[nasko, 2015-06-11 00:21:28]

Some basic comments, as I see there is some more work to be done on this CL.

https://codereview.chromium.org/972313002/diff/730001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/972313002/diff/730001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:192: // TODO(lazyboy):
We should remove IsMainFrame() check below since any frames
Should we? Wouldn't this cause us to skip the ref-counting and proxy creation
code for subframes inside the inner WebContents? I think the only special cased
proxy needs to be the one to the outer WebContents, right?

https://codereview.chromium.org/972313002/diff/730001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:204:
outer_delegate_frame_tree_node->frame_tree()->RemoveFrame(
Shouldn't this work be driven by the chrome/ layer? When the child WebContents
is destroyed?

https://codereview.chromium.org/972313002/diff/730001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1701:
outer_contents_site_instance, render_frame_host->render_view_host(),
I thought the idea was to not associate this proxy with any RenderViewHost.
Can't we pass nullptr here?

https://codereview.chromium.org/972313002/diff/730001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/972313002/diff/730001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1219: // Create a proxy in our
render manager, pointing to the SiteInstance of the
nit: s/render manager/top-level RenderFrameHostManager/

https://codereview.chromium.org/972313002/diff/730001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:4441: return -1;
nit: Let's add a symbolic constant in FrameTreeNode and use that instead of
arbitrary number here.

[lazyboy, 2015-06-11 22:14:48]

Patchset #38 (id:750001) has been deleted

[lazyboy, 2015-06-11 22:15:14]

Patchset #38 (id:770001) has been deleted

[lazyboy, 2015-06-11 22:20:29]

Hi Nasko,
There shouldn't be any pending tasks left to do in this CL,
other than figuring out the proxy/RVH related issues.
The known issues that are not addressed are listed as TODOs
and I think the rest can be treated as our basic OOPIF
support CL.
Let me know if I missed something, thanks.

https://chromiumcodereview.appspot.com/972313002/diff/730001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/730001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:192: // TODO(lazyboy):
We should remove IsMainFrame() check below since any frames
On 2015/06/11 00:21:28, nasko (slow) wrote:
> Should we? Wouldn't this cause us to skip the ref-counting and proxy creation
> code for subframes inside the inner WebContents? I think the only special
cased
> proxy needs to be the one to the outer WebContents, right?

Hmmm, I think so.
Out of process subframes inside inner WebContents is something we need
to figure out in detail though, I don't expect them to work as is.
I've changed the TODO.

https://chromiumcodereview.appspot.com/972313002/diff/730001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:204:
outer_delegate_frame_tree_node->frame_tree()->RemoveFrame(
On 2015/06/11 00:21:28, nasko (slow) wrote:
> Shouldn't this work be driven by the chrome/ layer? When the child WebContents
> is destroyed?

This is an <iframe> element in outer WebContents being destroyed, why should
we do this outside of content/?
Do we expose the notion of FTNs outside of content/ so it can be removed from
chrome/?

https://chromiumcodereview.appspot.com/972313002/diff/730001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:1701:
outer_contents_site_instance, render_frame_host->render_view_host(),
On 2015/06/11 00:21:28, nasko (slow) wrote:
> I thought the idea was to not associate this proxy with any RenderViewHost.
> Can't we pass nullptr here?

That works.
Done.

https://chromiumcodereview.appspot.com/972313002/diff/730001/content/browser/...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/730001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:1219: // Create a proxy in our
render manager, pointing to the SiteInstance of the
On 2015/06/11 00:21:28, nasko (slow) wrote:
> nit: s/render manager/top-level RenderFrameHostManager/

Done.

https://chromiumcodereview.appspot.com/972313002/diff/730001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:4441: return -1;
On 2015/06/11 00:21:28, nasko (slow) wrote:
> nit: Let's add a symbolic constant in FrameTreeNode and use that instead of
> arbitrary number here.

Done.

[Fady Samuel, 2015-06-11 22:27:21]

https://chromiumcodereview.appspot.com/972313002/diff/700001/content/browser/...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/700001/content/browser/...
content/browser/browser_plugin/browser_plugin_guest.h:82: // dropped on the
floor since we don't have a BrowserPlugin.
This seems like a prime candidate for refactoring.

https://chromiumcodereview.appspot.com/972313002/diff/730001/components/guest...
File components/guest_view/browser/guest_view_base.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/730001/components/guest...
components/guest_view/browser/guest_view_base.cc:566: // stay hidden during
"visibility:none" state.
Does OOPIF have visibility implemented yet?

https://chromiumcodereview.appspot.com/972313002/diff/730001/extensions/brows...
File extensions/browser/guest_view/web_view/web_view_guest.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/730001/extensions/brows...
extensions/browser/guest_view/web_view/web_view_guest.h:323: // TODO(lazyboy):
Use OpenURLParams as parameter.?
The WebViewGuest changes seem like something you can land separately. Might as
well do it now in a separate patch?

[nasko, 2015-06-15 23:00:14]

I've made another pass through content/. While there a lots of comments, most of
them are really just nits and cleanup related.

Thanks for doing this and being so persistent! It is almost done!

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
File content/browser/frame_host/cross_process_frame_connector.cc (right):

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.cc:151: //
|frame_proxy_in_parent_renderer_->frame_tree_node()->parent()| will fail.
nit: I found the code snippet in this comment harder to follow than if it was
omitted. Maybe drop it?

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:46: static const int
FRAME_TREE_NODE_ID_NONE = -1;
nit: kFrameTreeNodeInvalidID

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:193: // we have to make
sure that IsMainFrame() check below is appropriate.
File a bug and note it here, just to make sure we don't lose track of this work,
as it is important.

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1713:
GetProxyToOuterDelegate()->SetChildRWHView(child_rwhv);
The comment mentions that this should only be called for inner WebContents, but
we don't enforce this here. Add a DCHECK?

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:181: // this (inner)
WebContents.
nit: Lots of empty whitespace at end of this line. Start the next line of
comment at the end of this sentence.

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:183: // within the outer
WebContents.
nit: s/within/within the FrameTree of/

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:240:
RenderFrameProxyHost* GetProxyToOuterDelegate();
nit: Add a small comment to describe what this method does. It should address
the fact that it can return nullptr if there is no inner/outer relationship.

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:244: //
nit: no need for empty line.

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:451: // Called on the
RFHM of the inner WebContents to create a proxy for this
nit: s/proxy/RenderFrameProxyHost/

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:452: // inner
WebContents's frame in its outer WebContents's process.
nit: s/process/SiteInstance/. Also, the SiteInstance for the parent frame, not
just any frame in the outer WebContents.

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:456: // Sets the child
RWHV for this frame, which much be part of an inner
nit: s/RWHV/RenderWidgetHostView/

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:72: // WebContents's
FrameTreeNode in outer WebContents's process, then we need a
nit: s/process/SiteInstance/

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:158: // Inner
WebContents reaches this, so skip the NOTREACHED() check below.
Missing TODO on how to fix this?

https://codereview.chromium.org/972313002/diff/790001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/972313002/diff/790001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:310: // Do not instruct
the view to create a proxy if this is a swapped out
I'm trying to kill "swapped out", so let's not add any more usages of it : ). It
doesn't bring much additional meaning to the sentence that wouldn't exist
without it and there isn't actually any check based on the swapped out state of
the RenderViewHost.

https://codereview.chromium.org/972313002/diff/790001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:312: // The proxy will be
created using RFHM::SwapFrameWithProxy().
There is no SwapFrameWithProxy method anymore.

https://codereview.chromium.org/972313002/diff/790001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/972313002/diff/790001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1232: // postMessage to the
inner WebContents.
nit: Is postMessage the only thing that will use the proxy? Wouldn't it be all
cross-origin JS/messaging?

https://codereview.chromium.org/972313002/diff/790001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1621: // yet.
Is it meant to?

https://codereview.chromium.org/972313002/diff/790001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/972313002/diff/790001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:737: // The root is referred to
as "outer WebContents" and the descendents are
It is not just the root that will be outer WebContents, will it? Any two
WebContents's that are connected together are outer/inner, no?

https://codereview.chromium.org/972313002/diff/790001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:755: int
outer_contents_frame_tree_node_id_;
nit: Should this be const? The outer WebContents and the FTN id won't change for
the lifetime of this connection, right?

[alexmos, 2015-06-15 23:06:55]

alexmos@chromium.org changed reviewers:
+ alexmos@chromium.org

[alexmos, 2015-06-15 23:06:57]

https://codereview.chromium.org/972313002/diff/790001/testing/buildbot/chromi...
File testing/buildbot/chromium.fyi.json (left):

https://codereview.chromium.org/972313002/diff/790001/testing/buildbot/chromi...
testing/buildbot/chromium.fyi.json:3496:
"--gtest_filter=-AppApiTest.*:BlockedAppApiTest.*:BrowserTest.ClearPendingOnFailUnlessNTP:BrowserTest.OtherRedirectsDontForkProcess:BrowserTest.WindowOpenClose:ChromeAppAPITest.*:ChromeRenderProcessHostTest.*:ChromeRenderProcessHostTestWithCommandLine.*:DevToolsExperimentalExtensionTest.*:DevToolsExtensionTest.*:DnsProbeBrowserTest.*:ErrorPageTest.*:ExecuteScriptApiTest.ExecuteScriptPermissions:ExtensionApiTest.ActiveTab:ExtensionApiTest.ChromeRuntimeOpenOptionsPage:ExtensionApiTest.ContentScriptExtensionIframe:ExtensionApiTest.ContentScriptOtherExtensions:ExtensionApiTest.ContentScriptExtensionProcess:ExtensionApiTest.Tabs2:ExtensionApiTest.TabsOnUpdated:ExtensionApiTest.WindowOpenPopupIframe:ExtensionBrowserTest.LoadChromeExtensionsWithOptionsParamWhenEmbedded:ExtensionCrxInstallerTest.InstallDelayedUntilNextUpdate:ExtensionOptionsApiTest.ExtensionCanEmbedOwnOptions:ExtensionWebUITest.CanEmbedExtensionOptions:ExtensionWebUITest.ReceivesExtensionOptionsOnClose:InlineLoginUISafeIframeBrowserTest.*:IsolatedAppTest.*:LaunchWebAuthFlowFunctionTest.*:MimeHandlerViewTest.*:*.NewAvatarMenuEnabledInGuestMode:OptionsUIBrowserTest.*:*PDFExtensionTest.*:PhishingClassifierTest.*:PhishingDOMFeatureExtractorTest.*:PlatformAppUrlRedirectorBrowserTest.*:PopupBlockerBrowserTest.*:PrerenderBrowserTest.*:ProcessManagementTest.*:RedirectTest.*:ReferrerPolicyTest.*:SSLUITest.*:WebNavigationApiTest.CrossProcessFragment:WebNavigationApiTest.ServerRedirectSingleProcess:WebNavigationApiTest.CrossProcessHistory:WebViewCommonTest.*:WebViewDPITest.*:WebViewPluginTest.*:WebViewTest.*:ZoomControllerBrowserTest.*:*.NavigateFromNTPToOptionsInSameTab:*.ProfilesWithoutPagesNotLaunched:*.TabMove:*.WhitelistedExtension:*.NewTabPageURL:*.HomepageLocation:RestoreOnStartupPolicyTest*:PhishingClassifierDelegateTest.*:WebUIWebViewBrowserTest*:WebRtcBrowserTest.RunsAudioVideoWebRTCCallInTwoTabs"
Random drive-by comment: please fix this for the Site Isolation Win filter below
as well.

[lazyboy, 2015-06-16 17:59:52]

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
File content/browser/frame_host/cross_process_frame_connector.cc (right):

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.cc:151: //
|frame_proxy_in_parent_renderer_->frame_tree_node()->parent()| will fail.
On 2015/06/15 23:00:13, nasko (slow) wrote:
> nit: I found the code snippet in this comment harder to follow than if it was
> omitted. Maybe drop it?

Sure, done.

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:46: static const int
FRAME_TREE_NODE_ID_NONE = -1;
On 2015/06/15 23:00:13, nasko (slow) wrote:
> nit: kFrameTreeNodeInvalidID

Done.

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:193: // we have to make
sure that IsMainFrame() check below is appropriate.
On 2015/06/15 23:00:13, nasko (slow) wrote:
> File a bug and note it here, just to make sure we don't lose track of this
work,
> as it is important.

Done.

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1713:
GetProxyToOuterDelegate()->SetChildRWHView(child_rwhv);
On 2015/06/15 23:00:13, nasko (slow) wrote:
> The comment mentions that this should only be called for inner WebContents,
but
> we don't enforce this here. Add a DCHECK?

Added DCHECK.
Done.

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:181: // this (inner)
WebContents.
On 2015/06/15 23:00:13, nasko (slow) wrote:
> nit: Lots of empty whitespace at end of this line. Start the next line of
> comment at the end of this sentence.

Done.

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:183: // within the outer
WebContents.
On 2015/06/15 23:00:13, nasko (slow) wrote:
> nit: s/within/within the FrameTree of/

Done.

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:240:
RenderFrameProxyHost* GetProxyToOuterDelegate();
On 2015/06/15 23:00:13, nasko (slow) wrote:
> nit: Add a small comment to describe what this method does. It should address
> the fact that it can return nullptr if there is no inner/outer relationship.

Done.

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:244: //
On 2015/06/15 23:00:13, nasko (slow) wrote:
> nit: no need for empty line.

Done.

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:451: // Called on the
RFHM of the inner WebContents to create a proxy for this
On 2015/06/15 23:00:13, nasko (slow) wrote:
> nit: s/proxy/RenderFrameProxyHost/

Done.

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:452: // inner
WebContents's frame in its outer WebContents's process.
On 2015/06/15 23:00:13, nasko (slow) wrote:
> nit: s/process/SiteInstance/. Also, the SiteInstance for the parent frame, not
> just any frame in the outer WebContents.

Done.

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:456: // Sets the child
RWHV for this frame, which much be part of an inner
On 2015/06/15 23:00:13, nasko (slow) wrote:
> nit: s/RWHV/RenderWidgetHostView/

Done.

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:72: // WebContents's
FrameTreeNode in outer WebContents's process, then we need a
On 2015/06/15 23:00:13, nasko (slow) wrote:
> nit: s/process/SiteInstance/

Done.

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/972313002/diff/790001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:158: // Inner
WebContents reaches this, so skip the NOTREACHED() check below.
On 2015/06/15 23:00:13, nasko (slow) wrote:
> Missing TODO on how to fix this?

Hmmm, I don't think this needs to be fixed.
Subframe's RWHVChildFrame wouldn't reach this but inner WebContents's
one would because we'd see WebContentsImpl::DidStart/StopLoading().
Updated the comment so it doesn't sound like it is a TODO.

https://codereview.chromium.org/972313002/diff/790001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/972313002/diff/790001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:310: // Do not instruct
the view to create a proxy if this is a swapped out
On 2015/06/15 23:00:13, nasko (slow) wrote:
> I'm trying to kill "swapped out", so let's not add any more usages of it : ).
It
> doesn't bring much additional meaning to the sentence that wouldn't exist
> without it and there isn't actually any check based on the swapped out state
of
> the RenderViewHost.

Good point.
This code path is unused as we do not create RenderViewHost anymore
after switching from RFHM::CreateRenderFrameProxy() to
"new RenderFrameProxyHost()"... in RFHM::CreateOuterDelegateProxy().
So all of this can go away now.
Does this sound right?

https://codereview.chromium.org/972313002/diff/790001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:312: // The proxy will be
created using RFHM::SwapFrameWithProxy().
On 2015/06/15 23:00:13, nasko (slow) wrote:
> There is no SwapFrameWithProxy method anymore.

See above ^^^.

https://codereview.chromium.org/972313002/diff/790001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/972313002/diff/790001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1232: // postMessage to the
inner WebContents.
On 2015/06/15 23:00:13, nasko (slow) wrote:
> nit: Is postMessage the only thing that will use the proxy? Wouldn't it be all
> cross-origin JS/messaging?

We expose this proxy as <webview>.contentWindow, and that one is used
to send postMessage to <webview>.
Can you elaborate a bit about all cross-origin JS/messaging part.
In theory this would do everything a WindowProxy can do IIUC, are you
asking about that?

https://codereview.chromium.org/972313002/diff/790001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1621: // yet.
On 2015/06/15 23:00:13, nasko (slow) wrote:
> Is it meant to?

Yes, we need window.open to work from webview!

https://codereview.chromium.org/972313002/diff/790001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/972313002/diff/790001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:737: // The root is referred to
as "outer WebContents" and the descendents are
On 2015/06/15 23:00:13, nasko (slow) wrote:
> It is not just the root that will be outer WebContents, will it? Any two
> WebContents's that are connected together are outer/inner, no?

That's right, I've changed root -> parent.

https://codereview.chromium.org/972313002/diff/790001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:755: int
outer_contents_frame_tree_node_id_;
On 2015/06/15 23:00:14, nasko (slow) wrote:
> nit: Should this be const? The outer WebContents and the FTN id won't change
for
> the lifetime of this connection, right?

You can removeChild(<webview>) and then appendChild to another local
subframe in the outer WebContents, so this shouldn't be const.

https://codereview.chromium.org/972313002/diff/790001/testing/buildbot/chromi...
File testing/buildbot/chromium.fyi.json (left):

https://codereview.chromium.org/972313002/diff/790001/testing/buildbot/chromi...
testing/buildbot/chromium.fyi.json:3496:
"--gtest_filter=-AppApiTest.*:BlockedAppApiTest.*:BrowserTest.ClearPendingOnFailUnlessNTP:BrowserTest.OtherRedirectsDontForkProcess:BrowserTest.WindowOpenClose:ChromeAppAPITest.*:ChromeRenderProcessHostTest.*:ChromeRenderProcessHostTestWithCommandLine.*:DevToolsExperimentalExtensionTest.*:DevToolsExtensionTest.*:DnsProbeBrowserTest.*:ErrorPageTest.*:ExecuteScriptApiTest.ExecuteScriptPermissions:ExtensionApiTest.ActiveTab:ExtensionApiTest.ChromeRuntimeOpenOptionsPage:ExtensionApiTest.ContentScriptExtensionIframe:ExtensionApiTest.ContentScriptOtherExtensions:ExtensionApiTest.ContentScriptExtensionProcess:ExtensionApiTest.Tabs2:ExtensionApiTest.TabsOnUpdated:ExtensionApiTest.WindowOpenPopupIframe:ExtensionBrowserTest.LoadChromeExtensionsWithOptionsParamWhenEmbedded:ExtensionCrxInstallerTest.InstallDelayedUntilNextUpdate:ExtensionOptionsApiTest.ExtensionCanEmbedOwnOptions:ExtensionWebUITest.CanEmbedExtensionOptions:ExtensionWebUITest.ReceivesExtensionOptionsOnClose:InlineLoginUISafeIframeBrowserTest.*:IsolatedAppTest.*:LaunchWebAuthFlowFunctionTest.*:MimeHandlerViewTest.*:*.NewAvatarMenuEnabledInGuestMode:OptionsUIBrowserTest.*:*PDFExtensionTest.*:PhishingClassifierTest.*:PhishingDOMFeatureExtractorTest.*:PlatformAppUrlRedirectorBrowserTest.*:PopupBlockerBrowserTest.*:PrerenderBrowserTest.*:ProcessManagementTest.*:RedirectTest.*:ReferrerPolicyTest.*:SSLUITest.*:WebNavigationApiTest.CrossProcessFragment:WebNavigationApiTest.ServerRedirectSingleProcess:WebNavigationApiTest.CrossProcessHistory:WebViewCommonTest.*:WebViewDPITest.*:WebViewPluginTest.*:WebViewTest.*:ZoomControllerBrowserTest.*:*.NavigateFromNTPToOptionsInSameTab:*.ProfilesWithoutPagesNotLaunched:*.TabMove:*.WhitelistedExtension:*.NewTabPageURL:*.HomepageLocation:RestoreOnStartupPolicyTest*:PhishingClassifierDelegateTest.*:WebUIWebViewBrowserTest*:WebRtcBrowserTest.RunsAudioVideoWebRTCCallInTwoTabs"
On 2015/06/15 23:06:57, alexmos wrote:
> Random drive-by comment: please fix this for the Site Isolation Win filter
below
> as well.

I was under the impression based on trybots that this isn't used anywhere (:.
I'll update these too from now on. Thanks.

[lazyboy, 2015-06-17 23:43:30]

I've updated the CL to fix the crash I was mentioning
in the mtg yesterday. This should fix the newly
failing <webview> tests. Will send a trybot request.

https://chromiumcodereview.appspot.com/972313002/diff/850001/content/browser/...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/850001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:4019: if
(GetBrowserPluginEmbedder() &&
This is a new change after nasko's swapped out RV change.
This was causing the test crash I mentioned in the mtg yesterday.
The issue is:
We create a inner FTN proxy in outer SiteInstance, which we use to
send postMessage to <webview>. However, when the <webview> sends
a postMessage back to the app, the app tries to create a swapped
out render view again so that the app/outer WebContents can send
postMessage to <webview> [1]. We've already created the proxy in
the beginning for this, so RFPH::OnRouteMessageEvent can just use
that.

[1] This is the call to EnsureOpenerRenderViewsExist() from
RenderFrameProxyHost::OnRouteMessageEvent()
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

[Charlie Reis, 2015-06-18 00:13:17]

Here's some comments from patch set 40.  Largely nits, but I'm concerned about
RemoveOuterDelegateFrame.

Also, we need some tests for the new inner/outer WebContents logic inside
content.  Can you set something up in a browser test or unit test?

Otherwise this seems close.

https://codereview.chromium.org/972313002/diff/830001/components/guest_view/b...
File components/guest_view/browser/guest_view_base.cc (right):

https://codereview.chromium.org/972313002/diff/830001/components/guest_view/b...
components/guest_view/browser/guest_view_base.cc:571: // TODO(lazyboy): This
breaks guest visibility in --site-per-process because
Be sure to keep track of these TODOs for followup CLs, especially if we don't
have bugs filed for them.

https://codereview.chromium.org/972313002/diff/830001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/972313002/diff/830001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:113: // We don't directly
use the proxy to send postMessage in
nit: Drop first "directly"

https://codereview.chromium.org/972313002/diff/830001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:637: // parts of code
from this class should be exctracted to a separate class for
nit: extracted

https://codereview.chromium.org/972313002/diff/830001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/972313002/diff/830001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:46: static const int
kFrameTreeNodeInvalidID = -1;
nit: This should go above static methods.

https://codereview.chromium.org/972313002/diff/830001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/972313002/diff/830001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1706:
render_frame_host->Send(new FrameMsg_SwapOut(
Let's put a comment here (and/or the declaration) about this, since the method
name doesn't indicate that it's going to also do a swap operation.

https://codereview.chromium.org/972313002/diff/830001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/972313002/diff/830001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:181: // this (inner)
WebContents. This FrameTreeNode is the frame that
The second sentence is confusing because of the two "this" references.  Let's
rephrase the whole thing:

If the delegate is an inner WebContents, this method returns the FrameTreeNode
ID of the frame in the outer WebContents which represents the inner WebContents.
Returns FrameTreeNode::kFrameTreeNodeInvalidID if the delegate does not have an
outer WebContents.

(Or s/represents/hosts/?  Which is more accurate?)

https://codereview.chromium.org/972313002/diff/830001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:232: // Returns the
RenderWidgetHost of outer WebContents that can be used to fetch
nit: of the outer WebContents (if any)

https://codereview.chromium.org/972313002/diff/830001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:235: // remote frames.
Ask Ken if there's a bug number we can list for this, since he's working on it
actively.

https://codereview.chromium.org/972313002/diff/830001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:239: // Returns the proxy
to inner WebContents in the outer WebContents's
nit: to the inner

Also, put a blank line above this comment.

https://codereview.chromium.org/972313002/diff/830001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:248: bool
ForInnerDelegate();
nit: Let's move this above GetOuterRenderWidgetHostForKeyboardInput() (both here
and in .cc file), since it basically indicates whether the other methods make
sense to call or not.

https://codereview.chromium.org/972313002/diff/830001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:252: void
RemoveOuterDelegateFrame();
This method doesn't make sense to me.

It looks like it's called from RenderFrameProxyHost::OnDetach() of the guest's
main frame, but we shouldn't be getting OnDetach calls for the main frame. 
Normally we kill the renderer if that happens.  What would cause that to happen
for guests?

Similarly, what would happen if we started returning true from ForInnerDelegate
for subframes?  We wouldn't want that to trigger this.

It also seems like RFHM is the wrong place to be managing this.  Shouldn't the
outer WebContents notice when its inner WebContents goes away?

https://codereview.chromium.org/972313002/diff/830001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:456: //
|render_frame_host| in the outer WebContents
This is confusingly worded.  Can you rephrase it?

It's mixing "called on the RFHM" and "this inner WebContents," and the
SiteInstance vs render_frame_host reference is unclear.

https://codereview.chromium.org/972313002/diff/830001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:460: // Sets the child
RenderWidgetHostView for this frame, which much be part of
s/much/must/

https://codereview.chromium.org/972313002/diff/830001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/972313002/diff/830001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:158: // It is
valid for inner WebContent's SetIsLoading() to end up here.
nit: for an inner WebContents's

Can you add a bit about why this is valid (unlike the default case)?

https://codereview.chromium.org/972313002/diff/830001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/972313002/diff/830001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:317: // child WebContents do
not manage their own lifetime, then we would leak
nit: Let's be consistent about how we pluralize WebContents.

https://codereview.chromium.org/972313002/diff/830001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/972313002/diff/830001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:738: // referred to as "inner
WebContents".
Can you add a bit about whether the outer WebContents has a FrameTreeNode
corresponding to the inner WebContents or not?

https://codereview.chromium.org/972313002/diff/830001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:755: int
outer_contents_frame_tree_node_id_;
This should come after outer_web_contents_, since it's a reference to something
inside it.  

Also, please put comments on these members.

[Fady Samuel, 2015-06-18 04:47:07]

General guestview comment: Whenever things can be generalized across GuestView
types, please place in components.

https://codereview.chromium.org/972313002/diff/870001/extensions/browser/gues...
File extensions/browser/guest_view/extensions_guest_view_message_filter.cc
(right):

https://codereview.chromium.org/972313002/diff/870001/extensions/browser/gues...
extensions/browser/guest_view/extensions_guest_view_message_filter.cc:101: auto*
guest = WebViewGuest::FromWebContents(guest_web_contents);
Why do this for WebViewGuest  and not GuestViewBase?

Also, why is this code in extensions? This seems like it applies to all
GuestView types. It seems like this could be in components...

https://codereview.chromium.org/972313002/diff/870001/extensions/renderer/gue...
File extensions/renderer/guest_view/extensions_iframe_guest_view_container.cc
(right):

https://codereview.chromium.org/972313002/diff/870001/extensions/renderer/gue...
extensions/renderer/guest_view/extensions_iframe_guest_view_container.cc:15:
ExtensionsIframeGuestViewContainer::ExtensionsIframeGuestViewContainer(
Why is this in extensions? It seems like this could be in components...

https://codereview.chromium.org/972313002/diff/870001/extensions/renderer/gue...
File extensions/renderer/guest_view/extensions_iframe_guest_view_request.h
(right):

https://codereview.chromium.org/972313002/diff/870001/extensions/renderer/gue...
extensions/renderer/guest_view/extensions_iframe_guest_view_request.h:18: class
GuestViewAttachIframeRequest : public guest_view::GuestViewRequest {
Move to components?

[lazyboy, 2015-06-18 22:45:13]

Charlie:
I tried to comment on your RemoveOuterDelegateFrame() concern.
Let me know if that's enough.
I'll think of adding some unit tests to the new functions in RFHM and
WebContents.

Fady:
I'll start moving the code to components/. I'm not entirely sure I'm
just ready to apply things to GuestViewBase instead of WebViewGuest yet...
Will see.

https://chromiumcodereview.appspot.com/972313002/diff/830001/components/guest...
File components/guest_view/browser/guest_view_base.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/830001/components/guest...
components/guest_view/browser/guest_view_base.cc:571: // TODO(lazyboy): This
breaks guest visibility in --site-per-process because
On 2015/06/18 00:13:18, Charlie Reis (OOO soon) wrote:
> Be sure to keep track of these TODOs for followup CLs, especially if we don't
> have bugs filed for them.

OK. I'm tracking them on the shared doc.

https://chromiumcodereview.appspot.com/972313002/diff/830001/content/browser/...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/830001/content/browser/...
content/browser/browser_plugin/browser_plugin_guest.cc:113: // We don't directly
use the proxy to send postMessage in
On 2015/06/18 00:13:18, Charlie Reis (OOO soon) wrote:
> nit: Drop first "directly"

Done.

https://chromiumcodereview.appspot.com/972313002/diff/830001/content/browser/...
content/browser/browser_plugin/browser_plugin_guest.cc:637: // parts of code
from this class should be exctracted to a separate class for
On 2015/06/18 00:13:18, Charlie Reis (OOO soon) wrote:
> nit: extracted

Done.

https://chromiumcodereview.appspot.com/972313002/diff/830001/content/browser/...
File content/browser/frame_host/frame_tree_node.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/830001/content/browser/...
content/browser/frame_host/frame_tree_node.h:46: static const int
kFrameTreeNodeInvalidID = -1;
On 2015/06/18 00:13:18, Charlie Reis (OOO soon) wrote:
> nit: This should go above static methods.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/830001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/830001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:1706:
render_frame_host->Send(new FrameMsg_SwapOut(
On 2015/06/18 00:13:18, Charlie Reis (OOO soon) wrote:
> Let's put a comment here (and/or the declaration) about this, since the method
> name doesn't indicate that it's going to also do a swap operation.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/830001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/830001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:181: // this (inner)
WebContents. This FrameTreeNode is the frame that
On 2015/06/18 00:13:18, Charlie Reis (OOO soon) wrote:
> The second sentence is confusing because of the two "this" references.  Let's
> rephrase the whole thing:
> 
> If the delegate is an inner WebContents, this method returns the FrameTreeNode
> ID of the frame in the outer WebContents which represents the inner
WebContents.
> Returns FrameTreeNode::kFrameTreeNodeInvalidID if the delegate does not have
an
> outer WebContents.
Done.
> 
> (Or s/represents/hosts/?  Which is more accurate?)
Not sure, "hosts" seems fine.

https://chromiumcodereview.appspot.com/972313002/diff/830001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:232: // Returns the
RenderWidgetHost of outer WebContents that can be used to fetch
On 2015/06/18 00:13:18, Charlie Reis (OOO soon) wrote:
> nit: of the outer WebContents (if any)

Done.

https://chromiumcodereview.appspot.com/972313002/diff/830001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:235: // remote frames.
On 2015/06/18 00:13:18, Charlie Reis (OOO soon) wrote:
> Ask Ken if there's a bug number we can list for this, since he's working on it
> actively.

Ken's OOO, will do when he comes back. Added note in the docs so I don't forget.

https://chromiumcodereview.appspot.com/972313002/diff/830001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:239: // Returns the proxy
to inner WebContents in the outer WebContents's
On 2015/06/18 00:13:18, Charlie Reis (OOO soon) wrote:
> nit: to the inner
> 
> Also, put a blank line above this comment.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/830001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:248: bool
ForInnerDelegate();
On 2015/06/18 00:13:18, Charlie Reis (OOO soon) wrote:
> nit: Let's move this above GetOuterRenderWidgetHostForKeyboardInput() (both
here
> and in .cc file), since it basically indicates whether the other methods make
> sense to call or not.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/830001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:252: void
RemoveOuterDelegateFrame();
On 2015/06/18 00:13:18, Charlie Reis (OOO soon) wrote:
> This method doesn't make sense to me.
> 
> It looks like it's called from RenderFrameProxyHost::OnDetach() of the guest's
> main frame, but we shouldn't be getting OnDetach calls for the main frame. 
> Normally we kill the renderer if that happens.  What would cause that to
happen
> for guests?
[1]
> 
> Similarly, what would happen if we started returning true from
ForInnerDelegate
> for subframes?  We wouldn't want that to trigger this.
Subframes in <webview> would require some work later on anyway, we can fix this
when we fix subframes.
> 
> It also seems like RFHM is the wrong place to be managing this.  Shouldn't the
> outer WebContents notice when its inner WebContents goes away?


For removeChild, i.e. <webview>.parentNode.removeChild(<webview>),
the inner WebContents won't just go away, the first thing we notice is
the frame (which is a proxy after swap) in the embedder getting detached.
In this case this is the FTN/RFHM that is hosting the guest main frame but is
not a main frame itself. So the OnDetach is not getting called for main frame
[1].

https://chromiumcodereview.appspot.com/972313002/diff/830001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:456: //
|render_frame_host| in the outer WebContents
On 2015/06/18 00:13:18, Charlie Reis (OOO soon) wrote:
> This is confusingly worded.  Can you rephrase it?
> 
> It's mixing "called on the RFHM" and "this inner WebContents,"
I just dropped the "for this inner Webcontent's frame", cause "called on the
RFHM of the inner WC" already says that.

> SiteInstance vs render_frame_host reference is unclear.

Reworded the sentence a bit.

https://chromiumcodereview.appspot.com/972313002/diff/830001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:460: // Sets the child
RenderWidgetHostView for this frame, which much be part of
On 2015/06/18 00:13:18, Charlie Reis (OOO soon) wrote:
> s/much/must/

Done.

https://chromiumcodereview.appspot.com/972313002/diff/830001/content/browser/...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/830001/content/browser/...
content/browser/frame_host/render_widget_host_view_child_frame.cc:158: // It is
valid for inner WebContent's SetIsLoading() to end up here.
On 2015/06/18 00:13:18, Charlie Reis (OOO soon) wrote:
> nit: for an inner WebContents's
Done.
> 
> Can you add a bit about why this is valid (unlike the default case)?
OK.

https://chromiumcodereview.appspot.com/972313002/diff/830001/content/browser/...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/830001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:317: // child WebContents do
not manage their own lifetime, then we would leak
On 2015/06/18 00:13:18, Charlie Reis (OOO soon) wrote:
> nit: Let's be consistent about how we pluralize WebContents.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/830001/content/browser/...
File content/browser/web_contents/web_contents_impl.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/830001/content/browser/...
content/browser/web_contents/web_contents_impl.h:738: // referred to as "inner
WebContents".
On 2015/06/18 00:13:18, Charlie Reis (OOO soon) wrote:
> Can you add a bit about whether the outer WebContents has a FrameTreeNode
> corresponding to the inner WebContents or not?

Done.

https://chromiumcodereview.appspot.com/972313002/diff/830001/content/browser/...
content/browser/web_contents/web_contents_impl.h:755: int
outer_contents_frame_tree_node_id_;
On 2015/06/18 00:13:19, Charlie Reis (OOO soon) wrote:
> This should come after outer_web_contents_, since it's a reference to
something
> inside it.  
> 
> Also, please put comments on these members.

Done.

[lazyboy, 2015-06-22 19:41:58]

Patchset #44 (id:910001) has been deleted

[lazyboy, 2015-06-22 19:43:21]

https://chromiumcodereview.appspot.com/972313002/diff/870001/extensions/brows...
File extensions/browser/guest_view/extensions_guest_view_message_filter.cc
(right):

https://chromiumcodereview.appspot.com/972313002/diff/870001/extensions/brows...
extensions/browser/guest_view/extensions_guest_view_message_filter.cc:101: auto*
guest = WebViewGuest::FromWebContents(guest_web_contents);
On 2015/06/18 04:47:07, Fady Samuel wrote:
> Why do this for WebViewGuest  and not GuestViewBase?
> 
> Also, why is this code in extensions? This seems like it applies to all
> GuestView types. It seems like this could be in components...

Done.

https://chromiumcodereview.appspot.com/972313002/diff/870001/extensions/rende...
File extensions/renderer/guest_view/extensions_iframe_guest_view_container.cc
(right):

https://chromiumcodereview.appspot.com/972313002/diff/870001/extensions/rende...
extensions/renderer/guest_view/extensions_iframe_guest_view_container.cc:15:
ExtensionsIframeGuestViewContainer::ExtensionsIframeGuestViewContainer(
On 2015/06/18 04:47:07, Fady Samuel wrote:
> Why is this in extensions? It seems like this could be in components...

Done.

https://chromiumcodereview.appspot.com/972313002/diff/870001/extensions/rende...
File extensions/renderer/guest_view/extensions_iframe_guest_view_request.h
(right):

https://chromiumcodereview.appspot.com/972313002/diff/870001/extensions/rende...
extensions/renderer/guest_view/extensions_iframe_guest_view_request.h:18: class
GuestViewAttachIframeRequest : public guest_view::GuestViewRequest {
On 2015/06/18 04:47:07, Fady Samuel wrote:
> Move to components?

Done.

[nasko, 2015-06-23 13:39:23]

LGTM with nits

https://codereview.chromium.org/972313002/diff/930001/content/browser/frame_h...
File content/browser/frame_host/cross_process_frame_connector.cc (right):

https://codereview.chromium.org/972313002/diff/930001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.cc:149: if
(frame_proxy_in_parent_renderer_->frame_tree_node()
nit: Can you put a comment on why we are returning early here? It isn't
immediately obvious.

https://codereview.chromium.org/972313002/diff/930001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/972313002/diff/930001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:201: void
RenderFrameHostManager::RemoveOuterDelegateFrame() {
I also feel this method doesn't quite belong in RFHM, as it is WebContents that
gets linked and unlinked. However, this means adding specific method for this to
the delegate interface, which isn't great either and other code that deals with
linking is here. I'll defer to Charlie on this one.

https://codereview.chromium.org/972313002/diff/930001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1708: false /*
is_loading */, FrameReplicationState()));
nit: Do we still need a TODO for the is_loading?

https://codereview.chromium.org/972313002/diff/930001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/972313002/diff/930001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:460: // be swapped out by
the proxy.
nit: s/out/with/

https://codereview.chromium.org/972313002/diff/930001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/972313002/diff/930001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:159: // This
is because an inner WebContents's main frame's widget host view is a
nit: Let's avoid using "widget host view" and use the actual object name. It is
not as pretty because of the longish string, but it is actually more accurate
and we should aim for that.

[Fady Samuel, 2015-06-23 14:33:01]

https://codereview.chromium.org/972313002/diff/930001/components/guest_view/c...
File components/guest_view/common/guest_view_messages.h (right):

https://codereview.chromium.org/972313002/diff/930001/components/guest_view/c...
components/guest_view/common/guest_view_messages.h:43: // to a <webview>. This
message will attach the local frame to the <webview>
nit: avoid references to <webview>.

https://codereview.chromium.org/972313002/diff/930001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.h (left):

https://codereview.chromium.org/972313002/diff/930001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.h:82: class CONTENT_EXPORT
BrowserPluginGuest : public GuestHost,
Could we refactor this then?

GuestHostImpl + GuestHostDelegate where (BrowserPluginGuest is a
GuestHostDelegate)?

[lazyboy, 2015-06-23 22:59:19]

Patchset #45 (id:950001) has been deleted

[lazyboy, 2015-06-23 23:00:00]

https://chromiumcodereview.appspot.com/972313002/diff/930001/components/guest...
File components/guest_view/common/guest_view_messages.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/930001/components/guest...
components/guest_view/common/guest_view_messages.h:43: // to a <webview>. This
message will attach the local frame to the <webview>
On 2015/06/23 14:33:00, Fady Samuel wrote:
> nit: avoid references to <webview>.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/930001/content/browser/...
File content/browser/browser_plugin/browser_plugin_guest.h (left):

https://chromiumcodereview.appspot.com/972313002/diff/930001/content/browser/...
content/browser/browser_plugin/browser_plugin_guest.h:82: class CONTENT_EXPORT
BrowserPluginGuest : public GuestHost,
On 2015/06/23 14:33:01, Fady Samuel wrote:
> Could we refactor this then?
> 
> GuestHostImpl + GuestHostDelegate where (BrowserPluginGuest is a
> GuestHostDelegate)?

I'd like to do that in a follow up CL.

https://chromiumcodereview.appspot.com/972313002/diff/930001/content/browser/...
File content/browser/frame_host/cross_process_frame_connector.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/930001/content/browser/...
content/browser/frame_host/cross_process_frame_connector.cc:149: if
(frame_proxy_in_parent_renderer_->frame_tree_node()
On 2015/06/23 13:39:23, nasko (paris) wrote:
> nit: Can you put a comment on why we are returning early here? It isn't
> immediately obvious.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/930001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/930001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:201: void
RenderFrameHostManager::RemoveOuterDelegateFrame() {
On 2015/06/23 13:39:23, nasko (paris) wrote:
> I also feel this method doesn't quite belong in RFHM, as it is WebContents
that
> gets linked and unlinked. However, this means adding specific method for this
to
> the delegate interface, which isn't great either and other code that deals
with
> linking is here. 
I've added a TODO for this to move out of RFHM.

> I'll defer to Charlie on this one.
Charlie is away next 2 weeks... So we should keep this as is and attempt to land
and come back to the TODO above when Charlie comes back.

https://chromiumcodereview.appspot.com/972313002/diff/930001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:1708: false /*
is_loading */, FrameReplicationState()));
On 2015/06/23 13:39:23, nasko (paris) wrote:
> nit: Do we still need a TODO for the is_loading?

Done.

https://chromiumcodereview.appspot.com/972313002/diff/930001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/930001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:460: // be swapped out by
the proxy.
On 2015/06/23 13:39:23, nasko (paris) wrote:
> nit: s/out/with/

Done.

https://chromiumcodereview.appspot.com/972313002/diff/930001/content/browser/...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/930001/content/browser/...
content/browser/frame_host/render_widget_host_view_child_frame.cc:159: // This
is because an inner WebContents's main frame's widget host view is a
On 2015/06/23 13:39:23, nasko (paris) wrote:
> nit: Let's avoid using "widget host view" and use the actual object name. It
is
> not as pretty because of the longish string, but it is actually more accurate
> and we should aim for that.

Done.

[Fady Samuel, 2015-06-24 22:01:10]

LGTM + nits

https://codereview.chromium.org/972313002/diff/970001/components/guest_view/b...
File components/guest_view/browser/guest_view_message_filter.cc (right):

https://codereview.chromium.org/972313002/diff/970001/components/guest_view/b...
components/guest_view/browser/guest_view_message_filter.cc:127:
DCHECK(guest_web_contents);
I would consider CHECK'ing here. If the above fails, then the embedder is likely
malicious. Alternatively exit early, as the embedder will be killed anyway.

https://codereview.chromium.org/972313002/diff/970001/components/guest_view/b...
components/guest_view/browser/guest_view_message_filter.cc:134:
guest_web_contents->AttachToOuterWebContentsFrame(owner_web_contents,
Comment above this explaining briefly what it does.

https://codereview.chromium.org/972313002/diff/970001/components/guest_view/b...
components/guest_view/browser/guest_view_message_filter.cc:137:
manager->AttachGuest(render_process_id_, element_instance_id,
Comment above this explaining briefly what it does. Two calls to "Attach*" sound
similar, it's useful to document the differences.

https://codereview.chromium.org/972313002/diff/970001/components/guest_view/b...
components/guest_view/browser/guest_view_message_filter.cc:145:
base::Bind(&GuestViewMessageFilter::WillAttachCallback, this, guest));
nit: Can you bind directly?  base::Bind(&GuestViewBase::DidAttach, guest,
MSG_ROUTING_NONE)?

https://codereview.chromium.org/972313002/diff/970001/components/guest_view/r...
File components/guest_view/renderer/iframe_guest_view_container.h (right):

https://codereview.chromium.org/972313002/diff/970001/components/guest_view/r...
components/guest_view/renderer/iframe_guest_view_container.h:12: class
IframeGuestViewContainer : public GuestViewContainer {
nit: Description of this class and what it does that's unique to iframes.

https://codereview.chromium.org/972313002/diff/970001/components/guest_view/r...
File components/guest_view/renderer/iframe_guest_view_request.h (right):

https://codereview.chromium.org/972313002/diff/970001/components/guest_view/r...
components/guest_view/renderer/iframe_guest_view_request.h:15: class
GuestViewAttachIframeRequest : public guest_view::GuestViewRequest {
nit: Add a description of this class and what it does.

[lazyboy, 2015-06-24 23:01:51]

https://chromiumcodereview.appspot.com/972313002/diff/970001/components/guest...
File components/guest_view/browser/guest_view_message_filter.cc (right):

https://chromiumcodereview.appspot.com/972313002/diff/970001/components/guest...
components/guest_view/browser/guest_view_message_filter.cc:127:
DCHECK(guest_web_contents);
On 2015/06/24 22:01:10, Fady Samuel wrote:
> I would consider CHECK'ing here. If the above fails, then the embedder is
likely
> malicious. Alternatively exit early, as the embedder will be killed anyway.

I'm doing early exit.
Done.

https://chromiumcodereview.appspot.com/972313002/diff/970001/components/guest...
components/guest_view/browser/guest_view_message_filter.cc:134:
guest_web_contents->AttachToOuterWebContentsFrame(owner_web_contents,
On 2015/06/24 22:01:10, Fady Samuel wrote:
> Comment above this explaining briefly what it does.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/970001/components/guest...
components/guest_view/browser/guest_view_message_filter.cc:137:
manager->AttachGuest(render_process_id_, element_instance_id,
On 2015/06/24 22:01:10, Fady Samuel wrote:
> Comment above this explaining briefly what it does. Two calls to "Attach*"
sound
> similar, it's useful to document the differences.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/970001/components/guest...
components/guest_view/browser/guest_view_message_filter.cc:145:
base::Bind(&GuestViewMessageFilter::WillAttachCallback, this, guest));
On 2015/06/24 22:01:10, Fady Samuel wrote:
> nit: Can you bind directly?  base::Bind(&GuestViewBase::DidAttach, guest,
> MSG_ROUTING_NONE)?

passing |guest| as |this| wouldn't work as |guest| is a raw GuestViewBase
pointer.

https://chromiumcodereview.appspot.com/972313002/diff/970001/components/guest...
File components/guest_view/renderer/iframe_guest_view_container.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/970001/components/guest...
components/guest_view/renderer/iframe_guest_view_container.h:12: class
IframeGuestViewContainer : public GuestViewContainer {
On 2015/06/24 22:01:10, Fady Samuel wrote:
> nit: Description of this class and what it does that's unique to iframes.

Done.

https://chromiumcodereview.appspot.com/972313002/diff/970001/components/guest...
File components/guest_view/renderer/iframe_guest_view_request.h (right):

https://chromiumcodereview.appspot.com/972313002/diff/970001/components/guest...
components/guest_view/renderer/iframe_guest_view_request.h:15: class
GuestViewAttachIframeRequest : public guest_view::GuestViewRequest {
On 2015/06/24 22:01:10, Fady Samuel wrote:
> nit: Add a description of this class and what it does.

Done.

[lazyboy, 2015-06-29 14:55:05]

FYI, I'm going to CQ this one soon, the bots seems to be happy about the CL.

[lazyboy, 2015-06-29 15:13:19]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2015-06-29 15:13:19]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org,
fsamuel@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/972313002/#ps1060001 (title: "sync")

[commit-bot: I haz the power, 2015-06-29 15:13:30]

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

[commit-bot: I haz the power, 2015-06-29 15:18:22]

Committed patchset #50 (id:1060001)

[commit-bot: I haz the power, 2015-06-29 15:19:30]

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