Skip to content

Commit

Permalink
Deprecate NavigationController(Delegate)::GetWebContents.
Browse files Browse the repository at this point in the history
This is a layering violation as renderer_host is not permitted to depend
on WebContents. The DEPS check is bypassed using a forward declaration
instead of an #include.

This CL starts to deprecate the functions and adjust the callsites.

Change-Id: Ic0c941d47e07f954ea20bbb634ffc356118b3aeb
Bug: 1225205
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2996169
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Finnur Thorarinsson <finnur@chromium.org>
Reviewed-by: Mike Wasserman <msw@chromium.org>
Reviewed-by: Xinghui Lu <xinghuilu@chromium.org>
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Reviewed-by: Lucas Gadani <lfg@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#898727}
  • Loading branch information
mfalken authored and Chromium LUCI CQ committed Jul 6, 2021
1 parent 1b55dc6 commit 548ed15
Show file tree
Hide file tree
Showing 35 changed files with 82 additions and 68 deletions.
14 changes: 8 additions & 6 deletions chrome/browser/apps/guest_view/web_view_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -712,12 +712,13 @@ class WebViewTest : public extensions::PlatformAppBrowserTest {
guest_observer.Wait();
content::Source<content::NavigationController> source =
guest_observer.source();
EXPECT_TRUE(source->GetWebContents()
EXPECT_TRUE(source->DeprecatedGetWebContents()
->GetMainFrame()
->GetProcess()
->IsForGuestsOnly());

content::WebContents* guest_web_contents = source->GetWebContents();
content::WebContents* guest_web_contents =
source->DeprecatedGetWebContents();
return guest_web_contents;
}

Expand Down Expand Up @@ -1636,7 +1637,7 @@ IN_PROC_BROWSER_TEST_F(WebViewNewWindowTest,

content::Source<content::NavigationController> source =
empty_guest_observer.source();
EXPECT_TRUE(source->GetWebContents()
EXPECT_TRUE(source->DeprecatedGetWebContents()
->GetMainFrame()
->GetProcess()
->IsForGuestsOnly());
Expand All @@ -1651,7 +1652,8 @@ IN_PROC_BROWSER_TEST_F(WebViewNewWindowTest,
ASSERT_EQ(2u, guest_contents_list.size());
content::WebContents* new_window_guest_contents = guest_contents_list[0];

content::WebContents* empty_guest_web_contents = source->GetWebContents();
content::WebContents* empty_guest_web_contents =
source->DeprecatedGetWebContents();
ASSERT_EQ(empty_guest_web_contents, guest_contents_list[1]);
ASSERT_NE(empty_guest_web_contents, new_window_guest_contents);
content::WebContents* empty_guest_embedder =
Expand Down Expand Up @@ -1980,15 +1982,15 @@ IN_PROC_BROWSER_TEST_F(WebViewTest, Shim_TestRemoveWebviewOnExit) {

content::Source<content::NavigationController> source =
guest_observer.source();
EXPECT_TRUE(source->GetWebContents()
EXPECT_TRUE(source->DeprecatedGetWebContents()
->GetMainFrame()
->GetProcess()
->IsForGuestsOnly());

ASSERT_TRUE(guest_loaded_listener.WaitUntilSatisfied());

content::WebContentsDestroyedWatcher destroyed_watcher(
source->GetWebContents());
source->DeprecatedGetWebContents());

// Tell the embedder to kill the guest.
EXPECT_TRUE(content::ExecuteScript(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -318,12 +318,12 @@ class WebViewInteractiveTest : public extensions::PlatformAppBrowserTest {
guest_observer.Wait();
content::Source<content::NavigationController> source =
guest_observer.source();
EXPECT_TRUE(source->GetWebContents()
EXPECT_TRUE(source->DeprecatedGetWebContents()
->GetMainFrame()
->GetProcess()
->IsForGuestsOnly());

guest_web_contents_ = source->GetWebContents();
guest_web_contents_ = source->DeprecatedGetWebContents();
embedder_web_contents_ =
GuestViewBase::FromWebContents(guest_web_contents_)->
embedder_web_contents();
Expand Down
4 changes: 2 additions & 2 deletions chrome/browser/captive_portal/captive_portal_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ void MultiNavigationObserver::Observe(
content::NavigationController* controller =
content::Source<content::NavigationController>(source).ptr();
++num_navigations_;
++tab_navigation_map_[controller->GetWebContents()];
++tab_navigation_map_[controller->DeprecatedGetWebContents()];
if (waiting_for_navigation_ &&
num_navigations_to_wait_for_ == num_navigations_) {
waiting_for_navigation_ = false;
Expand Down Expand Up @@ -366,7 +366,7 @@ void FailLoadsAfterLoginObserver::Observe(
ASSERT_EQ(type, content::NOTIFICATION_LOAD_STOP);
content::NavigationController* controller =
content::Source<content::NavigationController>(source).ptr();
WebContents* contents = controller->GetWebContents();
WebContents* contents = controller->DeprecatedGetWebContents();

ASSERT_EQ(1u, tabs_needing_navigation_.count(contents));
ASSERT_EQ(0u, tabs_navigated_to_final_destination_.count(contents));
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/chromeos/boot_times_recorder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ const char kDisk[] = "disk";
static const char kBootTimes[] = "BootTimes";

RenderWidgetHost* GetRenderWidgetHost(NavigationController* tab) {
WebContents* web_contents = tab->GetWebContents();
WebContents* web_contents = tab->DeprecatedGetWebContents();
if (web_contents) {
RenderWidgetHostView* render_widget_host_view =
web_contents->GetRenderWidgetHostView();
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/extensions/api/identity/identity_apitest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ class WaitForGURLAndCloseWindow : public content::WindowedNotificationObserver {
content::NavigationController* web_auth_flow_controller =
content::Source<content::NavigationController>(source).ptr();
content::WebContents* web_contents =
web_auth_flow_controller->GetWebContents();
web_auth_flow_controller->DeprecatedGetWebContents();

if (web_contents->GetLastCommittedURL() == url_) {
// It is safe to keep the pointer here, because we know in a test, that
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/extensions/navigation_observer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ void NavigationObserver::PromptToEnableExtensionIfNecessary(
in_progress_prompt_navigation_controller_ = nav_controller;

extension_install_prompt_ = std::make_unique<ExtensionInstallPrompt>(
nav_controller->GetWebContents());
nav_controller->DeprecatedGetWebContents());
ExtensionInstallPrompt::PromptType type =
ExtensionInstallPrompt::GetReEnablePromptTypeForExtension(profile_,
extension);
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/extensions/window_open_apitest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest,
content::NavigationController* controller =
content::Source<content::NavigationController>(windowed_observer.source())
.ptr();
content::WebContents* newtab = controller->GetWebContents();
content::WebContents* newtab = controller->DeprecatedGetWebContents();
ASSERT_TRUE(newtab);

EXPECT_EQ(content::PAGE_TYPE_ERROR,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ class BackgroundTabLoadingBrowserTest : public InProcessBrowserTest {
content::NavigationController* controller = &tab->GetController();
// If tab content is not in a loading state and doesn't need reload.
if (!controller->NeedsReload() && !controller->GetPendingEntry() &&
!controller->GetWebContents()->IsLoading()) {
!tab->IsLoading()) {
return;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,15 +161,13 @@ TEST_F(SBNavigationObserverTest, TestNavigationEventList) {

TEST_F(SBNavigationObserverTest, BasicNavigationAndCommit) {
// Navigation in current tab.
content::NavigationController* controller =
&browser()->tab_strip_model()->GetWebContentsAt(0)->GetController();
auto* web_contents = browser()->tab_strip_model()->GetWebContentsAt(0);
browser()->OpenURL(
content::OpenURLParams(GURL("http://foo/1"), content::Referrer(),
WindowOpenDisposition::CURRENT_TAB,
ui::PAGE_TRANSITION_AUTO_BOOKMARK, false));
CommitPendingLoad(controller);
SessionID tab_id =
sessions::SessionTabHelper::IdForTab(controller->GetWebContents());
CommitPendingLoad(&web_contents->GetController());
SessionID tab_id = sessions::SessionTabHelper::IdForTab(web_contents);
auto* nav_list = navigation_event_list();
ASSERT_EQ(1U, nav_list->NavigationEventsSize());
VerifyNavigationEvent(GURL(), // source_url
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/sessions/session_restore_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ class SmartSessionRestoreTest : public SessionRestoreTest,
case content::NOTIFICATION_LOAD_START: {
content::NavigationController* controller =
content::Source<content::NavigationController>(source).ptr();
web_contents_.push_back(controller->GetWebContents());
web_contents_.push_back(controller->DeprecatedGetWebContents());
if (web_contents_.size() == num_tabs_)
message_loop_runner_->Quit();
break;
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/sessions/tab_restore_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ class TabRestoreTest : public InProcessBrowserTest {
void EnsureTabFinishedRestoring(content::WebContents* tab) {
content::NavigationController* controller = &tab->GetController();
if (!controller->NeedsReload() && !controller->GetPendingEntry() &&
!controller->GetWebContents()->IsLoading())
!tab->IsLoading())
return;

content::WindowedNotificationObserver observer(
Expand Down
5 changes: 3 additions & 2 deletions chrome/browser/translate/chrome_translate_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -90,15 +90,16 @@ TranslateEventProto::EventType BubbleResultToTranslateEvent(

ChromeTranslateClient::ChromeTranslateClient(content::WebContents* web_contents)
: content::WebContentsObserver(web_contents) {
DCHECK(web_contents);
if (translate::IsSubFrameTranslationEnabled()) {
per_frame_translate_driver_ =
std::make_unique<translate::PerFrameContentTranslateDriver>(
&web_contents->GetController(),
*web_contents, &web_contents->GetController(),
UrlLanguageHistogramFactory::GetForBrowserContext(
web_contents->GetBrowserContext()));
} else {
translate_driver_ = std::make_unique<translate::ContentTranslateDriver>(
&web_contents->GetController(),
*web_contents, &web_contents->GetController(),
UrlLanguageHistogramFactory::GetForBrowserContext(
web_contents->GetBrowserContext()),
TranslateModelServiceFactory::GetOrBuildForKey(
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/ui/browser_navigator_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1813,7 +1813,7 @@ IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest, MAYBE_ReuseRVHWithWebUI) {
content::NavigationController* controller =
content::Source<content::NavigationController>(windowed_observer.source())
.ptr();
WebContents* popup = controller->GetWebContents();
WebContents* popup = controller->DeprecatedGetWebContents();
ASSERT_TRUE(popup);
EXPECT_EQ(2, browser()->tab_strip_model()->count());
content::RenderViewHost* webui_rvh =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ void ChromeOmniboxNavigationObserver::Observe(
// listening.
content::NavigationController* controller =
content::Source<content::NavigationController>(source).ptr();
content::WebContents* web_contents = controller->GetWebContents();
content::WebContents* web_contents = controller->DeprecatedGetWebContents();
if (!infobars::ContentInfoBarManager::FromWebContents(web_contents))
return;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ class ThumbnailTabHelperBrowserTest : public InProcessBrowserTest {
void EnsureTabLoaded(content::WebContents* tab) {
content::NavigationController* controller = &tab->GetController();
if (!controller->NeedsReload() && !controller->GetPendingEntry() &&
!controller->GetWebContents()->IsLoading())
!tab->IsLoading())
return;

content::WindowedNotificationObserver observer(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -536,7 +536,7 @@ TEST_F(BackFwdMenuModelTest, FaviconLoadTest) {

BackForwardMenuModel back_model(browser.get(),
BackForwardMenuModel::ModelType::kBackward);
back_model.set_test_web_contents(controller().GetWebContents());
back_model.set_test_web_contents(web_contents());
back_model.SetMenuModelDelegate(&favicon_delegate);

SkBitmap new_icon_bitmap(CreateBitmap(SK_ColorRED));
Expand Down
2 changes: 1 addition & 1 deletion chrome/test/base/browser_with_test_window_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ void BrowserWithTestWindowTest::NavigateAndCommit(
NavigationController* controller,
const GURL& url) {
content::NavigationSimulator::NavigateAndCommitFromBrowser(
controller->GetWebContents(), url);
controller->DeprecatedGetWebContents(), url);
}

void BrowserWithTestWindowTest::NavigateAndCommitActiveTab(const GURL& url) {
Expand Down
3 changes: 2 additions & 1 deletion chrome/test/base/ui_test_utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -505,7 +505,8 @@ void UrlLoadObserver::Observe(
const content::NotificationDetails& details) {
NavigationController* controller =
content::Source<NavigationController>(source).ptr();
if (controller->GetWebContents()->GetURL() != url_)
NavigationEntry* entry = controller->GetVisibleEntry();
if (!entry || entry->GetVirtualURL() != url_)
return;

WindowedNotificationObserver::Observe(type, source, details);
Expand Down
20 changes: 9 additions & 11 deletions components/translate/content/browser/content_translate_driver.cc
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,11 @@ const base::Feature kAutoHrefTranslateAllOrigins{
} // namespace

ContentTranslateDriver::ContentTranslateDriver(
content::WebContents& web_contents,
content::NavigationController* nav_controller,
language::UrlLanguageHistogram* url_language_histogram,
translate::TranslateModelService* translate_model_service)
: content::WebContentsObserver(nav_controller->GetWebContents()),
: content::WebContentsObserver(&web_contents),
navigation_controller_(nav_controller),
translate_manager_(nullptr),
max_reload_check_attempts_(kMaxTranslateLoadCheckAttempts),
Expand Down Expand Up @@ -118,15 +119,13 @@ bool ContentTranslateDriver::IsLinkNavigation() {
}

void ContentTranslateDriver::OnTranslateEnabledChanged() {
content::WebContents* web_contents = navigation_controller_->GetWebContents();
for (auto& observer : translation_observers_)
observer.OnTranslateEnabledChanged(web_contents);
observer.OnTranslateEnabledChanged(web_contents());
}

void ContentTranslateDriver::OnIsPageTranslatedChanged() {
content::WebContents* web_contents = navigation_controller_->GetWebContents();
for (auto& observer : translation_observers_)
observer.OnIsPageTranslatedChanged(web_contents);
observer.OnIsPageTranslatedChanged(web_contents());
}

void ContentTranslateDriver::TranslatePage(int page_seq_no,
Expand Down Expand Up @@ -156,20 +155,19 @@ bool ContentTranslateDriver::IsIncognito() {
}

const std::string& ContentTranslateDriver::GetContentsMimeType() {
return navigation_controller_->GetWebContents()->GetContentsMimeType();
return web_contents()->GetContentsMimeType();
}

const GURL& ContentTranslateDriver::GetLastCommittedURL() {
return navigation_controller_->GetWebContents()->GetLastCommittedURL();
return web_contents()->GetLastCommittedURL();
}

const GURL& ContentTranslateDriver::GetVisibleURL() {
return navigation_controller_->GetWebContents()->GetVisibleURL();
return web_contents()->GetVisibleURL();
}

ukm::SourceId ContentTranslateDriver::GetUkmSourceId() {
return ukm::GetSourceIdForWebContentsDocument(
navigation_controller_->GetWebContents());
return ukm::GetSourceIdForWebContentsDocument(web_contents());
}

bool ContentTranslateDriver::HasCurrentPage() {
Expand All @@ -180,7 +178,7 @@ void ContentTranslateDriver::OpenUrlInNewTab(const GURL& url) {
content::OpenURLParams params(url, content::Referrer(),
WindowOpenDisposition::NEW_FOREGROUND_TAB,
ui::PAGE_TRANSITION_LINK, false);
navigation_controller_->GetWebContents()->OpenURL(params);
web_contents()->OpenURL(params);
}

void ContentTranslateDriver::InitiateTranslationIfReload(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,8 @@ class ContentTranslateDriver : public TranslateDriver,
}
};

ContentTranslateDriver(content::NavigationController* nav_controller,
ContentTranslateDriver(content::WebContents& web_contents,
content::NavigationController* nav_controller,
language::UrlLanguageHistogram* url_language_histogram,
TranslateModelService* translate_model_service);
~ContentTranslateDriver() override;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,11 @@ void PerFrameContentTranslateDriver::PendingRequestStats::Report() {
}

PerFrameContentTranslateDriver::PerFrameContentTranslateDriver(
content::WebContents& web_contents,
content::NavigationController* nav_controller,
language::UrlLanguageHistogram* url_language_histogram)
: ContentTranslateDriver(nav_controller,
: ContentTranslateDriver(web_contents,
nav_controller,
url_language_histogram,
/*translate_model_service=*/nullptr) {}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ struct LanguageDetectionDetails;
class PerFrameContentTranslateDriver : public ContentTranslateDriver {
public:
PerFrameContentTranslateDriver(
content::WebContents& web_contents,
content::NavigationController* nav_controller,
language::UrlLanguageHistogram* url_language_histogram);
~PerFrameContentTranslateDriver() override;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class PerFrameContentTranslateDriverTest
void SetUp() override {
content::RenderViewHostTestHarness::SetUp();
driver_ = std::make_unique<PerFrameContentTranslateDriver>(
&(web_contents()->GetController()),
*web_contents(), &(web_contents()->GetController()),
nullptr /* url_language_histogram */);
driver_->AddLanguageDetectionObserver(&observer_);
}
Expand Down
4 changes: 3 additions & 1 deletion content/browser/prerender/prerender_host.cc
Original file line number Diff line number Diff line change
Expand Up @@ -141,13 +141,15 @@ class PrerenderHost::PageHolder : public FrameTree::Delegate,
void NotifyNavigationEntriesDeleted() override {}
void ActivateAndShowRepostFormWarningDialog() override {}
bool ShouldPreserveAbortedURLs() override { return false; }
WebContents* GetWebContents() override { return &web_contents_; }
WebContents* DeprecatedGetWebContents() override { return GetWebContents(); }
void UpdateOverridingUserAgent() override {}

NavigationControllerImpl& GetNavigationController() {
return frame_tree_->controller();
}

WebContents* GetWebContents() { return &web_contents_; }

ActivateResult Activate(NavigationRequest& navigation_request) {
// There should be no ongoing main-frame navigation during activation.
// TODO(https://crbug.com/1190644): Make sure sub-frame navigations are
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ class NavigationControllerDelegate {
virtual bool IsBeingDestroyed() = 0;

// Methods from WebContentsImpl that NavigationControllerImpl needs to
// call.
// call. NavigationControllerImpl cannot call them directly because
// renderer_host/ cannot depend on WebContents.
virtual void NotifyBeforeFormRepostWarningShow() = 0;
virtual void NotifyNavigationEntryCommitted(
const LoadCommittedDetails& load_details) = 0;
Expand All @@ -43,9 +44,9 @@ class NavigationControllerDelegate {
// preserved in the omnibox. Defaults to false.
virtual bool ShouldPreserveAbortedURLs() = 0;

// This method is needed, since we are no longer guaranteed that the
// embedder for NavigationController will be a WebContents object.
virtual WebContents* GetWebContents() = 0;
// TODO(crbug.com/1225205): Remove this. It is a layering violation as
// renderer_host/ cannot depend on WebContents.
virtual WebContents* DeprecatedGetWebContents() = 0;

virtual void UpdateOverridingUserAgent() = 0;
};
Expand Down
4 changes: 2 additions & 2 deletions content/browser/renderer_host/navigation_controller_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -638,8 +638,8 @@ NavigationControllerImpl::~NavigationControllerImpl() {
DiscardNonCommittedEntries();
}

WebContents* NavigationControllerImpl::GetWebContents() {
return delegate_->GetWebContents();
WebContents* NavigationControllerImpl::DeprecatedGetWebContents() {
return delegate_->DeprecatedGetWebContents();
}

BrowserContext* NavigationControllerImpl::GetBrowserContext() {
Expand Down
Loading

0 comments on commit 548ed15

Please sign in to comment.