Ensure we properly set PageTransition for iframes.

content/renderer/render_frame_impl.cc

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index aae437eccf675b949c1f14058e2f1c680f971eff..36a362b320e2e8552d243dc6910ffc6f6a837bb7 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -1063,6 +1063,15 @@ void RenderFrameImpl::OnNavigate(
                  << commit_params.frame_to_navigate;
   }
 
+  if (ui::PageTransitionCoreTypeIs(params.common_params.transition,

[Charlie Reis, 2015/03/12 22:54:18]

This needs a comment since it's non-obvious.  I'm guessing the following?

Manual subframe navigation requests from the browser can only occur once the
frame has already committed at least one load.  Since this may be a newly
created RenderFrame for a cross-process navigation, we should ensure the
WebFrame knows this.

[Nate Chapin, 2015/03/13 21:48:36]

Yep. Added a comment to this effect.

+                                   ui::PAGE_TRANSITION_MANUAL_SUBFRAME)) {
+    CHECK(frame_->parent());
+    if (frame_->parent()->isWebRemoteFrame()) {
+        CHECK_EQ(frame, frame_);

[Charlie Reis, 2015/03/12 22:54:18]

nit: Wrong indent.

[Nate Chapin, 2015/03/13 21:48:35]

Done.

+        frame_->setCommittedFirstRealLoad();
+    }
+  }
+
   if (is_reload && !render_view_->history_controller()->GetCurrentEntry()) {
     // We cannot reload if we do not have any history state.  This happens, for
     // example, when recovering from a crash.
@@ -2343,6 +2352,11 @@ void RenderFrameImpl::didStartProvisionalLoad(blink::WebLocalFrame* frame,
     // handle loading of error pages.
     document_state->navigation_state()->set_transition_type(
         ui::PAGE_TRANSITION_AUTO_SUBFRAME);
+  } else if (ui::PageTransitionCoreTypeIs(
+                 document_state->navigation_state()->transition_type(),
+                 ui::PAGE_TRANSITION_LINK)) {
+    document_state->navigation_state()->set_transition_type(
+        ui::PAGE_TRANSITION_MANUAL_SUBFRAME);

[Charlie Reis, 2015/03/12 22:54:18]

This is not obvious from reading the code.  Avi and I have been trying to
reverse engineer a lot of code like this to understand what it means, so it
would help to have a comment here explaining why this is the right thing to do.

[Nate Chapin, 2015/03/13 21:48:36]

Commented.

   }
 
   FOR_EACH_OBSERVER(RenderViewObserver, render_view_->observers(),
@@ -2981,7 +2995,7 @@ void RenderFrameImpl::willSendRequest(
   // top-level frame, which is wrong.
   if (!top_frame || top_frame->isWebRemoteFrame())
     top_frame = frame;
-  WebDataSource* provisional_data_source = top_frame->provisionalDataSource();
+  WebDataSource* provisional_data_source = frame->provisionalDataSource();
   WebDataSource* top_data_source = top_frame->dataSource();
   WebDataSource* data_source =
       provisional_data_source ? provisional_data_source : top_data_source;

[Charlie Reis, 2015/03/12 22:54:18]

I'm pretty confused by this whole block.  Shouldn't they all be using the
current frame's data source, eliminating Nasko's hack?  Or are things like the
navigation_state only stored on the top-level frame?

At any rate, it seems wrong to me to mix the data sources of top_frame and frame
on this line.

[Nate Chapin, 2015/03/13 21:48:35]

Fair point. Converted to all this frame's data sources, except for some
prerendering histogram collection further down that I'm less sure of.