Reland of [Extensions] Limit Extension WebUI (patchset #1 id:1 of https://codereview.chromium.org/2463153003/ )
Reason for revert:
Not the cause of the original issue, but we should keep an eye on the bots after this re-lands.
Original issue's description:
> Revert of [Extensions] Limit Extension WebUI (patchset #6 id:140001 of https://codereview.chromium.org/2452773002/ )
>
> Reason for revert:
> Suspect causing failure:
> https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29/builds/54207
>
> Original issue's description:
> > [Extensions] Limit ExtensionWebUI
> >
> > The bookmark manager needs a WebUI controller in order to change the
> > transition type for links. However, right now, we're creating WebUI for
> > every single extension.
> >
> > Instead, *only* create WebUI for the bookmark manager.
> >
> > Note that the ExtensionWebUI class is currently used for both this WebUI
> > and for chrome url overrides, which are very separate concepts and
> > should be split up, but I'll save that for a later CL.
> >
> > BUG=659798
> >
> > Committed: https://crrev.com/f9bc386d7f73366b45cb85848d27b4343facf45d
> > Cr-Commit-Position: refs/heads/master@{#428747}
>
> [email protected],[email protected],[email protected]
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=659798
>
> Committed: https://crrev.com/17ec5af3f0f5ba7d0d5df76094001153f6dd45e7
> Cr-Commit-Position: refs/heads/master@{#428766}
[email protected],[email protected],[email protected]
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=659798
Review-Url: https://codereview.chromium.org/2459263005
Cr-Commit-Position: refs/heads/master@{#428813}
diff --git a/chrome/browser/extensions/extension_web_ui.cc b/chrome/browser/extensions/extension_web_ui.cc
index 024282ec85..d472a5e 100644
--- a/chrome/browser/extensions/extension_web_ui.cc
+++ b/chrome/browser/extensions/extension_web_ui.cc
@@ -333,12 +333,12 @@
"extensions.chrome_url_overrides";
ExtensionWebUI::ExtensionWebUI(content::WebUI* web_ui, const GURL& url)
- : WebUIController(web_ui),
- url_(url) {
+ : WebUIController(web_ui) {
Profile* profile = Profile::FromWebUI(web_ui);
const Extension* extension = extensions::ExtensionRegistry::Get(
profile)->enabled_extensions().GetExtensionOrAppByURL(url);
DCHECK(extension);
+ DCHECK_EQ(extension_misc::kBookmarkManagerId, extension->id());
// The base class defaults to enabling WebUI bindings, but we don't need
// those (this is also reflected in ChromeWebUIControllerFactory::
@@ -347,20 +347,26 @@
web_ui->SetBindings(bindings);
// Hack: A few things we specialize just for the bookmark manager.
- if (extension->id() == extension_misc::kBookmarkManagerId) {
- bookmark_manager_private_drag_event_router_.reset(
- new extensions::BookmarkManagerPrivateDragEventRouter(
- profile, web_ui->GetWebContents()));
+ bookmark_manager_private_drag_event_router_.reset(
+ new extensions::BookmarkManagerPrivateDragEventRouter(
+ profile, web_ui->GetWebContents()));
- web_ui->SetLinkTransitionType(ui::PAGE_TRANSITION_AUTO_BOOKMARK);
- }
+ web_ui->SetLinkTransitionType(ui::PAGE_TRANSITION_AUTO_BOOKMARK);
}
ExtensionWebUI::~ExtensionWebUI() {}
-extensions::BookmarkManagerPrivateDragEventRouter*
-ExtensionWebUI::bookmark_manager_private_drag_event_router() {
- return bookmark_manager_private_drag_event_router_.get();
+// static
+bool ExtensionWebUI::NeedsExtensionWebUI(
+ content::BrowserContext* browser_context,
+ const GURL& url) {
+ CHECK(browser_context);
+ const Extension* extension =
+ extensions::ExtensionRegistry::Get(browser_context)->enabled_extensions().
+ GetExtensionOrAppByURL(url);
+ // Only use bindings for the Bookmark Manager extension, which needs it as a
+ // hack (see ExtensionWebUI's constructor below).
+ return extension && extension->id() == extension_misc::kBookmarkManagerId;
}
////////////////////////////////////////////////////////////////////////////////
diff --git a/chrome/browser/extensions/extension_web_ui.h b/chrome/browser/extensions/extension_web_ui.h
index 3a9dbd5..79de7d1 100644
--- a/chrome/browser/extensions/extension_web_ui.h
+++ b/chrome/browser/extensions/extension_web_ui.h
@@ -8,6 +8,7 @@
#include <memory>
#include <string>
+#include "base/macros.h"
#include "chrome/common/extensions/chrome_manifest_url_handlers.h"
#include "components/favicon_base/favicon_callback.h"
#include "content/public/browser/web_ui_controller.h"
@@ -31,6 +32,8 @@
// the main tab contents area. For example, each extension can specify an
// "options_page", and that page is displayed in the tab contents area and is
// hosted by this class.
+// TODO(devlin): The above description has nothing to do with this class as far
+// as I can tell.
class ExtensionWebUI : public content::WebUIController {
public:
static const char kExtensionURLOverrides[];
@@ -39,9 +42,15 @@
~ExtensionWebUI() override;
- virtual extensions::BookmarkManagerPrivateDragEventRouter*
- bookmark_manager_private_drag_event_router();
+ // Returns true if the given url requires WebUI bindings.
+ static bool NeedsExtensionWebUI(content::BrowserContext* browser_context,
+ const GURL& url);
+ // TODO(devlin): The rest of this class is static methods dealing with
+ // chrome url overrides (e.g. changing chrome://newtab to go to an extension-
+ // provided new tab page). This should be in a separate class from the WebUI
+ // controller for the bookmark manager, and the WebUI controller should be
+ // renamed.
// BrowserURLHandler
static bool HandleChromeURLOverride(GURL* url,
content::BrowserContext* browser_context);
@@ -84,6 +93,11 @@
const GURL& page_url,
const favicon_base::FaviconResultsCallback& callback);
+ extensions::BookmarkManagerPrivateDragEventRouter*
+ bookmark_manager_private_drag_event_router() {
+ return bookmark_manager_private_drag_event_router_.get();
+ }
+
private:
// Unregister the specified override, and if it's the currently active one,
// ensure that something takes its place.
@@ -97,8 +111,7 @@
std::unique_ptr<extensions::BookmarkManagerPrivateDragEventRouter>
bookmark_manager_private_drag_event_router_;
- // The URL this WebUI was created for.
- GURL url_;
+ DISALLOW_COPY_AND_ASSIGN(ExtensionWebUI);
};
#endif // CHROME_BROWSER_EXTENSIONS_EXTENSION_WEB_UI_H_
diff --git a/chrome/browser/ui/views/bookmarks/bookmark_context_menu_unittest.cc b/chrome/browser/ui/views/bookmarks/bookmark_context_menu_unittest.cc
index e9eb06d..1fa8186 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_context_menu_unittest.cc
+++ b/chrome/browser/ui/views/bookmarks/bookmark_context_menu_unittest.cc
@@ -142,10 +142,10 @@
}
// Tests open all on a folder with a couple of bookmarks in incognito window.
-TEST_F(BookmarkContextMenuTest, OpenAllIngonito) {
+TEST_F(BookmarkContextMenuTest, OpenAllIncognito) {
const BookmarkNode* folder = model_->bookmark_bar_node()->GetChild(1);
chrome::OpenAll(NULL, &navigator_, folder,
- WindowOpenDisposition::OFF_THE_RECORD, NULL);
+ WindowOpenDisposition::OFF_THE_RECORD, profile_.get());
// Should have navigated to only f1a but not f2a.
ASSERT_EQ(static_cast<size_t>(1), navigator_.urls_.size());
diff --git a/chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc b/chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc
index d7404f2f..12ef174 100644
--- a/chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc
+++ b/chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc
@@ -285,23 +285,6 @@
}
#endif // defined(OS_WIN)
-#if defined(ENABLE_EXTENSIONS)
-// Only create ExtensionWebUI for URLs that are allowed extension bindings,
-// hosted by actual tabs.
-bool NeedsExtensionWebUI(Profile* profile, const GURL& url) {
- if (!profile)
- return false;
-
- const extensions::Extension* extension =
- extensions::ExtensionRegistry::Get(profile)->enabled_extensions().
- GetExtensionOrAppByURL(url);
- // Allow bindings for all packaged extensions and component hosted apps.
- return extension &&
- (!extension->is_hosted_app() ||
- extension->location() == extensions::Manifest::COMPONENT);
-}
-#endif
-
bool IsAboutUI(const GURL& url) {
return (url.host() == chrome::kChromeUIChromeURLsHost ||
url.host() == chrome::kChromeUICreditsHost ||
@@ -329,7 +312,7 @@
Profile* profile,
const GURL& url) {
#if defined(ENABLE_EXTENSIONS)
- if (NeedsExtensionWebUI(profile, url))
+ if (ExtensionWebUI::NeedsExtensionWebUI(profile, url))
return &NewWebUI<ExtensionWebUI>;
#endif
@@ -692,7 +675,7 @@
// Extensions are rendered via WebUI in tabs, but don't actually need WebUI
// bindings (see the ExtensionWebUI constructor).
needs_extensions_web_ui =
- NeedsExtensionWebUI(Profile::FromBrowserContext(browser_context), url);
+ ExtensionWebUI::NeedsExtensionWebUI(browser_context, url);
#endif
return !needs_extensions_web_ui && UseWebUIForURL(browser_context, url);
}
