We shouldn't be creating WebUI for all extensions |
|||
Issue descriptionWe 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.
,
Oct 26 2016
That would be great!
,
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
,
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
,
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
,
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
,
Dec 2 2016
Fixed, and seems unlikely to be reverted at this point. Yay! |
|||
►
Sign in to add a comment |
|||
Comment 1 by rdevlin....@chromium.org
, Oct 26 2016