Ensure we properly set PageTransition for iframes.

content/renderer/render_frame_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/render_frame_impl.h"
 
 #include <map>
 #include <string>
 
 #include "base/auto_reset.h"
@@ skipping 1045 matching lines @@
 
   WebFrame* frame = frame_;
   if (!commit_params.frame_to_navigate.empty()) {
     // TODO(nasko): Move this lookup to the browser process.
     frame = render_view_->webview()->findFrameByName(
         WebString::fromUTF8(commit_params.frame_to_navigate));
     CHECK(frame) << "Invalid frame name passed: "
                  << 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.
     is_reload = false;
     cache_policy = WebURLRequest::ReloadIgnoringCacheData;
   }
 
   render_view_->pending_navigation_params_.reset(new NavigationParams(
       common_params, start_params, commit_params, history_params));
 
@@ skipping 1260 matching lines @@
   if (is_top_most) {
     render_view_->set_navigation_gesture(
         WebUserGestureIndicator::isProcessingUserGesture() ?
             NavigationGestureUser : NavigationGestureAuto);
   } else if (ds->replacesCurrentHistoryItem()) {
     // Subframe navigations that don't add session history items must be
     // marked with AUTO_SUBFRAME. See also didFailProvisionalLoad for how we
     // 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(),
                     DidStartProvisionalLoad(frame));
   FOR_EACH_OBSERVER(RenderFrameObserver, observers_, DidStartProvisionalLoad());
 
   Send(new FrameHostMsg_DidStartProvisionalLoadForFrame(
        routing_id_, ds->request().url(), is_transition_navigation));
 }
 
@@ skipping 618 matching lines @@
       }
     }
   }
 
   WebFrame* top_frame = frame->top();
   // TODO(nasko): Hack around asking about top-frame data source. This means
   // for out-of-process iframes we are treating the current frame as the
   // 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.

 
   DocumentState* document_state = DocumentState::FromDataSource(data_source);
   DCHECK(document_state);
   InternalDocumentStateData* internal_data =
       InternalDocumentStateData::FromDocumentState(document_state);
   NavigationState* navigation_state = document_state->navigation_state();
   ui::PageTransition transition_type = navigation_state->transition_type();
   WebDataSource* frame_ds = frame->provisionalDataSource();
   if (frame_ds && frame_ds->isClientRedirect()) {
     transition_type = ui::PageTransitionFromInt(
@@ skipping 1517 matching lines @@
 
 #if defined(ENABLE_BROWSER_CDMS)
 RendererCdmManager* RenderFrameImpl::GetCdmManager() {
   if (!cdm_manager_)
     cdm_manager_ = new RendererCdmManager(this);
   return cdm_manager_;
 }
 #endif  // defined(ENABLE_BROWSER_CDMS)
 
 }  // namespace content