Skip to content

Commit

Permalink
webui: Remove UseWebUIBindingsForURL
Browse files Browse the repository at this point in the history 
In theory UseWebUIBindingsForURL could be used to check which WebUIs
have bindings and which don't, but in practice this method was
implemented as `return UseWebUIForURL()`, meaning all implementations
return true if there is a WebUIController for it. This CL removes this
method in favor of UseWebUIForURL().

UseWebUIBindingsForURL() was used for:
1. Deciding if we should force a process swap when navigating between
   two URLs.
2. Deciding if the host was appropriate for the URL.

Semantically this change means that we'll force a process swap for
all WebUIs with WebUIControllers, even if they don't use WebUI bindings,
which is desirable. In practice this was already the case because of
the implementations of UseWebUIBindingsForURL().

Bug: 1080384
Change-Id: I06cb5ad503a91582d6a4058b1cc77794bb6f0dd4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2583590
Reviewed-by: Sergey Volk <servolk@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: calamity <calamity@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#836944}
  • Loading branch information
Giovanni Ortuño Urquidi authored and Chromium LUCI CQ committed Dec 15, 2020
1 parent 6f14991 commit df652e7
Show file tree
Hide file tree
Showing 25 changed files with 14 additions and 105 deletions.
6 changes: 0 additions & 6 deletions android_webview/browser/aw_web_ui_controller_factory.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 +72,6 @@ bool AwWebUIControllerFactory::UseWebUIForURL(
return GetWebUIType(browser_context, url) != WebUI::kNoWebUI;
}

bool AwWebUIControllerFactory::UseWebUIBindingsForURL(
content::BrowserContext* browser_context,
const GURL& url) {
return UseWebUIForURL(browser_context, url);
}

std::unique_ptr<WebUIController>
AwWebUIControllerFactory::CreateWebUIControllerForURL(WebUI* web_ui,
const GURL& url) {
Expand Down
2 changes: 0 additions & 2 deletions android_webview/browser/aw_web_ui_controller_factory.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ class AwWebUIControllerFactory : public content::WebUIControllerFactory {
const GURL& url) override;
bool UseWebUIForURL(content::BrowserContext* browser_context,
const GURL& url) override;
bool UseWebUIBindingsForURL(content::BrowserContext* browser_context,
const GURL& url) override;
std::unique_ptr<content::WebUIController> CreateWebUIControllerForURL(
content::WebUI* web_ui,
const GURL& url) override;
Expand Down
6 changes: 0 additions & 6 deletions chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1003,12 +1003,6 @@ bool ChromeWebUIControllerFactory::UseWebUIForURL(
return GetWebUIType(browser_context, url) != WebUI::kNoWebUI;
}

bool ChromeWebUIControllerFactory::UseWebUIBindingsForURL(
content::BrowserContext* browser_context,
const GURL& url) {
return UseWebUIForURL(browser_context, url);
}

std::unique_ptr<WebUIController>
ChromeWebUIControllerFactory::CreateWebUIControllerForURL(WebUI* web_ui,
const GURL& url) {
Expand Down
2 changes: 0 additions & 2 deletions chrome/browser/ui/webui/chrome_web_ui_controller_factory.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,6 @@ class ChromeWebUIControllerFactory : public content::WebUIControllerFactory {
const GURL& url) override;
bool UseWebUIForURL(content::BrowserContext* browser_context,
const GURL& url) override;
bool UseWebUIBindingsForURL(content::BrowserContext* browser_context,
const GURL& url) override;
std::unique_ptr<content::WebUIController> CreateWebUIControllerForURL(
content::WebUI* web_ui,
const GURL& url) override;
Expand Down
6 changes: 0 additions & 6 deletions chrome/browser/web_applications/test/test_system_web_app_web_ui_controller_factory.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,3 @@ bool TestSystemWebAppWebUIControllerFactory::UseWebUIForURL(
return url.SchemeIs(content::kChromeUIScheme) &&
url.host_piece() == source_name_;
}

bool TestSystemWebAppWebUIControllerFactory::UseWebUIBindingsForURL(
content::BrowserContext* browser_context,
const GURL& url) {
return UseWebUIForURL(browser_context, url);
}
2 changes: 0 additions & 2 deletions chrome/browser/web_applications/test/test_system_web_app_web_ui_controller_factory.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,6 @@ class TestSystemWebAppWebUIControllerFactory

bool UseWebUIForURL(content::BrowserContext* browser_context,
const GURL& url) override;
bool UseWebUIBindingsForURL(content::BrowserContext* browser_context,
const GURL& url) override;

private:
std::string source_name_;
Expand Down
4 changes: 0 additions & 4 deletions chrome/test/data/webui/mojo/mojo_web_ui_controller_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,10 +144,6 @@ class TestWebUIControllerFactory : public content::WebUIControllerFactory {
const GURL& url) override {
return url.SchemeIs(content::kChromeUIScheme);
}
bool UseWebUIBindingsForURL(content::BrowserContext* browser_context,
const GURL& url) override {
return url.SchemeIs(content::kChromeUIScheme);
}

private:
DISALLOW_COPY_AND_ASSIGN(TestWebUIControllerFactory);
Expand Down
6 changes: 0 additions & 6 deletions chromecast/browser/webui/cast_webui_controller_factory.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,6 @@ bool CastWebUiControllerFactory::UseWebUIForURL(
return GetWebUIType(browser_context, url) != content::WebUI::kNoWebUI;
}

bool CastWebUiControllerFactory::UseWebUIBindingsForURL(
content::BrowserContext* browser_context,
const GURL& url) {
return UseWebUIForURL(browser_context, url);
}

std::unique_ptr<content::WebUIController>
CastWebUiControllerFactory::CreateWebUIControllerForURL(content::WebUI* web_ui,
const GURL& url) {
Expand Down
2 changes: 0 additions & 2 deletions chromecast/browser/webui/cast_webui_controller_factory.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,6 @@ class CastWebUiControllerFactory : public content::WebUIControllerFactory {
const GURL& url) override;
bool UseWebUIForURL(content::BrowserContext* browser_context,
const GURL& url) override;
bool UseWebUIBindingsForURL(content::BrowserContext* browser_context,
const GURL& url) override;
std::unique_ptr<content::WebUIController> CreateWebUIControllerForURL(
content::WebUI* web_ui,
const GURL& url) override;
Expand Down
5 changes: 0 additions & 5 deletions chromeos/components/web_applications/test/js_library_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,11 +120,6 @@ class JsLibraryTestWebUIControllerFactory
const GURL& url) override {
return IsSystemAppTestURL(url);
}

bool UseWebUIBindingsForURL(content::BrowserContext* browser_context,
const GURL& url) override {
return IsSystemAppTestURL(url);
}
};

} // namespace
Expand Down
4 changes: 0 additions & 4 deletions content/browser/network_service_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,6 @@ class WebUITestWebUIControllerFactory : public WebUIControllerFactory {
const GURL& url) override {
return HasWebUIScheme(url);
}
bool UseWebUIBindingsForURL(BrowserContext* browser_context,
const GURL& url) override {
return HasWebUIScheme(url);
}
};

class TestWebUIDataSource : public URLDataSource {
Expand Down
4 changes: 2 additions & 2 deletions content/browser/renderer_host/render_frame_host_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1363,7 +1363,7 @@ RenderFrameHostManager::ShouldSwapBrowsingInstancesForNavigation(
// page and one isn't, or if the WebUI types differ.
if (ChildProcessSecurityPolicyImpl::GetInstance()->HasWebUIBindings(
render_frame_host_->GetProcess()->GetID()) ||
WebUIControllerFactoryRegistry::GetInstance()->UseWebUIBindingsForURL(
WebUIControllerFactoryRegistry::GetInstance()->UseWebUIForURL(
browser_context, current_effective_url)) {
// If so, force a swap if destination is not an acceptable URL for Web UI.
// Here, data URLs are never allowed.
Expand All @@ -1382,7 +1382,7 @@ RenderFrameHostManager::ShouldSwapBrowsingInstancesForNavigation(
}
} else {
// Force a swap if it's a Web UI URL.
if (WebUIControllerFactoryRegistry::GetInstance()->UseWebUIBindingsForURL(
if (WebUIControllerFactoryRegistry::GetInstance()->UseWebUIForURL(
browser_context, destination_effective_url)) {
return ShouldSwapBrowsingInstance::kYes_ForceSwap;
}
Expand Down
5 changes: 0 additions & 5 deletions content/browser/renderer_host/render_frame_host_manager_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,11 +119,6 @@ class RenderFrameHostManagerTestWebUIControllerFactory
return HasWebUIScheme(url);
}

bool UseWebUIBindingsForURL(BrowserContext* browser_context,
const GURL& url) override {
return HasWebUIScheme(url);
}

private:
DISALLOW_COPY_AND_ASSIGN(RenderFrameHostManagerTestWebUIControllerFactory);
};
Expand Down
15 changes: 12 additions & 3 deletions content/browser/renderer_host/render_process_host_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3972,10 +3972,19 @@ bool RenderProcessHostImpl::IsSuitableHost(
CHECK(process_lock.is_invalid());
} else {
// WebUI checks.
bool url_requires_web_ui_bindings =
WebUIControllerFactoryRegistry::GetInstance()->UseWebUIBindingsForURL(
bool url_is_for_web_ui =
WebUIControllerFactoryRegistry::GetInstance()->UseWebUIForURL(
browser_context, site_info.site_url());
if (host_has_web_ui_bindings != url_requires_web_ui_bindings)
if (host_has_web_ui_bindings && !url_is_for_web_ui)
return false;
// A host with no bindings is not necessarily unsuitable for a WebUI, but we
// incorrectly return false here. For example, some WebUIs, like
// chrome://process-internals, don't have bindings, so this method would
// return false for a chrome://process-internals host and a
// chrome://process-internals target URL.
// TODO(crbug.com/1158277): Don't return false for suitable WebUI hosts
// and WebUI target URLs.
if (!host_has_web_ui_bindings && url_is_for_web_ui)
return false;

if (process_lock.is_locked_to_site()) {
Expand Down
6 changes: 0 additions & 6 deletions content/browser/webui/content_web_ui_controller_factory.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,6 @@ bool ContentWebUIControllerFactory::UseWebUIForURL(
return GetWebUIType(browser_context, url) != WebUI::kNoWebUI;
}

bool ContentWebUIControllerFactory::UseWebUIBindingsForURL(
BrowserContext* browser_context,
const GURL& url) {
return UseWebUIForURL(browser_context, url);
}

std::unique_ptr<WebUIController>
ContentWebUIControllerFactory::CreateWebUIControllerForURL(WebUI* web_ui,
const GURL& url) {
Expand Down
2 changes: 0 additions & 2 deletions content/browser/webui/content_web_ui_controller_factory.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@ class CONTENT_EXPORT ContentWebUIControllerFactory
const GURL& url) override;
bool UseWebUIForURL(BrowserContext* browser_context,
const GURL& url) override;
bool UseWebUIBindingsForURL(BrowserContext* browser_context,
const GURL& url) override;
std::unique_ptr<WebUIController> CreateWebUIControllerForURL(
WebUI* web_ui,
const GURL& url) override;
Expand Down
12 changes: 0 additions & 12 deletions content/browser/webui/web_ui_controller_factory_registry.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,18 +79,6 @@ bool WebUIControllerFactoryRegistry::UseWebUIForURL(
return false;
}

bool WebUIControllerFactoryRegistry::UseWebUIBindingsForURL(
BrowserContext* browser_context,
const GURL& url) {
std::vector<WebUIControllerFactory*>* factories =
g_web_ui_controller_factories.Pointer();
for (size_t i = 0; i < factories->size(); ++i) {
if ((*factories)[i]->UseWebUIBindingsForURL(browser_context, url))
return true;
}
return false;
}

bool WebUIControllerFactoryRegistry::IsURLAcceptableForWebUI(
BrowserContext* browser_context,
const GURL& url) {
Expand Down
2 changes: 0 additions & 2 deletions content/browser/webui/web_ui_controller_factory_registry.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@ class CONTENT_EXPORT WebUIControllerFactoryRegistry
const GURL& url) override;
bool UseWebUIForURL(BrowserContext* browser_context,
const GURL& url) override;
bool UseWebUIBindingsForURL(BrowserContext* browser_context,
const GURL& url) override;

// Returns true if the given URL can be loaded by Web UI system. This allows
// URLs that UseWebUIForURL returns true for, and also URLs that can be loaded
Expand Down
4 changes: 0 additions & 4 deletions content/browser/webui/web_ui_mojo_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,10 +173,6 @@ class TestWebUIControllerFactory : public WebUIControllerFactory {
const GURL& url) override {
return GetWebUIType(browser_context, url) != WebUI::kNoWebUI;
}
bool UseWebUIBindingsForURL(BrowserContext* browser_context,
const GURL& url) override {
return GetWebUIType(browser_context, url) != WebUI::kNoWebUI;
}

void set_web_ui_enabled(bool enabled) { web_ui_enabled_ = enabled; }

Expand Down
4 changes: 0 additions & 4 deletions content/public/browser/web_ui_controller_factory.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,6 @@ class CONTENT_EXPORT WebUIControllerFactory {
// (faster) and can be used to determine security policy.
virtual bool UseWebUIForURL(BrowserContext* browser_context,
const GURL& url) = 0;

// Returns true for the subset of WebUIs that actually need WebUI bindings.
virtual bool UseWebUIBindingsForURL(BrowserContext* browser_context,
const GURL& url) = 0;
};

} // namespace content
Expand Down
6 changes: 0 additions & 6 deletions content/public/test/web_ui_browsertest_util.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -212,10 +212,4 @@ bool TestWebUIControllerFactory::UseWebUIForURL(BrowserContext* browser_context,
return GetWebUIType(browser_context, url) != WebUI::kNoWebUI;
}

bool TestWebUIControllerFactory::UseWebUIBindingsForURL(
BrowserContext* browser_context,
const GURL& url) {
return GetWebUIType(browser_context, url) != WebUI::kNoWebUI;
}

} // namespace content
2 changes: 0 additions & 2 deletions content/public/test/web_ui_browsertest_util.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,6 @@ class TestWebUIControllerFactory : public WebUIControllerFactory {
const GURL& url) override;
bool UseWebUIForURL(BrowserContext* browser_context,
const GURL& url) override;
bool UseWebUIBindingsForURL(BrowserContext* browser_context,
const GURL& url) override;

private:
bool disable_xfo_ = false;
Expand Down
4 changes: 0 additions & 4 deletions content/renderer/render_view_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,10 +193,6 @@ class WebUITestWebUIControllerFactory : public WebUIControllerFactory {
const GURL& url) override {
return HasWebUIScheme(url);
}
bool UseWebUIBindingsForURL(BrowserContext* browser_context,
const GURL& url) override {
return HasWebUIScheme(url);
}
};

// FrameReplicationState is normally maintained in the browser process,
Expand Down
6 changes: 0 additions & 6 deletions weblayer/browser/webui/web_ui_controller_factory.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,6 @@ bool WebUIControllerFactory::UseWebUIForURL(
return GetWebUIType(browser_context, url) != content::WebUI::kNoWebUI;
}

bool WebUIControllerFactory::UseWebUIBindingsForURL(
content::BrowserContext* browser_context,
const GURL& url) {
return UseWebUIForURL(browser_context, url);
}

std::unique_ptr<content::WebUIController>
WebUIControllerFactory::CreateWebUIControllerForURL(content::WebUI* web_ui,
const GURL& url) {
Expand Down
2 changes: 0 additions & 2 deletions weblayer/browser/webui/web_ui_controller_factory.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ class WebUIControllerFactory : public content::WebUIControllerFactory {
const GURL& url) override;
bool UseWebUIForURL(content::BrowserContext* browser_context,
const GURL& url) override;
bool UseWebUIBindingsForURL(content::BrowserContext* browser_context,
const GURL& url) override;
std::unique_ptr<content::WebUIController> CreateWebUIControllerForURL(
content::WebUI* web_ui,
const GURL& url) override;
Expand Down

0 comments on commit df652e7

Please sign in to comment.