Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
484033003:
Open embedded extension options from the extension action context menu (Closed)
DescriptionOpen embedded extension options from the extension action context menu
This CL allows the extension action context menu to use the new embedded
extension options feature. If the enable-embedded-extension-options flag
is enabled, instead of opening an extension's options page in it's own
tab, it will open chrome://extensions, scroll down to the correct
extension, and then open the embedded options page popup.
The context menu opens a link to chrome://extensions/ with the query
string ?options=|extensionId|. This query string is appended and
removed whenever the embedded extension options popup is opened or
closed.
BUG=386842
Committed: https://crrev.com/f97b7c27ac0f16f163bbe9753cd84d79eafccb37
Cr-Commit-Position: refs/heads/master@{#291826}
Patch Set 1 #Patch Set 2 : Fix some bugs #
Total comments: 2
Patch Set 3 : Move tab opening code from chrome_pages to ExtensionTabUtil #
Total comments: 9
Patch Set 4 : Address devlin's comments #Patch Set 5 : Open options from context menu as a singleton tab, handle query string #
Total comments: 6
Patch Set 6 : Address nits #Patch Set 7 : Rebase #Patch Set 8 : Rebase #Patch Set 9 : Put FeatureSwitch includes back into ExtensionMenuModel #Patch Set 10 : Rebase #Patch Set 11 : Rebase #
Total comments: 17
Patch Set 12 : Refactor opening the overlay, replace state using uber utils #
Total comments: 2
Patch Set 13 : Address comments #Patch Set 14 : Set optionsShown_ to false in refactored method #Patch Set 15 : Fix missed cases in refactor - setting optionsShown_ in onclose, scrolling only in the context menu… #
Messages
Total messages: 29 (0 generated)
PTAL
https://codereview.chromium.org/484033003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/484033003/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:180: chrome::ShowExtensionOptions(browser_, extension->id()); will look at this more later, but first off, why not just put the check for how to (and implementation to) open the options page in ExtensionTabUtil::OpenOptionsPage()? Are there going to be times when we want to show the legacy page?
https://codereview.chromium.org/484033003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/484033003/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:180: chrome::ShowExtensionOptions(browser_, extension->id()); On 2014/08/19 17:39:07, Devlin wrote: > will look at this more later, but first off, why not just put the check for how > to (and implementation to) open the options page in > ExtensionTabUtil::OpenOptionsPage()? Are there going to be times when we want > to show the legacy page? That's a good idea; I just did it this way because I was using the extension highlighting when you click the 'manage' context menu option as an example.
https://codereview.chromium.org/484033003/diff/40001/chrome/browser/extension... File chrome/browser/extensions/extension_tab_util.cc (right): https://codereview.chromium.org/484033003/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_tab_util.cc:571: if (extensions::FeatureSwitch::embedded_extension_options()->IsEnabled()) { no extensions:: https://codereview.chromium.org/484033003/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_tab_util.cc:571: if (extensions::FeatureSwitch::embedded_extension_options()->IsEnabled()) { Maybe a quick comment to say what the implications of all this are. https://codereview.chromium.org/484033003/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_tab_util.cc:576: query += extension->id(); use StringPrintf here: std::string query = base::StringPrintf("options=%s", extension->id().c_str()); https://codereview.chromium.org/484033003/diff/40001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/484033003/diff/40001/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:39: * to prevent showExtensionNodes_ from opening the options more than once. Can this happen? https://codereview.chromium.org/484033003/diff/40001/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:69: if (idToHighlight && $(idToHighlight)) { no brackets around single-line ifs.
https://codereview.chromium.org/484033003/diff/40001/chrome/browser/extension... File chrome/browser/extensions/extension_tab_util.cc (right): https://codereview.chromium.org/484033003/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_tab_util.cc:571: if (extensions::FeatureSwitch::embedded_extension_options()->IsEnabled()) { On 2014/08/19 22:20:41, Devlin wrote: > no extensions:: Done. https://codereview.chromium.org/484033003/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_tab_util.cc:576: query += extension->id(); On 2014/08/19 22:20:41, Devlin wrote: > use StringPrintf here: > std::string query = base::StringPrintf("options=%s", extension->id().c_str()); Done. https://codereview.chromium.org/484033003/diff/40001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/484033003/diff/40001/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:39: * to prevent showExtensionNodes_ from opening the options more than once. On 2014/08/19 22:20:41, Devlin wrote: > Can this happen? Yes, I was getting weird behavior where showExtensionNodes_ kept getting called, and new <extensionoptions> elements were being added to the opened overlay. https://codereview.chromium.org/484033003/diff/40001/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:69: if (idToHighlight && $(idToHighlight)) { On 2014/08/19 22:20:41, Devlin wrote: > no brackets around single-line ifs. Done.
ping?
per in-person demo: - Ideally, if we already have an extensions: page open, we use that (rather than opening a new one). - You have a bug where if you open an extension's options page, close it, and then try to open it again, it fails.
On 2014/08/20 20:56:52, Devlin wrote: > per in-person demo: > - Ideally, if we already have an extensions: page open, we use that (rather than > opening a new one). > - You have a bug where if you open an extension's options page, close it, and > then try to open it again, it fails. This should work as expected now. The context menu option uses the singleton_tabs utils to use existing chrome://extension tabs to show the options pages, even if it has a options query string already. I fixed the bug you mentioned by removing the query string when the popup is closed, and adding it when opened from inside chrome://extensions.
I'll probably want another demo later today :) https://codereview.chromium.org/484033003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_tab_util.cc (right): https://codereview.chromium.org/484033003/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_tab_util.cc:576: chrome::GetSingletonTabNavigateParams(browser, indentation https://codereview.chromium.org/484033003/diff/80001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/484033003/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:225: // Add the options query string nit: . https://codereview.chromium.org/484033003/diff/80001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_options_overlay.js (right): https://codereview.chromium.org/484033003/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/extension_options_overlay.js:54: // Remove the options query string nit: .
On 2014/08/21 17:18:19, Devlin wrote: > I'll probably want another demo later today :) > > https://codereview.chromium.org/484033003/diff/80001/chrome/browser/extension... > File chrome/browser/extensions/extension_tab_util.cc (right): > > https://codereview.chromium.org/484033003/diff/80001/chrome/browser/extension... > chrome/browser/extensions/extension_tab_util.cc:576: > chrome::GetSingletonTabNavigateParams(browser, > indentation > > https://codereview.chromium.org/484033003/diff/80001/chrome/browser/resources... > File chrome/browser/resources/extensions/extension_list.js (right): > > https://codereview.chromium.org/484033003/diff/80001/chrome/browser/resources... > chrome/browser/resources/extensions/extension_list.js:225: // Add the options > query string > nit: . > > https://codereview.chromium.org/484033003/diff/80001/chrome/browser/resources... > File chrome/browser/resources/extensions/extension_options_overlay.js (right): > > https://codereview.chromium.org/484033003/diff/80001/chrome/browser/resources... > chrome/browser/resources/extensions/extension_options_overlay.js:54: // Remove > the options query string > nit: . After those, lgtm.
https://codereview.chromium.org/484033003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_tab_util.cc (right): https://codereview.chromium.org/484033003/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_tab_util.cc:576: chrome::GetSingletonTabNavigateParams(browser, On 2014/08/21 17:18:19, Devlin wrote: > indentation Done. https://codereview.chromium.org/484033003/diff/80001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/484033003/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:225: // Add the options query string On 2014/08/21 17:18:19, Devlin wrote: > nit: . Done. https://codereview.chromium.org/484033003/diff/80001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_options_overlay.js (right): https://codereview.chromium.org/484033003/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/extension_options_overlay.js:54: // Remove the options query string On 2014/08/21 17:18:19, Devlin wrote: > nit: . Done.
ping kalman
https://codereview.chromium.org/484033003/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/extension_tab_util.cc (right): https://codereview.chromium.org/484033003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_tab_util.cc:578: if (!extension->id().empty()) { I don't think it's possible for Extensions to have an empty ID. Is this defensive or did you run into it? https://codereview.chromium.org/484033003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_tab_util.cc:581: base::StringPrintf("options=%s", extension->id().c_str()); Using StringPrintf here is overkill. Just "options=" + extension->id() is fine. I'd even inline it. https://codereview.chromium.org/484033003/diff/200001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/484033003/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:70: this.scrollToNode_(idToHighlight); I'm seeing some weird behaviour when you specify an id=gighmmpiobklfepjocnamgkkbiglidom and an options=gighmmpiobklfepjocnamgkkbiglidom. It removes the ID but still highlights the extension, then when you dismiss the dialogue it continues to highlight the extension. Basically: I don't think it should hide the ID. You shouldn't be messing with the parameters so much. Just add/remove the options parameter, that's all. https://codereview.chromium.org/484033003/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:75: $(idToOpenOptions).querySelector('.options-link').click(); Why clicking on this link? Seems kinda roundabout. Could you pull the click handling logic into a member function and call that from here and the event handler? You could also do the "!this.optionsShown_" check, and setting it, in that function. https://codereview.chromium.org/484033003/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:225: // Add the options query string. Mention why you need to postMessage here. https://codereview.chromium.org/484033003/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:231: replace: false I think that clicking on an icon should replace the state. Clicking on a link which opens a dialogue doesn't seem like it should be a new navigation. I.e. if you: 1. go to google.com 2. go to chrome://extensions 3. click on an options link 4. press back Should you go to chrome://extensions or google.com? I think google.com. It's weird if not. Site note: when I try this with your patch in, pressing back doesn't dismiss the options dialogue. It should (but replacing state will bypass that issue). HOWEVER when I press back a second time to go to google.com it's 100% reproducible that the renderer crashes. Unfortunately I don't get a stack trace. It might not be your patch, but, seems... plausible. https://codereview.chromium.org/484033003/diff/200001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_options_overlay.js (right): https://codereview.chromium.org/484033003/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_options_overlay.js:62: params: {state: {}, path: '', replace: false} Same comments here in the other file.
https://codereview.chromium.org/484033003/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/extension_tab_util.cc (right): https://codereview.chromium.org/484033003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_tab_util.cc:581: base::StringPrintf("options=%s", extension->id().c_str()); On 2014/08/25 18:11:07, kalman wrote: > Using StringPrintf here is overkill. Just "options=" + extension->id() is fine. > I'd even inline it. I told him to, as I've been told repeatedly to use it (even when just concatting two).
https://codereview.chromium.org/484033003/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/extension_tab_util.cc (right): https://codereview.chromium.org/484033003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_tab_util.cc:578: if (!extension->id().empty()) { On 2014/08/25 18:11:07, kalman wrote: > I don't think it's possible for Extensions to have an empty ID. Is this > defensive or did you run into it? This part was adapted from chrome_pages.cc, which handles the 'Manage' entry in the context menu. They do this check as well (I have no idea if its necessary). https://codereview.chromium.org/484033003/diff/200001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/484033003/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:75: $(idToOpenOptions).querySelector('.options-link').click(); On 2014/08/25 18:11:07, kalman wrote: > Why clicking on this link? Seems kinda roundabout. Could you pull the click > handling logic into a member function and call that from here and the event > handler? You could also do the "!this.optionsShown_" check, and setting it, in > that function. Done. https://codereview.chromium.org/484033003/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:225: // Add the options query string. On 2014/08/25 18:11:07, kalman wrote: > Mention why you need to postMessage here. Done. https://codereview.chromium.org/484033003/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:231: replace: false On 2014/08/25 18:11:07, kalman wrote: > I think that clicking on an icon should replace the state. Clicking on a link > which opens a dialogue doesn't seem like it should be a new navigation. > > I.e. if you: > 1. go to http://google.com > 2. go to chrome://extensions > 3. click on an options link > 4. press back > > Should you go to chrome://extensions or google.com? I think http://google.com. It's > weird if not. > > Site note: when I try this with your patch in, pressing back doesn't dismiss the > options dialogue. It should (but replacing state will bypass that issue). > HOWEVER when I press back a second time to go to http://google.com it's 100% > reproducible that the renderer crashes. Unfortunately I don't get a stack trace. > It might not be your patch, but, seems... plausible. Replace state is done. It looks like the crash is actually a regression caused by the window-closing CL; it has something to do with how my guest view is implementing WebContentsDelegate::CloseContents. It happens whenever you try to navigate away and an <extensionoptions> is in the DOM. I'll see if I can fix that in separate CL. https://codereview.chromium.org/484033003/diff/200001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_options_overlay.js (right): https://codereview.chromium.org/484033003/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_options_overlay.js:62: params: {state: {}, path: '', replace: false} On 2014/08/25 18:11:07, kalman wrote: > Same comments here in the other file. Done.
lgtm https://codereview.chromium.org/484033003/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/extension_tab_util.cc (right): https://codereview.chromium.org/484033003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_tab_util.cc:578: if (!extension->id().empty()) { On 2014/08/25 21:42:34, ericzeng wrote: > On 2014/08/25 18:11:07, kalman wrote: > > I don't think it's possible for Extensions to have an empty ID. Is this > > defensive or did you run into it? > > This part was adapted from chrome_pages.cc, which handles the 'Manage' entry in > the context menu. They do this check as well (I have no idea if its necessary). Let's kill both. They're silly. https://codereview.chromium.org/484033003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_tab_util.cc:581: base::StringPrintf("options=%s", extension->id().c_str()); On 2014/08/25 18:12:51, Devlin wrote: > On 2014/08/25 18:11:07, kalman wrote: > > Using StringPrintf here is overkill. Just "options=" + extension->id() is > fine. > > I'd even inline it. > > I told him to, as I've been told repeatedly to use it (even when just concatting > two). Btw Devlin and I follow up on this offline with Jeffrey. Conclusion: a single "+" is the same efficiency as StringPrintf, and the readability of using StringPrintf is "debatable". IMO given it's twice the amount of typing, "+" is much better. 2 or more "+"s is likely less efficient than StringPrintf. Anyhow, in the spirit of not changing anything, leave it the way it is. But no need to change it next time. https://codereview.chromium.org/484033003/diff/220001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/484033003/diff/220001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:465: // However, this should't happen unless the URL is manually typed in. There is another case where this happens actually, when you go to Manage then click on Options. It kills the ID part. That's not worth worrying about. So I'd just ignore past-me and delete this comment :)
https://codereview.chromium.org/484033003/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/extension_tab_util.cc (right): https://codereview.chromium.org/484033003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_tab_util.cc:578: if (!extension->id().empty()) { On 2014/08/25 21:53:29, kalman wrote: > On 2014/08/25 21:42:34, ericzeng wrote: > > On 2014/08/25 18:11:07, kalman wrote: > > > I don't think it's possible for Extensions to have an empty ID. Is this > > > defensive or did you run into it? > > > > This part was adapted from chrome_pages.cc, which handles the 'Manage' entry > in > > the context menu. They do this check as well (I have no idea if its > necessary). > > Let's kill both. They're silly. But then I would need an owner for chrome/browser/ui/ :( https://codereview.chromium.org/484033003/diff/220001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/484033003/diff/220001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:465: // However, this should't happen unless the URL is manually typed in. On 2014/08/25 21:53:29, kalman wrote: > There is another case where this happens actually, when you go to Manage then > click on Options. It kills the ID part. > > That's not worth worrying about. So I'd just ignore past-me and delete this > comment :) Done.
https://codereview.chromium.org/484033003/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/extension_tab_util.cc (right): https://codereview.chromium.org/484033003/diff/200001/chrome/browser/extensio... chrome/browser/extensions/extension_tab_util.cc:578: if (!extension->id().empty()) { On 2014/08/25 22:04:04, ericzeng wrote: > On 2014/08/25 21:53:29, kalman wrote: > > On 2014/08/25 21:42:34, ericzeng wrote: > > > On 2014/08/25 18:11:07, kalman wrote: > > > > I don't think it's possible for Extensions to have an empty ID. Is this > > > > defensive or did you run into it? > > > > > > This part was adapted from chrome_pages.cc, which handles the 'Manage' entry > > in > > > the context menu. They do this check as well (I have no idea if its > > necessary). > > > > Let's kill both. They're silly. > > But then I would need an owner for chrome/browser/ui/ :( Ok, just remove this one then :)
The CQ bit was checked by ericzeng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ericzeng@chromium.org/484033003/260001
The CQ bit was unchecked by ericzeng@chromium.org
The CQ bit was checked by ericzeng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ericzeng@chromium.org/484033003/280001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to commit the patch.
The CQ bit was checked by ericzeng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ericzeng@chromium.org/484033003/280001
Message was sent while issue was closed.
Committed patchset #15 (280001) as 25e02de2817823f384721764956aa9a75a69adfb
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/f97b7c27ac0f16f163bbe9753cd84d79eafccb37 Cr-Commit-Position: refs/heads/master@{#291826} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
