Ensure we properly set PageTransition for iframes.

content/browser/frame_host/navigation_controller_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/browser/frame_host/navigation_controller_impl.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
@@ skipping 887 matching lines @@
 
 NavigationType NavigationControllerImpl::ClassifyNavigation(
     RenderFrameHostImpl* rfh,
     const FrameHostMsg_DidCommitProvisionalLoad_Params& params) const {
   if (params.page_id == -1) {
     // TODO(nasko, creis):  An out-of-process child frame has no way of
     // knowing the page_id of its parent, so it is passing back -1. The
     // semantics here should be re-evaluated during session history refactor
     // (see http://crbug.com/236848). For now, we assume this means the
     // child frame loaded and proceed. Note that this may do the wrong thing
     // for cross-process AUTO_SUBFRAME navigations.

[Avi (use Gerrit), 2015/03/12 20:40:28]

This comment seems out-of-date with this change.

[Nate Chapin, 2015/03/12 20:51:12]

Removed.

-    if (rfh->IsCrossProcessSubframe())
-      return NAVIGATION_TYPE_NEW_SUBFRAME;
+    if (rfh->IsCrossProcessSubframe()) {
+      CHECK(!ui::PageTransitionIsMainFrame(params.transition));
+      if (ui::PageTransitionCoreTypeIs(params.transition,
+                                       ui::PAGE_TRANSITION_MANUAL_SUBFRAME)) {
+        return NAVIGATION_TYPE_NEW_SUBFRAME;
+      } else {
+        return NAVIGATION_TYPE_AUTO_SUBFRAME;
+      }
+    }
 
     // The renderer generates the page IDs, and so if it gives us the invalid
     // page ID (-1) we know it didn't actually navigate. This happens in a few
     // cases:
     //
     // - If a page makes a popup navigated to about blank, and then writes
     //   stuff like a subframe navigated to a real page. We'll get the commit
     //   for the subframe, but there won't be any commit for the outer page.
     //
     // - We were also getting these for failed loads (for example, bug 21849).
@@ skipping 315 matching lines @@
                                     ui::PAGE_TRANSITION_MANUAL_SUBFRAME)) {
     // There was a comment here that said, "This is not user-initiated. Ignore."
     // But this makes no sense; non-user-initiated navigations should be
     // determined to be of type NAVIGATION_TYPE_AUTO_SUBFRAME and sent to
     // RendererDidNavigateAutoSubframe below.
     //
     // This if clause dates back to https://codereview.chromium.org/115919 and
     // the handling of immediate redirects. TODO(avi): Is this still valid? I'm
     // pretty sure that's there's nothing left of that code and that we should
     // take this out.
-    //
-    // Except for cross-process iframes; this doesn't work yet for them.
-    if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kSitePerProcess)) {
-      NOTREACHED();
-    }
-
     DiscardNonCommittedEntriesInternal();
     return;

[Avi (use Gerrit), 2015/03/12 20:40:28]

This is the wrong fix; this would always ignore a mismatch.

Remove this entire if clause and do a simple:

DCHECK(ui::PageTransitionCoreTypeIs(params.transition,
                                    ui::PAGE_TRANSITION_MANUAL_SUBFRAME));

[Nate Chapin, 2015/03/12 20:51:12]

Done.

   }
 
   // Manual subframe navigations just get the current entry cloned so the user
   // can go back or forward to it. The actual subframe information will be
   // stored in the page state for each of those entries. This happens out of
   // band with the actual navigations.
   DCHECK(GetLastCommittedEntry()) << "ClassifyNavigation should guarantee "
                                   << "that a last committed entry exists.";
   NavigationEntryImpl* new_entry =
       new NavigationEntryImpl(*GetLastCommittedEntry());
@@ skipping 549 matching lines @@
     }
   }
 }
 
 void NavigationControllerImpl::SetGetTimestampCallbackForTest(
     const base::Callback<base::Time()>& get_timestamp_callback) {
   get_timestamp_callback_ = get_timestamp_callback;
 }
 
 }  // namespace content