Decouple RVH creation from CrossProcessFrameConnector.

Decouple RVH creation from CrossProcessFrameConnector.

The RenderViewHost creation today takes a CrossProcessFrameConnector pointer, which it uses to decide whether to create a top-level or child frame view. Replace this with a boolean and associate the CrossProcessFrameConnector with the view at a later point in time.

BUG=357747
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271461

[kenrb, 2014-05-08 19:56:14]

https://codereview.chromium.org/270883003/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/270883003/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:1140:
old_render_frame_host->render_view_host()->GetView();
Which RenderViewHost does old_render_frame_host->render_view_host() return?

[nasko, 2014-05-09 22:33:02]

Nick or Ken,
Can you review this CL for me? 
Once it looks fine, feel free to send it to the CQ, as our working schedules
might not overlap for the next week or so.
Thanks!
Nasko

[ncarter (slow), 2014-05-10 00:32:42]

lgtm with one suggestion and one nit

https://codereview.chromium.org/270883003/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/cross_process_frame_connector.cc (right):

https://codereview.chromium.org/270883003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/cross_process_frame_connector.cc:60:
OnInitializeChildFrame(child_frame_rect_, device_scale_factor_);
|device_scale_factor_| will be ignored by OnInitializeChildFrame because of its
idempotence check. It doesn't matter in practice, I think, because of how
RenderWidgetHostImpl will look up the screen_info_ on the first WasResized() --
but I think it might be best to avoid this pattern anyhow.

So it might be better to have both OnInitializeChildFrame and this function call
into a shared helper that updates the view based on the values of
child_frame_rect_ and device_scale_factor_ ? Or something like that?

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

https://codereview.chromium.org/270883003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.h:63: //
InitWithExistingID.
I was going to ask if you should document |main_frame| -- but does the current
comment match what this method does at all? e.g. InitWithExistingID doesn't seem
to exist, and I wonder whether the "automatically called from LoadURL" is
accurate now either.

[nasko, 2014-05-16 16:17:30]

Fixes based on Nick's review.

Charlie, can you do an OWNERS review for content/?
Thanks!

https://codereview.chromium.org/270883003/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/cross_process_frame_connector.cc (right):

https://codereview.chromium.org/270883003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/cross_process_frame_connector.cc:60:
OnInitializeChildFrame(child_frame_rect_, device_scale_factor_);
On 2014/05/10 00:32:43, ncarter wrote:
> |device_scale_factor_| will be ignored by OnInitializeChildFrame because of
its
> idempotence check. It doesn't matter in practice, I think, because of how
> RenderWidgetHostImpl will look up the screen_info_ on the first WasResized()
--
> but I think it might be best to avoid this pattern anyhow.
> 
> So it might be better to have both OnInitializeChildFrame and this function
call
> into a shared helper that updates the view based on the values of
> child_frame_rect_ and device_scale_factor_ ? Or something like that?

Done.

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

https://codereview.chromium.org/270883003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.h:63: //
InitWithExistingID.
On 2014/05/10 00:32:43, ncarter wrote:
> I was going to ask if you should document |main_frame| -- but does the current
> comment match what this method does at all? e.g. InitWithExistingID doesn't
seem
> to exist, and I wonder whether the "automatically called from LoadURL" is
> accurate now either.

The LoadURL is indeed calling into this method. I'm not sure I'd call it
"automatically", but I think it implies that the caller of LoadURL doesn't need
to make a call to this method explicitly.

The InitWithExistingID comment I'm removing in another CL of mine :).

[Charlie Reis, 2014-05-16 17:04:06]

LGTM with nits.

I think you mentioned we don't have any tests yet that could catch regressions
here, since this is about the view?  (I don't think
SitePerProcessTest.CrossSiteIframe would catch it, right?)  That's something we
should find a way to add soon.

https://codereview.chromium.org/270883003/diff/100001/content/browser/frame_h...
File content/browser/frame_host/cross_process_frame_connector.h (right):

https://codereview.chromium.org/270883003/diff/100001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.h:104: void
SetDeveiceScaleFactor(float scale_factor);
nit: typo in Device

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

https://codereview.chromium.org/270883003/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:60: // automatically
called from LoadURL. |main_frame| indicates whether this
This comment doesn't quite make sense, because a RenderViewHost can be used for
both a main frame and a subframe during its lifetime.  I think we want to say it
indicates whether |render_view_host| is being created for a main frame.

https://codereview.chromium.org/270883003/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:61: // RenderViewHost is
used to render a top-level frame or subframe, so the
nit: drop "or subframe"

https://codereview.chromium.org/270883003/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:69: bool main_frame) = 0;
nit: for_main_frame
(for the same reason mentioned above)

https://codereview.chromium.org/270883003/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:384: bool main_frame);
nit: for_main_frame

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

https://codereview.chromium.org/270883003/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:524: bool main_frame) OVERRIDE;
nit: for_main_frame

[nasko, 2014-05-16 17:26:02]

I've added an item line for writing pixel tests in our TODO spreadsheet, as I
don't know how to write such a test at this time.

https://codereview.chromium.org/270883003/diff/100001/content/browser/frame_h...
File content/browser/frame_host/cross_process_frame_connector.h (right):

https://codereview.chromium.org/270883003/diff/100001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.h:104: void
SetDeveiceScaleFactor(float scale_factor);
On 2014/05/16 17:04:06, Charlie Reis(OOO as of May 17) wrote:
> nit: typo in Device

Done.

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

https://codereview.chromium.org/270883003/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:60: // automatically
called from LoadURL. |main_frame| indicates whether this
On 2014/05/16 17:04:06, Charlie Reis(OOO as of May 17) wrote:
> This comment doesn't quite make sense, because a RenderViewHost can be used
for
> both a main frame and a subframe during its lifetime.  I think we want to say
it
> indicates whether |render_view_host| is being created for a main frame.

At this point in time, I don't think RVH can be reused for both main frame and
subframes. We need to tackle this, but I'm guessing it will come as we make
RenderWidgetHost be owned by RFH and no longer needing the RVH for the widget
part of it.

https://codereview.chromium.org/270883003/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:61: // RenderViewHost is
used to render a top-level frame or subframe, so the
On 2014/05/16 17:04:06, Charlie Reis(OOO as of May 17) wrote:
> nit: drop "or subframe"

Done.

https://codereview.chromium.org/270883003/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:69: bool main_frame) = 0;
On 2014/05/16 17:04:06, Charlie Reis(OOO as of May 17) wrote:
> nit: for_main_frame
> (for the same reason mentioned above)

Done.

https://codereview.chromium.org/270883003/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:384: bool main_frame);
On 2014/05/16 17:04:06, Charlie Reis(OOO as of May 17) wrote:
> nit: for_main_frame

Done.

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

https://codereview.chromium.org/270883003/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:524: bool main_frame) OVERRIDE;
On 2014/05/16 17:04:06, Charlie Reis(OOO as of May 17) wrote:
> nit: for_main_frame

Done.

[kenrb, 2014-05-16 17:44:54]

I'm fine with this, just a question.

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

https://codereview.chromium.org/270883003/diff/120001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1149: // If this is a
subframe, it should have a CrossProcessFrameConnector
Would it make sense to make CrossProcessFrameConnector entirely owned and
lifetime managed by RenderFrameProxyHost?

[nasko, 2014-05-16 18:24:17]

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

https://codereview.chromium.org/270883003/diff/120001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1149: // If this is a
subframe, it should have a CrossProcessFrameConnector
On 2014/05/16 17:44:54, kenrb wrote:
> Would it make sense to make CrossProcessFrameConnector entirely owned and
> lifetime managed by RenderFrameProxyHost?

I think it might make sense. It will require some changes though, as currently
it relies on the RenderFrame to send it sizing information and if it moves to be
owned by RenderFrameProxyHost, then sizing needs to come through the proxy
object (not hard, just change in behavior).

Since it doesn't expect to receive any IPCs directly from the RFH rendering the
frame in the new process, it makes even more sense.

It will still be 1:1 with RenderWidgetHostViewChildFrame though, correct? Which
means that messages from another proxy will have to trace up the tree to get to
the connector. I think this still holds true even if RFHM was to own it, but we
don't have that today.

Is it ok if this work is done in a different CL or should it be done now as part
of this one?

[Charlie Reis, 2014-05-16 18:38:56]

Minor nit: Is there a reasonable bug number to list in the CL description? 
(Perhaps this is related to the proxy effort?)

[nasko, 2014-05-16 18:47:52]

On 2014/05/16 18:38:56, Charlie Reis(OOO as of May 17) wrote:
> Minor nit: Is there a reasonable bug number to list in the CL description? 
> (Perhaps this is related to the proxy effort?)

Yes, fixed.

[kenrb, 2014-05-16 19:01:00]

lgtm

On 2014/05/16 18:24:17, nasko wrote:
> It will still be 1:1 with RenderWidgetHostViewChildFrame though, correct?
Which
> means that messages from another proxy will have to trace up the tree to get
to
> the connector. I think this still holds true even if RFHM was to own it, but
we
> don't have that today.
> 

It is 1:1 with RWHVChildFrame (one useful point, though, is that RWHVChildFrame
can change over the lifetime of a CrossProcessFrameConnector but it should only
ever be attached to one RenderFrameProxyHost).

I don't think there should have to be anything having to trace up to the tree to
get to the frame connector. In the renderer process, all RenderFrames should use
the same RenderWidget, which will send messages to RenderWidgetHost and
RenderWidgetHostView. CrossProcessFrameConnector should not be visible to the
RenderFrames and RenderFrameHosts under the RenderFrameHostManager that it is
associated with.

> Is it ok if this work is done in a different CL or should it be done now as
part
> of this one?

A TODO would be good, it was mostly just an observation.

[nasko, 2014-05-16 19:09:36]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2014-05-16 19:10:44]

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

[commit-bot: I haz the power, 2014-05-17 05:40:26]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-17 07:03:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-17 07:03:40]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[nasko, 2014-05-17 07:48:32]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2014-05-17 07:49:56]

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

[commit-bot: I haz the power, 2014-05-17 09:04:18]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-17 09:55:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-17 09:55:38]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[nasko, 2014-05-18 22:08:49]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2014-05-18 22:08:56]

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

[commit-bot: I haz the power, 2014-05-18 23:53:18]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-19 01:12:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-19 01:12:24]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[nasko, 2014-05-19 14:18:14]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2014-05-19 14:18:52]

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

[commit-bot: I haz the power, 2014-05-19 14:19:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-19 14:19:50]

Failed to apply patch for
content/browser/frame_host/render_frame_host_manager.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/frame_host/render_frame_host_manager.cc
  Hunk #3 FAILED at 177.
  Hunk #4 FAILED at 187.
  Hunk #5 succeeded at 503 (offset 1 line).
  Hunk #6 FAILED at 950.
  Hunk #7 FAILED at 968.
  Hunk #8 FAILED at 990.
  Hunk #9 FAILED at 1146.
  6 out of 9 hunks FAILED -- saving rejects to file
content/browser/frame_host/render_frame_host_manager.cc.rej

Patch:       content/browser/frame_host/render_frame_host_manager.cc
Index: content/browser/frame_host/render_frame_host_manager.cc
diff --git a/content/browser/frame_host/render_frame_host_manager.cc
b/content/browser/frame_host/render_frame_host_manager.cc
index
d6429034aa33cc3a773fc7c3bcb3ce82a040d4e7..b7611e92e4f890a371ab1a4be447a9dbc6f64987
100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -22,6 +22,7 @@
 #include "content/browser/frame_host/render_frame_host_factory.h"
 #include "content/browser/frame_host/render_frame_host_impl.h"
 #include "content/browser/frame_host/render_frame_proxy_host.h"
+#include "content/browser/frame_host/render_widget_host_view_child_frame.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
 #include "content/browser/renderer_host/render_view_host_factory.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
@@ -34,6 +35,7 @@
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_widget_host_iterator.h"
+#include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/user_metrics.h"
 #include "content/public/browser/web_ui_controller.h"
 #include "content/public/common/content_switches.h"
@@ -175,7 +177,8 @@ RenderFrameHostImpl* RenderFrameHostManager::Navigate(
     // Note: we don't call InitRenderView here because we are navigating away
     // soon anyway, and we don't have the NavigationEntry for this host.
     delegate_->CreateRenderViewForRenderManager(
-        render_frame_host_->render_view_host(), MSG_ROUTING_NONE, NULL);
+        render_frame_host_->render_view_host(), MSG_ROUTING_NONE,
+        frame_tree_node_->IsMainFrame());
   }
 
   // If the renderer crashed, then try to create a new one to satisfy this
@@ -185,7 +188,7 @@ RenderFrameHostImpl* RenderFrameHostManager::Navigate(
     int opener_route_id = delegate_->CreateOpenerRenderViewsForRenderManager(
         dest_render_frame_host->GetSiteInstance());
     if (!InitRenderView(dest_render_frame_host->render_view_host(),
-                        opener_route_id))
+                        opener_route_id, frame_tree_node_->IsMainFrame()))
       return NULL;
 
     // Now that we've created a new renderer, be sure to hide it if it isn't
@@ -500,6 +503,9 @@ void RenderFrameHostManager::SwapOutOldPage() {
     // process navigations, but it will be destroyed if the Frame is
     // navigated back to the same site instance as its parent.
     // TODO(kenrb): This will change when RenderFrameProxyHost is created.
+    // TODO(nasko): Move CrossProcessFrameConnector to be owned by
+    // RenderFrameProxyHost instead of RenderFrameHostManager once proxy
+    // support lands.
     if (!cross_process_frame_connector_) {
       cross_process_frame_connector_ =
           new CrossProcessFrameConnector(render_frame_host_.get());
@@ -945,7 +951,8 @@ int RenderFrameHostManager::CreateRenderFrame(
           new_render_frame_host.Pass());
     }
 
-    bool success = InitRenderView(render_view_host, opener_route_id);
+    bool success = InitRenderView(
+        render_view_host, opener_route_id, frame_tree_node_->IsMainFrame());
     if (success && frame_tree_node_->IsMainFrame()) {
       // Don't show the main frame's view until we get a DidNavigate from it.
       render_view_host->GetView()->Hide();
@@ -963,7 +970,8 @@ int RenderFrameHostManager::CreateRenderFrame(
 }
 
 bool RenderFrameHostManager::InitRenderView(RenderViewHost* render_view_host,
-                                            int opener_route_id) {
+                                            int opener_route_id,
+                                            bool for_main_frame) {
   // We may have initialized this RenderViewHost for another RenderFrameHost.
   if (render_view_host->IsRenderViewLive())
     return true;
@@ -985,7 +993,7 @@ bool RenderFrameHostManager::InitRenderView(RenderViewHost*
render_view_host,
   }
 
   return delegate_->CreateRenderViewForRenderManager(
-      render_view_host, opener_route_id, cross_process_frame_connector_);
+      render_view_host, opener_route_id, for_main_frame);
 }
 
 void RenderFrameHostManager::CommitPending() {
@@ -1141,6 +1149,16 @@ void RenderFrameHostManager::CommitPending() {
       old_render_frame_host.reset();
       ShutdownRenderFrameHostsInSiteInstance(old_site_instance_id);
     } else {
+      // If this is a subframe, it should have a CrossProcessFrameConnector
+      // created already and we just need to link it to the proper view in the
+      // new process.
+      if (!is_main_frame) {
+        RenderWidgetHostView* rwhv =
+            render_frame_host_->render_view_host()->GetView();
+        RenderWidgetHostViewChildFrame* rwhv_child =
+            static_cast<RenderWidgetHostViewChildFrame*>(rwhv);
+        cross_process_frame_connector_->set_view(rwhv_child);
+      }
       proxy_hosts_[old_site_instance_id] = new RenderFrameProxyHost(
           old_render_frame_host.Pass());
     }

[nasko, 2014-05-19 15:29:34]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2014-05-19 15:30:06]

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

[commit-bot: I haz the power, 2014-05-19 18:59:57]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-19 20:16:50]

Change committed as 271461

[boliu, 2014-05-20 01:49:56]

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

https://codereview.chromium.org/270883003/diff/180001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3835: NULL);
The android_webview failure is caused by this line. The last param should be
updated to true.

[nasko, 2014-05-20 17:30:22]

I'll fix this in https://codereview.chromium.org/299703002.

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

https://codereview.chromium.org/270883003/diff/180001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3835: NULL);
On 2014/05/20 01:49:57, boliu wrote:
> The android_webview failure is caused by this line. The last param should be
> updated to true.

Thanks for catching this!