Cross-process postMessage

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

This requires keeping a frame map in the browser process, which tracks the
relationships between tabs, subframes, and openers.  These relationships
don't change as a tab navigates from one process to another.

With this map, the browser can then create "swapped out" RenderViews for
a tab's opener(s).  All swapped out RenderViews can then act as proxies to
send and receive postMessages via the browser process.

BUG=99202
TEST=Use Facebook Connect within a hosted app.

(Revised and committed in http://codereview.chromium.org/9108001/)

[Charlie Reis, 2011-12-01 23:13:02]

Just a few thoughts to get the review started, as you continue working on this. 
I'm still trying to understand the frame wrapper setup and whether there's an
easier way to represent it.

We may also want to think about better names for things like ProxyViewHost, to
convey more about what they're doing.  I don't have better suggestions yet,
though.

http://codereview.chromium.org/8760024/diff/1/chrome/browser/ui/browser_navig...
File chrome/browser/ui/browser_navigator.h (right):

http://codereview.chromium.org/8760024/diff/1/chrome/browser/ui/browser_navig...
chrome/browser/ui/browser_navigator.h:183: int64
opener_browsing_instance_frame_id;
Maybe put this up near is_renderer_initiated and give it a comment.

http://codereview.chromium.org/8760024/diff/1/chrome/common/chrome_content_cl...
File chrome/common/chrome_content_client.cc (right):

http://codereview.chromium.org/8760024/diff/1/chrome/common/chrome_content_cl...
chrome/common/chrome_content_client.cc:307: case
ChromeViewHostMsg_DomOperationResponse::ID:
Is this a worthwhile change independent of the rest of the code, or is it only
needed when we change about:swappedout?  Just wondering if we should land it in
its own CL.

http://codereview.chromium.org/8760024/diff/1/chrome/common/chrome_content_cl...
File chrome/common/chrome_content_client.h (left):

http://codereview.chromium.org/8760024/diff/1/chrome/common/chrome_content_cl...
chrome/common/chrome_content_client.h:26: virtual bool
CanSendWhileSwappedOut(const IPC::Message* msg) OVERRIDE;
This will need to stick around if we don't change about:swapped out, right?

http://codereview.chromium.org/8760024/diff/1/chrome/renderer/chrome_content_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/8760024/diff/1/chrome/renderer/chrome_content_...
chrome/renderer/chrome_content_renderer_client.cc:736: // TODO(supersat): Make
sure this change is correct
Yes, this is what I intended.

http://codereview.chromium.org/8760024/diff/1/content/browser/browser_context.h
File content/browser/browser_context.h (right):

http://codereview.chromium.org/8760024/diff/1/content/browser/browser_context...
content/browser/browser_context.h:109: FrameMapper frame_mapper_;
Hmm, you might get pushback on this.  It might need to be created on the various
implementations of BrowserContext (ProfileImpl, OffTheRecordProfile,
ShellBrowserContext, TestBrowserContext).

There's a bigger question here if Albert does make isolated apps use a different
BrowserContext, though.  That's uncertain, but if so, a tab will change its
BrowserContext over time (and may send postMessages to tabs in other
BrowserContexts).

http://codereview.chromium.org/8760024/diff/1/content/browser/browsing_instan...
File content/browser/browsing_instance.cc (right):

http://codereview.chromium.org/8760024/diff/1/content/browser/browsing_instan...
content/browser/browsing_instance.cc:10: #include
"content/browser/browsing_instance_frame_id.h"
Necessary?

http://codereview.chromium.org/8760024/diff/1/content/browser/browsing_instan...
File content/browser/browsing_instance.h (right):

http://codereview.chromium.org/8760024/diff/1/content/browser/browsing_instan...
content/browser/browsing_instance.h:14: #include <list>
Remove?

http://codereview.chromium.org/8760024/diff/1/content/browser/browsing_instan...
File content/browser/browsing_instance_frame_id.cc (right):

http://codereview.chromium.org/8760024/diff/1/content/browser/browsing_instan...
content/browser/browsing_instance_frame_id.cc:2: // Author: supersat@google.com
(Karl Koscher)
Nit: wrong template.

http://codereview.chromium.org/8760024/diff/1/content/browser/browsing_instan...
File content/browser/browsing_instance_frame_id.h (right):

http://codereview.chromium.org/8760024/diff/1/content/browser/browsing_instan...
content/browser/browsing_instance_frame_id.h:6: #define
CONTENT_BROWSER_WORKER_HOST_BROWSING_INSTANCE_FRAME_ID_H_
Nit: remove WORKER_HOST

http://codereview.chromium.org/8760024/diff/1/content/browser/browsing_instan...
content/browser/browsing_instance_frame_id.h:15: class BrowsingInstanceFrame {
I don't understand this name.  A BrowsingInstance is a collection of tabs that
can script each other, but this class seems to represent the browser's notion of
a frame (either a tab or a sub-frame).

Also, should this keep track of all the WebKitFrameIdTuples for this frame? 
We'll need to tell them all to go away this frame closes.

http://codereview.chromium.org/8760024/diff/1/content/browser/browsing_instan...
content/browser/browsing_instance_frame_id.h:63:
std::list<BrowsingInstanceFrame*> children_;
Parent and child frames usually refer to the frame tree in a particular tab.  Do
you mean the opener and opened frames?

http://codereview.chromium.org/8760024/diff/1/content/browser/frame_mapper.h
File content/browser/frame_mapper.h (right):

http://codereview.chromium.org/8760024/diff/1/content/browser/frame_mapper.h#...
content/browser/frame_mapper.h:14: class CONTENT_EXPORT FrameMapper {
Maybe FrameMap?

http://codereview.chromium.org/8760024/diff/1/content/browser/renderer_host/r...
File content/browser/renderer_host/render_process_host_impl.h (right):

http://codereview.chromium.org/8760024/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/render_process_host_impl.h:161: const
ViewMsg_PostMessage_Params& params);
Why put this on RenderProcessHost?  I would have thought you'd look up the
RenderViewHost and send the message from there.

http://codereview.chromium.org/8760024/diff/1/content/browser/tab_contents/na...
File content/browser/tab_contents/navigation_controller.h (right):

http://codereview.chromium.org/8760024/diff/1/content/browser/tab_contents/na...
content/browser/tab_contents/navigation_controller.h:183: // will this ever be
set?
Reasonable question.  If you hit shift or control + enter on a URL in the
omnibox, for example, it will create a new window/tab.  We'll keep that in the
same SiteInstance if it's part of the same site, but we may change that. 
Regardless, window.opener is not set and we wouldn't be able to script the new
window.

I'm thinking a browser-initiated navigation should not set this.

http://codereview.chromium.org/8760024/diff/1/content/browser/tab_contents/na...
File content/browser/tab_contents/navigation_entry.h (right):

http://codereview.chromium.org/8760024/diff/1/content/browser/tab_contents/na...
content/browser/tab_contents/navigation_entry.h:448: int64
opener_browsing_instance_frame_id_;
Do we need to persist this?  (I suppose it could let tabs script each other
after a session restore if we do, which would be pretty cool.)  The comment
above suggests we need to be very careful about that, though.

http://codereview.chromium.org/8760024/diff/1/content/browser/tab_contents/pa...
File content/browser/tab_contents/page_navigator.cc (right):

http://codereview.chromium.org/8760024/diff/1/content/browser/tab_contents/pa...
content/browser/tab_contents/page_navigator.cc:30: is_renderer_initiated(false)
{
Looks like we should set it to -1 here rather than in the .h file to be
consistent.

http://codereview.chromium.org/8760024/diff/1/content/browser/tab_contents/re...
File content/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/8760024/diff/1/content/browser/tab_contents/re...
content/browser/tab_contents/render_view_host_manager.cc:773: // We can pass
-1,-1,-1 because there is no pending response in the
Leftover from a previous change?

http://codereview.chromium.org/8760024/diff/1/content/renderer/dom_proxy_list...
File content/renderer/dom_proxy_listener.cc (right):

http://codereview.chromium.org/8760024/diff/1/content/renderer/dom_proxy_list...
content/renderer/dom_proxy_listener.cc:43: browsing_instance_frame_id_,
params));
How will we extend this to handle other types of events, like close()?

http://codereview.chromium.org/8760024/diff/1/content/renderer/render_view_im...
File content/renderer/render_view_impl.cc (left):

http://codereview.chromium.org/8760024/diff/1/content/renderer/render_view_im...
content/renderer/render_view_impl.cc:1990: // TODO(creis): Remove this when we
fix OnSwapOut to not need a navigation.
This can't be removed any more, can it?

http://codereview.chromium.org/8760024/diff/1/content/renderer/render_view_im...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8760024/diff/1/content/renderer/render_view_im...
content/renderer/render_view_impl.cc:810: }
Hmm, what happens if we go back to the NavigationEntry that created the window? 
Will we set the opener again?

http://codereview.chromium.org/8760024/diff/1/content/renderer/render_view_im...
content/renderer/render_view_impl.cc:4060: new DOMProxyInstaller(this,
params.browsing_instance_frame_id);
I wonder if there's a clearer way to handle this.

[supersat, 2011-12-09 23:08:19]

An updated patch. What I think is still missing:

* Tearing down all of the proxy frames when the real frame goes away.
* Tests.
* Some renaming.

http://codereview.chromium.org/8760024/diff/1/chrome/browser/ui/browser_navig...
File chrome/browser/ui/browser_navigator.h (right):

http://codereview.chromium.org/8760024/diff/1/chrome/browser/ui/browser_navig...
chrome/browser/ui/browser_navigator.h:183: int64
opener_browsing_instance_frame_id;
On 2011/12/01 23:13:02, creis wrote:
> Maybe put this up near is_renderer_initiated and give it a comment.

Done.

http://codereview.chromium.org/8760024/diff/1/chrome/common/chrome_content_cl...
File chrome/common/chrome_content_client.cc (right):

http://codereview.chromium.org/8760024/diff/1/chrome/common/chrome_content_cl...
chrome/common/chrome_content_client.cc:307: case
ChromeViewHostMsg_DomOperationResponse::ID:
On 2011/12/01 23:13:02, creis wrote:
> Is this a worthwhile change independent of the rest of the code, or is it only
> needed when we change about:swappedout?  Just wondering if we should land it
in
> its own CL.

It's only needed if we change about:swappedout.

http://codereview.chromium.org/8760024/diff/1/chrome/common/chrome_content_cl...
File chrome/common/chrome_content_client.h (left):

http://codereview.chromium.org/8760024/diff/1/chrome/common/chrome_content_cl...
chrome/common/chrome_content_client.h:26: virtual bool
CanSendWhileSwappedOut(const IPC::Message* msg) OVERRIDE;
On 2011/12/01 23:13:02, creis wrote:
> This will need to stick around if we don't change about:swapped out, right?

Yeah. Part of the swapped out changes leaked through to this CL since it was
originally dependent on those changes.

http://codereview.chromium.org/8760024/diff/1/chrome/renderer/chrome_content_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/8760024/diff/1/chrome/renderer/chrome_content_...
chrome/renderer/chrome_content_renderer_client.cc:736: // TODO(supersat): Make
sure this change is correct
On 2011/12/01 23:13:02, creis wrote:
> Yes, this is what I intended.

Done.

http://codereview.chromium.org/8760024/diff/1/content/browser/browser_context.h
File content/browser/browser_context.h (right):

http://codereview.chromium.org/8760024/diff/1/content/browser/browser_context...
content/browser/browser_context.h:109: FrameMapper frame_mapper_;
On 2011/12/01 23:13:02, creis wrote:
> Hmm, you might get pushback on this.  It might need to be created on the
various
> implementations of BrowserContext (ProfileImpl, OffTheRecordProfile,
> ShellBrowserContext, TestBrowserContext).
> 
> There's a bigger question here if Albert does make isolated apps use a
different
> BrowserContext, though.  That's uncertain, but if so, a tab will change its
> BrowserContext over time (and may send postMessages to tabs in other
> BrowserContexts).

Are frames in different browser contexts able to script each other?

http://codereview.chromium.org/8760024/diff/1/content/browser/browsing_instan...
File content/browser/browsing_instance.cc (right):

http://codereview.chromium.org/8760024/diff/1/content/browser/browsing_instan...
content/browser/browsing_instance.cc:10: #include
"content/browser/browsing_instance_frame_id.h"
On 2011/12/01 23:13:02, creis wrote:
> Necessary?

No. Leftover crud from a refactor.

http://codereview.chromium.org/8760024/diff/1/content/browser/browsing_instan...
File content/browser/browsing_instance.h (right):

http://codereview.chromium.org/8760024/diff/1/content/browser/browsing_instan...
content/browser/browsing_instance.h:14: #include <list>
On 2011/12/01 23:13:02, creis wrote:
> Remove?

Done.

http://codereview.chromium.org/8760024/diff/1/content/browser/browsing_instan...
File content/browser/browsing_instance_frame_id.cc (right):

http://codereview.chromium.org/8760024/diff/1/content/browser/browsing_instan...
content/browser/browsing_instance_frame_id.cc:2: // Author: supersat@google.com
(Karl Koscher)
On 2011/12/01 23:13:02, creis wrote:
> Nit: wrong template.

Done.

http://codereview.chromium.org/8760024/diff/1/content/browser/browsing_instan...
File content/browser/browsing_instance_frame_id.h (right):

http://codereview.chromium.org/8760024/diff/1/content/browser/browsing_instan...
content/browser/browsing_instance_frame_id.h:6: #define
CONTENT_BROWSER_WORKER_HOST_BROWSING_INSTANCE_FRAME_ID_H_
On 2011/12/01 23:13:02, creis wrote:
> Nit: remove WORKER_HOST

Done.

http://codereview.chromium.org/8760024/diff/1/content/browser/browsing_instan...
content/browser/browsing_instance_frame_id.h:15: class BrowsingInstanceFrame {
On 2011/12/01 23:13:02, creis wrote:
> I don't understand this name.  A BrowsingInstance is a collection of tabs that
> can script each other, but this class seems to represent the browser's notion
of
> a frame (either a tab or a sub-frame).

I agree the name could improve. Any suggestions? This class represents a
user-perceived frame whose IDs are unique per browsing instance.

> Also, should this keep track of all the WebKitFrameIdTuples for this frame? 
> We'll need to tell them all to go away this frame closes.

Yes. Added.

http://codereview.chromium.org/8760024/diff/1/content/browser/browsing_instan...
content/browser/browsing_instance_frame_id.h:63:
std::list<BrowsingInstanceFrame*> children_;
On 2011/12/01 23:13:02, creis wrote:
> Parent and child frames usually refer to the frame tree in a particular tab. 
Do
> you mean the opener and opened frames?

I'm actually not sure what I intended these to be used for. They're not used
right now, so I'll just take them out.

http://codereview.chromium.org/8760024/diff/1/content/browser/frame_mapper.h
File content/browser/frame_mapper.h (right):

http://codereview.chromium.org/8760024/diff/1/content/browser/frame_mapper.h#...
content/browser/frame_mapper.h:14: class CONTENT_EXPORT FrameMapper {
On 2011/12/01 23:13:02, creis wrote:
> Maybe FrameMap?

Done.

http://codereview.chromium.org/8760024/diff/1/content/browser/renderer_host/r...
File content/browser/renderer_host/render_process_host_impl.h (right):

http://codereview.chromium.org/8760024/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/render_process_host_impl.h:161: const
ViewMsg_PostMessage_Params& params);
On 2011/12/01 23:13:02, creis wrote:
> Why put this on RenderProcessHost?  I would have thought you'd look up the
> RenderViewHost and send the message from there.

The ProxyViewHost (which can send this message on behalf of its frame) doesn't
have a corresponding RenderViewHost. We also don't need anything specific to a
particular RenderViewHost to route the message to the right destination.

http://codereview.chromium.org/8760024/diff/1/content/browser/tab_contents/na...
File content/browser/tab_contents/navigation_controller.h (right):

http://codereview.chromium.org/8760024/diff/1/content/browser/tab_contents/na...
content/browser/tab_contents/navigation_controller.h:183: // will this ever be
set?
On 2011/12/01 23:13:02, creis wrote:
> Reasonable question.  If you hit shift or control + enter on a URL in the
> omnibox, for example, it will create a new window/tab.  We'll keep that in the
> same SiteInstance if it's part of the same site, but we may change that. 
> Regardless, window.opener is not set and we wouldn't be able to script the new
> window.
> 
> I'm thinking a browser-initiated navigation should not set this.

Done.

http://codereview.chromium.org/8760024/diff/1/content/browser/tab_contents/na...
File content/browser/tab_contents/navigation_entry.h (right):

http://codereview.chromium.org/8760024/diff/1/content/browser/tab_contents/na...
content/browser/tab_contents/navigation_entry.h:448: int64
opener_browsing_instance_frame_id_;
On 2011/12/01 23:13:02, creis wrote:
> Do we need to persist this?  (I suppose it could let tabs script each other
> after a session restore if we do, which would be pretty cool.)  The comment
> above suggests we need to be very careful about that, though.

We'll only need to persist this if we restore scripting connections. However,
I'm not sure we necessarily want to do that because same-origin scripting
connections will not be persisted (AFAIK), and I think we should try to be
consistent.

http://codereview.chromium.org/8760024/diff/1/content/browser/tab_contents/pa...
File content/browser/tab_contents/page_navigator.cc (right):

http://codereview.chromium.org/8760024/diff/1/content/browser/tab_contents/pa...
content/browser/tab_contents/page_navigator.cc:30: is_renderer_initiated(false)
{
On 2011/12/01 23:13:02, creis wrote:
> Looks like we should set it to -1 here rather than in the .h file to be
> consistent.

I added an initializer for opener_browsing_instance_frame_id here, but I don't
think this constructor is used at all? It's declared private.

http://codereview.chromium.org/8760024/diff/1/content/browser/tab_contents/re...
File content/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/8760024/diff/1/content/browser/tab_contents/re...
content/browser/tab_contents/render_view_host_manager.cc:773: // We can pass
-1,-1,-1 because there is no pending response in the
On 2011/12/01 23:13:02, creis wrote:
> Leftover from a previous change?

Yes.

http://codereview.chromium.org/8760024/diff/1/content/renderer/dom_proxy_list...
File content/renderer/dom_proxy_listener.cc (right):

http://codereview.chromium.org/8760024/diff/1/content/renderer/dom_proxy_list...
content/renderer/dom_proxy_listener.cc:43: browsing_instance_frame_id_,
params));
On 2011/12/01 23:13:02, creis wrote:
> How will we extend this to handle other types of events, like close()?

We'll get WebFrameClient callbacks like willClose(). This class is going away if
we stick with Adam's approach.

http://codereview.chromium.org/8760024/diff/1/content/renderer/render_view_im...
File content/renderer/render_view_impl.cc (left):

http://codereview.chromium.org/8760024/diff/1/content/renderer/render_view_im...
content/renderer/render_view_impl.cc:1990: // TODO(creis): Remove this when we
fix OnSwapOut to not need a navigation.
On 2011/12/01 23:13:02, creis wrote:
> This can't be removed any more, can it?

No. Done.

http://codereview.chromium.org/8760024/diff/1/content/renderer/render_view_im...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8760024/diff/1/content/renderer/render_view_im...
content/renderer/render_view_impl.cc:810: }
On 2011/12/01 23:13:02, creis wrote:
> Hmm, what happens if we go back to the NavigationEntry that created the
window? 
> Will we set the opener again?

Yeah, and we shouldn't. Fixed. (But we should test...)

http://codereview.chromium.org/8760024/diff/1/content/renderer/render_view_im...
content/renderer/render_view_impl.cc:4060: new DOMProxyInstaller(this,
params.browsing_instance_frame_id);
On 2011/12/01 23:13:02, creis wrote:
> I wonder if there's a clearer way to handle this.

If we go with Adam's approach, we don't need the DOMProxyInstaller anymore.

[Charlie Reis, 2011-12-12 22:20:36]

Thanks Karl!  Making progress.  A few questions and style nits inline.

http://codereview.chromium.org/8760024/diff/1/chrome/common/chrome_content_cl...
File chrome/common/chrome_content_client.cc (right):

http://codereview.chromium.org/8760024/diff/1/chrome/common/chrome_content_cl...
chrome/common/chrome_content_client.cc:307: case
ChromeViewHostMsg_DomOperationResponse::ID:
On 2011/12/09 23:08:20, supersat wrote:
> On 2011/12/01 23:13:02, creis wrote:
> > Is this a worthwhile change independent of the rest of the code, or is it
only
> > needed when we change about:swappedout?  Just wondering if we should land it
> in
> > its own CL.
> 
> It's only needed if we change about:swappedout.

Ok.  It's still there in patch set 2, so you should probably remove it.

http://codereview.chromium.org/8760024/diff/1/content/browser/browser_context.h
File content/browser/browser_context.h (right):

http://codereview.chromium.org/8760024/diff/1/content/browser/browser_context...
content/browser/browser_context.h:109: FrameMapper frame_mapper_;
On 2011/12/09 23:08:20, supersat wrote:
> On 2011/12/01 23:13:02, creis wrote:
> > Hmm, you might get pushback on this.  It might need to be created on the
> various
> > implementations of BrowserContext (ProfileImpl, OffTheRecordProfile,
> > ShellBrowserContext, TestBrowserContext).
> > 
> > There's a bigger question here if Albert does make isolated apps use a
> different
> > BrowserContext, though.  That's uncertain, but if so, a tab will change its
> > BrowserContext over time (and may send postMessages to tabs in other
> > BrowserContexts).
> 
> Are frames in different browser contexts able to script each other? 

Yes.  Picture an isolated app that opens a window to a different site, then
tries to use postMessage.  The isolated app would be in its own BrowserContext,
but could script windows outside of it.  (This does depart a bit from the idea
of an isolated app as living in a separate web browser, but I argue it makes a
lot of sense to allow postMessages across that boundary.)

Note that it's seeming more likely we're going down that path, so maybe we
should find a different place to store this map...

http://codereview.chromium.org/8760024/diff/1/content/browser/browsing_instan...
File content/browser/browsing_instance_frame_id.h (right):

http://codereview.chromium.org/8760024/diff/1/content/browser/browsing_instan...
content/browser/browsing_instance_frame_id.h:15: class BrowsingInstanceFrame {
On 2011/12/09 23:08:20, supersat wrote:
> On 2011/12/01 23:13:02, creis wrote:
> > I don't understand this name.  A BrowsingInstance is a collection of tabs
that
> > can script each other, but this class seems to represent the browser's
notion
> of
> > a frame (either a tab or a sub-frame).
> 
> I agree the name could improve. Any suggestions? This class represents a
> user-perceived frame whose IDs are unique per browsing instance.
> 

I see that you're trying to get the scope of the ID in the class name, but I'm
not sure we need that.  Maybe something like ContentFrame?

http://codereview.chromium.org/8760024/diff/1/content/browser/renderer_host/r...
File content/browser/renderer_host/render_process_host_impl.h (right):

http://codereview.chromium.org/8760024/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/render_process_host_impl.h:161: const
ViewMsg_PostMessage_Params& params);
On 2011/12/09 23:08:20, supersat wrote:
> On 2011/12/01 23:13:02, creis wrote:
> > Why put this on RenderProcessHost?  I would have thought you'd look up the
> > RenderViewHost and send the message from there.
> 
> The ProxyViewHost (which can send this message on behalf of its frame) doesn't
> have a corresponding RenderViewHost. We also don't need anything specific to a
> particular RenderViewHost to route the message to the right destination.

Ah, fair enough.

http://codereview.chromium.org/8760024/diff/1/content/browser/tab_contents/na...
File content/browser/tab_contents/navigation_entry.h (right):

http://codereview.chromium.org/8760024/diff/1/content/browser/tab_contents/na...
content/browser/tab_contents/navigation_entry.h:448: int64
opener_browsing_instance_frame_id_;
On 2011/12/09 23:08:20, supersat wrote:
> On 2011/12/01 23:13:02, creis wrote:
> > Do we need to persist this?  (I suppose it could let tabs script each other
> > after a session restore if we do, which would be pretty cool.)  The comment
> > above suggests we need to be very careful about that, though.
> 
> We'll only need to persist this if we restore scripting connections. However,
> I'm not sure we necessarily want to do that because same-origin scripting
> connections will not be persisted (AFAIK), and I think we should try to be
> consistent.

Let's avoid persisting it for now, then, and we can persist it later if we want
to restore scripting connections.  In that case, please move this to the bottom
and add a comment including the fact that this won't be persisted.

http://codereview.chromium.org/8760024/diff/9001/content/browser/browsing_ins...
File content/browser/browsing_instance_frame_id.h (right):

http://codereview.chromium.org/8760024/diff/9001/content/browser/browsing_ins...
content/browser/browsing_instance_frame_id.h:15: class BrowsingInstanceFrame {
Don't forget comments in this class.  It's basically in charge of knowing how to
reach all active and non-active WebKit Frames for any frame the browser knows
about, right?

http://codereview.chromium.org/8760024/diff/9001/content/browser/browsing_ins...
content/browser/browsing_instance_frame_id.h:16: friend class FrameMap;
Why not just use setters?

http://codereview.chromium.org/8760024/diff/9001/content/browser/browsing_ins...
content/browser/browsing_instance_frame_id.h:18: public:
Indent is wrong.

http://codereview.chromium.org/8760024/diff/9001/content/browser/frame_map.cc
File content/browser/frame_map.cc (right):

http://codereview.chromium.org/8760024/diff/9001/content/browser/frame_map.cc...
content/browser/frame_map.cc:21: bool is_top_level) {
Indent wrong here and below.

http://codereview.chromium.org/8760024/diff/9001/content/browser/frame_map.cc...
content/browser/frame_map.cc:82: {
Brace on previous line.

http://codereview.chromium.org/8760024/diff/9001/content/browser/frame_map.h
File content/browser/frame_map.h (right):

http://codereview.chromium.org/8760024/diff/9001/content/browser/frame_map.h#...
content/browser/frame_map.h:14: class CONTENT_EXPORT FrameMap {
Comments needed in this class, too.

http://codereview.chromium.org/8760024/diff/9001/content/browser/frame_map.h#...
content/browser/frame_map.h:20: BrowsingInstanceFrame* InitializeFrameId(int64
frame_id, bool is_top_level);
Drop the "Id" from this and AllocateNewTopLevelFrameId?

http://codereview.chromium.org/8760024/diff/9001/content/browser/frame_map.h#...
content/browser/frame_map.h:23: BrowsingInstanceFrame* FindById(int64 id);
The "By" part seems redundant to the argument list, though I realize we don't
allow overloading.

Maybe FindFrame(id), FindRendererFrame(process_id, frame_id), and
FindTopLevelFrame(process_id, route_id)?

http://codereview.chromium.org/8760024/diff/9001/content/browser/renderer_hos...
File content/browser/renderer_host/render_process_host_impl.cc (right):

http://codereview.chromium.org/8760024/diff/9001/content/browser/renderer_hos...
content/browser/renderer_host/render_process_host_impl.cc:1335: const
ViewMsg_PostMessage_Params& params) {
Line length

http://codereview.chromium.org/8760024/diff/9001/content/browser/renderer_hos...
content/browser/renderer_host/render_process_host_impl.cc:1343: if (!frame) {
No braces on a one line if.

http://codereview.chromium.org/8760024/diff/9001/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/8760024/diff/9001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.h:596: const
ViewMsg_PostMessage_Params& params);
What's this?  It isn't implemented in render_view_host.cc.

http://codereview.chromium.org/8760024/diff/9001/content/browser/tab_contents...
File content/browser/tab_contents/navigation_entry.h (right):

http://codereview.chromium.org/8760024/diff/9001/content/browser/tab_contents...
content/browser/tab_contents/navigation_entry.h:334: // Used to support swapouts
of frames with non-null openers
End comments with a period.

http://codereview.chromium.org/8760024/diff/9001/content/renderer/dom_proxy_i...
File content/renderer/dom_proxy_installer.h (right):

http://codereview.chromium.org/8760024/diff/9001/content/renderer/dom_proxy_i...
content/renderer/dom_proxy_installer.h:14: class DOMProxyInstaller : public
RenderViewObserver {
Do we still need this after moving to interceptPostMessage?

http://codereview.chromium.org/8760024/diff/9001/content/renderer/proxy_view_...
File content/renderer/proxy_view_host.h (right):

http://codereview.chromium.org/8760024/diff/9001/content/renderer/proxy_view_...
content/renderer/proxy_view_host.h:25: class ProxyViewHost : public
WebKit::WebViewClient,
Let's add comments for this class to explain what it's for.

http://codereview.chromium.org/8760024/diff/9001/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8760024/diff/9001/content/renderer/render_view...
content/renderer/render_view_impl.cc:980: // TODO(supersat): support subframes
What else do you need to support subframes?

http://codereview.chromium.org/8760024/diff/9001/content/renderer/render_view...
content/renderer/render_view_impl.cc:3813:
//webview()->mainFrame()->setOpener();
What's the plan for this?

http://codereview.chromium.org/8760024/diff/9001/content/renderer/render_view...
content/renderer/render_view_impl.cc:4120: GURL("about:swappedout"), false);
Looks like no change.  Why reformat this?

http://codereview.chromium.org/8760024/diff/9001/content/renderer/render_view...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/8760024/diff/9001/content/renderer/render_view...
content/renderer/render_view_impl.h:1223: int64
proxy_of_browsing_instance_frame_id_;
This seems a bit confusing to me, partly because of the hard-to-parse name. 
Maybe something like active_content_frame_id_?  (It's -1 if this renderer has
the active frame, correct?)

http://codereview.chromium.org/8760024/diff/9001/content/renderer/render_widg...
File content/renderer/render_widget.cc (left):

http://codereview.chromium.org/8760024/diff/9001/content/renderer/render_widg...
content/renderer/render_widget.cc:218: // most outgoing messages while swapped
out.
We don't want to remove this anymore, do we?

[supersat, 2011-12-15 19:30:47]

I posted a new version of this patch. There are still a few loose ends that need
to be tied up:

1. Proper handling in transfer_navigation_resource_handler.cc.
2. Removing entries from the FrameMap when they no longer exist.
3. Moving FrameMap out of BrowserContext.
4. TESTS!

http://codereview.chromium.org/8760024/diff/1/chrome/common/chrome_content_cl...
File chrome/common/chrome_content_client.cc (right):

http://codereview.chromium.org/8760024/diff/1/chrome/common/chrome_content_cl...
chrome/common/chrome_content_client.cc:307: case
ChromeViewHostMsg_DomOperationResponse::ID:
On 2011/12/12 22:20:36, creis wrote:
> On 2011/12/09 23:08:20, supersat wrote:
> > On 2011/12/01 23:13:02, creis wrote:
> > > Is this a worthwhile change independent of the rest of the code, or is it
> only
> > > needed when we change about:swappedout?  Just wondering if we should land
it
> > in
> > > its own CL.
> > 
> > It's only needed if we change about:swappedout.
> 
> Ok.  It's still there in patch set 2, so you should probably remove it.

Done. Not sure why it wasn't in patch set 2.

http://codereview.chromium.org/8760024/diff/1/content/browser/browser_context.h
File content/browser/browser_context.h (right):

http://codereview.chromium.org/8760024/diff/1/content/browser/browser_context...
content/browser/browser_context.h:109: FrameMapper frame_mapper_;
On 2011/12/12 22:20:36, creis wrote:
> On 2011/12/09 23:08:20, supersat wrote:
> > On 2011/12/01 23:13:02, creis wrote:
> > > Hmm, you might get pushback on this.  It might need to be created on the
> > various
> > > implementations of BrowserContext (ProfileImpl, OffTheRecordProfile,
> > > ShellBrowserContext, TestBrowserContext).
> > > 
> > > There's a bigger question here if Albert does make isolated apps use a
> > different
> > > BrowserContext, though.  That's uncertain, but if so, a tab will change
its
> > > BrowserContext over time (and may send postMessages to tabs in other
> > > BrowserContexts).
> > 
> > Are frames in different browser contexts able to script each other? 
> 
> Yes.  Picture an isolated app that opens a window to a different site, then
> tries to use postMessage.  The isolated app would be in its own
BrowserContext,
> but could script windows outside of it.  (This does depart a bit from the idea
> of an isolated app as living in a separate web browser, but I argue it makes a
> lot of sense to allow postMessages across that boundary.)
> 
> Note that it's seeming more likely we're going down that path, so maybe we
> should find a different place to store this map...

Okay. A BrowsingInstance initially seemed like the best place to put this, but
it seems like a BrowsingInstance can only have one BrowserContext. So, perhaps
we should make this global to the whole browser?

http://codereview.chromium.org/8760024/diff/1/content/browser/browsing_instan...
File content/browser/browsing_instance_frame_id.h (right):

http://codereview.chromium.org/8760024/diff/1/content/browser/browsing_instan...
content/browser/browsing_instance_frame_id.h:15: class BrowsingInstanceFrame {
On 2011/12/12 22:20:36, creis wrote:
> On 2011/12/09 23:08:20, supersat wrote:
> > On 2011/12/01 23:13:02, creis wrote:
> > > I don't understand this name.  A BrowsingInstance is a collection of tabs
> that
> > > can script each other, but this class seems to represent the browser's
> notion
> > of
> > > a frame (either a tab or a sub-frame).
> > 
> > I agree the name could improve. Any suggestions? This class represents a
> > user-perceived frame whose IDs are unique per browsing instance.
> > 
> 
> I see that you're trying to get the scope of the ID in the class name, but I'm
> not sure we need that.  Maybe something like ContentFrame?

SGTM. Renamed.

http://codereview.chromium.org/8760024/diff/1/content/browser/tab_contents/na...
File content/browser/tab_contents/navigation_entry.h (right):

http://codereview.chromium.org/8760024/diff/1/content/browser/tab_contents/na...
content/browser/tab_contents/navigation_entry.h:448: int64
opener_browsing_instance_frame_id_;
On 2011/12/12 22:20:36, creis wrote:
> On 2011/12/09 23:08:20, supersat wrote:
> > On 2011/12/01 23:13:02, creis wrote:
> > > Do we need to persist this?  (I suppose it could let tabs script each
other
> > > after a session restore if we do, which would be pretty cool.)  The
comment
> > > above suggests we need to be very careful about that, though.
> > 
> > We'll only need to persist this if we restore scripting connections.
However,
> > I'm not sure we necessarily want to do that because same-origin scripting
> > connections will not be persisted (AFAIK), and I think we should try to be
> > consistent.
> 
> Let's avoid persisting it for now, then, and we can persist it later if we
want
> to restore scripting connections.  In that case, please move this to the
bottom
> and add a comment including the fact that this won't be persisted.

Done.

http://codereview.chromium.org/8760024/diff/9001/content/browser/browsing_ins...
File content/browser/browsing_instance_frame_id.h (right):

http://codereview.chromium.org/8760024/diff/9001/content/browser/browsing_ins...
content/browser/browsing_instance_frame_id.h:15: class BrowsingInstanceFrame {
On 2011/12/12 22:20:36, creis wrote:
> Don't forget comments in this class.  It's basically in charge of knowing how
to
> reach all active and non-active WebKit Frames for any frame the browser knows
> about, right?

Done.

http://codereview.chromium.org/8760024/diff/9001/content/browser/browsing_ins...
content/browser/browsing_instance_frame_id.h:16: friend class FrameMap;
On 2011/12/12 22:20:36, creis wrote:
> Why not just use setters?

The idea was to make sure that people update ContentFrames through the FrameMap
so it has a consistent view of all ContentFrames. If this is too weird I can
change it.

http://codereview.chromium.org/8760024/diff/9001/content/browser/browsing_ins...
content/browser/browsing_instance_frame_id.h:18: public:
On 2011/12/12 22:20:36, creis wrote:
> Indent is wrong.

Done.

http://codereview.chromium.org/8760024/diff/9001/content/browser/browsing_ins...
content/browser/browsing_instance_frame_id.h:18: public:
On 2011/12/12 22:20:36, creis wrote:
> Indent is wrong.

Done.

http://codereview.chromium.org/8760024/diff/9001/content/browser/frame_map.cc
File content/browser/frame_map.cc (right):

http://codereview.chromium.org/8760024/diff/9001/content/browser/frame_map.cc...
content/browser/frame_map.cc:21: bool is_top_level) {
On 2011/12/12 22:20:36, creis wrote:
> Indent wrong here and below.

Done.

http://codereview.chromium.org/8760024/diff/9001/content/browser/frame_map.h
File content/browser/frame_map.h (right):

http://codereview.chromium.org/8760024/diff/9001/content/browser/frame_map.h#...
content/browser/frame_map.h:14: class CONTENT_EXPORT FrameMap {
On 2011/12/12 22:20:36, creis wrote:
> Comments needed in this class, too.

Done.

http://codereview.chromium.org/8760024/diff/9001/content/browser/frame_map.h#...
content/browser/frame_map.h:20: BrowsingInstanceFrame* InitializeFrameId(int64
frame_id, bool is_top_level);
On 2011/12/12 22:20:36, creis wrote:
> Drop the "Id" from this and AllocateNewTopLevelFrameId?

Done.

http://codereview.chromium.org/8760024/diff/9001/content/browser/frame_map.h#...
content/browser/frame_map.h:23: BrowsingInstanceFrame* FindById(int64 id);
On 2011/12/12 22:20:36, creis wrote:
> The "By" part seems redundant to the argument list, though I realize we don't
> allow overloading.
> 
> Maybe FindFrame(id), FindRendererFrame(process_id, frame_id), and
> FindTopLevelFrame(process_id, route_id)?

Done.

http://codereview.chromium.org/8760024/diff/9001/content/browser/renderer_hos...
File content/browser/renderer_host/render_process_host_impl.cc (right):

http://codereview.chromium.org/8760024/diff/9001/content/browser/renderer_hos...
content/browser/renderer_host/render_process_host_impl.cc:1335: const
ViewMsg_PostMessage_Params& params) {
On 2011/12/12 22:20:36, creis wrote:
> Line length

Done.

http://codereview.chromium.org/8760024/diff/9001/content/browser/renderer_hos...
content/browser/renderer_host/render_process_host_impl.cc:1343: if (!frame) {
On 2011/12/12 22:20:36, creis wrote:
> No braces on a one line if.

Done.

http://codereview.chromium.org/8760024/diff/9001/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/8760024/diff/9001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.h:596: const
ViewMsg_PostMessage_Params& params);
On 2011/12/12 22:20:36, creis wrote:
> What's this?  It isn't implemented in render_view_host.cc.

Old code. Removed.

http://codereview.chromium.org/8760024/diff/9001/content/browser/tab_contents...
File content/browser/tab_contents/navigation_entry.h (right):

http://codereview.chromium.org/8760024/diff/9001/content/browser/tab_contents...
content/browser/tab_contents/navigation_entry.h:334: // Used to support swapouts
of frames with non-null openers
On 2011/12/12 22:20:36, creis wrote:
> End comments with a period.

Done.

http://codereview.chromium.org/8760024/diff/9001/content/renderer/dom_proxy_i...
File content/renderer/dom_proxy_installer.h (right):

http://codereview.chromium.org/8760024/diff/9001/content/renderer/dom_proxy_i...
content/renderer/dom_proxy_installer.h:14: class DOMProxyInstaller : public
RenderViewObserver {
On 2011/12/12 22:20:36, creis wrote:
> Do we still need this after moving to interceptPostMessage?

No. Removed.

http://codereview.chromium.org/8760024/diff/9001/content/renderer/proxy_view_...
File content/renderer/proxy_view_host.h (right):

http://codereview.chromium.org/8760024/diff/9001/content/renderer/proxy_view_...
content/renderer/proxy_view_host.h:25: class ProxyViewHost : public
WebKit::WebViewClient,
On 2011/12/12 22:20:36, creis wrote:
> Let's add comments for this class to explain what it's for.

I'm probably just going to migrate over to creating a swapped-out RenderView for
this to keep things consistent.

http://codereview.chromium.org/8760024/diff/9001/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8760024/diff/9001/content/renderer/render_view...
content/renderer/render_view_impl.cc:980: // TODO(supersat): support subframes
On 2011/12/12 22:20:36, creis wrote:
> What else do you need to support subframes?

It depends on how we do subframes. If we put them in a separate renderview, then
nothing. However, if we keep them in the same renderview, then we need a way to
get a WebFrame based on its WebKit id. There's currently a way to look up a
WebFrame by name, so adding a method to look it up by id shouldn't be too
difficult.

http://codereview.chromium.org/8760024/diff/9001/content/renderer/render_view...
content/renderer/render_view_impl.cc:3813:
//webview()->mainFrame()->setOpener();
On 2011/12/12 22:20:36, creis wrote:
> What's the plan for this?

Old cruft. It's gone.

http://codereview.chromium.org/8760024/diff/9001/content/renderer/render_view...
content/renderer/render_view_impl.cc:4120: GURL("about:swappedout"), false);
On 2011/12/12 22:20:36, creis wrote:
> Looks like no change.  Why reformat this?

This line was changed at one point to use the new swapout method, but I reverted
that change manually. It should now match the original code.

http://codereview.chromium.org/8760024/diff/9001/content/renderer/render_view...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/8760024/diff/9001/content/renderer/render_view...
content/renderer/render_view_impl.h:1223: int64
proxy_of_browsing_instance_frame_id_;
On 2011/12/12 22:20:36, creis wrote:
> This seems a bit confusing to me, partly because of the hard-to-parse name. 
> Maybe something like active_content_frame_id_?  (It's -1 if this renderer has
> the active frame, correct?)

Done. Yes.

http://codereview.chromium.org/8760024/diff/9001/content/renderer/render_widg...
File content/renderer/render_widget.cc (left):

http://codereview.chromium.org/8760024/diff/9001/content/renderer/render_widg...
content/renderer/render_widget.cc:218: // most outgoing messages while swapped
out.
On 2011/12/12 22:20:36, creis wrote:
> We don't want to remove this anymore, do we?

Nope.

[Charlie Reis, 2011-12-15 19:38:34]

Thanks Karl!  Darin, can you take a look at the approach to see if it's
reasonable?

[Matt Perry, 2011-12-20 19:46:35]

drive-by

http://codereview.chromium.org/8760024/diff/34001/chrome/browser/extensions/a...
File chrome/browser/extensions/app_process_apitest.cc (right):

http://codereview.chromium.org/8760024/diff/34001/chrome/browser/extensions/a...
chrome/browser/extensions/app_process_apitest.cc:363: //    third load stop
This comment should be updated. We were trying to explain why 3 navigations
occur for what should have been a 2 navigation event.

I think you've fixed it so this redirect happens in 2 hops, like it should.
First navigation is to path1/redirect.html, which loads, then completes, then
issues a meta refresh. Second starts out going to localhost/server-redirect,
then redirects back to path1/empty.html, then completes.

[awong, 2011-12-21 01:06:38]

Hey Karl,

I took a first pass at ContentFrame + FrameMap.

So far, my comments are all style + low-level code factoring, but I wanted to
send them to you first.

I'll keep chugging along and see how these classes weave in with everything
else.

http://codereview.chromium.org/8760024/diff/34001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/render_view_host_delegate_helper.h (right):

http://codereview.chromium.org/8760024/diff/34001/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_host_delegate_helper.h:70: TabContents*
CreateNewWindowFromTabContents(
This function needs to be renamed.

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
File content/browser/content_frame.cc (right):

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.cc:22: const WebKitFrameIdTuple& o) const {
single letter variable names are frowned upon.

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.cc:37: new Observer(this);
Doesn't this leak?

I think you need to keep the Observer in a scoped_ptr<>.  Then you can also just
memberwise initalize this too.

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.cc:46: WebKitFrameIdTuple
newWebKitTuple(new_process_host_id, new_route_id,
local variables are lowercase_with_underscores.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Explic...

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.cc:51: // TODO(supersat): Clear out subframes?
Explain why?

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.cc:54: ContentFrame::Observer::Observer(
Can be on one line.

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
File content/browser/content_frame.h (right):

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:12: #include <list>
C++ includes go first. Separate by a newline.

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:14: namespace content {
newline after namespace opener.

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:21: // to the WebKit frame that's currently
active.
For single class files, It's generally preferable to stuff this as the file
comment.

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:23: friend class FrameMap;
Ick. friends. I don't like friends. That's probably why I don't have any.

Can we get rid of your friends too? ;)

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:26: // TODO(supersat): Comment this once I
figure out exactly what it'll contain
Can we pull this out into the content namespace?  Nested classes are annoying. 
Since this is an identifier that is used external to this class, it feels like
it should be de-nested.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Nested...

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:27: struct WebKitFrameIdTuple {
Assuming this doesn't get fragmented/renamed into smaller structs, I would drop
the "Tuple" from the name. It doesn't add more any more description to the name,
and "struct" is more of a record type than a tuple since the members are
label-based rather than positional.

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:30: // The == operator does NOT compare the
route_id!
Is it possible to pull route_id out of this struct then?

It sounds like you have the identifying pair (process_host_id, frame_id), and
then you have the data it maps to (route_id).  This struct seems to comingle the
two ideas.

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:31: bool operator==(const WebKitFrameIdTuple&
other) const;
Chromium style does not allow operator overloading.  Use an external comparator
instead?

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Operat...

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:42: virtual ~ContentFrame();
No methods are virtual.  Why is the destructor virtual?

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:49: const WebKitFrameIdTuple
current_webkit_frame() const {
What do you think about "active" instead of "current" as variable/API name? 
That's the adjective you're using in the comments, and it seems more
descriptive/less "context dependent" than "current."

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:54: // frame.
These are duplicate accessors for data that can be retrieved via
current_webkit_frame().  Remove one API?

Having 2 mechanisms to access the same data make it harder to trace down state
dependencies for future maintainers.

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:66: // this frame. Note that the WebKit frame id
is only unique per-process!
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Nested...


The first comment seems pretty obvious from the function name.  I would bias
towards describing what these are in the class comment and then letting the
accessors just stay uncommented unless you have a non-obvious caveat.

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:84: const std::list<WebKitFrameIdTuple>
all_webkit_frames() const {
const&?

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:89: void UpdateFrame(int new_process_host_id,
How about SetActiveFrame() or ReplaceActiveFrame() or ChangeActiveFrame()?

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:101: Observer(ContentFrame* frame);
explicit.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Explic...

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:136: std::size_t operator()(const
bad wrap...

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:138: return p.process_host_id * 257 +
p.frame_id;
Magic number! Explain?

http://codereview.chromium.org/8760024/diff/34001/content/browser/frame_map.cc
File content/browser/frame_map.cc (right):

http://codereview.chromium.org/8760024/diff/34001/content/browser/frame_map.c...
content/browser/frame_map.cc:65: return 0;
NULL.

http://codereview.chromium.org/8760024/diff/34001/content/browser/frame_map.h
File content/browser/frame_map.h (right):

http://codereview.chromium.org/8760024/diff/34001/content/browser/frame_map.h...
content/browser/frame_map.h:17: // globally-unique identifiers.
Should ContentFrames only be created via FrameMap?

http://codereview.chromium.org/8760024/diff/34001/content/browser/frame_map.h...
content/browser/frame_map.h:18: class CONTENT_EXPORT FrameMap {
Would a better name be ContentFrameMap, ContentFrameManager, or
ContentFrameFactory?

http://codereview.chromium.org/8760024/diff/34001/content/browser/frame_map.h...
content/browser/frame_map.h:21: virtual ~FrameMap();
Why is this virtual?

http://codereview.chromium.org/8760024/diff/34001/content/browser/frame_map.h...
content/browser/frame_map.h:27: ContentFrame* InitializeFrame(int64 frame_id,
Since this returns an object, CreateFrame() is more appropriate.

http://codereview.chromium.org/8760024/diff/34001/content/browser/frame_map.h...
content/browser/frame_map.h:32: ContentFrame* FindFrame(int64 id);
Which id space is |id| from?  In particular, how is it different from
FindRendererFrame()?

http://codereview.chromium.org/8760024/diff/34001/content/browser/frame_map.h...
content/browser/frame_map.h:33: ContentFrame* FindRendererFrame(int
render_process_host_id,
The class comment should explain the various cancepts (eg., TopLeveFrame versus
RendererFrame).

http://codereview.chromium.org/8760024/diff/34001/content/browser/frame_map.h...
content/browser/frame_map.h:60: }  // namespace content
newline before #endif

http://codereview.chromium.org/8760024/diff/34001/content/public/browser/brow...
File content/public/browser/browser_context.h (right):

http://codereview.chromium.org/8760024/diff/34001/content/public/browser/brow...
content/public/browser/browser_context.h:107: FrameMap& frame_mapper() { return
frame_mapper_; }
We should keep BrowserContext a pure interface.  Othewise, this can start
growing into "real inheritance" with all the fragile base class issues that
entails.

Just say no to inheritance.

Also, returning the mapper by reference seems odd. Return it by pointer if it's
modifyable.

[awong, 2011-12-21 01:56:07]

http://codereview.chromium.org/8760024/diff/34001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/render_view_host_delegate_helper.h (right):

http://codereview.chromium.org/8760024/diff/34001/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_host_delegate_helper.h:101:
content::ContentFrame* opener,
The fact that these two APIs take a ContentFrame and return a TabContents makes
me wonder slightly about the relationship between ContentFrame and TabContents.

A ContentFrame is a handle to any WebFrame (top-level of not).

A TabContents seems to be both a handle onto a top-level frame as well as an
anchor point for various bits of navigation data + UI data.

Logically, has the handle to the "top-level" change from being it's TabContents
object to the corresponding ContentFrame object?

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
File content/browser/content_frame.cc (right):

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.cc:100: delete this;
Ah, I see why there is no leak.

Would it make more sense to use a scoped_ptr and to unregister in the destructor
of your Observer class?  Or is this the "right" way to do things?  I don't use
observers much...

http://codereview.chromium.org/8760024/diff/34001/content/browser/frame_map.cc
File content/browser/frame_map.cc (right):

http://codereview.chromium.org/8760024/diff/34001/content/browser/frame_map.c...
content/browser/frame_map.cc:33: std::pair<int64, ContentFrame*>(frame->id(),
frame));
std::make_pair(frame->id(), frame)

http://codereview.chromium.org/8760024/diff/34001/content/browser/frame_map.c...
content/browser/frame_map.cc:37: ContentFrame* FrameMap::FindFrame(int64 id) {
if you lookup a missing ID, this will segfault. Is that intended?

http://codereview.chromium.org/8760024/diff/34001/content/browser/frame_map.c...
content/browser/frame_map.cc:76: DCHECK(frame);
Should this be a CHECK()? If this is a production bug, it'd be easier to track
if we have the callstack right here.

http://codereview.chromium.org/8760024/diff/34001/content/browser/frame_map.c...
content/browser/frame_map.cc:97: std::pair<ContentFrame::WebKitFrameIdTuple,
make_pair

http://codereview.chromium.org/8760024/diff/34001/content/browser/frame_map.c...
content/browser/frame_map.cc:107:
std::list<ContentFrame::WebKitFrameIdTuple>::const_iterator iter;
Scope this in the for loop?

http://codereview.chromium.org/8760024/diff/34001/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

http://codereview.chromium.org/8760024/diff/34001/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:1318: DLOG(WARNING) <<
"OnSendPostMessage";
I assume we'll sweep through and clean up some DLOGs?

http://codereview.chromium.org/8760024/diff/34001/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:1324: // for a frame
that no longer exists. If that's the case, just drop the msg.
Should there be a TODO here as well?

http://codereview.chromium.org/8760024/diff/34001/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:1332: // the mapper
properly.
Can we state the condition on which this should be removed?  Makes it easier for
future maintainers to know if they can safely fix a TODO.

http://codereview.chromium.org/8760024/diff/34001/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/8760024/diff/34001/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host.cc:336: content::FrameMap&
frame_mapper =
Should this be a const_ref?

http://codereview.chromium.org/8760024/diff/34001/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host.cc:806: content::FrameMap&
frame_map =
const ref?

http://codereview.chromium.org/8760024/diff/34001/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/8760024/diff/34001/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host.h:235: void SwapOutAndProxy(int
new_render_process_host_id,
Should this be private?

http://codereview.chromium.org/8760024/diff/34001/content/browser/tab_content...
File content/browser/tab_contents/navigation_controller.h (right):

http://codereview.chromium.org/8760024/diff/34001/content/browser/tab_content...
content/browser/tab_contents/navigation_controller.h:193: content::ContentFrame*
opener = NULL);
Can we get rid of the default-parameter?

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Defaul...

http://codereview.chromium.org/8760024/diff/34001/content/browser/tab_content...
content/browser/tab_contents/navigation_controller.h:364: content::ContentFrame*
opener = NULL);
Same here.

If this is too spiraling of a change, add a TODO and file a bug.

http://codereview.chromium.org/8760024/diff/34001/content/browser/tab_content...
File content/browser/tab_contents/navigation_entry.cc (right):

http://codereview.chromium.org/8760024/diff/34001/content/browser/tab_content...
content/browser/tab_contents/navigation_entry.cc:47:
opener_content_frame_id_(-1) {
There should be a constant for an invalid frame ID.

The id generator should make sure to know to avoid this constant.

http://codereview.chromium.org/8760024/diff/34001/content/browser/tab_content...
File content/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/8760024/diff/34001/content/browser/tab_content...
content/browser/tab_contents/render_view_host_manager.cc:845: // If we already
have a swapped out RVH
Use a period at the end of a comment.

http://codereview.chromium.org/8760024/diff/34001/content/browser/tab_content...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/8760024/diff/34001/content/browser/tab_content...
content/browser/tab_contents/tab_contents.cc:223:
browser_context->frame_mapper().InitializeFrame(
Is it possible for to change InitializeFrame() so that the unique_id is just
generated internally rather than needing us to pass
browser_context->frame_mapper().AllocateFrameId() in by hand?

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

http://codereview.chromium.org/8760024/diff/34001/content/common/view_message...
content/common/view_messages.h:573: // The identifier of the ContentFrame we
need are becoming a proxy for.
s/need//

http://codereview.chromium.org/8760024/diff/34001/content/common/view_message...
content/common/view_messages.h:643: // The route ID of the opener RenderView if
we need to set one (-1 otherwise)
use a constant?

http://codereview.chromium.org/8760024/diff/34001/content/common/view_message...
content/common/view_messages.h:657: IPC_STRUCT_MEMBER(string16, sourceOrigin)
lowercase_and_underscores.

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

http://codereview.chromium.org/8760024/diff/34001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:991: // canBubble and cancellable are
always false
|canBubble| and |cancellable| are always false.

http://codereview.chromium.org/8760024/diff/34001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:3187: if (!targetOrig.isNull())
s/targetOrig/targetOrigin/g

[jam, 2011-12-22 20:06:53]

I glanced through this, here are my initial comments. no major issues stick out
to me re the design.

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
File content/browser/content_frame.cc (right):

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.cc:5: #include "content_frame.h"
nit: full path

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
File content/browser/content_frame.h (right):

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:21: // to the WebKit frame that's currently
active.
On 2011/12/21 01:06:38, awong wrote:
> For single class files, It's generally preferable to stuff this as the file
> comment.

really? i didn't know that.

the style is to actually put one class per file. old-style was to put multiple
classes/structs in a single file, which we now discourage. i find it personally
more readable to have the class comment next to the class itself, as opposed to
separated by all the headers, forward declares etc

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:22: class ContentFrame {
I'm curious why you picked ContentFrame? I ask because it doesn't match the
existing name for process/pages
(RenderProcessHost/RenderViewHost/RenderWidgetHost) and hence RenderFrameHost
would be more consistent?

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:26: // TODO(supersat): Comment this once I
figure out exactly what it'll contain
On 2011/12/21 01:06:38, awong wrote:
> Can we pull this out into the content namespace?  Nested classes are annoying.

> Since this is an identifier that is used external to this class, it feels like
> it should be de-nested.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Nested...
> 

nit: we try to avoid nested classes per the style guide, since then if anyone
else wants to refer to the struct in a header they have to include this whole
header. agree that putting it into content namespace is better, but also in a
separate file

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:27: struct WebKitFrameIdTuple {
On 2011/12/21 01:06:38, awong wrote:
> Assuming this doesn't get fragmented/renamed into smaller structs, I would
drop
> the "Tuple" from the name. It doesn't add more any more description to the
name,
> and "struct" is more of a record type than a tuple since the members are
> label-based rather than positional.

+1

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:35: int64 frame_id;
why use 64 bit numbers for frame id? 32bit is fine for route_ids, and i really
doubt we'll ever wrap around

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:99: class Observer : public TabContentsObserver
{
nit: this class is an internal implementation and doesn't need to be declared in
this header. usually we prefer to put these classes just in the cc file, to keep
the header minimal

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:122: TabContents& tab_contents_;
nit: i've never seen us storing references to TabContents before. usually a
pointer.

http://codereview.chromium.org/8760024/diff/34001/content/browser/frame_map.cc
File content/browser/frame_map.cc (right):

http://codereview.chromium.org/8760024/diff/34001/content/browser/frame_map.c...
content/browser/frame_map.cc:31: DCHECK(frame);
nit: we avoid dchecks on pointers before using them. in release mode, this does
nothing. in debug builds, the crash on the next line is just as good of a signal

http://codereview.chromium.org/8760024/diff/34001/content/browser/frame_map.h
File content/browser/frame_map.h (right):

http://codereview.chromium.org/8760024/diff/34001/content/browser/frame_map.h...
content/browser/frame_map.h:24: int64 AllocateFrameId();
why are these global frame ids? since for each ContentFrame you have a render
process id, at best these only need to be unique per process, and of course
really only needs to be per view

http://codereview.chromium.org/8760024/diff/34001/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/8760024/diff/34001/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host.cc:341: DCHECK(topLevelFrame);
nit: dcheck not needed, will crash below. also top_level_frame

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

http://codereview.chromium.org/8760024/diff/34001/content/common/view_message...
content/common/view_messages.h:644: IPC_STRUCT_MEMBER(int32, opener_route_id)
nit: we usually send routing ids as just ints

[awong, 2011-12-22 20:09:04]

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
File content/browser/content_frame.h (right):

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:21: // to the WebKit frame that's currently
active.
On 2011/12/22 20:06:53, John Abd-El-Malek wrote:
> On 2011/12/21 01:06:38, awong wrote:
> > For single class files, It's generally preferable to stuff this as the file
> > comment.
> 
> really? i didn't know that.
> 
> the style is to actually put one class per file. old-style was to put multiple
> classes/structs in a single file, which we now discourage. i find it
personally
> more readable to have the class comment next to the class itself, as opposed
to
> separated by all the headers, forward declares etc

Yes...Karl and I talked about this and I was mistaken.

[Charlie Reis, 2011-12-22 20:13:21]

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
File content/browser/content_frame.h (right):

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:22: class ContentFrame {
On 2011/12/22 20:06:53, John Abd-El-Malek wrote:
> I'm curious why you picked ContentFrame? I ask because it doesn't match the
> existing name for process/pages
> (RenderProcessHost/RenderViewHost/RenderWidgetHost) and hence RenderFrameHost
> would be more consistent?

Wouldn't that imply that there's a RenderFrame it's communicating with in the
renderer process?  Actually, I suppose there's a WebFrame that this corresponds
to.  Should it be called WebFrameHost?

[supersat, 2011-12-23 03:22:46]

Haven't addressed everything yet...

http://codereview.chromium.org/8760024/diff/34001/chrome/browser/extensions/a...
File chrome/browser/extensions/app_process_apitest.cc (right):

http://codereview.chromium.org/8760024/diff/34001/chrome/browser/extensions/a...
chrome/browser/extensions/app_process_apitest.cc:363: //    third load stop
On 2011/12/20 19:46:36, Matt Perry wrote:
> This comment should be updated. We were trying to explain why 3 navigations
> occur for what should have been a 2 navigation event.
> 
> I think you've fixed it so this redirect happens in 2 hops, like it should.
> First navigation is to path1/redirect.html, which loads, then completes, then
> issues a meta refresh. Second starts out going to localhost/server-redirect,
> then redirects back to path1/empty.html, then completes.

Done.

http://codereview.chromium.org/8760024/diff/34001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/render_view_host_delegate_helper.h (right):

http://codereview.chromium.org/8760024/diff/34001/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_host_delegate_helper.h:70: TabContents*
CreateNewWindowFromTabContents(
On 2011/12/21 01:06:38, awong wrote:
> This function needs to be renamed.

Done.

http://codereview.chromium.org/8760024/diff/34001/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_host_delegate_helper.h:101:
content::ContentFrame* opener,
On 2011/12/21 01:56:07, awong wrote:
> The fact that these two APIs take a ContentFrame and return a TabContents
makes
> me wonder slightly about the relationship between ContentFrame and
TabContents.
> 
> A ContentFrame is a handle to any WebFrame (top-level of not).
> 
> A TabContents seems to be both a handle onto a top-level frame as well as an
> anchor point for various bits of navigation data + UI data.
> 
> Logically, has the handle to the "top-level" change from being it's
TabContents
> object to the corresponding ContentFrame object?

When the opener TabContents was passed in before, it was only used to copy the
size of the TabContents
(http://google.com/codesearch#OAMlx_jo-ck/src/content/browser/tab_contents/tab...).
Now we're also setting the opener of the top-level ContentFrame in TabContents.
This could be a subframe of another top-level frame, so we need to pass in a
ContentFrame instead. However, we still copy the container size from the
ContentFrame's enclosing TabContents.

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
File content/browser/content_frame.h (right):

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:12: #include <list>
On 2011/12/21 01:06:38, awong wrote:
> C++ includes go first. Separate by a newline.

Done.

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:14: namespace content {
On 2011/12/21 01:06:38, awong wrote:
> newline after namespace opener.

Done.

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:21: // to the WebKit frame that's currently
active.
On 2011/12/21 01:06:38, awong wrote:
> For single class files, It's generally preferable to stuff this as the file
> comment.

Discussed offline. Agreed that keeping the comment here is most consistent.

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:23: friend class FrameMap;
On 2011/12/21 01:06:38, awong wrote:
> Ick. friends. I don't like friends. That's probably why I don't have any.
> 
> Can we get rid of your friends too? ;)

Everyone seems to be hating on friends, so I'll remove it. The idea was to
prevent anyone but the FrameMap from updating the ContentFrame, since the maps
in FrameMap need to stay in sync with the ContentFrame.

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:26: // TODO(supersat): Comment this once I
figure out exactly what it'll contain
On 2011/12/21 01:06:38, awong wrote:
> Can we pull this out into the content namespace?  Nested classes are annoying.

> Since this is an identifier that is used external to this class, it feels like
> it should be de-nested.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Nested...
> 

Done.

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:27: struct WebKitFrameIdTuple {
On 2011/12/21 01:06:38, awong wrote:
> Assuming this doesn't get fragmented/renamed into smaller structs, I would
drop
> the "Tuple" from the name. It doesn't add more any more description to the
name,
> and "struct" is more of a record type than a tuple since the members are
> label-based rather than positional.

Done.

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:35: int64 frame_id;
On 2011/12/22 20:06:53, John Abd-El-Malek wrote:
> why use 64 bit numbers for frame id? 32bit is fine for route_ids, and i really
> doubt we'll ever wrap around

WebFrame's identifier is 64-bit. I'm not sure why it's 64-bit (it was this way
before WebFrame was upstreamed into WebKit), but it is.

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:42: virtual ~ContentFrame();
On 2011/12/21 01:06:38, awong wrote:
> No methods are virtual.  Why is the destructor virtual?

Done.

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:49: const WebKitFrameIdTuple
current_webkit_frame() const {
On 2011/12/21 01:06:38, awong wrote:
> What do you think about "active" instead of "current" as variable/API name? 
> That's the adjective you're using in the comments, and it seems more
> descriptive/less "context dependent" than "current."

Done.

http://codereview.chromium.org/8760024/diff/34001/content/browser/content_fra...
content/browser/content_frame.h:54: // frame.
On 2011/12/21 01:06:38, awong wrote:
> These are duplicate accessors for data that can be retrieved via
> current_webkit_frame().  Remove one API?
> 
> Having 2 mechanisms to access the same data make it harder to trace down state
> dependencies for future maintainers.

Done.

http://codereview.chromium.org/8760024/diff/34001/content/browser/tab_content...
File content/browser/tab_contents/navigation_entry.cc (right):

http://codereview.chromium.org/8760024/diff/34001/content/browser/tab_content...
content/browser/tab_contents/navigation_entry.cc:47:
opener_content_frame_id_(-1) {
On 2011/12/21 01:56:07, awong wrote:
> There should be a constant for an invalid frame ID.
> 
> The id generator should make sure to know to avoid this constant.

Done.

http://codereview.chromium.org/8760024/diff/34001/content/browser/tab_content...
File content/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/8760024/diff/34001/content/browser/tab_content...
content/browser/tab_contents/render_view_host_manager.cc:845: // If we already
have a swapped out RVH
On 2011/12/21 01:56:07, awong wrote:
> Use a period at the end of a comment.

Done.

http://codereview.chromium.org/8760024/diff/34001/content/browser/tab_content...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/8760024/diff/34001/content/browser/tab_content...
content/browser/tab_contents/tab_contents.cc:223:
browser_context->frame_mapper().InitializeFrame(
On 2011/12/21 01:56:07, awong wrote:
> Is it possible for to change InitializeFrame() so that the unique_id is just
> generated internally rather than needing us to pass
> browser_context->frame_mapper().AllocateFrameId() in by hand?

Previously, no. Now, yes.

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

http://codereview.chromium.org/8760024/diff/34001/content/common/view_message...
content/common/view_messages.h:573: // The identifier of the ContentFrame we
need are becoming a proxy for.
On 2011/12/21 01:56:07, awong wrote:
> s/need//

Done.

http://codereview.chromium.org/8760024/diff/34001/content/common/view_message...
content/common/view_messages.h:643: // The route ID of the opener RenderView if
we need to set one (-1 otherwise)
On 2011/12/21 01:56:07, awong wrote:
> use a constant?

Done.

http://codereview.chromium.org/8760024/diff/34001/content/common/view_message...
content/common/view_messages.h:644: IPC_STRUCT_MEMBER(int32, opener_route_id)
On 2011/12/22 20:06:53, John Abd-El-Malek wrote:
> nit: we usually send routing ids as just ints

Done.

http://codereview.chromium.org/8760024/diff/34001/content/common/view_message...
content/common/view_messages.h:657: IPC_STRUCT_MEMBER(string16, sourceOrigin)
On 2011/12/21 01:56:07, awong wrote:
> lowercase_and_underscores.

Done.

http://codereview.chromium.org/8760024/diff/34001/content/public/browser/brow...
File content/public/browser/browser_context.h (right):

http://codereview.chromium.org/8760024/diff/34001/content/public/browser/brow...
content/public/browser/browser_context.h:107: FrameMap& frame_mapper() { return
frame_mapper_; }
On 2011/12/21 01:06:38, awong wrote:
> We should keep BrowserContext a pure interface.  Othewise, this can start
> growing into "real inheritance" with all the fragile base class issues that
> entails.
> 
> Just say no to inheritance.
> 
> Also, returning the mapper by reference seems odd. Return it by pointer if
it's
> modifyable.

Done.

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

http://codereview.chromium.org/8760024/diff/34001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:991: // canBubble and cancellable are
always false
On 2011/12/21 01:56:07, awong wrote:
> |canBubble| and |cancellable| are always false.

Done.

http://codereview.chromium.org/8760024/diff/34001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:3187: if (!targetOrig.isNull())
On 2011/12/21 01:56:07, awong wrote:
> s/targetOrig/targetOrigin/g

Done.