Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2452773002:
[Extensions] Limit Extension WebUI (Closed)
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}
Patch Set 1 : done #Patch Set 2 : Add TODOs #
Total comments: 7
Patch Set 3 : Dan's #
Total comments: 2
Patch Set 4 : add missing include #Patch Set 5 : Test fix #
Total comments: 2
Patch Set 6 : sky's #
Messages
Total messages: 47 (35 generated)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Description was changed from ========== Kill webui BUG= ========== to ========== [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 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
rdevlin.cronin@chromium.org changed reviewers: + dbeam@chromium.org
Dan, can you take a look? (Note that this is not WebUI in the sense of chrome:// webui pages.) Nasko, FYI since you pointed out this craziness. :)
https://codereview.chromium.org/2452773002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_web_ui.cc (right): https://codereview.chromium.org/2452773002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_web_ui.cc:339: if (!browser_context) can we really not have a browser_context validly? https://codereview.chromium.org/2452773002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_web_ui.cc:343: extensions::ExtensionRegistry::Get(browser_context)->enabled_extensions(). does this exist in all cases (i.e. guest mode)? i can never remember whether this or extension service (?) don't exist in that (or other) cases https://codereview.chromium.org/2452773002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_web_ui.cc:356: DCHECK_EQ(extension_misc::kBookmarkManagerId, extension->id()); should we rename this class then? to like BookmarkManagerExtensionWebUI?
https://codereview.chromium.org/2452773002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_web_ui.cc (right): https://codereview.chromium.org/2452773002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_web_ui.cc:339: if (!browser_context) On 2016/10/26 21:44:19, Dan Beam wrote: > can we really not have a browser_context validly? I copied this from the old code, but I'd kind of hope not (given this is being called for webui, which needs to have web contents, render frames, etc.). Switched to a CHECK. https://codereview.chromium.org/2452773002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_web_ui.cc:343: extensions::ExtensionRegistry::Get(browser_context)->enabled_extensions(). On 2016/10/26 21:44:19, Dan Beam wrote: > does this exist in all cases (i.e. guest mode)? i can never remember whether > this or extension service (?) don't exist in that (or other) cases ExtensionRegistry should always, always, always exist... as long as extensions are enabled (this isn't even compiled if they're not). https://codereview.chromium.org/2452773002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_web_ui.cc:356: DCHECK_EQ(extension_misc::kBookmarkManagerId, extension->id()); On 2016/10/26 21:44:19, Dan Beam wrote: > should we rename this class then? to like BookmarkManagerExtensionWebUI? Yes. But it's got too much unrelated stuff in it right now (because having settings url overrides in a class called "BookmarkManagerExtensionWebUI" makes even less sense). I've added that as an explicit part of the TODO in the header file for when we split this from the url overrides functionality.
dbeam - friendly ping.
lgtm https://codereview.chromium.org/2452773002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/extension_web_ui.h (right): https://codereview.chromium.org/2452773002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/extension_web_ui.h:10: nit: #include "base/macros.h" for DISALLOW_COPY_AND_ASSIGN
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
rdevlin.cronin@chromium.org changed reviewers: + sky@chromium.org
+sky@ - Scott, can you take a look at the bookmark_context_menu_unittest.cc update? (see comment in extension_web_ui.cc for reason) https://codereview.chromium.org/2452773002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_web_ui.cc (right): https://codereview.chromium.org/2452773002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_web_ui.cc:339: if (!browser_context) On 2016/10/26 21:54:50, Devlin wrote: > On 2016/10/26 21:44:19, Dan Beam wrote: > > can we really not have a browser_context validly? > > I copied this from the old code, but I'd kind of hope not (given this is being > called for webui, which needs to have web contents, render frames, etc.). > Switched to a CHECK. So, not validly - but apparently yes in a unittest. :) Fixed the test to pass in a valid context. https://codereview.chromium.org/2452773002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/extension_web_ui.h (right): https://codereview.chromium.org/2452773002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/extension_web_ui.h:10: On 2016/10/28 22:26:52, Dan Beam wrote: > nit: #include "base/macros.h" > > for DISALLOW_COPY_AND_ASSIGN Done.
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2452773002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/extension_web_ui.h (right): https://codereview.chromium.org/2452773002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/extension_web_ui.h:41: // Returns true if the given url requires WebUI bindings. Style guide says constructor/destructor before other functions.
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2452773002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/extension_web_ui.h (right): https://codereview.chromium.org/2452773002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/extension_web_ui.h:41: // Returns true if the given url requires WebUI bindings. On 2016/10/31 15:11:54, sky wrote: > Style guide says constructor/destructor before other functions. Done.
The CQ bit was checked by rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2452773002/#ps140001 (title: "sky's")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [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 ========== to ========== [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 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [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 ========== to ========== [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} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/f9bc386d7f73366b45cb85848d27b4343facf45d Cr-Commit-Position: refs/heads/master@{#428747}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:140001) has been created in https://codereview.chromium.org/2463153003/ by xidachen@chromium.org. The reason for reverting is: Suspect causing failure: https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2.... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
