Revert "Ensure we properly set PageTransition for iframes."
This reverts commit 735aa03be45990a5e2feb3c38087e6f76eb920ac AKA refs/heads/master@{#326404}
Reverting because it caused a bug which is currently the top crasher on canary.
Original description: Ensure we properly set PageTransition for iframes.
We currently don't set subframe navigations as manual when it isn't the first
navigation of the iframe. Also, we don't propagate the state correctly in the
case of a cross-process transition.
Original Review URL: https://codereview.chromium.org/1002953004
Original Cr-Commit-Position: refs/heads/master@{#326404}
TBR=creis@chromium.org, japhet@chromium.org
BUG=464014,480839
Review URL: https://codereview.chromium.org/1108863002
Cr-Commit-Position: refs/heads/master@{#327100}
diff --git a/content/browser/frame_host/navigator.h b/content/browser/frame_host/navigator.h
index 3a27601e..8771dd2 100644
--- a/content/browser/frame_host/navigator.h
+++ b/content/browser/frame_host/navigator.h
@@ -101,7 +101,6 @@
const GURL& url,
SiteInstance* source_site_instance,
const Referrer& referrer,
- ui::PageTransition page_transition,
WindowOpenDisposition disposition,
bool should_replace_current_entry,
bool user_gesture) {}
diff --git a/content/browser/frame_host/navigator_impl.cc b/content/browser/frame_host/navigator_impl.cc
index 1f9a3b3..91c3a6d 100644
--- a/content/browser/frame_host/navigator_impl.cc
+++ b/content/browser/frame_host/navigator_impl.cc
@@ -518,7 +518,6 @@
const GURL& url,
SiteInstance* source_site_instance,
const Referrer& referrer,
- ui::PageTransition page_transition,
WindowOpenDisposition disposition,
bool should_replace_current_entry,
bool user_gesture) {
@@ -540,9 +539,9 @@
// redirects. http://crbug.com/311721.
std::vector<GURL> redirect_chain;
RequestTransferURL(render_frame_host, url, source_site_instance,
- redirect_chain, referrer, page_transition, disposition,
- GlobalRequestID(), should_replace_current_entry,
- user_gesture);
+ redirect_chain, referrer, ui::PAGE_TRANSITION_LINK,
+ disposition, GlobalRequestID(),
+ should_replace_current_entry, user_gesture);
}
void NavigatorImpl::RequestTransferURL(
diff --git a/content/browser/frame_host/navigator_impl.h b/content/browser/frame_host/navigator_impl.h
index 95ad6a7..7dd2a02 100644
--- a/content/browser/frame_host/navigator_impl.h
+++ b/content/browser/frame_host/navigator_impl.h
@@ -57,7 +57,6 @@
const GURL& url,
SiteInstance* source_site_instance,
const Referrer& referrer,
- ui::PageTransition page_transition,
WindowOpenDisposition disposition,
bool should_replace_current_entry,
bool user_gesture) override;
diff --git a/content/browser/frame_host/render_frame_host_impl.cc b/content/browser/frame_host/render_frame_host_impl.cc
index 57ba8e25..b7b4ee8e 100644
--- a/content/browser/frame_host/render_frame_host_impl.cc
+++ b/content/browser/frame_host/render_frame_host_impl.cc
@@ -1672,14 +1672,8 @@
TRACE_EVENT1("navigation", "RenderFrameHostImpl::OpenURL", "url",
validated_url.possibly_invalid_spec());
- ui::PageTransition transition = ui::PAGE_TRANSITION_LINK;
- if (frame_tree_node_->parent()) {
- transition = params.should_replace_current_entry
- ? ui::PAGE_TRANSITION_AUTO_SUBFRAME
- : ui::PAGE_TRANSITION_MANUAL_SUBFRAME;
- }
frame_tree_node_->navigator()->RequestOpenURL(
- this, validated_url, source_site_instance, params.referrer, transition,
+ this, validated_url, source_site_instance, params.referrer,
params.disposition, params.should_replace_current_entry,
params.user_gesture);
}
diff --git a/content/browser/security_exploit_browsertest.cc b/content/browser/security_exploit_browsertest.cc
index 0044aa4..62d3b62c 100644
--- a/content/browser/security_exploit_browsertest.cc
+++ b/content/browser/security_exploit_browsertest.cc
@@ -70,7 +70,7 @@
WebContentsImpl* wc = static_cast<WebContentsImpl*>(shell->web_contents());
wc->GetFrameTree()->root()->navigator()->RequestOpenURL(
wc->GetFrameTree()->root()->current_frame_host(), extension_url, nullptr,
- Referrer(), ui::PAGE_TRANSITION_LINK, CURRENT_TAB, false, true);
+ Referrer(), CURRENT_TAB, false, true);
// Since the navigation above requires a cross-process swap, there will be a
// pending RenderViewHost. Ensure it exists and is in a different process
diff --git a/content/browser/site_per_process_browsertest.cc b/content/browser/site_per_process_browsertest.cc
index a078c4d..3660abd 100644
--- a/content/browser/site_per_process_browsertest.cc
+++ b/content/browser/site_per_process_browsertest.cc
@@ -18,7 +18,6 @@
#include "content/browser/renderer_host/render_view_host_impl.h"
#include "content/browser/web_contents/web_contents_impl.h"
#include "content/common/frame_messages.h"
-#include "content/public/browser/navigation_details.h"
#include "content/public/browser/notification_observer.h"
#include "content/public/browser/notification_service.h"
#include "content/public/browser/notification_types.h"
@@ -2141,33 +2140,4 @@
DepictFrameTree(root));
}
-IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
- PageTransitionForSecondaryIframeNavigation) {
- GURL main_url(embedded_test_server()->GetURL("/site_per_process_main.html"));
- NavigateToURL(shell(), main_url);
-
- // It is safe to obtain the root frame tree node here, as it doesn't change.
- FrameTreeNode* root = static_cast<WebContentsImpl*>(shell()->web_contents())
- ->GetFrameTree()
- ->root();
-
- TestNavigationObserver observer(shell()->web_contents());
-
- // Load same-site page into iframe.
- FrameTreeNode* child = root->child_at(0);
- GURL http_url(embedded_test_server()->GetURL("/title1.html"));
- NavigateFrameToURL(child, http_url);
- EXPECT_EQ(http_url, observer.last_navigation_url());
- EXPECT_TRUE(observer.last_navigation_succeeded());
-
- // Load cross-site page into iframe.
- TestFrameNavigationObserver frame_observer(child, 1);
- GURL url = embedded_test_server()->GetURL("foo.com", "/title2.html");
- NavigateIframeToURL(shell()->web_contents(), "test", url);
- frame_observer.Wait();
-
- EXPECT_EQ(NAVIGATION_TYPE_NEW_SUBFRAME,
- frame_observer.load_committed_details().type);
-}
-
} // namespace content
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index 933dd389..decaf5e 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -1091,19 +1091,6 @@
GetContentClient()->SetActiveURL(common_params.url);
- // If this frame isn't in the same process as its parent, it will naively
- // assume that this is the first navigation in the iframe, but this may not
- // actually be the case. The PageTransition differentiates between the first
- // navigation in a subframe and subsequent navigations, so if this is a
- // subsequent navigation, force the frame's state machine forward.
- if (ui::PageTransitionCoreTypeIs(common_params.transition,
- ui::PAGE_TRANSITION_MANUAL_SUBFRAME)) {
- CHECK(frame_->parent());
- if (frame_->parent()->isWebRemoteFrame()) {
- 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.
@@ -2558,8 +2545,6 @@
document_state->set_start_load_time(Time::Now());
bool is_top_most = !frame->parent();
- NavigationStateImpl* navigation_state =
- static_cast<NavigationStateImpl*>(document_state->navigation_state());
if (is_top_most) {
render_view_->set_navigation_gesture(
WebUserGestureIndicator::isProcessingUserGesture() ?
@@ -2568,17 +2553,8 @@
// 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.
- navigation_state->set_transition_type(ui::PAGE_TRANSITION_AUTO_SUBFRAME);
- } else if (ui::PageTransitionCoreTypeIs(navigation_state->GetTransitionType(),
- ui::PAGE_TRANSITION_LINK)) {
- // Subframe navigations that are creating a new history item should be
- // marked MANUAL_SUBFRAME, unless it has already been marked as a
- // FORM_SUBMIT. This state will be attached to a main resource request
- // in the process that began the request. If the request is transferred
- // to a different process, this state will be used in
- // RenderFrameImpl::OnNavigate() in the new process (as well as in the
- // browser process).
- navigation_state->set_transition_type(ui::PAGE_TRANSITION_MANUAL_SUBFRAME);
+ static_cast<NavigationStateImpl*>(document_state->navigation_state())
+ ->set_transition_type(ui::PAGE_TRANSITION_AUTO_SUBFRAME);
}
FOR_EACH_OBSERVER(RenderViewObserver, render_view_->observers(),
@@ -3162,9 +3138,16 @@
}
}
- WebDataSource* provisional_data_source = frame->provisionalDataSource();
+ 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* top_data_source = top_frame->dataSource();
WebDataSource* data_source =
- provisional_data_source ? provisional_data_source : frame->dataSource();
+ provisional_data_source ? provisional_data_source : top_data_source;
DocumentState* document_state = DocumentState::FromDataSource(data_source);
DCHECK(document_state);
@@ -3303,14 +3286,8 @@
extra_data->set_stream_override(stream_override.Pass());
request.setExtraData(extra_data);
- 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;
DocumentState* top_document_state =
- DocumentState::FromDataSource(top_frame->dataSource());
+ DocumentState::FromDataSource(top_data_source);
if (top_document_state) {
// TODO(gavinp): separate out prefetching and prerender field trials
// if the rel=prerender rel type is sticking around.
diff --git a/content/test/test_frame_navigation_observer.cc b/content/test/test_frame_navigation_observer.cc
index e3cbccecd..c6d151bc 100644
--- a/content/test/test_frame_navigation_observer.cc
+++ b/content/test/test_frame_navigation_observer.cc
@@ -67,7 +67,6 @@
++navigations_completed_;
if (navigations_completed_ == number_of_navigations_) {
- load_committed_details_ = details;
navigation_started_ = false;
message_loop_runner_->Quit();
}
diff --git a/content/test/test_frame_navigation_observer.h b/content/test/test_frame_navigation_observer.h
index 5330c48..9ff42c5 100644
--- a/content/test/test_frame_navigation_observer.h
+++ b/content/test/test_frame_navigation_observer.h
@@ -10,7 +10,6 @@
#include "base/callback.h"
#include "base/compiler_specific.h"
#include "base/memory/scoped_ptr.h"
-#include "content/public/browser/navigation_details.h"
#include "content/public/browser/web_contents_observer.h"
#include "content/public/test/test_utils.h"
@@ -20,6 +19,7 @@
class FrameTreeNode;
class RenderFrameHostImpl;
class WebContents;
+struct LoadCommittedDetails;
// For content_browsertests, which run on the UI thread, run a second
// MessageLoop and quit when the navigation in a specific frame completes
@@ -38,11 +38,6 @@
// navigations are complete.
void Wait();
- // Returns the LoadCommittedDetails for the last navigation to commit.
- const LoadCommittedDetails& load_committed_details() const {
- return load_committed_details_;
- }
-
private:
// WebContentsObserver
void DidStartProvisionalLoadForFrame(RenderFrameHost* render_frame_host,
@@ -65,8 +60,6 @@
// The number of navigations to wait for.
int number_of_navigations_;
- LoadCommittedDetails load_committed_details_;
-
// The MessageLoopRunner used to spin the message loop.
scoped_refptr<MessageLoopRunner> message_loop_runner_;
