Support frame tree propagation between renderers in the same browsing instance.

Support frame tree propagation between renderers in the same browsing instance.


BUG=128767
TEST=


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

[awong, 2012-08-20 21:56:31]

High-level comment is that I don't see any place with overview documentation
explaining how/when these messages are sent all around.

I worry that future maintainers a few years down the line won't be able to
easily evaluate:
  1) What JSON format is correct for frame_tree_
  2) When the events are sent
  3) Who should receive the events and why they should receive them.

https://chromiumcodereview.appspot.com/10827078/diff/5038/content/browser/ren...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10827078/diff/5038/content/browser/ren...
content/browser/renderer_host/render_view_host_impl.cc:1653: const std::string&
RenderViewHostImpl::GetFrameTree() {
Should this method be const?

https://chromiumcodereview.appspot.com/10827078/diff/5038/content/browser/ren...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://chromiumcodereview.appspot.com/10827078/diff/5038/content/browser/ren...
content/browser/renderer_host/render_view_host_impl.h:407: void
UpdateFrameTree(int process_id,
Note, move this up to under SendOrientatinChangeEvent() so we don't split apart
all the accessibility functions?

https://chromiumcodereview.appspot.com/10827078/diff/5038/content/browser/ren...
content/browser/renderer_host/render_view_host_impl.h:648: // Serialized frame
tree structure for the page displayed by the render view.
Add something that describes what format this is expected to be in?

https://chromiumcodereview.appspot.com/10827078/diff/5038/content/browser/ren...
File content/browser/renderer_host/render_view_host_manager_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10827078/diff/5038/content/browser/ren...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1092:
ListValue* subtree1;
Pointer OCDness...can we initialize these things to NULL even though you're
assigning right below?

[nasko, 2012-08-21 00:34:45]

I've added some documentation as per Albert's comment and fixed the rest of the
issues.

I've also added an explicit test for frame addition/removal initiated by
JavaScript.

https://chromiumcodereview.appspot.com/10827078/diff/5038/content/browser/ren...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10827078/diff/5038/content/browser/ren...
content/browser/renderer_host/render_view_host_impl.cc:1653: const std::string&
RenderViewHostImpl::GetFrameTree() {
On 2012/08/20 21:56:31, awong wrote:
> Should this method be const?

Done.

https://chromiumcodereview.appspot.com/10827078/diff/5038/content/browser/ren...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://chromiumcodereview.appspot.com/10827078/diff/5038/content/browser/ren...
content/browser/renderer_host/render_view_host_impl.h:407: void
UpdateFrameTree(int process_id,
On 2012/08/20 21:56:31, awong wrote:
> Note, move this up to under SendOrientatinChangeEvent() so we don't split
apart
> all the accessibility functions?

Done.

https://chromiumcodereview.appspot.com/10827078/diff/5038/content/browser/ren...
content/browser/renderer_host/render_view_host_impl.h:648: // Serialized frame
tree structure for the page displayed by the render view.
On 2012/08/20 21:56:31, awong wrote:
> Add something that describes what format this is expected to be in?

Done.

https://chromiumcodereview.appspot.com/10827078/diff/5038/content/browser/ren...
File content/browser/renderer_host/render_view_host_manager_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10827078/diff/5038/content/browser/ren...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1092:
ListValue* subtree1;
On 2012/08/20 21:56:31, awong wrote:
> Pointer OCDness...can we initialize these things to NULL even though you're
> assigning right below?

Done.

[Charlie Reis, 2012-08-21 01:40:39]

Great.  I've only just started the review, but these are mostly standalone
comments that I can send before getting to the rest tomorrow.

https://chromiumcodereview.appspot.com/10827078/diff/18005/content/common/vie...
File content/common/view_messages.h (right):

https://chromiumcodereview.appspot.com/10827078/diff/18005/content/common/vie...
content/common/view_messages.h:576: // MSG_ROUTING_NONE if the source frame
isn't supported (e.g., subframes).
The "e.g., subframes" part is stale now.  Is there ever a case where we'll set
it to MSG_ROUTING_NONE now?

https://chromiumcodereview.appspot.com/10827078/diff/18005/content/common/vie...
content/common/view_messages.h:580: IPC_STRUCT_MEMBER(int, remote_process_id)
These need a comment explaining why they're here.  It's not immediately obvious
that the renderer should have this information (without our previous
discussion), rather than just sending up the destination frame ID from the
current renderer process and letting the browser translate it.

Also, perhaps rename them to dest_ instead of remote_ (to be consistent with
source_).

https://chromiumcodereview.appspot.com/10827078/diff/18005/content/common/vie...
content/common/view_messages.h:1047: IPC_MESSAGE_ROUTED3(ViewMsg_FrameTree,
Should probably include some kind of verb to make it clear what this is for. 
ViewMsg_UpdateFrameTree?  (This is an instruction from the browser to the
renderer, right?)

https://chromiumcodereview.appspot.com/10827078/diff/18005/content/common/vie...
content/common/view_messages.h:1048: int, /* the child process id of the source
renderer */
Maybe say "active" instead of "source"?  (I was confusing this with
postMessage's source/dest.)

https://chromiumcodereview.appspot.com/10827078/diff/18005/content/common/vie...
content/common/view_messages.h:2208: IPC_MESSAGE_ROUTED1(ViewHostMsg_FrameTree,
As above, ViewHostMsg_FrameTreeUpdated.  Also, maybe mention "from the active
renderer process" so that it's clear swapped out RenderViews won't be sending
this.

https://chromiumcodereview.appspot.com/10827078/diff/18005/content/public/bro...
File content/public/browser/render_view_host.h (right):

https://chromiumcodereview.appspot.com/10827078/diff/18005/content/public/bro...
content/public/browser/render_view_host.h:244: virtual const std::string&
GetFrameTree() const = 0;
Is this ever needed outside the content module?  It seems like an implementation
detail we could probably keep in RenderViewHostImpl.

[nasko, 2012-08-21 17:49:42]

Changes based on initial comments.

https://chromiumcodereview.appspot.com/10827078/diff/18005/content/common/vie...
File content/common/view_messages.h (right):

https://chromiumcodereview.appspot.com/10827078/diff/18005/content/common/vie...
content/common/view_messages.h:576: // MSG_ROUTING_NONE if the source frame
isn't supported (e.g., subframes).
On 2012/08/21 01:40:39, creis wrote:
> The "e.g., subframes" part is stale now.  Is there ever a case where we'll set
> it to MSG_ROUTING_NONE now?

It is still technically possible (in code) to return MSG_ROUTING_NONE, since
FromWebView can return NULL. I'm not sure if there is ever a case where
FromWebView will fail. I've removed the last sentence, but I'd like to
understand the behavior here.

https://chromiumcodereview.appspot.com/10827078/diff/18005/content/common/vie...
content/common/view_messages.h:580: IPC_STRUCT_MEMBER(int, remote_process_id)
On 2012/08/21 01:40:39, creis wrote:
> These need a comment explaining why they're here.  It's not immediately
obvious
> that the renderer should have this information (without our previous
> discussion), rather than just sending up the destination frame ID from the
> current renderer process and letting the browser translate it.
> 
> Also, perhaps rename them to dest_ instead of remote_ (to be consistent with
> source_).

Actually, I've added the description to the ViewMsg_UpdateFrameTree IPC message,
since the same set of parameters will likely be present in other JS calls being
routed. I've also made it consistent with the post message parameters and called
it target_ instead of remote_.

https://chromiumcodereview.appspot.com/10827078/diff/18005/content/common/vie...
content/common/view_messages.h:1047: IPC_MESSAGE_ROUTED3(ViewMsg_FrameTree,
On 2012/08/21 01:40:39, creis wrote:
> Should probably include some kind of verb to make it clear what this is for. 
> ViewMsg_UpdateFrameTree?  (This is an instruction from the browser to the
> renderer, right?)

Done.

https://chromiumcodereview.appspot.com/10827078/diff/18005/content/common/vie...
content/common/view_messages.h:1048: int, /* the child process id of the source
renderer */
On 2012/08/21 01:40:39, creis wrote:
> Maybe say "active" instead of "source"?  (I was confusing this with
> postMessage's source/dest.)

Done.

https://chromiumcodereview.appspot.com/10827078/diff/18005/content/common/vie...
content/common/view_messages.h:2208: IPC_MESSAGE_ROUTED1(ViewHostMsg_FrameTree,
On 2012/08/21 01:40:39, creis wrote:
> As above, ViewHostMsg_FrameTreeUpdated.  Also, maybe mention "from the active
> renderer process" so that it's clear swapped out RenderViews won't be sending
> this.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/18005/content/public/bro...
File content/public/browser/render_view_host.h (right):

https://chromiumcodereview.appspot.com/10827078/diff/18005/content/public/bro...
content/public/browser/render_view_host.h:244: virtual const std::string&
GetFrameTree() const = 0;
On 2012/08/21 01:40:39, creis wrote:
> Is this ever needed outside the content module?  It seems like an
implementation
> detail we could probably keep in RenderViewHostImpl.

Done.

[awong, 2012-08-21 19:27:28]

Mostly nits.

I don't see a place that explains the overarching explanation for when the
frametree updates are sent, and why we are sending them.

My litmus test is question: if a new engineer joined our team and we told there
there's a bug in "frame tree updates", where's the comment I'd point them that
explains what these things are, why they are here, and how we roughly expect
them to work so they could start validating the system?

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/ren...
File content/browser/renderer_host/render_view_host_manager_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/ren...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1076:
namespace {
Anonymous namespaces should go at the top of the file.

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/ren...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1122:
EXPECT_FALSE(frame_tree.empty());
Failing any of these EXPECTs feels like it should abort the rest of the TEST_F. 
Maybe use ASSERT_ here paired with:
 
http://code.google.com/p/googletest/wiki/AdvancedGuide#Asserting_on_Subroutines

You'd need to make the DictionaryValue an output parameter.

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/ren...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1133: //
same browsing context.
Can we add a few brief sentences describing the basic framework of the test? 
Eg., create a page that's swapped out, update its frame tree, then verify that
XXX occurs.

I'm lazy and don't just want to read the whole test script. :)

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/web...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/web...
content/browser/web_contents/render_view_host_manager.cc:244:
DCHECK(render_view_host == current_host());
DCHECK_EQ?

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/web...
content/browser/web_contents/render_view_host_manager.cc:246:
RenderViewHostImpl* rvh = static_cast<RenderViewHostImpl*>(render_view_host);
Shortenings like rvh aren't commonly used in this file.

Maybe render_view_host_impl? (yeah, it's longer :( )

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/web...
content/browser/web_contents/render_view_host_manager.cc:247: if (!rvh)
For parameter validation, do this as close to the top as possible (before the
cast even, but probably after the DCHECK which is enforcing a stronger
invariant).

However, I would have expected this function to never be called if rvh is NULL. 
When could rvh be NULL in normal operation?  If that's not supposed to happen,
then we should crash rather than try to silently recover and mask bad behavior.

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/web...
content/browser/web_contents/render_view_host_manager.cc:253:
DCHECK(iter->second->GetSiteInstance() !=
DCHECK_NE?

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/web...
content/browser/web_contents/render_view_host_manager.cc:517: // If we are
navigating from blank SiteInstance to WebUI, make sure we
from blank -> from a blank
to WebUI -> to a WebUI.=

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/web...
content/browser/web_contents/render_view_host_manager.cc:518: // create a new
one.
"new one" is ambiguous. "new SiteInstance" is clearer.

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/web...
content/browser/web_contents/render_view_host_manager.cc:522:
web_ui_factory->UseWebUIForURL(browser_context, dest_url))
Use braces around the body of a conditional that has multiple lines in either
the condition or the statement.

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/web...
content/browser/web_contents/render_view_host_manager.cc:612: // active one are
tracked down and resolved.
File a bug for this?

Also, I can't tell how severe this behavior is. In this comment and/or bug, can
we describe what is the effect of having this workaround?

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/web...
content/browser/web_contents/render_view_host_manager.cc:613: if (swapped_out) {
remove brace here :)

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

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/web...
content/browser/web_contents/web_contents_impl.cc:2851:
active_rvh->GetProcess()->GetID();
This log isn't going to be seen by anyone. If we need to track this, maybe we
should register a UMA stat (I've never done that before).  Charlie, any
thoughts?

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/web...
content/browser/web_contents/web_contents_impl.cc:2854:
DCHECK(params.target_frame_id != 0);
If it is 0, what happens below?

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/common/stri...
File content/common/string_constants.h (right):

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/common/stri...
content/common/string_constants.h:5: #ifndef CONTENT_COMMON_STRING_CONSTANTS_H_
string_constants is too generic.  We should name it something like FrameTree
constants, or render view constants, or something indicates the types of
constants that should be in here.

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/common/stri...
content/common/string_constants.h:10: extern const char kFrameTreeNodeNameKey[];
Add comment referring to location that describes why these constants exist
(probably the comment block in the ipc message)

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/common/view...
File content/common/view_messages.h (right):

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/common/view...
content/common/view_messages.h:1056: // response to this particular message.
Describe the json format here. Then in other spots that store the string and/or
generate the tree, refer to this comment.

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/renderer/re...
File content/renderer/render_view_impl.h (right):

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/renderer/re...
content/renderer/render_view_impl.h:1057: // of each frame.
This comment seems out of date.  There's no return value.

Also, where is the serialization format defined?

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/renderer/re...
content/renderer/render_view_impl.h:1465: // been sent to the browser due to
loading a resource.
due to loading a resource -> because a resource is loading.

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/renderer/re...
content/renderer/render_view_impl.h:1468: // If this render view is swapped out
mirror of an active render view in a
is swapped -> is a swapped

Can we add a bit explaining why we record this?

[nasko, 2012-08-21 22:24:12]

Fixes for almost all comments. One of the test issues I'll do later and one was
a generic comment.

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/ren...
File content/browser/renderer_host/render_view_host_manager_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/ren...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1076:
namespace {
On 2012/08/21 19:27:28, awong wrote:
> Anonymous namespaces should go at the top of the file.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/ren...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1133: //
same browsing context.
On 2012/08/21 19:27:28, awong wrote:
> Can we add a few brief sentences describing the basic framework of the test? 
> Eg., create a page that's swapped out, update its frame tree, then verify that
> XXX occurs.
> 
> I'm lazy and don't just want to read the whole test script. :)

Done.

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/web...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/web...
content/browser/web_contents/render_view_host_manager.cc:244:
DCHECK(render_view_host == current_host());
On 2012/08/21 19:27:28, awong wrote:
> DCHECK_EQ?

Done.

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/web...
content/browser/web_contents/render_view_host_manager.cc:246:
RenderViewHostImpl* rvh = static_cast<RenderViewHostImpl*>(render_view_host);
On 2012/08/21 19:27:28, awong wrote:
> Shortenings like rvh aren't commonly used in this file.
> 
> Maybe render_view_host_impl? (yeah, it's longer :( )

Done.

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/web...
content/browser/web_contents/render_view_host_manager.cc:247: if (!rvh)
On 2012/08/21 19:27:28, awong wrote:
> For parameter validation, do this as close to the top as possible (before the
> cast even, but probably after the DCHECK which is enforcing a stronger
> invariant).
> 
> However, I would have expected this function to never be called if rvh is
NULL. 
> When could rvh be NULL in normal operation?  If that's not supposed to happen,
> then we should crash rather than try to silently recover and mask bad
behavior.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/web...
content/browser/web_contents/render_view_host_manager.cc:253:
DCHECK(iter->second->GetSiteInstance() !=
On 2012/08/21 19:27:28, awong wrote:
> DCHECK_NE?

Done.

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/web...
content/browser/web_contents/render_view_host_manager.cc:517: // If we are
navigating from blank SiteInstance to WebUI, make sure we
On 2012/08/21 19:27:28, awong wrote:
> from blank -> from a blank
> to WebUI -> to a WebUI.=

Done.

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/web...
content/browser/web_contents/render_view_host_manager.cc:518: // create a new
one.
On 2012/08/21 19:27:28, awong wrote:
> "new one" is ambiguous. "new SiteInstance" is clearer.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/web...
content/browser/web_contents/render_view_host_manager.cc:522:
web_ui_factory->UseWebUIForURL(browser_context, dest_url))
On 2012/08/21 19:27:28, awong wrote:
> Use braces around the body of a conditional that has multiple lines in either
> the condition or the statement.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/web...
content/browser/web_contents/render_view_host_manager.cc:612: // active one are
tracked down and resolved.
On 2012/08/21 19:27:28, awong wrote:
> File a bug for this?
> 
> Also, I can't tell how severe this behavior is. In this comment and/or bug,
can
> we describe what is the effect of having this workaround?

I'm not quite sure myself yet. I've hit almost 400 cases in tests where we are
creating a new RVH with the same site instance as the current one. This doesn't
seem right, but I also don't want to spend time investigating all of those cases
prior to committing this CL.

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/web...
content/browser/web_contents/render_view_host_manager.cc:613: if (swapped_out) {
On 2012/08/21 19:27:28, awong wrote:
> remove brace here :)

Done.

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

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/web...
content/browser/web_contents/web_contents_impl.cc:2854:
DCHECK(params.target_frame_id != 0);
On 2012/08/21 19:27:28, awong wrote:
> If it is 0, what happens below?

0 is an invalid frame ID, so the call won't succeed. We shouldn't be in this
situation though, hence the DCHECK.

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/common/stri...
File content/common/string_constants.h (right):

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/common/stri...
content/common/string_constants.h:5: #ifndef CONTENT_COMMON_STRING_CONSTANTS_H_
On 2012/08/21 19:27:28, awong wrote:
> string_constants is too generic.  We should name it something like FrameTree
> constants, or render view constants, or something indicates the types of
> constants that should be in here.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/common/stri...
content/common/string_constants.h:10: extern const char kFrameTreeNodeNameKey[];
On 2012/08/21 19:27:28, awong wrote:
> Add comment referring to location that describes why these constants exist
> (probably the comment block in the ipc message)

Done.

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/common/view...
File content/common/view_messages.h (right):

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/common/view...
content/common/view_messages.h:1056: // response to this particular message.
On 2012/08/21 19:27:28, awong wrote:
> Describe the json format here. Then in other spots that store the string
and/or
> generate the tree, refer to this comment.

The description was in the render view, but I've moved it to the
ViewHostMsg_FrameTreeUpdated, since this is where it all originates.

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/renderer/re...
File content/renderer/render_view_impl.h (right):

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/renderer/re...
content/renderer/render_view_impl.h:1057: // of each frame.
On 2012/08/21 19:27:28, awong wrote:
> This comment seems out of date.  There's no return value.
> 
> Also, where is the serialization format defined?

Done.

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/renderer/re...
content/renderer/render_view_impl.h:1465: // been sent to the browser due to
loading a resource.
On 2012/08/21 19:27:28, awong wrote:
> due to loading a resource -> because a resource is loading.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/renderer/re...
content/renderer/render_view_impl.h:1468: // If this render view is swapped out
mirror of an active render view in a
On 2012/08/21 19:27:28, awong wrote:
> is swapped -> is a swapped
> 
> Can we add a bit explaining why we record this?

Done.

[Charlie Reis, 2012-08-22 22:08:34]

There's a lot of comment here, but I think it looks good overall.  I'm happy
with the approach and glad we have tests for it!

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

https://chromiumcodereview.appspot.com/10827078/diff/1021/content/browser/web...
content/browser/web_contents/web_contents_impl.cc:2851:
active_rvh->GetProcess()->GetID();
On 2012/08/21 19:27:28, awong wrote:
> This log isn't going to be seen by anyone. If we need to track this, maybe we
> should register a UMA stat (I've never done that before).  Charlie, any
> thoughts?

Yeah, I had a similar comment in patch set 10.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/re...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/re...
content/browser/renderer_host/render_view_host_impl.cc:1655: int process_id, int
route_id, const std::string& frame_tree) {
Style: These each need to be on their own line if they don't all fit on a single
line (with the function name).

http://www.chromium.org/developers/coding-style
"For function declarations and definitions, put each argument on a separate line
unless everything fits on one line."

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/re...
File content/browser/renderer_host/render_view_host_manager_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:39: return
false;
Should this be return false, or a failed EXPECT?  (Do we ever expect this to
return false in practice?)  Ditto below.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:67:
!subtree2->GetDictionary(i, &child2))
nit: Needs braces, since condition doesn't fit on one line.  (Same above.)

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:462: // 4)
Post a message from _blank to "foo".
Please briefly describe steps 5 and 6 as well.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:611: // 5)
Lastly, verify that the _blank window can post a message to subframe
nit: This should be 6.  Also, "to a subframe"

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1132: //
Test for correct propagation of frames hierarchy across processes in the
nit: frames -> the frame

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1133: //
same browsing context. The test starts by navigating to a page that has
nit: browsing context -> BrowsingInstance

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1135: //
to a separate site, so at the end we have 3 site instances. It checks
nit: site instances -> SiteInstances.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1222:
rvhm1->current_host()->GetSiteInstance()))));
Maybe grab a reference to this SiteInstance to make this slightly less verbose?

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/we...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/we...
content/browser/web_contents/render_view_host_manager.cc:244:
DCHECK_EQ(render_view_host, current_host());
Are there any races that could cause this to be called when render_view_host is
no longer active (e.g., right after the browser process has swapped in a new
one), or does the filtering of swapped out messages prevent that?

Also, should we use a CHECK here, or do we need to return early if the DCHECK
fails?

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/we...
content/browser/web_contents/render_view_host_manager.cc:256:
render_view_host_impl->GetProcess()->GetID(),
Interesting...  I wonder what happens if one of these processes has crashed. 
Does its RenderViewHosts automatically get removed from the swapped out list, or
might we be creating a new process here in by calling GetProcess()?

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/we...
content/browser/web_contents/render_view_host_manager.cc:519: // create a new
SiteInstance.
Wait, shouldn't HasWrongProcessForURL have caught this case?  The comment on
line 504 suggests that it's supposed to catch WebUI.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/we...
content/browser/web_contents/render_view_host_manager.cc:609: // If the current
host is already in the site instance we are looking for
nit: site instance -> SiteInstance

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/we...
content/browser/web_contents/render_view_host_manager.cc:614: // active one are
tracked down and resolved.
Nice.  Please mention http://crbug.com/123007 here.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/we...
content/browser/web_contents/render_view_host_manager.cc:615: if (swapped_out)
This is actually a little confusing.  The caller is requesting a swapped_out
RenderView, but we're returning the routing ID of an active one.  I'm now
wondering if it's clearer to have the caller check whether there's an active one
in the given SiteInstance before requesting a swapped out one.  Thoughts?

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/we...
content/browser/web_contents/render_view_host_manager.cc:650: // If we are
creating a swapped out RVH, send a message to update the
nit: the -> its

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/we...
File content/browser/web_contents/render_view_host_manager.h (right):

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/we...
content/browser/web_contents/render_view_host_manager.h:165: // Called when a
renderer has navigated and its frame tree is updated.
nit: and when its frame tree

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/we...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/we...
content/browser/web_contents/web_contents_impl.cc:2849: LOG(ERROR) << "Possibly
race condition trying to deliver to " <<
nit: Possibly -> Possible

Hmm, I'm a little concerned about this.  If we think it's actually possible,
then we should decide on the right behavior (e.g., drop the message) and
document it, rather than logging an error.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/common/ren...
File content/common/renderer_constants.h (right):

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/common/ren...
content/common/renderer_constants.h:6: #define
CONTENT_COMMON_RENDERER_CONSTANTS_H_
I don't have a better name yet, but it feels weird to me to have a
renderer_constants.h in common.  I agree that they don't belong in
public/common/content_constants.h, but I wonder if some kind of
content_constants_impl.h or frame_tree_constants.h would be better.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/common/vie...
File content/common/view_messages.h (right):

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/common/vie...
content/common/view_messages.h:580: // The full set of identifiers to uniquely
descirbe the target frame. See
nit: describe

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/common/vie...
content/common/view_messages.h:1051: // This message must be send to swapped out
renderers every time the browser
nit: send -> sent

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/common/vie...
content/common/view_messages.h:1055: 
nit: Remove blank lines.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/common/vie...
content/common/view_messages.h:2217: // achive this, each active renderer in the
source process has a swapped out
nit: achieve
nit: renderer -> RenderView
(Same below.  In general, "renderer" means the process, which may have multiple
RenderViews.)

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/common/vie...
content/common/view_messages.h:2218: // mirror in the destination process. The
active renderer and its mirror need to
Hmm, we haven't been using this "mirror" term before, but I suppose it's apt. 
Please put the first occurrence in quotes.

Also, we're calling this target instead of destination in other places.  Let's
be consistent with one or the other.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/common/vie...
content/common/view_messages.h:2223: // ViewMsg_UpdateFrameTree.
This is a great explanation.  Since we say they "need to have identical frame
tree structure," I think we also need to note something about concurrency. 
(e.g., We use best effort to maintain these trees across processes.)

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/common/vie...
content/common/view_messages.h:2228: // browser was to keep a mapping of the
frame ids across processes, it will
nit: will -> would

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/common/vie...
content/common/view_messages.h:2237: // * subtree - a list of DictionaryValue
objects for each frame that is
nit: that is a direct child

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/content_te...
File content/content_tests.gypi (right):

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/content_te...
content/content_tests.gypi:102: 'common/renderer_constants.h',
Do you need this here as well?  We usually don't need files to show up in both
gypi files.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.cc:351: for (WebFrame* frame = root; frame;
frame = frame->traverseNext(false))
nit: This for loop needs braces.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.cc:464: WebKit::WebFrame* exclude_subtree,
nit: Wrong indent.

Also, what is this parameter for?

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.cc:480: if (children->GetSize() > 0) {
nit: No braces on one-line if.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.cc:2502: if (!updating_frame_tree_) {
nit: No braces.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.cc:3671: // This function sends a those
updates to the browser and updates the RVH
nit: sends those

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.cc:3930: if (it != frames_map_.end()) {
nit: no braces

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.cc:4664: // Find the frame, which is a target
of this message. The source tags the
nit: Find the target frame of this message.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.cc:5047: // If a frame tree is specified,
recursively recreate it using the data
I feel weird about putting this logic in this function, since it doesn't really
have much to do with the original purpose or name.

Maybe we should have a separate recursive function that simply calls
NavigateToSwappedOutURL(frame) as its first action?

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.cc:5054: name != string16())
nit: Needs braces.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.cc:5072: if (subframe) {
nit: No braces.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.cc:5943: if (tree.get() != NULL &&
tree->IsType(base::Value::TYPE_DICTIONARY))
nit: You don't need "!= NULL"

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.cc:5963: for (WebFrame* f =
webview()->mainFrame(); f; f = f->traverseNext(false))
This part is just FindFrameByID, right?  No sense in duplicating the code.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
File content/renderer/render_view_impl.h (right):

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.h:1058: void
SendUpdatedFrameTree(WebKit::WebFrame* frame, bool exclude);
What's |exclude| about?

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.h:1061: // |remote_frame_id|.
I don't understand what remote_frame_id is referring to.  Is this the frame ID
of the corresponding frame in the active process, when this RenderView is
swapped out?  We should be more explicit.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.h:1106: // Make this RenderView show an empty,
unscriptable page.
Please update the comment for what the parameters mean.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.h:1460: // tree for this renderer. If true, we
won't be sending frame tree updates
nit: renderer -> RenderView.

"Updating the frame tree" is a bit of an overloaded term at this point.  I was
originally reading it as "letting the browser process know what our frame tree
is" rather than "recreating the current frame tree from the active RenderView,
given that this is a swapped out RenderView."  Can you make this clearer?

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.h:1464: // Boolean indicating that the frame
tree has changed, but message has not
nit: a message

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.h:1465: // been sent to the browser because a
resource has been loading.
Why does a loading resource block us?

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.h:1468: // If this render view is swapped out
mirror of an active render view in a
nit: is the swapped out "mirror"

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.h:1474: // A mapping of current process frame
ids to ids in the remote active render
nit: A map of the current process's

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.h:1475: // view, if this is a swapped out
mirror render view.
nit: We don't need to say mirror here.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.h:1476: std::map<int, int> frames_map_;
This name could be more descriptive.  active_frame_id_map_, perhaps?

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/test/data/...
File content/test/data/frame_tree/top.html (right):

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/test/data/...
content/test/data/frame_tree/top.html:30: setInterval(function() {
Do you mean setTimeout?  setInterval repeats.

(Why does it need to be delayed?)

[awong, 2012-08-22 22:25:46]

deferring to Charlie's review.  The meta comment is anywhere people might go
"what the heck is this function for?" add something that refers them to the big
comment you wrote.

http://codereview.chromium.org/10827078/diff/22001/content/browser/renderer_h...
File content/browser/renderer_host/render_view_host_impl.h (right):

http://codereview.chromium.org/10827078/diff/22001/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_impl.h:401: // process to keep
them in sync.
Refer to comment in view_messages.h.

http://codereview.chromium.org/10827078/diff/22001/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_impl.h:652: // in the render
view.
Refer to the comment in view_messages so people can figure out what the format
should be.

http://codereview.chromium.org/10827078/diff/22001/content/common/view_messag...
File content/common/view_messages.h (right):

http://codereview.chromium.org/10827078/diff/22001/content/common/view_messag...
content/common/view_messages.h:2234: // properties:
Since this is JSON, I'd define this in terms of JSON rather than talking bout
DictionaryValue since a "DictionaryValue" is an implementaiton detail.

[nasko, 2012-08-23 21:55:53]

A ton of changes. I hope it covers all the issues raised : ).

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/re...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/re...
content/browser/renderer_host/render_view_host_impl.cc:1655: int process_id, int
route_id, const std::string& frame_tree) {
On 2012/08/22 22:08:34, creis wrote:
> Style: These each need to be on their own line if they don't all fit on a
single
> line (with the function name).
> 
> http://www.chromium.org/developers/coding-style
> "For function declarations and definitions, put each argument on a separate
line
> unless everything fits on one line."

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/re...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/re...
content/browser/renderer_host/render_view_host_impl.h:401: // process to keep
them in sync.
On 2012/08/22 22:25:47, awong wrote:
> Refer to comment in view_messages.h.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/re...
content/browser/renderer_host/render_view_host_impl.h:652: // in the render
view.
On 2012/08/22 22:25:47, awong wrote:
> Refer to the comment in view_messages so people can figure out what the format
> should be.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/re...
File content/browser/renderer_host/render_view_host_manager_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:39: return
false;
On 2012/08/22 22:08:34, creis wrote:
> Should this be return false, or a failed EXPECT?  (Do we ever expect this to
> return false in practice?)  Ditto below.

No. The semantics of this function is just to return TRUE/FALSE based on whether
the two trees are identical. The EXPECT is done in the main test function based
on the result of this.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:67:
!subtree2->GetDictionary(i, &child2))
On 2012/08/22 22:08:34, creis wrote:
> nit: Needs braces, since condition doesn't fit on one line.  (Same above.)
> 

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:462: // 4)
Post a message from _blank to "foo".
On 2012/08/22 22:08:34, creis wrote:
> Please briefly describe steps 5 and 6 as well.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:611: // 5)
Lastly, verify that the _blank window can post a message to subframe
On 2012/08/22 22:08:34, creis wrote:
> nit: This should be 6.  Also, "to a subframe"

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1132: //
Test for correct propagation of frames hierarchy across processes in the
On 2012/08/22 22:08:34, creis wrote:
> nit: frames -> the frame

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1133: //
same browsing context. The test starts by navigating to a page that has
On 2012/08/22 22:08:34, creis wrote:
> nit: browsing context -> BrowsingInstance

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1135: //
to a separate site, so at the end we have 3 site instances. It checks
On 2012/08/22 22:08:34, creis wrote:
> nit: site instances -> SiteInstances.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1222:
rvhm1->current_host()->GetSiteInstance()))));
On 2012/08/22 22:08:34, creis wrote:
> Maybe grab a reference to this SiteInstance to make this slightly less
verbose?

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/we...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/we...
content/browser/web_contents/render_view_host_manager.cc:244:
DCHECK_EQ(render_view_host, current_host());
On 2012/08/22 22:08:34, creis wrote:
> Are there any races that could cause this to be called when render_view_host
is
> no longer active (e.g., right after the browser process has swapped in a new
> one), or does the filtering of swapped out messages prevent that?
> 
> Also, should we use a CHECK here, or do we need to return early if the DCHECK
> fails?

Based on the cross-site transition timeline, it shouldn't be possible to receive
a frame tree update because of the filtering. The renderer starts filtering the
moment it runs the unload handler. It is possible for a frame update to be sent
prior to that, but the swap of pending render to active renderer doesn't happen
until we do RVHM::CommitPending. At that point, we either have processed any
outstanding frame update messages or are filtering it in the renderer. 
As such, I believe a CHECK is useful here.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/we...
content/browser/web_contents/render_view_host_manager.cc:256:
render_view_host_impl->GetProcess()->GetID(),
On 2012/08/22 22:08:34, creis wrote:
> Interesting...  I wonder what happens if one of these processes has crashed. 
> Does its RenderViewHosts automatically get removed from the swapped out list,
or
> might we be creating a new process here in by calling GetProcess()?

GetProcess() returns the RenderProcessHost, which is the wrapper around the
actual underlying process. It's ID doesn't change, even though the underlying OS
process can crash and be re-spawned. So this line should not be creating a new
OS process.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/we...
content/browser/web_contents/render_view_host_manager.cc:519: // create a new
SiteInstance.
On 2012/08/22 22:08:34, creis wrote:
> Wait, shouldn't HasWrongProcessForURL have caught this case?  The comment on
> line 504 suggests that it's supposed to catch WebUI.

The first thing HasWrongProcessForURL checks is whether there is a process
attached to the site instance or not. If not, it returns false. This is the
reason why it doesn't catch it, since the site instance is empty and no process
has been created to render "about:blank".

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/we...
content/browser/web_contents/render_view_host_manager.cc:609: // If the current
host is already in the site instance we are looking for
On 2012/08/22 22:08:34, creis wrote:
> nit: site instance -> SiteInstance

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/we...
content/browser/web_contents/render_view_host_manager.cc:614: // active one are
tracked down and resolved.
On 2012/08/22 22:08:34, creis wrote:
> Nice.  Please mention http://crbug.com/123007 here.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/we...
content/browser/web_contents/render_view_host_manager.cc:615: if (swapped_out)
On 2012/08/22 22:08:34, creis wrote:
> This is actually a little confusing.  The caller is requesting a swapped_out
> RenderView, but we're returning the routing ID of an active one.  I'm now
> wondering if it's clearer to have the caller check whether there's an active
one
> in the given SiteInstance before requesting a swapped out one.  Thoughts?

Yes, changing the caller makes a bit more sense. I've done it and removed this
piece of code. I will file a separate bug about creating duplicate renderers for
the same SiteInstance and we can track those cases separately.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/we...
content/browser/web_contents/render_view_host_manager.cc:650: // If we are
creating a swapped out RVH, send a message to update the
On 2012/08/22 22:08:34, creis wrote:
> nit: the -> its

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/we...
File content/browser/web_contents/render_view_host_manager.h (right):

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/we...
content/browser/web_contents/render_view_host_manager.h:165: // Called when a
renderer has navigated and its frame tree is updated.
On 2012/08/22 22:08:34, creis wrote:
> nit: and when its frame tree

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/we...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/we...
content/browser/web_contents/web_contents_impl.cc:2849: LOG(ERROR) << "Possibly
race condition trying to deliver to " <<
On 2012/08/22 22:08:34, creis wrote:
> nit: Possibly -> Possible
> 
> Hmm, I'm a little concerned about this.  If we think it's actually possible,
> then we should decide on the right behavior (e.g., drop the message) and
> document it, rather than logging an error.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/common/ren...
File content/common/renderer_constants.h (right):

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/common/ren...
content/common/renderer_constants.h:6: #define
CONTENT_COMMON_RENDERER_CONSTANTS_H_
On 2012/08/22 22:08:34, creis wrote:
> I don't have a better name yet, but it feels weird to me to have a
> renderer_constants.h in common.  I agree that they don't belong in
> public/common/content_constants.h, but I wonder if some kind of
> content_constants_impl.h or frame_tree_constants.h would be better.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/common/vie...
File content/common/view_messages.h (right):

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/common/vie...
content/common/view_messages.h:580: // The full set of identifiers to uniquely
descirbe the target frame. See
On 2012/08/22 22:08:34, creis wrote:
> nit: describe

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/common/vie...
content/common/view_messages.h:1051: // This message must be send to swapped out
renderers every time the browser
On 2012/08/22 22:08:34, creis wrote:
> nit: send -> sent

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/common/vie...
content/common/view_messages.h:1055: 
On 2012/08/22 22:08:34, creis wrote:
> nit: Remove blank lines.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/common/vie...
content/common/view_messages.h:2217: // achive this, each active renderer in the
source process has a swapped out
On 2012/08/22 22:08:34, creis wrote:
> nit: achieve
> nit: renderer -> RenderView
> (Same below.  In general, "renderer" means the process, which may have
multiple
> RenderViews.)

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/common/vie...
content/common/view_messages.h:2218: // mirror in the destination process. The
active renderer and its mirror need to
On 2012/08/22 22:08:34, creis wrote:
> Hmm, we haven't been using this "mirror" term before, but I suppose it's apt. 
> Please put the first occurrence in quotes.
> 
> Also, we're calling this target instead of destination in other places.  Let's
> be consistent with one or the other.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/common/vie...
content/common/view_messages.h:2223: // ViewMsg_UpdateFrameTree.
On 2012/08/22 22:08:34, creis wrote:
> This is a great explanation.  Since we say they "need to have identical frame
> tree structure," I think we also need to note something about concurrency. 
> (e.g., We use best effort to maintain these trees across processes.)

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/common/vie...
content/common/view_messages.h:2228: // browser was to keep a mapping of the
frame ids across processes, it will
On 2012/08/22 22:08:34, creis wrote:
> nit: will -> would

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/common/vie...
content/common/view_messages.h:2234: // properties:
On 2012/08/22 22:25:47, awong wrote:
> Since this is JSON, I'd define this in terms of JSON rather than talking bout
> DictionaryValue since a "DictionaryValue" is an implementaiton detail.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/common/vie...
content/common/view_messages.h:2237: // * subtree - a list of DictionaryValue
objects for each frame that is
On 2012/08/22 22:08:34, creis wrote:
> nit: that is a direct child

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/content_te...
File content/content_tests.gypi (right):

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/content_te...
content/content_tests.gypi:102: 'common/renderer_constants.h',
On 2012/08/22 22:08:34, creis wrote:
> Do you need this here as well?  We usually don't need files to show up in both
> gypi files.

Yes, since the content_browsertests does not get linked to the files in the
common directory. I moved it to the actual content_browsertests section, since
the android build bot got an error of multiple definitions.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.cc:351: for (WebFrame* frame = root; frame;
frame = frame->traverseNext(false))
On 2012/08/22 22:08:34, creis wrote:
> nit: This for loop needs braces.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.cc:464: WebKit::WebFrame* exclude_subtree,
On 2012/08/22 22:08:34, creis wrote:
> nit: Wrong indent.
> 
> Also, what is this parameter for?

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.cc:480: if (children->GetSize() > 0) {
On 2012/08/22 22:08:34, creis wrote:
> nit: No braces on one-line if.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.cc:2502: if (!updating_frame_tree_) {
On 2012/08/22 22:08:34, creis wrote:
> nit: No braces.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.cc:3671: // This function sends a those
updates to the browser and updates the RVH
On 2012/08/22 22:08:34, creis wrote:
> nit: sends those

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.cc:3930: if (it != frames_map_.end()) {
On 2012/08/22 22:08:34, creis wrote:
> nit: no braces

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.cc:4664: // Find the frame, which is a target
of this message. The source tags the
On 2012/08/22 22:08:34, creis wrote:
> nit: Find the target frame of this message.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.cc:5047: // If a frame tree is specified,
recursively recreate it using the data
On 2012/08/22 22:08:34, creis wrote:
> I feel weird about putting this logic in this function, since it doesn't
really
> have much to do with the original purpose or name.
> 
> Maybe we should have a separate recursive function that simply calls
> NavigateToSwappedOutURL(frame) as its first action?

Separated it into a separate function. Fits much better.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.cc:5054: name != string16())
On 2012/08/22 22:08:34, creis wrote:
> nit: Needs braces.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.cc:5072: if (subframe) {
On 2012/08/22 22:08:34, creis wrote:
> nit: No braces.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.cc:5943: if (tree.get() != NULL &&
tree->IsType(base::Value::TYPE_DICTIONARY))
On 2012/08/22 22:08:34, creis wrote:
> nit: You don't need "!= NULL"

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.cc:5963: for (WebFrame* f =
webview()->mainFrame(); f; f = f->traverseNext(false))
On 2012/08/22 22:08:34, creis wrote:
> This part is just FindFrameByID, right?  No sense in duplicating the code.

Yes, indeed.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
File content/renderer/render_view_impl.h (right):

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.h:1058: void
SendUpdatedFrameTree(WebKit::WebFrame* frame, bool exclude);
On 2012/08/22 22:08:34, creis wrote:
> What's |exclude| about?

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.h:1061: // |remote_frame_id|.
On 2012/08/22 22:08:34, creis wrote:
> I don't understand what remote_frame_id is referring to.  Is this the frame ID
> of the corresponding frame in the active process, when this RenderView is
> swapped out?  We should be more explicit.

I've renamed it from Remote to Mapped. The idea of this function is to find the
frame through the frame mapping. In an essence, we are doing translation of ids
and look up based on that translation.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.h:1106: // Make this RenderView show an empty,
unscriptable page.
On 2012/08/22 22:08:34, creis wrote:
> Please update the comment for what the parameters mean.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.h:1460: // tree for this renderer. If true, we
won't be sending frame tree updates
On 2012/08/22 22:08:34, creis wrote:
> nit: renderer -> RenderView.
> 
> "Updating the frame tree" is a bit of an overloaded term at this point.  I was
> originally reading it as "letting the browser process know what our frame tree
> is" rather than "recreating the current frame tree from the active RenderView,
> given that this is a swapped out RenderView."  Can you make this clearer?

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.h:1464: // Boolean indicating that the frame
tree has changed, but message has not
On 2012/08/22 22:08:34, creis wrote:
> nit: a message

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.h:1465: // been sent to the browser because a
resource has been loading.
On 2012/08/22 22:08:34, creis wrote:
> Why does a loading resource block us?

Loading a resource doesn't block us. This is to avoid all the updates we will be
sending when navigating away from a page with a bunch of frames. Added a comment
to clarify it.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.h:1468: // If this render view is swapped out
mirror of an active render view in a
On 2012/08/22 22:08:34, creis wrote:
> nit: is the swapped out "mirror"

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.h:1474: // A mapping of current process frame
ids to ids in the remote active render
On 2012/08/22 22:08:34, creis wrote:
> nit: A map of the current process's

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.h:1475: // view, if this is a swapped out
mirror render view.
On 2012/08/22 22:08:34, creis wrote:
> nit: We don't need to say mirror here.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/renderer/r...
content/renderer/render_view_impl.h:1476: std::map<int, int> frames_map_;
On 2012/08/22 22:08:34, creis wrote:
> This name could be more descriptive.  active_frame_id_map_, perhaps?

Done.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/test/data/...
File content/test/data/frame_tree/top.html (right):

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/test/data/...
content/test/data/frame_tree/top.html:30: setInterval(function() {
On 2012/08/22 22:08:34, creis wrote:
> Do you mean setTimeout?  setInterval repeats.
> 
> (Why does it need to be delayed?)

I've changed the observer to use different event, so this is no longer needed.
The longer story is that frames are created by the frame loader, which doesn't
run synchronously to the appendChild() call in this script.

[Charlie Reis, 2012-08-24 23:26:07]

Looking better!

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/re...
File content/browser/renderer_host/render_view_host_manager_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:39: return
false;
On 2012/08/23 21:55:53, nasko wrote:
> On 2012/08/22 22:08:34, creis wrote:
> > Should this be return false, or a failed EXPECT?  (Do we ever expect this to
> > return false in practice?)  Ditto below.
> 
> No. The semantics of this function is just to return TRUE/FALSE based on
whether
> the two trees are identical. The EXPECT is done in the main test function
based
> on the result of this.

I just meant that we always expect the GetString (etc) calls to succeed, and if
they didn't, it would represent a deeper bug.  Wrapping them with EXPECT_TRUE
might help us catch that more quickly, and then you can still return false if
the names are not equal.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/we...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/we...
content/browser/web_contents/render_view_host_manager.cc:256:
render_view_host_impl->GetProcess()->GetID(),
On 2012/08/23 21:55:53, nasko wrote:
> On 2012/08/22 22:08:34, creis wrote:
> > Interesting...  I wonder what happens if one of these processes has crashed.

> > Does its RenderViewHosts automatically get removed from the swapped out
list,
> or
> > might we be creating a new process here in by calling GetProcess()?
> 
> GetProcess() returns the RenderProcessHost, which is the wrapper around the
> actual underlying process. It's ID doesn't change, even though the underlying
OS
> process can crash and be re-spawned. So this line should not be creating a new
> OS process.

Ah, right you are!  I was thinking of SiteInstance::GetProcess.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/we...
content/browser/web_contents/render_view_host_manager.cc:519: // create a new
SiteInstance.
On 2012/08/23 21:55:53, nasko wrote:
> On 2012/08/22 22:08:34, creis wrote:
> > Wait, shouldn't HasWrongProcessForURL have caught this case?  The comment on
> > line 504 suggests that it's supposed to catch WebUI.
> 
> The first thing HasWrongProcessForURL checks is whether there is a process
> attached to the site instance or not. If not, it returns false. This is the
> reason why it doesn't catch it, since the site instance is empty and no
process
> has been created to render "about:blank".

:(

Good catch.  This is fine, then, and I'll clean it up as part of 123007.

https://chromiumcodereview.appspot.com/10827078/diff/22001/content/browser/we...
content/browser/web_contents/render_view_host_manager.cc:615: if (swapped_out)
On 2012/08/23 21:55:53, nasko wrote:
> On 2012/08/22 22:08:34, creis wrote:
> > This is actually a little confusing.  The caller is requesting a swapped_out
> > RenderView, but we're returning the routing ID of an active one.  I'm now
> > wondering if it's clearer to have the caller check whether there's an active
> one
> > in the given SiteInstance before requesting a swapped out one.  Thoughts?
> 
> Yes, changing the caller makes a bit more sense. I've done it and removed this
> piece of code. I will file a separate bug about creating duplicate renderers
for
> the same SiteInstance and we can track those cases separately.

Great.  Please CC me on the bug.

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/browser/re...
File content/browser/renderer_host/render_view_host_manager_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1139: //
to a separate site, so at the end we have 3 SiteInstances. It checks
Just for clarity, let's call out that the opened windows have swapped out
RenderViews corresponding to the opener, so these swapped out views must have a
matching frame hierarchy.  (This will help other developers understand what it's
for.)

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1165:
NavigateToURL(shell(), test_server()->GetURL("files/simple_page.html"));
Since the earlier description said we were starting with the page with frames,
let's put a comment here like "First navigate to a page with no frames and
ensure the frame tree has no subtrees."

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1195:
NavigateToURL(shell1, https_server.GetURL("files/title1.html"));
It would help to add a comment here like "Now navigate the new window to a
different SiteInstance," just to help me follow along at a glance.

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1237:
EXPECT_FALSE(CompareTrees(
Comment: "Verify that the frame trees from different windows aren't equal."

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1244:
NavigateToURL(shell(), frame_tree_url);
There might be a race possible here, since we might not receive the
FrameTreeUpdated message before the CompareTrees call below.

Sounds like we can fix it by sending FrameTreeUpdated before DidStopLoading, and
also waiting for NOTIFICATION_LOAD_STOP.

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1267:
EXPECT_TRUE(subtree->GetSize() == 2);
nit: EXPECT_EQ(2, ...)
Same below.

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/common/vie...
File content/common/view_messages.h (right):

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/common/vie...
content/common/view_messages.h:1049: // process id and route id of the source
renderer.
nit: renderer -> RenderView (here and below)

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/common/vie...
content/common/view_messages.h:2216: // need to have identical frame tree
structure, so calls originating and
nit: originating in

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/common/vie...
content/common/view_messages.h:2220: // through the ViewMsg_UpdateFrameTree. We
use best effort to maintain these
nit: maintain -> keep

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/content_co...
File content/content_common.gypi (right):

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/content_co...
content/content_common.gypi:354: 'common/content_constants_internal.h',
nit: Please alphabetize.

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/renderer/r...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/renderer/r...
content/renderer/render_view_impl.cc:3676: WebKit::WebFrame* frame, bool
exclude_subtree) {
Hmm, "frame" makes it sound like we're sending the tree starting at frame. 
Since we always start at the top and only really need to know "frame" when we're
excluding it, maybe we could combine these two arguments into "exclude_frame,"
which is NULL if you don't need to exclude anything.

(Then f would just be webview()->mainFrame().)

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/renderer/r...
content/renderer/render_view_impl.cc:3711: if
(frame->document().body().appendChild(element)) {
I wonder if we should do something if this fails?  Since we don't expect this to
happen, we could have an else block that LOGs.  (This is different than the LOG
statement in the previous patch set, since we expect that to happen in
practice.)

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/renderer/r...
File content/renderer/render_view_impl.h (right):

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/renderer/r...
content/renderer/render_view_impl.h:1069: // |remote_frame_id|.
remote_frame_id is now stale.   Also, I'm still not sure I understand what this
is about.  Can the comment say a little more about what the map is?

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/renderer/r...
content/renderer/render_view_impl.h:1473: // been sent to the browser because a
resource has been loading. This helps
nit: resource -> page

As we discussed, please also put a TODO somewhere like DidStopLoading (with a
corresponding bug number), since this might cause us to delay frame tree updates
while some of the frames are already actively running script.

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/renderer/r...
content/renderer/render_view_impl.h:1479: // If this render view is swapped out
"mirror" of an active render view in a
nit: is a

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/test/data/...
File content/test/data/post_message2.html (right):

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/test/data/...
content/test/data/post_message2.html:3: <head><title>Post message tests</title>
nit: Post message subframe tests

[nasko, 2012-08-27 18:53:37]

More fixes.

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/browser/re...
File content/browser/renderer_host/render_view_host_manager_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1139: //
to a separate site, so at the end we have 3 SiteInstances. It checks
On 2012/08/24 23:26:07, creis wrote:
> Just for clarity, let's call out that the opened windows have swapped out
> RenderViews corresponding to the opener, so these swapped out views must have
a
> matching frame hierarchy.  (This will help other developers understand what
it's
> for.)

Done.

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1165:
NavigateToURL(shell(), test_server()->GetURL("files/simple_page.html"));
On 2012/08/24 23:26:07, creis wrote:
> Since the earlier description said we were starting with the page with frames,
> let's put a comment here like "First navigate to a page with no frames and
> ensure the frame tree has no subtrees."

Done.

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1195:
NavigateToURL(shell1, https_server.GetURL("files/title1.html"));
On 2012/08/24 23:26:07, creis wrote:
> It would help to add a comment here like "Now navigate the new window to a
> different SiteInstance," just to help me follow along at a glance.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1237:
EXPECT_FALSE(CompareTrees(
On 2012/08/24 23:26:07, creis wrote:
> Comment: "Verify that the frame trees from different windows aren't equal."

Done.

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1244:
NavigateToURL(shell(), frame_tree_url);
On 2012/08/24 23:26:07, creis wrote:
> There might be a race possible here, since we might not receive the
> FrameTreeUpdated message before the CompareTrees call below.
> 
> Sounds like we can fix it by sending FrameTreeUpdated before DidStopLoading,
and
> also waiting for NOTIFICATION_LOAD_STOP.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1267:
EXPECT_TRUE(subtree->GetSize() == 2);
On 2012/08/24 23:26:07, creis wrote:
> nit: EXPECT_EQ(2, ...)
> Same below.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/common/vie...
File content/common/view_messages.h (right):

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/common/vie...
content/common/view_messages.h:1049: // process id and route id of the source
renderer.
On 2012/08/24 23:26:07, creis wrote:
> nit: renderer -> RenderView (here and below)

Done.

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/common/vie...
content/common/view_messages.h:2216: // need to have identical frame tree
structure, so calls originating and
On 2012/08/24 23:26:07, creis wrote:
> nit: originating in

Done.

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/common/vie...
content/common/view_messages.h:2220: // through the ViewMsg_UpdateFrameTree. We
use best effort to maintain these
On 2012/08/24 23:26:07, creis wrote:
> nit: maintain -> keep

Done.

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/content_co...
File content/content_common.gypi (right):

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/content_co...
content/content_common.gypi:354: 'common/content_constants_internal.h',
On 2012/08/24 23:26:07, creis wrote:
> nit: Please alphabetize.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/renderer/r...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/renderer/r...
content/renderer/render_view_impl.cc:3676: WebKit::WebFrame* frame, bool
exclude_subtree) {
On 2012/08/24 23:26:07, creis wrote:
> Hmm, "frame" makes it sound like we're sending the tree starting at frame. 
> Since we always start at the top and only really need to know "frame" when
we're
> excluding it, maybe we could combine these two arguments into "exclude_frame,"
> which is NULL if you don't need to exclude anything.
> 
> (Then f would just be webview()->mainFrame().)

Done.

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/renderer/r...
content/renderer/render_view_impl.cc:3711: if
(frame->document().body().appendChild(element)) {
On 2012/08/24 23:26:07, creis wrote:
> I wonder if we should do something if this fails?  Since we don't expect this
to
> happen, we could have an else block that LOGs.  (This is different than the
LOG
> statement in the previous patch set, since we expect that to happen in
> practice.)

Done.

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/renderer/r...
File content/renderer/render_view_impl.h (right):

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/renderer/r...
content/renderer/render_view_impl.h:1069: // |remote_frame_id|.
On 2012/08/24 23:26:07, creis wrote:
> remote_frame_id is now stale.   Also, I'm still not sure I understand what
this
> is about.  Can the comment say a little more about what the map is?

Done.

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/renderer/r...
content/renderer/render_view_impl.h:1473: // been sent to the browser because a
resource has been loading. This helps
On 2012/08/24 23:26:07, creis wrote:
> nit: resource -> page
> 
> As we discussed, please also put a TODO somewhere like DidStopLoading (with a
> corresponding bug number), since this might cause us to delay frame tree
updates
> while some of the frames are already actively running script.

Done.

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/renderer/r...
content/renderer/render_view_impl.h:1479: // If this render view is swapped out
"mirror" of an active render view in a
On 2012/08/24 23:26:07, creis wrote:
> nit: is a

Done.

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/test/data/...
File content/test/data/post_message2.html (right):

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/test/data/...
content/test/data/post_message2.html:3: <head><title>Post message tests</title>
On 2012/08/24 23:26:07, creis wrote:
> nit: Post message subframe tests

Done.

[Charlie Reis, 2012-08-27 22:27:06]

LGTM, once we resolve the test flakiness question below.

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/browser/re...
File content/browser/renderer_host/render_view_host_manager_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1244:
NavigateToURL(shell(), frame_tree_url);
On 2012/08/27 18:53:37, nasko wrote:
> On 2012/08/24 23:26:07, creis wrote:
> > There might be a race possible here, since we might not receive the
> > FrameTreeUpdated message before the CompareTrees call below.
> > 
> > Sounds like we can fix it by sending FrameTreeUpdated before DidStopLoading,
> and
> > also waiting for NOTIFICATION_LOAD_STOP.
> 
> Done.

Don't we need to wait for NOTIFICATION_LOAD_STOP as well?  (I can never remember
if NavigateToURL waits for commit or load stopped, but I thought it was commit.)

[nasko, 2012-08-27 22:45:42]

Answered the outstanding question.

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/browser/re...
File content/browser/renderer_host/render_view_host_manager_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10827078/diff/23003/content/browser/re...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1244:
NavigateToURL(shell(), frame_tree_url);
On 2012/08/27 22:27:06, creis wrote:
> On 2012/08/27 18:53:37, nasko wrote:
> > On 2012/08/24 23:26:07, creis wrote:
> > > There might be a race possible here, since we might not receive the
> > > FrameTreeUpdated message before the CompareTrees call below.
> > > 
> > > Sounds like we can fix it by sending FrameTreeUpdated before
DidStopLoading,
> > and
> > > also waiting for NOTIFICATION_LOAD_STOP.
> > 
> > Done.
> 
> Don't we need to wait for NOTIFICATION_LOAD_STOP as well?  (I can never
remember
> if NavigateToURL waits for commit or load stopped, but I thought it was
commit.)

NavigateToURL is monitoring for NOTIFICATION_LOAD_STOP events (through
TestNavigationObserver::Observe). As such, it should not create flakiness in the
test.

[commit-bot: I haz the power, 2012-08-28 14:47:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/10827078/34006

[commit-bot: I haz the power, 2012-08-28 19:23:44]

Change committed as 153710

[not at google - send to devlin, 2012-08-30 05:01:40]

https://chromiumcodereview.appspot.com/10827078/diff/34006/content/browser/we...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://chromiumcodereview.appspot.com/10827078/diff/34006/content/browser/we...
content/browser/web_contents/render_view_host_manager.cc:253:
current_host()->GetSiteInstance());
This check seems to be flakily but consistently failing on the linux_chromeos
trybot, tickled by ExtensionApiTest.TabEvents:

http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...

http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...

http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...

and others.

[not at google - send to devlin, 2012-08-30 05:31:11]

See http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos

I'm going to disable that test for now.