New issue
Advanced search Search tips
Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

We shouldn't be creating WebUI for all extensions

Project Member Reported by rdevlin....@chromium.org, Oct 26 2016 Back to list

Issue description

We currently create ExtensionWebUI for every single extension, but we should only be doing it for the BookmarkManager, since its only use is in adjusting the link transition type.

We might even be able to just move that single call somewhere else and just nuke ExtensionWebUI entirely.
 

Comment 2 by creis@chromium.org, Oct 26 2016

Cc: creis@chromium.org
Components: UI>Browser>WebUI
That would be great!
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 31 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f9bc386d7f73366b45cb85848d27b4343facf45d

commit f9bc386d7f73366b45cb85848d27b4343facf45d
Author: rdevlin.cronin <rdevlin.cronin@chromium.org>
Date: Mon Oct 31 17:04:26 2016

[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 

Review-Url: https://codereview.chromium.org/2452773002
Cr-Commit-Position: refs/heads/master@{#428747}

[modify] https://crrev.com/f9bc386d7f73366b45cb85848d27b4343facf45d/chrome/browser/extensions/extension_web_ui.cc
[modify] https://crrev.com/f9bc386d7f73366b45cb85848d27b4343facf45d/chrome/browser/extensions/extension_web_ui.h
[modify] https://crrev.com/f9bc386d7f73366b45cb85848d27b4343facf45d/chrome/browser/ui/views/bookmarks/bookmark_context_menu_unittest.cc
[modify] https://crrev.com/f9bc386d7f73366b45cb85848d27b4343facf45d/chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Oct 31 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/17ec5af3f0f5ba7d0d5df76094001153f6dd45e7

commit 17ec5af3f0f5ba7d0d5df76094001153f6dd45e7
Author: xidachen <xidachen@chromium.org>
Date: Mon Oct 31 19:26:53 2016

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}

TBR=dbeam@chromium.org,sky@chromium.org,rdevlin.cronin@chromium.org
# 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/2463153003
Cr-Commit-Position: refs/heads/master@{#428766}

[modify] https://crrev.com/17ec5af3f0f5ba7d0d5df76094001153f6dd45e7/chrome/browser/extensions/extension_web_ui.cc
[modify] https://crrev.com/17ec5af3f0f5ba7d0d5df76094001153f6dd45e7/chrome/browser/extensions/extension_web_ui.h
[modify] https://crrev.com/17ec5af3f0f5ba7d0d5df76094001153f6dd45e7/chrome/browser/ui/views/bookmarks/bookmark_context_menu_unittest.cc
[modify] https://crrev.com/17ec5af3f0f5ba7d0d5df76094001153f6dd45e7/chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Oct 31 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2f98b6bb35e391918dd1e57ad42b289fc6940bdf

commit 2f98b6bb35e391918dd1e57ad42b289fc6940bdf
Author: xidachen <xidachen@chromium.org>
Date: Mon Oct 31 21:18:06 2016

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}
>
> TBR=dbeam@chromium.org,sky@chromium.org,rdevlin.cronin@chromium.org
> # 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}

TBR=dbeam@chromium.org,sky@chromium.org,rdevlin.cronin@chromium.org
# 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}

[modify] https://crrev.com/2f98b6bb35e391918dd1e57ad42b289fc6940bdf/chrome/browser/extensions/extension_web_ui.cc
[modify] https://crrev.com/2f98b6bb35e391918dd1e57ad42b289fc6940bdf/chrome/browser/extensions/extension_web_ui.h
[modify] https://crrev.com/2f98b6bb35e391918dd1e57ad42b289fc6940bdf/chrome/browser/ui/views/bookmarks/bookmark_context_menu_unittest.cc
[modify] https://crrev.com/2f98b6bb35e391918dd1e57ad42b289fc6940bdf/chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Nov 9 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a32a0900c104b2143e6462febc81e6900dbd2104

commit a32a0900c104b2143e6462febc81e6900dbd2104
Author: rdevlin.cronin <rdevlin.cronin@chromium.org>
Date: Wed Nov 09 15:50:20 2016

[Extensions] Remove ExtensionWebUI

ExtensionWebUI only did two things:
- Set the link transition type for the bookmark manager to
  PAGE_TRANSITION_AUTO_BOOKMARK.
- Instantiated the BookmarkManagerPrivateDragEventRouter.

Neither of these things should require WebUI, and having extension
WebContents have WebUI is bad.

Instead, handle the link transition in OverrideOpenUrlParams (as well
as modifying the referrer and the renderer-initiated, as we
currently do for WebUI navigations), and make the
BookmarkManagerPrivateDragEventRouter a WebContentsUserData
instantiated from extensions::TabHelper.

BUG= 659798 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2468673003
Cr-Commit-Position: refs/heads/master@{#430940}

[modify] https://crrev.com/a32a0900c104b2143e6462febc81e6900dbd2104/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/a32a0900c104b2143e6462febc81e6900dbd2104/chrome/browser/chrome_content_browser_client.h
[modify] https://crrev.com/a32a0900c104b2143e6462febc81e6900dbd2104/chrome/browser/extensions/api/bookmark_manager_private/bookmark_manager_private_api.cc
[modify] https://crrev.com/a32a0900c104b2143e6462febc81e6900dbd2104/chrome/browser/extensions/api/bookmark_manager_private/bookmark_manager_private_api.h
[modify] https://crrev.com/a32a0900c104b2143e6462febc81e6900dbd2104/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
[modify] https://crrev.com/a32a0900c104b2143e6462febc81e6900dbd2104/chrome/browser/extensions/chrome_content_browser_client_extensions_part.h
[modify] https://crrev.com/a32a0900c104b2143e6462febc81e6900dbd2104/chrome/browser/extensions/extension_web_ui.cc
[modify] https://crrev.com/a32a0900c104b2143e6462febc81e6900dbd2104/chrome/browser/extensions/extension_web_ui.h
[modify] https://crrev.com/a32a0900c104b2143e6462febc81e6900dbd2104/chrome/browser/extensions/tab_helper.cc
[modify] https://crrev.com/a32a0900c104b2143e6462febc81e6900dbd2104/chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc
[modify] https://crrev.com/a32a0900c104b2143e6462febc81e6900dbd2104/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/a32a0900c104b2143e6462febc81e6900dbd2104/content/browser/webui/web_ui_impl.cc
[modify] https://crrev.com/a32a0900c104b2143e6462febc81e6900dbd2104/content/browser/webui/web_ui_impl.h
[modify] https://crrev.com/a32a0900c104b2143e6462febc81e6900dbd2104/content/public/browser/content_browser_client.h
[modify] https://crrev.com/a32a0900c104b2143e6462febc81e6900dbd2104/content/public/browser/web_ui.h
[modify] https://crrev.com/a32a0900c104b2143e6462febc81e6900dbd2104/content/public/test/test_web_ui.cc
[modify] https://crrev.com/a32a0900c104b2143e6462febc81e6900dbd2104/content/public/test/test_web_ui.h

Status: Fixed (was: Assigned)
Fixed, and seems unlikely to be reverted at this point.  Yay!

Sign in to add a comment