Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
475543003:
Display extension options in a WebUI overlay instead of in a new tab (Closed)
DescriptionDisplay extension options in a WebUI overlay instead of in a new tab
Embed extension options pages directly into chrome://extensions by
displaying an overlay containing an <extensionoptions> tag. The
<extensionoptions> tag hosts the extension options page out-of-process.
This feature is hidden behind the flag enable-embedded-extension-options.
BUG=386842
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290415
Patch Set 1 #
Total comments: 51
Patch Set 2 : Address comments #
Total comments: 2
Patch Set 3 : Address comment #Patch Set 4 : Diff from getCurrent CL #
Total comments: 2
Patch Set 5 : Address dbeam's comments #Patch Set 6 : Diff from getCurrent CL again #
Total comments: 4
Patch Set 7 : Move command line check to C++ #
Total comments: 10
Patch Set 8 : Address comments #Patch Set 9 : Rebase #
Total comments: 2
Messages
Total messages: 26 (0 generated)
punt --> devlin.
Ok... un-punting. It's not the afternoon anymore. Looks great! Could you include a screenshot? I'm also going to add dbeam here, a WebUI owner, for a sanity check. I'm not familiar with these dialogues, nor really so confident about WebUI+JS+HTML idioms. https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... File chrome/browser/resources/extensions/extension_options_overlay.js (right): https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:8: function ExtensionOptionsOverlay() {} /** * Comment. * @constructor */ https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:15: * @type {??} @type {string} https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:18: extensionId_: undefined, Weak nit: I prefer null over undefined when you're initializing class variables (initializing a variable seems like defining it to me, so undefined looks strange). However, I'm not up to speed on the idioms of this. Perhaps a WebUI owner can comment. https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:28: console.log('initializePage'); Remove logging .: logging. https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:39: this.overlayDiv_ = $('extension-options-overlay'); Define this as a class property under |extensionId_|? Mainly so that you can put the jsdoc on that, rather than here. I'm not sure the closure compiler can do as much with it here. https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:63: extensionoptions.onsizechanged = function(evt) { TODO(ericzeng): Resize in a non-jarring way. I don't think this is possible without some non-trivial creative CSS or JS, so not worth it for now (the complication being that you need to resize to an arbitrary size, not a pre-determined one). https://codereview.chromium.org/475543003/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/475543003/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/_api_features.json:220: "channel": "trunk", Better make it stable. https://codereview.chromium.org/475543003/diff/1/chrome/renderer/resources/ex... File chrome/renderer/resources/extensions/extension_options.js (right): https://codereview.chromium.org/475543003/diff/1/chrome/renderer/resources/ex... chrome/renderer/resources/extensions/extension_options.js:19: 'minwidth': 32 Can we at least make these max values based on the screen size? window.innerWidth/Height should do it? (Take care with background pages).
https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... File chrome/browser/resources/extensions/extension_options_overlay.js (right): https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:8: function ExtensionOptionsOverlay() {} docs https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:15: * @type {??} type {??} is probably bad :P https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:28: console.log('initializePage'); leftover from testing? https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:41: this.setVisible = function(isVisible) { docs https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:42: console.log('setVisible'); leftover? https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:47: handleDismiss_: function(event) { docs https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:56: setExtensionAndShowOverlay: function(extensionId, extensionName) { docs https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:57: console.log('setExtensionAndShowOverlay'); leftover? https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:64: console.log(evt.width + 'x' + evt.height); leftover? https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:72: '#extension-options-overlay .extension-options-overlay-title') do we need the "#extension-options-overlay"? How many extension-options-overlay-titles are there gonna be? :P
huh. It was unpunted while I was reviewing (so didn't notice 'til after comments sent). Sorry for any duplicates.
https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... File chrome/browser/resources/extensions/extension_options_overlay.js (right): https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:18: extensionId_: undefined, On 2014/08/14 15:33:20, kalman wrote: > Weak nit: I prefer null over undefined when you're initializing class variables > (initializing a variable seems like defining it to me, so undefined looks > strange). However, I'm not up to speed on the idioms of this. Perhaps a WebUI > owner can comment. dbeam will be able to confirm, but I think I've been told undefined lately. https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:39: this.overlayDiv_ = $('extension-options-overlay'); On 2014/08/14 15:33:20, kalman wrote: > Define this as a class property under |extensionId_|? Mainly so that you can put > the jsdoc on that, rather than here. I'm not sure the closure compiler can do as > much with it here. again, dbeam to confirm, but he's told me in the past to just initialize and doc here. (This may or may not have changed since we're actually gonna be using the CC)
https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... File chrome/browser/resources/extensions/extension_options_overlay.js (right): https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:72: '#extension-options-overlay .extension-options-overlay-title') On 2014/08/14 15:37:51, Devlin wrote: > do we need the "#extension-options-overlay"? How many > extension-options-overlay-titles are there gonna be? :P The extension-options-overlay ID is nice, but Devlin has a point, perhaps you can do away with the .extension-options-overlay-title class and use '#extensions-options-overlay h1" here. If you want.
Forgot to clean up a lot of things the first go-around :/ https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... File chrome/browser/resources/extensions/extension_options_overlay.js (right): https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:8: function ExtensionOptionsOverlay() {} On 2014/08/14 15:37:51, Devlin wrote: > docs Done. https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:15: * @type {??} On 2014/08/14 15:33:20, kalman wrote: > @type {string} Done. https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:28: console.log('initializePage'); On 2014/08/14 15:37:51, Devlin wrote: > leftover from testing? Yeeeuuup https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:39: this.overlayDiv_ = $('extension-options-overlay'); On 2014/08/14 15:40:36, Devlin wrote: > On 2014/08/14 15:33:20, kalman wrote: > > Define this as a class property under |extensionId_|? Mainly so that you can > put > > the jsdoc on that, rather than here. I'm not sure the closure compiler can do > as > > much with it here. > > again, dbeam to confirm, but he's told me in the past to just initialize and doc > here. (This may or may not have changed since we're actually gonna be using the > CC) The extension error overlay initializes it here so I'm just going to leave it until we get a definitive answer. https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:41: this.setVisible = function(isVisible) { On 2014/08/14 15:37:51, Devlin wrote: > docs Done. https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:42: console.log('setVisible'); On 2014/08/14 15:37:52, Devlin wrote: > leftover? leftover. https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:47: handleDismiss_: function(event) { On 2014/08/14 15:37:52, Devlin wrote: > docs Done. https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:56: setExtensionAndShowOverlay: function(extensionId, extensionName) { On 2014/08/14 15:37:52, Devlin wrote: > docs Done. https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:57: console.log('setExtensionAndShowOverlay'); On 2014/08/14 15:37:51, Devlin wrote: > leftover? affirmative https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:63: extensionoptions.onsizechanged = function(evt) { On 2014/08/14 15:33:20, kalman wrote: > TODO(ericzeng): Resize in a non-jarring way. > > I don't think this is possible without some non-trivial creative CSS or JS, so > not worth it for now (the complication being that you need to resize to an > arbitrary size, not a pre-determined one). Done (but not the todo). https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:64: console.log(evt.width + 'x' + evt.height); On 2014/08/14 15:37:52, Devlin wrote: > leftover? yes https://codereview.chromium.org/475543003/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/475543003/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/_api_features.json:220: "channel": "trunk", On 2014/08/14 15:33:20, kalman wrote: > Better make it stable. Done. https://codereview.chromium.org/475543003/diff/1/chrome/renderer/resources/ex... File chrome/renderer/resources/extensions/extension_options.js (right): https://codereview.chromium.org/475543003/diff/1/chrome/renderer/resources/ex... chrome/renderer/resources/extensions/extension_options.js:19: 'minwidth': 32 On 2014/08/14 15:33:21, kalman wrote: > Can we at least make these max values based on the screen size? > window.innerWidth/Height should do it? > > (Take care with background pages). Done.
lgtm, but wait for dbeam. https://codereview.chromium.org/475543003/diff/20001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_options_overlay.js (right): https://codereview.chromium.org/475543003/diff/20001/chrome/browser/resources... chrome/browser/resources/extensions/extension_options_overlay.js:62: if (!this.extensionId_) Actually: do you need |extensionId_| at all? It seems like you could replace this check by seeing if the querySelector() call found anything. The only other reference is in setExtensionAndShowOverlay, but that's where it's passed in.
https://codereview.chromium.org/475543003/diff/20001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_options_overlay.js (right): https://codereview.chromium.org/475543003/diff/20001/chrome/browser/resources... chrome/browser/resources/extensions/extension_options_overlay.js:62: if (!this.extensionId_) On 2014/08/14 18:19:30, kalman wrote: > Actually: do you need |extensionId_| at all? It seems like you could replace > this check by seeing if the querySelector() call found anything. > > The only other reference is in setExtensionAndShowOverlay, but that's where it's > passed in. Yeah it doesn't need to be there. I only need it to create the <extensionoptions> element.
https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_list.js:196: chrome.commandLinePrivate.hasSwitch( ^ why are we doing this in JS instead of in C++ and sending the result to JS? https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_list.js:197: 'enable-embedded-extension-options', function(result) { chrome.commandLinePrivate.hasSwitch( 'enable-embedded-extension-options', function(result) { ... }); or chrome.commandLinePrivate.hasSwitch( 'enable-embedded-extension-options', function(result) { ... }); https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_list.js:200: .setExtensionAndShowOverlay([extension.id], extension.name); dot at end: extensions.ExtensionOptionsOverlay.getInstance(). setExtensionAndShowOverlay( https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... File chrome/browser/resources/extensions/extension_options_overlay.js (right): https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:41: this.setVisible = function(isVisible) { move to prototype, make @private e.g. /** * @param {boolean} Whether something or other should be visible. * @private */ setVisible_: function(isVisible) { showOverlay(isVisible ? this.overlayDiv_ : null); }, https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:59: var extensionoptions = new ExtensionOptions(); nit: extensionOptions https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:63: extensionoptions.onsizechanged = function(evt) { On 2014/08/14 15:33:20, kalman wrote: > TODO(ericzeng): Resize in a non-jarring way. > > I don't think this is possible without some non-trivial creative CSS or JS, so > not worth it for now (the complication being that you need to resize to an > arbitrary size, not a pre-determined one). just keep the current JS and just add: transition: height 200ms, width 200ms; or something in the CSS https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:72: '#extension-options-overlay .extension-options-overlay-title') On 2014/08/14 15:40:53, kalman wrote: > On 2014/08/14 15:37:51, Devlin wrote: > > do we need the "#extension-options-overlay"? How many > > extension-options-overlay-titles are there gonna be? :P > > The extension-options-overlay ID is nice, but Devlin has a point, perhaps you > can do away with the .extension-options-overlay-title class and use > '#extensions-options-overlay h1" here. If you want. if there's only ever going to be 1, use an #id. if there's going to be more, use a .class. https://codereview.chromium.org/475543003/diff/60001/chrome/renderer/resource... File chrome/renderer/resources/extensions/extension_options.js (right): https://codereview.chromium.org/475543003/diff/60001/chrome/renderer/resource... chrome/renderer/resources/extensions/extension_options.js:17: 'maxwidth': window.innerWidth, doesn't this only measure the size of the web contents rather than the whole screen? what coordinate system is AUTO_SIZE_ATTRIBUTES in?
https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_list.js:196: chrome.commandLinePrivate.hasSwitch( On 2014/08/14 19:01:26, Dan Beam wrote: > ^ why are we doing this in JS instead of in C++ and sending the result to JS? Is there a convenient way to send the flags check down to C++? I haven't worked in WebUI before so I'm not sure what the best way to do this is. https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_list.js:197: 'enable-embedded-extension-options', function(result) { On 2014/08/14 19:01:26, Dan Beam wrote: > chrome.commandLinePrivate.hasSwitch( > 'enable-embedded-extension-options', > function(result) { > ... > }); > > or > > chrome.commandLinePrivate.hasSwitch( > 'enable-embedded-extension-options', > function(result) { > ... > }); Done. https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_list.js:200: .setExtensionAndShowOverlay([extension.id], extension.name); On 2014/08/14 19:01:26, Dan Beam wrote: > dot at end: > > extensions.ExtensionOptionsOverlay.getInstance(). > setExtensionAndShowOverlay( Done. https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... File chrome/browser/resources/extensions/extension_options_overlay.js (right): https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:41: this.setVisible = function(isVisible) { On 2014/08/14 19:01:27, Dan Beam wrote: > move to prototype, make @private e.g. > > /** > * @param {boolean} Whether something or other should be visible. > * @private > */ > setVisible_: function(isVisible) { > showOverlay(isVisible ? this.overlayDiv_ : null); > }, Done. https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:63: extensionoptions.onsizechanged = function(evt) { On 2014/08/14 19:01:27, Dan Beam wrote: > On 2014/08/14 15:33:20, kalman wrote: > > TODO(ericzeng): Resize in a non-jarring way. > > > > I don't think this is possible without some non-trivial creative CSS or JS, so > > not worth it for now (the complication being that you need to resize to an > > arbitrary size, not a pre-determined one). > > just keep the current JS and just add: > > transition: height 200ms, width 200ms; > > or something in the CSS Unfortunately that doesn't really help with the pop-in. It's an unfortunate limitation with <extensionoptions> and the underlying browser plugin. The embedded webview needs to be attached to the DOM and be visible for it to begin autosizing. https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:72: '#extension-options-overlay .extension-options-overlay-title') On 2014/08/14 19:01:27, Dan Beam wrote: > On 2014/08/14 15:40:53, kalman wrote: > > On 2014/08/14 15:37:51, Devlin wrote: > > > do we need the "#extension-options-overlay"? How many > > > extension-options-overlay-titles are there gonna be? :P > > > > The extension-options-overlay ID is nice, but Devlin has a point, perhaps you > > can do away with the .extension-options-overlay-title class and use > > '#extensions-options-overlay h1" here. If you want. > > if there's only ever going to be 1, use an #id. if there's going to be more, > use a .class. Done. https://codereview.chromium.org/475543003/diff/60001/chrome/renderer/resource... File chrome/renderer/resources/extensions/extension_options.js (right): https://codereview.chromium.org/475543003/diff/60001/chrome/renderer/resource... chrome/renderer/resources/extensions/extension_options.js:17: 'maxwidth': window.innerWidth, On 2014/08/14 19:01:27, Dan Beam wrote: > doesn't this only measure the size of the web contents rather than the whole > screen? what coordinate system is AUTO_SIZE_ATTRIBUTES in? By web contents do you mean the embedded web contents in the webview? I think this should get the whole screen, since this custom element script is injected into the embedder page (chrome://extensions-frame).
lgtm once these are fixed. :) https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_list.js:196: chrome.commandLinePrivate.hasSwitch( On 2014/08/14 22:34:13, ericzeng wrote: > On 2014/08/14 19:01:26, Dan Beam wrote: > > ^ why are we doing this in JS instead of in C++ and sending the result to JS? > > Is there a convenient way to send the flags check down to C++? I haven't worked > in WebUI before so I'm not sure what the best way to do this is. Since this is in ExtensionsList, we could just set a new bool on the results (which goes to ExtensionsList.data_). For an example, look at what we do with e.g. "incognitoAvailable" here and in extension_settings_handler.cc. https://codereview.chromium.org/475543003/diff/100001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_options_overlay.html (right): https://codereview.chromium.org/475543003/diff/100001/chrome/browser/resource... chrome/browser/resources/extensions/extension_options_overlay.html:9: <div class="extension-options-overlay-guest"></div> Will we have more than one of these? https://codereview.chromium.org/475543003/diff/100001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_options_overlay.js (right): https://codereview.chromium.org/475543003/diff/100001/chrome/browser/resource... chrome/browser/resources/extensions/extension_options_overlay.js:47: remove blank line
Comments-on-comments. https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_list.js:196: chrome.commandLinePrivate.hasSwitch( On 2014/08/15 15:19:50, Devlin wrote: > On 2014/08/14 22:34:13, ericzeng wrote: > > On 2014/08/14 19:01:26, Dan Beam wrote: > > > ^ why are we doing this in JS instead of in C++ and sending the result to > JS? > > > > Is there a convenient way to send the flags check down to C++? I haven't > worked > > in WebUI before so I'm not sure what the best way to do this is. > > Since this is in ExtensionsList, we could just set a new bool on the results > (which goes to ExtensionsList.data_). For an example, look at what we do with > e.g. "incognitoAvailable" here and in extension_settings_handler.cc. I kind of like this using an extension API :) less WebUI. But no big deal. One disadvantage is that the switch might be missed if somebody is grepping through removing it (though unlikely). https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... File chrome/browser/resources/extensions/extension_options_overlay.js (right): https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_options_overlay.js:63: extensionoptions.onsizechanged = function(evt) { On 2014/08/14 22:34:13, ericzeng wrote: > On 2014/08/14 19:01:27, Dan Beam wrote: > > On 2014/08/14 15:33:20, kalman wrote: > > > TODO(ericzeng): Resize in a non-jarring way. > > > > > > I don't think this is possible without some non-trivial creative CSS or JS, > so > > > not worth it for now (the complication being that you need to resize to an > > > arbitrary size, not a pre-determined one). > > > > just keep the current JS and just add: > > > > transition: height 200ms, width 200ms; > > > > or something in the CSS > > Unfortunately that doesn't really help with the pop-in. It's an unfortunate > limitation with <extensionoptions> and the underlying browser plugin. The > embedded webview needs to be attached to the DOM and be visible for it to begin > autosizing. I think you could get some kind of decent approximation which solves the jarring-ness - use webanimations to fade out the guestview, resize, then fade it back in again - for example. If it's a really fast animation I reckon it'd look neat (like 80s special effects neat). Maybe later though. https://codereview.chromium.org/475543003/diff/100001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_options_overlay.html (right): https://codereview.chromium.org/475543003/diff/100001/chrome/browser/resource... chrome/browser/resources/extensions/extension_options_overlay.html:9: <div class="extension-options-overlay-guest"></div> On 2014/08/15 15:19:51, Devlin wrote: > Will we have more than one of these? I don't think it hurts to have this a class, and the name implies generic-ness.
https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/475543003/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_list.js:196: chrome.commandLinePrivate.hasSwitch( On 2014/08/15 15:51:06, kalman wrote: > On 2014/08/15 15:19:50, Devlin wrote: > > On 2014/08/14 22:34:13, ericzeng wrote: > > > On 2014/08/14 19:01:26, Dan Beam wrote: > > > > ^ why are we doing this in JS instead of in C++ and sending the result to > > JS? > > > > > > Is there a convenient way to send the flags check down to C++? I haven't > > worked > > > in WebUI before so I'm not sure what the best way to do this is. > > > > Since this is in ExtensionsList, we could just set a new bool on the results > > (which goes to ExtensionsList.data_). For an example, look at what we do with > > e.g. "incognitoAvailable" here and in extension_settings_handler.cc. > > I kind of like this using an extension API :) less WebUI. But no big deal. One > disadvantage is that the switch might be missed if somebody is grepping through > removing it (though unlikely). Done. https://codereview.chromium.org/475543003/diff/100001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_options_overlay.js (right): https://codereview.chromium.org/475543003/diff/100001/chrome/browser/resource... chrome/browser/resources/extensions/extension_options_overlay.js:47: On 2014/08/15 15:19:51, Devlin wrote: > remove blank line Done.
ping dbeam
(still lg) https://codereview.chromium.org/475543003/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right): https://codereview.chromium.org/475543003/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_settings_handler.cc:885: command_line->HasSwitch(switches::kEnableEmbeddedExtensionOptions)); nit: prefer feature switch use.
lgtm w/nits https://codereview.chromium.org/475543003/diff/120001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_options_overlay.js (right): https://codereview.chromium.org/475543003/diff/120001/chrome/browser/resource... chrome/browser/resources/extensions/extension_options_overlay.js:18: nit: remove \n https://codereview.chromium.org/475543003/diff/120001/chrome/browser/resource... chrome/browser/resources/extensions/extension_options_overlay.js:21: * @type {function} Function https://codereview.chromium.org/475543003/diff/120001/chrome/browser/resource... chrome/browser/resources/extensions/extension_options_overlay.js:39: this.showOverlay_ = showOverlay; nit: sprinkle some \n in here https://codereview.chromium.org/475543003/diff/120001/chrome/browser/resource... chrome/browser/resources/extensions/extension_options_overlay.js:46: this.overlayDiv_ = $('extension-options-overlay'); why not just $('extension-options-overlay') whenever you need a reference?
https://codereview.chromium.org/475543003/diff/120001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_options_overlay.js (right): https://codereview.chromium.org/475543003/diff/120001/chrome/browser/resource... chrome/browser/resources/extensions/extension_options_overlay.js:18: On 2014/08/18 17:31:32, Dan Beam wrote: > nit: remove \n Done. https://codereview.chromium.org/475543003/diff/120001/chrome/browser/resource... chrome/browser/resources/extensions/extension_options_overlay.js:21: * @type {function} On 2014/08/18 17:31:32, Dan Beam wrote: > Function Done. https://codereview.chromium.org/475543003/diff/120001/chrome/browser/resource... chrome/browser/resources/extensions/extension_options_overlay.js:39: this.showOverlay_ = showOverlay; On 2014/08/18 17:31:32, Dan Beam wrote: > nit: sprinkle some \n in here Done. https://codereview.chromium.org/475543003/diff/120001/chrome/browser/resource... chrome/browser/resources/extensions/extension_options_overlay.js:46: this.overlayDiv_ = $('extension-options-overlay'); On 2014/08/18 17:31:32, Dan Beam wrote: > why not just $('extension-options-overlay') whenever you need a reference? I used the extension error overlay as a reference while writing this file. It used the overlay div field, but looks like I don't really need to have it. https://codereview.chromium.org/475543003/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right): https://codereview.chromium.org/475543003/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_settings_handler.cc:885: command_line->HasSwitch(switches::kEnableEmbeddedExtensionOptions)); On 2014/08/18 17:28:04, Devlin wrote: > nit: prefer feature switch use. Done.
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/475543003/160001
Message was sent while issue was closed.
Committed patchset #9 (160001) as 290415
Message was sent while issue was closed.
https://codereview.chromium.org/475543003/diff/160001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/475543003/diff/160001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:198: setExtensionAndShowOverlay([extension.id], extension.name); setExtensionAndShowOverlay() gets {string} as first parameter. Please, fix the call here or the JSDoc in extension_options_overlay.js
Message was sent while issue was closed.
https://codereview.chromium.org/475543003/diff/160001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_options_overlay.js (right): https://codereview.chromium.org/475543003/diff/160001/chrome/browser/resource... chrome/browser/resources/extensions/extension_options_overlay.js:63: var extensionoptions = new ExtensionOptions(); Constructor "ExtensionOptions()" is never defined in our codebase: https://code.google.com/p/chromium/codesearch#search/&q=%22ExtensionOptions(%... Is this code already dead? rdevlin.cronin@, do you know the status of this code?
Message was sent while issue was closed.
On 2014/09/30 01:31:28, Vitaly Pavlenko wrote: > https://codereview.chromium.org/475543003/diff/160001/chrome/browser/resource... > File chrome/browser/resources/extensions/extension_options_overlay.js (right): > > https://codereview.chromium.org/475543003/diff/160001/chrome/browser/resource... > chrome/browser/resources/extensions/extension_options_overlay.js:63: var > extensionoptions = new ExtensionOptions(); > Constructor "ExtensionOptions()" is never defined in our codebase: > > https://code.google.com/p/chromium/codesearch#search/&q=%22ExtensionOptions(%... > > Is this code already dead? rdevlin.cronin@, do you know the status of this code? ExtensionOptions is a custom element register here. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re... fsamuel@ is the best person to ask about this - he owns the browser tag custom elements.
Message was sent while issue was closed.
On 2014/09/30 01:31:28, Vitaly Pavlenko wrote: > https://codereview.chromium.org/475543003/diff/160001/chrome/browser/resource... > File chrome/browser/resources/extensions/extension_options_overlay.js (right): > > https://codereview.chromium.org/475543003/diff/160001/chrome/browser/resource... > chrome/browser/resources/extensions/extension_options_overlay.js:63: var > extensionoptions = new ExtensionOptions(); > Constructor "ExtensionOptions()" is never defined in our codebase: > > https://code.google.com/p/chromium/codesearch#search/&q=%22ExtensionOptions(%... > > Is this code already dead? rdevlin.cronin@, do you know the status of this code? ExtensionOptions is a custom element register here. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re... fsamuel@ is the best person to ask about this - he owns the browser tag custom elements. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
