Skip to content

Commit

Permalink
Move WebContentsTester::TestDidNavigate to use RFH's. This eliminates…
Browse files Browse the repository at this point in the history
… one usage

of the wretched RenderViewHost::main_frame_routing_id, which is the main
motivation of this CL.

Fix up tests accordingly, which entails a substantial amount of rvh-to-rfh
reorientation. In most of the updated tests, the local variables are changed
from RVH's to RFH's. When the RVH is needed, just grab it from the RFH.

Add TestRVH/TestRFH covariant overrides of TRFH::GetRenderViewHost(),
TestWebContents::GetMainFrame(), etc. These provide a simpler, more
discoverable, and less static_cast<>-ey way to get at the test-enabled version
of the object. The difference is felt especially by those tests that
employ a second WebContents. The remapping goes according to the new
comments added to RenderViewHostImplTestHarness, copied here.

  // test_rvh() is equivalent to any of the following:
  //   contents()->GetMainFrame()->GetRenderViewHost()
  //   contents()->GetRenderViewHost()
  //   static_cast<TestRenderViewHost*>(rvh())
  //
  // Since most functionality will eventually shift from RVH to RFH, you may
  // prefer to use the GetMainFrame() method in tests.
  //
  // pending_test_rvh() is equivalent to all of the following:
  //   contents()->GetPendingMainFrame()->GetRenderViewHost()
  //   contents()->GetPendingRenderViewHost()
  //   static_cast<TestRenderViewHost*>(pending_rvh())
  //
  // Since most functionality will eventually shift from RVH to RFH, you may
  // prefer to use the GetPendingMainFrame() method in tests.
  //
  // active_test_rvh() is equivalent to:
  //   contents()->GetPendingRenderViewHost() ?
  //        contents()->GetPendingRenderViewHost() :
  //        contents()->GetRenderViewHost();
  //
  // main_test_rfh() is equivalent to contents()->GetMainFrame()
  //

In addition, main_pending_test_rfh() (which is removed) is equivalent
to contents()->GetPendingMainFrame().

BUG=304341 
TBR=sky@chromium.org

Review URL: https://codereview.chromium.org/457093004

Cr-Commit-Position: refs/heads/master@{#289798}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@289798 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
nick@chromium.org committed Aug 15, 2014
1 parent 6485243 commit 6b50e36
Show file tree
Hide file tree
Showing 20 changed files with 597 additions and 539 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ class MergeSessionLoadPageTest : public ChromeRenderViewHostTestHarness {

void Navigate(const char* url, int page_id) {
WebContentsTester::For(web_contents())->TestDidNavigate(
web_contents()->GetRenderViewHost(), page_id, GURL(url),
web_contents()->GetMainFrame(), page_id, GURL(url),
content::PAGE_TRANSITION_TYPED);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ class OfflineLoadPageTest : public ChromeRenderViewHostTestHarness {

void Navigate(const char* url, int page_id) {
WebContentsTester::For(web_contents())->TestDidNavigate(
web_contents()->GetRenderViewHost(), page_id, GURL(url),
web_contents()->GetMainFrame(), page_id, GURL(url),
content::PAGE_TRANSITION_TYPED);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,14 +134,14 @@ class BrowserFeatureExtractorTest : public ChromeRenderViewHostTestHarness {
type, std::string());

static int page_id = 0;
content::RenderViewHost* rvh =
WebContentsTester::For(web_contents())->GetPendingRenderViewHost();
if (!rvh) {
rvh = web_contents()->GetRenderViewHost();
content::RenderFrameHost* rfh =
WebContentsTester::For(web_contents())->GetPendingMainFrame();
if (!rfh) {
rfh = web_contents()->GetMainFrame();
}
WebContentsTester::For(web_contents())->ProceedWithCrossSiteNavigation();
WebContentsTester::For(web_contents())->TestDidNavigateWithReferrer(
rvh, ++page_id, url,
rfh, ++page_id, url,
content::Referrer(referrer, blink::WebReferrerPolicyDefault), type);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ class SafeBrowsingBlockingPageTest : public ChromeRenderViewHostTestHarness {

void Navigate(const char* url, int page_id) {
WebContentsTester::For(web_contents())->TestDidNavigate(
web_contents()->GetRenderViewHost(), page_id, GURL(url),
web_contents()->GetMainFrame(), page_id, GURL(url),
content::PAGE_TRANSITION_TYPED);
}

Expand All @@ -134,11 +134,11 @@ class SafeBrowsingBlockingPageTest : public ChromeRenderViewHostTestHarness {
web_contents()->GetController().GoBack();

// The pending RVH should commit for cross-site navigations.
content::RenderViewHost* rvh = is_cross_site ?
WebContentsTester::For(web_contents())->GetPendingRenderViewHost() :
web_contents()->GetRenderViewHost();
content::RenderFrameHost* rfh = is_cross_site ?
WebContentsTester::For(web_contents())->GetPendingMainFrame() :
web_contents()->GetMainFrame();
WebContentsTester::For(web_contents())->TestDidNavigate(
rvh,
rfh,
entry->GetPageID(),
GURL(entry->GetURL()),
content::PAGE_TRANSITION_TYPED);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ class GeneratedCreditCardBubbleControllerTest : public testing::Test {

void NavigateWithTransition(content::PageTransition trans) {
content::WebContentsTester::For(test_web_contents_.get())->TestDidNavigate(
test_web_contents_->GetRenderViewHost(), 1, GURL("about:blank"), trans);
test_web_contents_->GetMainFrame(), 1, GURL("about:blank"), trans);
}

private:
Expand Down
6 changes: 4 additions & 2 deletions content/browser/devtools/devtools_manager_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,8 @@ TEST_F(DevToolsManagerTest, ReattachOnCancelPendingNavigation) {
const GURL url("http://www.google.com");
controller().LoadURL(
url, Referrer(), PAGE_TRANSITION_TYPED, std::string());
contents()->TestDidNavigate(rvh(), 1, url, PAGE_TRANSITION_TYPED);
contents()->TestDidNavigate(
contents()->GetMainFrame(), 1, url, PAGE_TRANSITION_TYPED);
EXPECT_FALSE(contents()->cross_navigation_pending());

TestDevToolsClientHost client_host;
Expand All @@ -196,7 +197,8 @@ TEST_F(DevToolsManagerTest, ReattachOnCancelPendingNavigation) {
// Interrupt pending navigation and navigate back to the original site.
controller().LoadURL(
url, Referrer(), PAGE_TRANSITION_TYPED, std::string());
contents()->TestDidNavigate(rvh(), 1, url, PAGE_TRANSITION_TYPED);
contents()->TestDidNavigate(
contents()->GetMainFrame(), 1, url, PAGE_TRANSITION_TYPED);
EXPECT_FALSE(contents()->cross_navigation_pending());
EXPECT_EQ(devtools_manager->GetDevToolsAgentHostFor(&client_host),
DevToolsAgentHost::GetOrCreateFor(web_contents()));
Expand Down
1 change: 1 addition & 0 deletions content/browser/frame_host/frame_tree_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "content/public/test/mock_render_process_host.h"
#include "content/public/test/test_browser_context.h"
#include "content/public/test/test_browser_thread_bundle.h"
#include "content/test/test_render_frame_host.h"
#include "content/test/test_render_view_host.h"
#include "content/test/test_web_contents.h"
#include "testing/gtest/include/gtest/gtest.h"
Expand Down
45 changes: 21 additions & 24 deletions content/browser/frame_host/navigation_controller_impl_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include "content/public/test/mock_render_process_host.h"
#include "content/public/test/test_notification_tracker.h"
#include "content/public/test/test_utils.h"
#include "content/test/test_render_frame_host.h"
#include "content/test/test_render_view_host.h"
#include "content/test/test_web_contents.h"
#include "net/base/net_util.h"
Expand Down Expand Up @@ -415,8 +416,7 @@ TEST_F(NavigationControllerTest, LoadURL) {
// Simulate the beforeunload ack for the cross-site transition, and then the
// commit.
test_rvh()->SendBeforeUnloadACK(true);
static_cast<TestRenderViewHost*>(
contents()->GetPendingRenderViewHost())->SendNavigate(1, url2);
contents()->GetPendingMainFrame()->SendNavigate(1, url2);
EXPECT_EQ(1U, navigation_entry_committed_counter_);
navigation_entry_committed_counter_ = 0;

Expand Down Expand Up @@ -747,8 +747,7 @@ TEST_F(NavigationControllerTest, LoadURL_NewPending) {
// After the beforeunload but before it commits, do a new navigation.
test_rvh()->SendBeforeUnloadACK(true);
const GURL kNewURL("http://see");
static_cast<TestRenderViewHost*>(
contents()->GetPendingRenderViewHost())->SendNavigate(3, kNewURL);
contents()->GetPendingMainFrame()->SendNavigate(3, kNewURL);

// There should no longer be any pending entry, and the third navigation we
// just made should be committed.
Expand Down Expand Up @@ -826,16 +825,15 @@ TEST_F(NavigationControllerTest, LoadURL_PrivilegedPending) {
controller.LoadURL(
kExistingURL2, Referrer(), PAGE_TRANSITION_TYPED, std::string());
test_rvh()->SendBeforeUnloadACK(true);
TestRenderViewHost* foo_rvh = static_cast<TestRenderViewHost*>(
contents()->GetPendingRenderViewHost());
foo_rvh->SendNavigate(1, kExistingURL2);
TestRenderFrameHost* foo_rfh = contents()->GetPendingMainFrame();
foo_rfh->SendNavigate(1, kExistingURL2);
EXPECT_EQ(1U, navigation_entry_committed_counter_);
navigation_entry_committed_counter_ = 0;

// Now make a pending back/forward navigation to a privileged entry.
// The zeroth entry should be pending.
controller.GoBack();
foo_rvh->SendBeforeUnloadACK(true);
foo_rfh->GetRenderViewHost()->SendBeforeUnloadACK(true);
EXPECT_EQ(0U, notifications.size());
EXPECT_EQ(0, controller.GetPendingEntryIndex());
EXPECT_EQ(1, controller.GetLastCommittedEntryIndex());
Expand All @@ -845,7 +843,7 @@ TEST_F(NavigationControllerTest, LoadURL_PrivilegedPending) {
// Before that commits, do a new navigation.
const GURL kNewURL("http://foo/bee");
LoadCommittedDetails details;
foo_rvh->SendNavigate(3, kNewURL);
foo_rfh->SendNavigate(3, kNewURL);

// There should no longer be any pending entry, and the third navigation we
// just made should be committed.
Expand Down Expand Up @@ -1083,33 +1081,32 @@ TEST_F(NavigationControllerTest, LoadURL_WithBindings) {
controller.GetPendingEntry())->bindings());

// Commit.
TestRenderViewHost* orig_rvh = static_cast<TestRenderViewHost*>(test_rvh());
orig_rvh->SendNavigate(0, url1);
TestRenderFrameHost* orig_rfh = contents()->GetMainFrame();
orig_rfh->SendNavigate(0, url1);
EXPECT_EQ(controller.GetEntryCount(), 1);
EXPECT_EQ(0, controller.GetLastCommittedEntryIndex());
EXPECT_EQ(0, NavigationEntryImpl::FromNavigationEntry(
controller.GetLastCommittedEntry())->bindings());

// Manually increase the number of active views in the SiteInstance
// that orig_rvh belongs to, to prevent it from being destroyed when
// it gets swapped out, so that we can reuse orig_rvh when the
// that orig_rfh belongs to, to prevent it from being destroyed when
// it gets swapped out, so that we can reuse orig_rfh when the
// controller goes back.
static_cast<SiteInstanceImpl*>(orig_rvh->GetSiteInstance())->
static_cast<SiteInstanceImpl*>(orig_rfh->GetSiteInstance())->
increment_active_view_count();

// Navigate to a second URL, simulate the beforeunload ack for the cross-site
// transition, run the unload handler, and set bindings on the pending
// RenderViewHost to simulate a privileged url.
controller.LoadURL(url2, Referrer(), PAGE_TRANSITION_TYPED, std::string());
orig_rvh->SendBeforeUnloadACK(true);
orig_rfh->GetRenderViewHost()->SendBeforeUnloadACK(true);
contents()->GetRenderManagerForTesting()->OnCrossSiteResponse(
contents()->GetRenderManagerForTesting()->pending_frame_host(),
contents()->GetPendingMainFrame(),
GlobalRequestID(0, 0), scoped_ptr<CrossSiteTransferringRequest>(),
url_chain, Referrer(), PAGE_TRANSITION_TYPED, false);
TestRenderViewHost* new_rvh =
static_cast<TestRenderViewHost*>(contents()->GetPendingRenderViewHost());
new_rvh->AllowBindings(1);
new_rvh->SendNavigate(1, url2);
TestRenderFrameHost* new_rfh = contents()->GetPendingMainFrame();
new_rfh->GetRenderViewHost()->AllowBindings(1);
new_rfh->SendNavigate(1, url2);

// The second load should be committed, and bindings should be remembered.
EXPECT_EQ(controller.GetEntryCount(), 2);
Expand All @@ -1120,12 +1117,12 @@ TEST_F(NavigationControllerTest, LoadURL_WithBindings) {

// Going back, the first entry should still appear unprivileged.
controller.GoBack();
new_rvh->SendBeforeUnloadACK(true);
new_rfh->GetRenderViewHost()->SendBeforeUnloadACK(true);
contents()->GetRenderManagerForTesting()->OnCrossSiteResponse(
contents()->GetRenderManagerForTesting()->pending_frame_host(),
contents()->GetPendingMainFrame(),
GlobalRequestID(0, 0), scoped_ptr<CrossSiteTransferringRequest>(),
url_chain, Referrer(), PAGE_TRANSITION_TYPED, false);
orig_rvh->SendNavigate(0, url1);
orig_rfh->SendNavigate(0, url1);
EXPECT_EQ(0, controller.GetLastCommittedEntryIndex());
EXPECT_EQ(0, NavigationEntryImpl::FromNavigationEntry(
controller.GetLastCommittedEntry())->bindings());
Expand Down Expand Up @@ -4300,7 +4297,7 @@ TEST_F(NavigationControllerTest, ClearHistoryList) {
EXPECT_TRUE(entry->should_clear_history_list());

// Assume that the RV correctly cleared its history and commit the navigation.
static_cast<TestRenderViewHost*>(contents()->GetPendingRenderViewHost())->
contents()->GetPendingMainFrame()->GetRenderViewHost()->
set_simulate_history_list_was_cleared(true);
contents()->CommitPendingNavigation();

Expand Down
Loading

0 comments on commit 6b50e36

Please sign in to comment.