Start using RenderFrameProxyHost objects.

Start using RenderFrameProxyHost objects.

BUG=

[ncarter (slow), 2014-06-25 01:07:57]

Publishing our comments from our in-pair review.

https://codereview.chromium.org/241223002/diff/140001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/241223002/diff/140001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:212: return iter->second;
(note to self) Do we need to bump the reference count of the rvh here in the
reuse case, or is that handled by the caller?

https://codereview.chromium.org/241223002/diff/140001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.h (right):

https://codereview.chromium.org/241223002/diff/140001/content/browser/frame_h...
content/browser/frame_host/frame_tree.h:75: // given |site_instance| in every
node except the |source| one.
This comment should say "in every node except those currently hosting a frame
belonging to |site_instance|" or something.

https://codereview.chromium.org/241223002/diff/140001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_browsertest.cc (right):

https://codereview.chromium.org/241223002/diff/140001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:211:
EXPECT_TRUE(root->render_manager()->proxy_hosts_[site_instance->GetId()]);
Should we also check:

EXPECT_TRUE(child->render_manager()->proxy_hosts_[shell()->web_contents()->GetSiteInstance()->GetId()]);

?

https://codereview.chromium.org/241223002/diff/140001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/241223002/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:350: int parent_routing_id)
{
NIT PLEASE INDENT FOUR SPACES OR USE CLANG FORMAT GOSH.

https://codereview.chromium.org/241223002/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:371: return
GetProcess()->HasConnection() && renderer_initialized_;
If I recall correctly, RVHImpl::IsRenderFrameLive would transition to false upon
a crash. It may be important that we guarantee that semantic here, as well (I
know the task manager wants to know about dead frames, and isn't currently told)

https://codereview.chromium.org/241223002/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:591:
has_received_swap_out_ack_ = false;
Do we need this at all? (when looking through this, nasko said we might not)

https://codereview.chromium.org/241223002/diff/140001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/241223002/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:76: bool
CreateRenderFrame(int parent_routing_id);
A comment for this function that helps clarify exactly what |parent_routing_id|
means would be useful (e.g., how is it different from
GetParent()->GetRoutingID() ?)

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

https://codereview.chromium.org/241223002/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1046:
proxy_hosts_[instance->GetId()] = proxy;
Probably worthwhile to insert while checking for duplicates (should be satisfied
since we call GetRenderFrameProxyHost above)

https://codereview.chromium.org/241223002/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1549:
proxy_hosts_[site_instance->GetId()] = proxy;
Is there some invariant that guarantees that a proxy host for this site instance
doesn't already exist?

https://codereview.chromium.org/241223002/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1554:
proxy_hosts_[site_instance->GetId()] = proxy;
I'm fairly certain that the proxy is already in the map at this point.

Details of proof left as an exercise to the reader.

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

https://codereview.chromium.org/241223002/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:308: int
GetRoutingIdForSiteInstance(SiteInstance* site_instance);
A great comment here would be one that indicates that the returned routing ID
could be a proxy or a frame.

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

https://codereview.chromium.org/241223002/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:119: parent_routing_id =
frame_tree_node_->parent()->render_manager()->
I wonder if |parent_routing_id| should/could be a const member set in the ctor
(am worried about weird orderings here where this is called in the middle of a
parent change .... ??!) -- this just looks like something that Would Be Bad if
it were to change dynamically.

https://codereview.chromium.org/241223002/diff/140001/content/browser/frame_h...
File content/browser/frame_host/render_frame_proxy_host.h (right):

https://codereview.chromium.org/241223002/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.h:63: return
site_instance_->HasProcess() ? site_instance_->GetProcess() : NULL;
Is this needed? My hunch is that site_instance_->GetProcess() will have been
called by the time you get here.

https://codereview.chromium.org/241223002/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.h:66: // Initializaes the
object and creates the RenderFrameProxy in the process
Initializaes -> typo

https://codereview.chromium.org/241223002/diff/140001/content/renderer/child_...
File content/renderer/child_frame_compositing_helper.cc (right):

https://codereview.chromium.org/241223002/diff/140001/content/renderer/child_...
content/renderer/child_frame_compositing_helper.cc:190:
SkColorSetARGBInline(255, 0, 0, 255));
A+++ would change again excellent color shipped on time.

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

https://codereview.chromium.org/241223002/diff/140001/content/renderer/render...
content/renderer/render_frame_impl.cc:388: blink::WebLocalFrame* web_frame =
parent_web_frame->createLocalChild("", render_frame);
Is "" the correct value for the name of the frame?
If not, is "TODO" the correct value for the prefix of the comment preceding this
line?
If so, ...

https://codereview.chromium.org/241223002/diff/140001/content/renderer/render...
content/renderer/render_frame_impl.cc:711:
//GetContentClient()->SetActiveURL(frame_->document().url());
frame doesn't always have a document. If the frame is local we can use the
document URL -- otherwise there's no document on remote frames.

https://codereview.chromium.org/241223002/diff/140001/content/renderer/render...
content/renderer/render_frame_impl.cc:786: GetWebFrame() ==
render_view_->webview()->mainFrame()) {
Could the second half of this clause be written as a check for null parent? Or
would that be subtly different somehow?

https://codereview.chromium.org/241223002/diff/140001/content/renderer/render...
content/renderer/render_frame_impl.cc:1054: if (is_site_per_process) {
This if check might be redundant -- if we're getting a swap out message to a
child RenderFrame, I think that implies oopif.

https://codereview.chromium.org/241223002/diff/140001/content/renderer/render...
content/renderer/render_frame_impl.cc:1056: // proxy.
Maybe use DeleteSoon to accomplish this TODO here.

https://codereview.chromium.org/241223002/diff/140001/content/renderer/render...
content/renderer/render_frame_impl.cc:1844: if
(frame->parent()->isWebLocalFrame()) {
Might be worth extracting this logic into a utility method (probably doesn't
belong in either RenderFrameImpl or RenderFrameProxy

https://codereview.chromium.org/241223002/diff/140001/content/renderer/render...
content/renderer/render_frame_impl.cc:2455:
frame->top()->document().firstPartyForCookies());
Just "top". It's cleaner.

https://codereview.chromium.org/241223002/diff/140001/content/renderer/render...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/241223002/diff/140001/content/renderer/render...
content/renderer/render_frame_proxy.cc:83: parent_routing_id);
|proxy| here is shadowing |proxy| up there. Strongly recommend putting down
crack pipe.

[kenrb, 2014-07-17 21:54:23]

continued as https://codereview.chromium.org/404613005/ ...