Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
378783002:
Initial implementation of the <extensionoptions> GuestView tag (Closed)
|
Created:
6 years, 5 months ago by ericzeng Modified:
6 years, 5 months ago Reviewers:
Fady Samuel, Devlin, jochen (gone - plz use gerrit), lazyboy, not at google - send to devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionExtensions can now use the <extensionoptions> page to embed another extension's options page with basic iframe-like behavior and features.
In this CL:
- Create a new guest view (ExtensionOptionsGuest) for safely embedding extension options
- Modify existing guest view-related classes to support the new view
- Add API tests to verify that the guest view can be created and that it has access to privileged APIs.
BUG=386838
TEST=browser_tests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285518
Patch Set 1 #
Total comments: 21
Patch Set 2 : Fix minor issues from comments #Patch Set 3 : Add constants file #
Total comments: 24
Patch Set 4 : Fix custom element registration issues, add tests #Patch Set 5 : Fix problems with attaching guest view, add extension function dispatcher #
Total comments: 19
Patch Set 6 : Address fsamuel's comments #Patch Set 7 : Rebase onto latest changes #Patch Set 8 : Rebase again #Patch Set 9 : Handle changing attribute values, prevent cross-extension options embedding #
Total comments: 30
Patch Set 10 : Address Devlin's comments, modify tests #
Total comments: 6
Patch Set 11 : Fix tests, Extension APIs in guest views, and permissions #Patch Set 12 : Remove stray console.log() lines #Patch Set 13 : Remove WIP test #
Total comments: 38
Patch Set 14 : Address Devlin's comments, fix style #
Total comments: 16
Patch Set 15 : ^ #
Total comments: 17
Patch Set 16 : Address kalman's comments, add new test for privileged extension apis #
Total comments: 6
Patch Set 17 : Address lazyboy's comments #
Total comments: 8
Patch Set 18 : Comments #
Total comments: 7
Patch Set 19 : Improve test structure #
Total comments: 4
Patch Set 20 : Restructure tests again #
Total comments: 26
Patch Set 21 : #
Total comments: 2
Patch Set 22 : Rebase, update ExtensionOptionsGuest construction #
Total comments: 2
Patch Set 23 : Remove stray FeatureSwitch includes #Patch Set 24 : Rebase to fix patch failure #Patch Set 25 : Skip the extension options permission in PermissionsTest because it is trivial #Patch Set 26 : Rebase #Patch Set 27 : Rebase #Messages
Total messages: 81 (0 generated)
Hi everyone, this CL is the first step towards implementing the <extensionoptions> tag, PTAL. Fady - please review the changes to the guest_view base files, as well as extension_options_guest, extension_options.js, and extension_options_deny.js. Ben and Devlin - please review the other code for permissions and the renderer
https://codereview.chromium.org/378783002/diff/1/chrome/browser/guest_view/ex... File chrome/browser/guest_view/extension_options/extension_options_guest.cc (right): https://codereview.chromium.org/378783002/diff/1/chrome/browser/guest_view/ex... chrome/browser/guest_view/extension_options/extension_options_guest.cc:38: create_params.GetString("extensionId", &extension_id); Make extensionId a constant. https://codereview.chromium.org/378783002/diff/1/chrome/browser/guest_view/ex... chrome/browser/guest_view/extension_options/extension_options_guest.cc:50: content::SiteInstance::CreateForURL(browser_context, options_page); Does this end up in the same process as the app? https://codereview.chromium.org/378783002/diff/1/chrome/renderer/resources/ex... File chrome/renderer/resources/extensions/extension_options.js (right): https://codereview.chromium.org/378783002/diff/1/chrome/renderer/resources/ex... chrome/renderer/resources/extensions/extension_options.js:50: self.attachWindowAndSetUpEvents(self.instanceId, self.src); This code doesn't do anything...
nice work! https://codereview.chromium.org/378783002/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/378783002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/_permission_features.json:414: "extension_types": ["extension"] I think for testing we can have a permission for this, but eventually we won't. it'll just be made automatically available to webui (and maybe some whitelsited extensions like the developer tool?). please restrict to "trunk" channel not "dev" though. https://codereview.chromium.org/378783002/diff/1/chrome/renderer/extensions/c... File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc (right): https://codereview.chromium.org/378783002/diff/1/chrome/renderer/extensions/c... chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:254: source_map->RegisterSource("denyExtensionOptions", I don't think we need this "deny" logic. As I understand it that's to provide a friendly surface for webview/appview to print a message like "you need to request the web/appview permission to use the tag" but we don't actually need that in the long term, so not much point adding code for it now. https://codereview.chromium.org/378783002/diff/1/chrome/renderer/extensions/c... chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:315: } you might as well add some observable behaviour for all of this; how about checking the command line flag + permission here, then Requiring that module you added? same as appview basically, without the "deny" part. that should also let you write some kind of test. Fady, what's a reasonable minimal browser test for this stuff? maybe add a test which - given extension A and B - loads extension A with an <extensionoptions> tag for extension B in its options page - checks that extension B has its options page open by sending a message to its background page and using chrome.extension.getViews() devlin or I can help you with that today. a quick search through the code for a simple test that might be a good starting point: test runner: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... extension with options page: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/e... https://codereview.chromium.org/378783002/diff/1/extensions/renderer/dispatch... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/378783002/diff/1/extensions/renderer/dispatch... extensions/renderer/dispatcher.cc:806: blink::WebCustomElement::addEmbedderCustomElementName("browserplugin"); fady why does "browserplugin" need to be in here? (particularly since this file is in src/extensions) https://codereview.chromium.org/378783002/diff/1/extensions/renderer/dispatch... extensions/renderer/dispatcher.cc:808: "extensionoptionsplugin"); can we put this list in alphabetic order?
https://codereview.chromium.org/378783002/diff/1/chrome/renderer/extensions/c... File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc (right): https://codereview.chromium.org/378783002/diff/1/chrome/renderer/extensions/c... chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:315: } On 2014/07/08 15:43:39, kalman wrote: > you might as well add some observable behaviour for all of this; how about > checking the command line flag + permission here, then Requiring that module you > added? same as appview basically, without the "deny" part. > > that should also let you write some kind of test. > > Fady, what's a reasonable minimal browser test for this stuff? maybe add a test > which > - given extension A and B > - loads extension A with an <extensionoptions> tag for extension B in its > options page > - checks that extension B has its options page open by sending a message to its > background page and using chrome.extension.getViews() > > devlin or I can help you with that today. a quick search through the code for a > simple test that might be a good starting point: > > test runner: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > extension with options page: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/e... I will be sending you the following for review shortly :-) It includes a browser test. https://codereview.chromium.org/354483004/ https://codereview.chromium.org/378783002/diff/1/extensions/renderer/dispatch... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/378783002/diff/1/extensions/renderer/dispatch... extensions/renderer/dispatcher.cc:806: blink::WebCustomElement::addEmbedderCustomElementName("browserplugin"); On 2014/07/08 15:43:39, kalman wrote: > fady why does "browserplugin" need to be in here? (particularly since this file > is in src/extensions) Good catch. That can be removed.
https://codereview.chromium.org/378783002/diff/1/chrome/browser/guest_view/ex... File chrome/browser/guest_view/extension_options/extension_options_guest.cc (right): https://codereview.chromium.org/378783002/diff/1/chrome/browser/guest_view/ex... chrome/browser/guest_view/extension_options/extension_options_guest.cc:38: create_params.GetString("extensionId", &extension_id); On 2014/07/08 14:23:40, Fady Samuel wrote: > Make extensionId a constant. A constant just in this class or in some constants file for all of extensions? https://codereview.chromium.org/378783002/diff/1/chrome/browser/guest_view/ex... chrome/browser/guest_view/extension_options/extension_options_guest.cc:50: content::SiteInstance::CreateForURL(browser_context, options_page); On 2014/07/08 14:23:40, Fady Samuel wrote: > Does this end up in the same process as the app? I'm not exactly certain how this works and what process it is in. I was reading web_view_guest and it does something similar where it creates a SiteInstance with the embedder RenderProcessHost's BrowserContext and uses that to create the WebContents. Is this the right approach? https://codereview.chromium.org/378783002/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/378783002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/_permission_features.json:414: "extension_types": ["extension"] On 2014/07/08 15:43:39, kalman wrote: > I think for testing we can have a permission for this, but eventually we won't. > it'll just be made automatically available to webui (and maybe some whitelsited > extensions like the developer tool?). > > please restrict to "trunk" channel not "dev" though. Right, this is just a temporary measure for before I start the webui part. https://codereview.chromium.org/378783002/diff/1/chrome/renderer/extensions/c... File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc (right): https://codereview.chromium.org/378783002/diff/1/chrome/renderer/extensions/c... chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:254: source_map->RegisterSource("denyExtensionOptions", On 2014/07/08 15:43:39, kalman wrote: > I don't think we need this "deny" logic. As I understand it that's to provide a > friendly surface for webview/appview to print a message like "you need to > request the web/appview permission to use the tag" but we don't actually need > that in the long term, so not much point adding code for it now. Done. https://codereview.chromium.org/378783002/diff/1/chrome/renderer/resources/ex... File chrome/renderer/resources/extensions/extension_options.js (right): https://codereview.chromium.org/378783002/diff/1/chrome/renderer/resources/ex... chrome/renderer/resources/extensions/extension_options.js:50: self.attachWindowAndSetUpEvents(self.instanceId, self.src); On 2014/07/08 14:23:40, Fady Samuel wrote: > This code doesn't do anything... Oops, done https://codereview.chromium.org/378783002/diff/1/extensions/renderer/dispatch... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/378783002/diff/1/extensions/renderer/dispatch... extensions/renderer/dispatcher.cc:806: blink::WebCustomElement::addEmbedderCustomElementName("browserplugin"); On 2014/07/08 15:46:48, Fady Samuel wrote: > On 2014/07/08 15:43:39, kalman wrote: > > fady why does "browserplugin" need to be in here? (particularly since this > file > > is in src/extensions) > > Good catch. That can be removed. Done. https://codereview.chromium.org/378783002/diff/1/extensions/renderer/dispatch... extensions/renderer/dispatcher.cc:808: "extensionoptionsplugin"); On 2014/07/08 15:43:39, kalman wrote: > can we put this list in alphabetic order? Done.
https://codereview.chromium.org/378783002/diff/1/chrome/browser/guest_view/ex... File chrome/browser/guest_view/extension_options/extension_options_guest.cc (right): https://codereview.chromium.org/378783002/diff/1/chrome/browser/guest_view/ex... chrome/browser/guest_view/extension_options/extension_options_guest.cc:38: create_params.GetString("extensionId", &extension_id); On 2014/07/08 18:10:48, Eric Zeng wrote: > On 2014/07/08 14:23:40, Fady Samuel wrote: > > Make extensionId a constant. > A constant just in this class or in some constants file for all of extensions? A constants file. See web_view_constants as an example. https://codereview.chromium.org/378783002/diff/1/chrome/browser/guest_view/ex... chrome/browser/guest_view/extension_options/extension_options_guest.cc:50: content::SiteInstance::CreateForURL(browser_context, options_page); On 2014/07/08 18:10:48, Eric Zeng wrote: > On 2014/07/08 14:23:40, Fady Samuel wrote: > > Does this end up in the same process as the app? > I'm not exactly certain how this works and what process it is in. I was reading > web_view_guest and it does something similar where it creates a SiteInstance > with the embedder RenderProcessHost's BrowserContext and uses that to create the > WebContents. Is this the right approach? Use the task manager built into Chrome. Does the options page inside this element end up in the same process as the app?
https://codereview.chromium.org/378783002/diff/1/chrome/browser/guest_view/ex... File chrome/browser/guest_view/extension_options/extension_options_guest.cc (right): https://codereview.chromium.org/378783002/diff/1/chrome/browser/guest_view/ex... chrome/browser/guest_view/extension_options/extension_options_guest.cc:38: create_params.GetString("extensionId", &extension_id); On 2014/07/08 18:13:30, Fady Samuel wrote: > On 2014/07/08 18:10:48, Eric Zeng wrote: > > On 2014/07/08 14:23:40, Fady Samuel wrote: > > > Make extensionId a constant. > > A constant just in this class or in some constants file for all of extensions? > > A constants file. See web_view_constants as an example. Done.
i'm not sure about the JS code here, but let's work on that test and see what happens. https://codereview.chromium.org/378783002/diff/1/chrome/renderer/resources/ex... File chrome/renderer/resources/extensions/extension_options.js (right): https://codereview.chromium.org/378783002/diff/1/chrome/renderer/resources/ex... chrome/renderer/resources/extensions/extension_options.js:51: }); FYI a nice pattern (perhaps appview doesn't do this) to be able to use |this| rather than the |self| hack is to do a .bind(this) on the anonymous function. function(instanceId) { ... }.bind(this) https://codereview.chromium.org/378783002/diff/40001/chrome/common/extensions... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/378783002/diff/40001/chrome/common/extensions... chrome/common/extensions/api/_permission_features.json:412: "embeddedExtensionOptions": { "extensionoptions" would actually be a more consistent name for this "permission". https://codereview.chromium.org/378783002/diff/40001/chrome/renderer/chrome_c... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/378783002/diff/40001/chrome/renderer/chrome_c... chrome/renderer/chrome_content_renderer_client.cc:528: extensions::APIPermission::kEmbeddedExtensionOptions, nit: alphabetic https://codereview.chromium.org/378783002/diff/40001/chrome/renderer/resource... File chrome/renderer/resources/extensions/extension_options.js (right): https://codereview.chromium.org/378783002/diff/40001/chrome/renderer/resource... chrome/renderer/resources/extensions/extension_options.js:51: function registerBrowserPluginElement() { fady is there some reuse we can do with appview, perhaps pull something into GuestViewInternal? shame to be repeating ourselves here. https://codereview.chromium.org/378783002/diff/40001/chrome/renderer/resource... chrome/renderer/resources/extensions/extension_options.js:62: var unused = this.nonExistentAttribute; how does this imply loading the plugin automatically? https://codereview.chromium.org/378783002/diff/40001/chrome/renderer/resource... chrome/renderer/resources/extensions/extension_options.js:65: ExtensionOptionsInternal.BrowserPlugin = why do you need to assign this? https://codereview.chromium.org/378783002/diff/40001/chrome/renderer/resource... chrome/renderer/resources/extensions/extension_options.js:91: DocumentNatives.RegisterElement('extensionoptions', {prototype: proto}); why do you need to assign this to window? https://codereview.chromium.org/378783002/diff/40001/chrome/renderer/resource... chrome/renderer/resources/extensions/extension_options.js:94: // behavior. is this standard custom element practice? https://codereview.chromium.org/378783002/diff/40001/extensions/renderer/disp... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/378783002/diff/40001/extensions/renderer/disp... extensions/renderer/dispatcher.cc:807: blink::WebCustomElement::addEmbedderCustomElementName("webview"); fady out of curiosity what are the 'plugin' elements for and why doesn't webview need one?
https://codereview.chromium.org/378783002/diff/40001/chrome/renderer/resource... File chrome/renderer/resources/extensions/extension_options.js (right): https://codereview.chromium.org/378783002/diff/40001/chrome/renderer/resource... chrome/renderer/resources/extensions/extension_options.js:51: function registerBrowserPluginElement() { On 2014/07/08 19:17:25, kalman wrote: > fady is there some reuse we can do with appview, perhaps pull something into > GuestViewInternal? shame to be repeating ourselves here. Perhaps. It's not yet clear how much code reuse there will actually be here. https://codereview.chromium.org/378783002/diff/40001/chrome/renderer/resource... chrome/renderer/resources/extensions/extension_options.js:62: var unused = this.nonExistentAttribute; On 2014/07/08 19:17:25, kalman wrote: > how does this imply loading the plugin automatically? I don't actually know. This was added by Dominic Cooney or some other custom elements engineer. https://codereview.chromium.org/378783002/diff/40001/chrome/renderer/resource... chrome/renderer/resources/extensions/extension_options.js:65: ExtensionOptionsInternal.BrowserPlugin = On 2014/07/08 19:17:25, kalman wrote: > why do you need to assign this? So we can easily create this type of BrowserPlugin. This isn't strictly necessary. As long as is="extensionoptionsplugin" is set, it'll get all the behaviors we want. https://codereview.chromium.org/378783002/diff/40001/chrome/renderer/resource... chrome/renderer/resources/extensions/extension_options.js:91: DocumentNatives.RegisterElement('extensionoptions', {prototype: proto}); On 2014/07/08 19:17:25, kalman wrote: > why do you need to assign this to window? This permits the following: var eo = new ExtensionOptions(); Do we care in this case? https://codereview.chromium.org/378783002/diff/40001/chrome/renderer/resource... chrome/renderer/resources/extensions/extension_options.js:94: // behavior. On 2014/07/08 19:17:25, kalman wrote: > is this standard custom element practice? Yes, we should delete all callbacks. See web_view.js https://codereview.chromium.org/378783002/diff/40001/extensions/renderer/disp... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/378783002/diff/40001/extensions/renderer/disp... extensions/renderer/dispatcher.cc:807: blink::WebCustomElement::addEmbedderCustomElementName("webview"); On 2014/07/08 19:17:26, kalman wrote: > fady out of curiosity what are the 'plugin' elements for and why doesn't webview > need one? Doh! I misspoke earlier. WebView has a browserplugin element. The purpose of this custom element is to listen to attribute changes on the <object> and respond to them appropriately.
https://codereview.chromium.org/378783002/diff/40001/chrome/common/extensions... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/378783002/diff/40001/chrome/common/extensions... chrome/common/extensions/api/_permission_features.json:412: "embeddedExtensionOptions": { On 2014/07/08 19:17:25, kalman wrote: > "extensionoptions" would actually be a more consistent name for this > "permission". Is it? I think that "extensionoptions" is specific to the tag <extensionoptions>, while embeddedExtensionOptions is a more general descriptor for the project. I guess it depends on whether the permission is for allowing the use of the tag or whether it is for allowing extension options to be embedded. https://codereview.chromium.org/378783002/diff/40001/chrome/renderer/chrome_c... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/378783002/diff/40001/chrome/renderer/chrome_c... chrome/renderer/chrome_content_renderer_client.cc:528: extensions::APIPermission::kEmbeddedExtensionOptions, On 2014/07/08 19:17:25, kalman wrote: > nit: alphabetic Done. https://codereview.chromium.org/378783002/diff/40001/extensions/renderer/disp... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/378783002/diff/40001/extensions/renderer/disp... extensions/renderer/dispatcher.cc:807: blink::WebCustomElement::addEmbedderCustomElementName("webview"); On 2014/07/08 19:23:57, Fady Samuel wrote: > On 2014/07/08 19:17:26, kalman wrote: > > fady out of curiosity what are the 'plugin' elements for and why doesn't > webview > > need one? > > Doh! I misspoke earlier. WebView has a browserplugin element. The purpose of > this custom element is to listen to attribute changes on the <object> and > respond to them appropriately. So I should put that line back in? Perhaps we should name it something else so that it is more clear that it is webview-specific.
https://codereview.chromium.org/378783002/diff/40001/chrome/renderer/resource... File chrome/renderer/resources/extensions/extension_options.js (right): https://codereview.chromium.org/378783002/diff/40001/chrome/renderer/resource... chrome/renderer/resources/extensions/extension_options.js:65: ExtensionOptionsInternal.BrowserPlugin = On 2014/07/08 19:23:57, Fady Samuel wrote: > On 2014/07/08 19:17:25, kalman wrote: > > why do you need to assign this? > > So we can easily create this type of BrowserPlugin. This isn't strictly > necessary. As long as is="extensionoptionsplugin" is set, it'll get all the > behaviors we want. I see. yeah that would be better, or we can just save it to a variable within this file, no need to assign it to ExtensionOptionsInternal. https://codereview.chromium.org/378783002/diff/40001/chrome/renderer/resource... chrome/renderer/resources/extensions/extension_options.js:77: } semicolon https://codereview.chromium.org/378783002/diff/40001/chrome/renderer/resource... chrome/renderer/resources/extensions/extension_options.js:80: for (var i = 0; methods[i]; ++i) { where is |methods| defined? https://codereview.chromium.org/378783002/diff/40001/chrome/renderer/resource... chrome/renderer/resources/extensions/extension_options.js:83: var internal = privates(this).internal; I'd rather we be explicit rather than using "internal" everywhere, and besides which, keep "privates" calls to within the object to which it's private. so, add a method like ExtensionOptionsInternal.FromElement or something, which does the privates(this).internal functionality. https://codereview.chromium.org/378783002/diff/40001/chrome/renderer/resource... chrome/renderer/resources/extensions/extension_options.js:91: DocumentNatives.RegisterElement('extensionoptions', {prototype: proto}); On 2014/07/08 19:23:57, Fady Samuel wrote: > On 2014/07/08 19:17:25, kalman wrote: > > why do you need to assign this to window? > > This permits the following: > > var eo = new ExtensionOptions(); Do we care in this case? ah right. can you do the same thing with document.createElement('extensionoptions')? I forget the subtleties of custom elements. I remember there being a case where that doesn't work, and me being surprised that it didn't.
https://codereview.chromium.org/378783002/diff/40001/chrome/renderer/resource... File chrome/renderer/resources/extensions/extension_options.js (right): https://codereview.chromium.org/378783002/diff/40001/chrome/renderer/resource... chrome/renderer/resources/extensions/extension_options.js:80: for (var i = 0; methods[i]; ++i) { On 2014/07/08 19:41:16, kalman wrote: > where is |methods| defined? This code doesn't apply yet. Delete this entire block. https://codereview.chromium.org/378783002/diff/40001/chrome/renderer/resource... chrome/renderer/resources/extensions/extension_options.js:91: DocumentNatives.RegisterElement('extensionoptions', {prototype: proto}); On 2014/07/08 19:41:16, kalman wrote: > On 2014/07/08 19:23:57, Fady Samuel wrote: > > On 2014/07/08 19:17:25, kalman wrote: > > > why do you need to assign this to window? > > > > This permits the following: > > > > var eo = new ExtensionOptions(); Do we care in this case? > > ah right. can you do the same thing with > document.createElement('extensionoptions')? I forget the subtleties of custom > elements. I remember there being a case where that doesn't work, and me being > surprised that it didn't. document.createElement also works.
I'm excited that you have this working! I sent out some initial thoughts. Let me know if I can be any more help. I'll be on vacation next week, though. I believe at some point Ben told me he'd like to be able to inject script and/or CSS into extensions options pages. See script_executor in webview, and see the file extension_code_function for more information on how that works. https://codereview.chromium.org/378783002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/guest_view/guest_view_internal_api.cc (right): https://codereview.chromium.org/378783002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/guest_view/guest_view_internal_api.cc:38: APIPermission::kEmbeddedExtensionOptions)) { This is landing in a few minutes, please update: https://codereview.chromium.org/378873002/ Permissions should happen on a per-GuestView basis. https://codereview.chromium.org/378783002/diff/80001/chrome/browser/guest_vie... File chrome/browser/guest_view/extension_options/extension_options_guest.cc (right): https://codereview.chromium.org/378783002/diff/80001/chrome/browser/guest_vie... chrome/browser/guest_view/extension_options/extension_options_guest.cc:55: DCHECK(!extension_id.empty()); if (extension_id.empty())) { callback.Run(NULL); return; } The above will cause GuestViewInternal.createGuest to fail. https://codereview.chromium.org/378783002/diff/80001/chrome/browser/guest_vie... chrome/browser/guest_view/extension_options/extension_options_guest.cc:56: GURL extensionUrl = extension_url https://codereview.chromium.org/378783002/diff/80001/chrome/browser/guest_vie... chrome/browser/guest_view/extension_options/extension_options_guest.cc:57: extensions::Extension::GetBaseURLFromExtensionId(extension_id); Check that this URL is valid. I would be surprised if it fails but that means that the embedder is trying to do something funky. Maybe we should kill the embedder? I'm not sure. if (!extension_url.is_valid()) { callback.Run(NULL); return; } https://codereview.chromium.org/378783002/diff/80001/chrome/browser/guest_vie... chrome/browser/guest_view/extension_options/extension_options_guest.cc:65: options_page_ = extensions::ManifestURL::GetOptionsPage(extension); We should probably fail if this extension doesn't have an options page, right? https://codereview.chromium.org/378783002/diff/80001/chrome/browser/guest_vie... chrome/browser/guest_view/extension_options/extension_options_guest.cc:67: // Create a Webcontents using the extension URL The options page should live in the same process as the extension. https://codereview.chromium.org/378783002/diff/80001/chrome/browser/guest_vie... File chrome/browser/guest_view/guest_view_base.cc (right): https://codereview.chromium.org/378783002/diff/80001/chrome/browser/guest_vie... chrome/browser/guest_view/guest_view_base.cc:140: } else if (view_type == ExtensionOptionsGuest::Type) { Add a TODO at the top: // TODO(fsamuel): All these string comparisons are a little inefficient. Maybe we want a registry of GuestView types in the future. https://codereview.chromium.org/378783002/diff/80001/chrome/common/extensions... File chrome/common/extensions/api/guest_view_internal.json (right): https://codereview.chromium.org/378783002/diff/80001/chrome/common/extensions... chrome/common/extensions/api/guest_view_internal.json:30: "extensionId": { Revert. This change is no longer necessary. createGuest's createParams now accepts anything. https://codereview.chromium.org/378783002/diff/80001/chrome/renderer/resource... File chrome/renderer/resources/extensions/extension_options.js (left): https://codereview.chromium.org/378783002/diff/80001/chrome/renderer/resource... chrome/renderer/resources/extensions/extension_options.js:74: delete proto.attributeChangedCallback; You want to delete these callbacks too. https://codereview.chromium.org/378783002/diff/80001/chrome/renderer/resource... chrome/renderer/resources/extensions/extension_options.js:96: delete proto.attributeChangedCallback; You want to delete these callbacks too.
https://codereview.chromium.org/378783002/diff/80001/chrome/browser/guest_vie... File chrome/browser/guest_view/extension_options/extension_options_guest.cc (right): https://codereview.chromium.org/378783002/diff/80001/chrome/browser/guest_vie... chrome/browser/guest_view/extension_options/extension_options_guest.cc:55: DCHECK(!extension_id.empty()); On 2014/07/11 21:08:05, Fady Samuel wrote: > if (extension_id.empty())) { > callback.Run(NULL); > return; > } > > The above will cause GuestViewInternal.createGuest to fail. Done. https://codereview.chromium.org/378783002/diff/80001/chrome/browser/guest_vie... chrome/browser/guest_view/extension_options/extension_options_guest.cc:56: GURL extensionUrl = On 2014/07/11 21:08:05, Fady Samuel wrote: > extension_url Done. https://codereview.chromium.org/378783002/diff/80001/chrome/browser/guest_vie... chrome/browser/guest_view/extension_options/extension_options_guest.cc:57: extensions::Extension::GetBaseURLFromExtensionId(extension_id); On 2014/07/11 21:08:05, Fady Samuel wrote: > Check that this URL is valid. I would be surprised if it fails but that means > that the embedder is trying to do something funky. Maybe we should kill the > embedder? I'm not sure. > > if (!extension_url.is_valid()) { > callback.Run(NULL); > return; > } Done. https://codereview.chromium.org/378783002/diff/80001/chrome/browser/guest_vie... chrome/browser/guest_view/extension_options/extension_options_guest.cc:65: options_page_ = extensions::ManifestURL::GetOptionsPage(extension); On 2014/07/11 21:08:05, Fady Samuel (OOO July 14 - 18) wrote: > We should probably fail if this extension doesn't have an options page, right? Done. https://codereview.chromium.org/378783002/diff/80001/chrome/browser/guest_vie... File chrome/browser/guest_view/guest_view_base.cc (right): https://codereview.chromium.org/378783002/diff/80001/chrome/browser/guest_vie... chrome/browser/guest_view/guest_view_base.cc:140: } else if (view_type == ExtensionOptionsGuest::Type) { On 2014/07/11 21:08:05, Fady Samuel (OOO July 14 - 18) wrote: > Add a TODO at the top: > > // TODO(fsamuel): All these string comparisons are a little inefficient. Maybe > we want a registry of GuestView types in the future. Done. https://codereview.chromium.org/378783002/diff/80001/chrome/renderer/resource... File chrome/renderer/resources/extensions/extension_options.js (left): https://codereview.chromium.org/378783002/diff/80001/chrome/renderer/resource... chrome/renderer/resources/extensions/extension_options.js:74: delete proto.attributeChangedCallback; On 2014/07/11 21:08:05, Fady Samuel (OOO July 14 - 18) wrote: > You want to delete these callbacks too. Done. https://codereview.chromium.org/378783002/diff/80001/chrome/renderer/resource... chrome/renderer/resources/extensions/extension_options.js:96: delete proto.attributeChangedCallback; On 2014/07/11 21:08:05, Fady Samuel (OOO July 14 - 18) wrote: > You want to delete these callbacks too. Done.
https://codereview.chromium.org/378783002/diff/80001/chrome/common/extensions... File chrome/common/extensions/api/guest_view_internal.json (right): https://codereview.chromium.org/378783002/diff/80001/chrome/common/extensions... chrome/common/extensions/api/guest_view_internal.json:30: "extensionId": { On 2014/07/11 21:08:05, Fady Samuel (OOO July 14 - 18) wrote: > Revert. This change is no longer necessary. createGuest's createParams now > accepts anything. Done.
https://codereview.chromium.org/378783002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/guest_view/guest_view_internal_api.cc (right): https://codereview.chromium.org/378783002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/guest_view/guest_view_internal_api.cc:38: APIPermission::kEmbeddedExtensionOptions)) { On 2014/07/11 21:08:05, Fady Samuel (OOO July 14 - 18) wrote: > This is landing in a few minutes, please update: > https://codereview.chromium.org/378873002/ > > Permissions should happen on a per-GuestView basis. Done.
https://codereview.chromium.org/378783002/diff/160001/chrome/renderer/extensi... File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc (right): https://codereview.chromium.org/378783002/diff/160001/chrome/renderer/extensi... chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:319: if (CommandLine::ForCurrentProcess()->HasSwitch( nit: This should probably use FeatureSwitch (otherwise ScopedOverrides only work in one place). https://codereview.chromium.org/378783002/diff/160001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/extension_options.js (right): https://codereview.chromium.org/378783002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/extensions/extension_options.js:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. no (c) https://codereview.chromium.org/378783002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/extensions/extension_options.js:48: }); I find it cleaner to do .bind(this) and use |this| rather than do the self = this trick. https://codereview.chromium.org/378783002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/extensions/extension_options.js:53: if (name == 'extension') { hmm... how about: if (name != 'extension) return; ? https://codereview.chromium.org/378783002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/extensions/extension_options.js:59: if (oldValue === newValue) { nit: no brackets around single-line ifs. https://codereview.chromium.org/378783002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/extensions/extension_options.js:65: if (!this.instanceId) { why separate ifs? if (!this.instanceId && this.parseExtensionAttribute()) { } https://codereview.chromium.org/378783002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/extensions/extension_options.js:67: this.browserPluginNode = this.createBrowserPluginNode(); factor so this and line 14-20 share code. https://codereview.chromium.org/378783002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/extensions/extension_options.js:75: // that functionality nit: add a period. https://codereview.chromium.org/378783002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/extensions/extension_options.js:82: // Only allow extensions to embed their own options page nit: add a period. https://codereview.chromium.org/378783002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/extensions/extension_options.js:123: if (!internal) { nit: no brackets. https://codereview.chromium.org/378783002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/extensions/extension_options.js:132: // Delete the callbacks so developers cannot call them and produce unexpected duplicate comment on 108. https://codereview.chromium.org/378783002/diff/160001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/options.html (right): https://codereview.chromium.org/378783002/diff/160001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/options.html:2: * Copyright (c) 2014 The Chromium Authors. All rights reserved. Use of this no (c) https://codereview.chromium.org/378783002/diff/160001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/options.js (right): https://codereview.chromium.org/378783002/diff/160001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/options.js:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. no (c) https://codereview.chromium.org/378783002/diff/160001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/test.html (right): https://codereview.chromium.org/378783002/diff/160001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/test.html:2: * Copyright (c) 2014 The Chromium Authors. All rights reserved. Use of this no (c) (gonna stop saying this - but it goes for any other files with a 2014 year). https://codereview.chromium.org/378783002/diff/160001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/test.js (right): https://codereview.chromium.org/378783002/diff/160001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/test.js:11: chrome.test.assertTrue("hi" == message); prefer single-line quotes in js files.
https://codereview.chromium.org/378783002/diff/160001/chrome/renderer/extensi... File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc (right): https://codereview.chromium.org/378783002/diff/160001/chrome/renderer/extensi... chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:319: if (CommandLine::ForCurrentProcess()->HasSwitch( On 2014/07/15 18:07:35, Devlin wrote: > nit: This should probably use FeatureSwitch (otherwise ScopedOverrides only work > in one place). Done. https://codereview.chromium.org/378783002/diff/160001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/extension_options.js (right): https://codereview.chromium.org/378783002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/extensions/extension_options.js:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/07/15 18:07:35, Devlin wrote: > no (c) Done. https://codereview.chromium.org/378783002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/extensions/extension_options.js:53: if (name == 'extension') { On 2014/07/15 18:07:36, Devlin wrote: > hmm... how about: > if (name != 'extension) > return; > ? Done. https://codereview.chromium.org/378783002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/extensions/extension_options.js:59: if (oldValue === newValue) { On 2014/07/15 18:07:36, Devlin wrote: > nit: no brackets around single-line ifs. Done. https://codereview.chromium.org/378783002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/extensions/extension_options.js:65: if (!this.instanceId) { On 2014/07/15 18:07:36, Devlin wrote: > why separate ifs? > if (!this.instanceId && this.parseExtensionAttribute()) { > } Done. https://codereview.chromium.org/378783002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/extensions/extension_options.js:67: this.browserPluginNode = this.createBrowserPluginNode(); On 2014/07/15 18:07:36, Devlin wrote: > factor so this and line 14-20 share code. Done. https://codereview.chromium.org/378783002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/extensions/extension_options.js:75: // that functionality On 2014/07/15 18:07:36, Devlin wrote: > nit: add a period. Done. https://codereview.chromium.org/378783002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/extensions/extension_options.js:82: // Only allow extensions to embed their own options page On 2014/07/15 18:07:36, Devlin wrote: > nit: add a period. Done. https://codereview.chromium.org/378783002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/extensions/extension_options.js:123: if (!internal) { On 2014/07/15 18:07:36, Devlin wrote: > nit: no brackets. Done. https://codereview.chromium.org/378783002/diff/160001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/options.html (right): https://codereview.chromium.org/378783002/diff/160001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/options.html:2: * Copyright (c) 2014 The Chromium Authors. All rights reserved. Use of this On 2014/07/15 18:07:36, Devlin wrote: > no (c) Done. https://codereview.chromium.org/378783002/diff/160001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/options.js (right): https://codereview.chromium.org/378783002/diff/160001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/options.js:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/07/15 18:07:36, Devlin wrote: > no (c) Done. https://codereview.chromium.org/378783002/diff/160001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/test.html (right): https://codereview.chromium.org/378783002/diff/160001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/test.html:2: * Copyright (c) 2014 The Chromium Authors. All rights reserved. Use of this On 2014/07/15 18:07:36, Devlin wrote: > no (c) (gonna stop saying this - but it goes for any other files with a 2014 > year). Done. https://codereview.chromium.org/378783002/diff/160001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/test.js (right): https://codereview.chromium.org/378783002/diff/160001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/test.js:11: chrome.test.assertTrue("hi" == message); On 2014/07/15 18:07:36, Devlin wrote: > prefer single-line quotes in js files. Done.
https://codereview.chromium.org/378783002/diff/180001/chrome/renderer/extensi... File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc (right): https://codereview.chromium.org/378783002/diff/180001/chrome/renderer/extensi... chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:319: context_type == extensions::Feature::UNBLESSED_EXTENSION_CONTEXT) { why unblessed? It can't hurt, but I also don't really see the point.
I'm noticing some weird things here, notably that the options page only loads if devtools is open. Not sure why that would be... https://codereview.chromium.org/378783002/diff/180001/chrome/browser/guest_vi... File chrome/browser/guest_view/extension_options/extension_options_guest.cc (right): https://codereview.chromium.org/378783002/diff/180001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:99: void ExtensionOptionsGuest::DidInitialize() { you need to create an instance of a ChromeExtensionWebContentsObserver here for messaging to work. extensions::ChromeExtensionWebContentsObserver::CreateForWebContents( guest_web_contents()); and #include "chrome/browser/extensions/chrome_extension_web_contents_observer.h"
https://codereview.chromium.org/378783002/diff/160001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/extension_options.js (right): https://codereview.chromium.org/378783002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/extensions/extension_options.js:48: }); On 2014/07/15 18:07:35, Devlin wrote: > I find it cleaner to do .bind(this) and use |this| rather than do the self = > this trick. Won't change for consistency with webview and appview https://codereview.chromium.org/378783002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/extensions/extension_options.js:132: // Delete the callbacks so developers cannot call them and produce unexpected On 2014/07/15 18:07:36, Devlin wrote: > duplicate comment on 108. Done. https://codereview.chromium.org/378783002/diff/180001/chrome/browser/guest_vi... File chrome/browser/guest_view/extension_options/extension_options_guest.cc (right): https://codereview.chromium.org/378783002/diff/180001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:99: void ExtensionOptionsGuest::DidInitialize() { On 2014/07/16 00:10:57, kalman wrote: > you need to create an instance of a ChromeExtensionWebContentsObserver here for > messaging to work. > > extensions::ChromeExtensionWebContentsObserver::CreateForWebContents( > guest_web_contents()); > > and #include > "chrome/browser/extensions/chrome_extension_web_contents_observer.h" Done. https://codereview.chromium.org/378783002/diff/180001/chrome/renderer/extensi... File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc (right): https://codereview.chromium.org/378783002/diff/180001/chrome/renderer/extensi... chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:319: context_type == extensions::Feature::UNBLESSED_EXTENSION_CONTEXT) { On 2014/07/15 21:08:55, kalman wrote: > why unblessed? It can't hurt, but I also don't really see the point. Done.
Hi everyone, please take a look at my latest patch. I think that most of the basic functionality has been implemented, and there is a basic browser test to verify that the guest view successfully loads the options page. Thanks to everyone for helping me figure out all of the guest view and extensions code over the past few weeks.
https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... File chrome/browser/guest_view/extension_options/extension_options_apitest.cc (right): https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_apitest.cc:16: extensions::switches::kEnableEmbeddedExtensionOptions); Why do we need to modify the command line _and_ use the feature switch? https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... File chrome/browser/guest_view/extension_options/extension_options_guest.cc (right): https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:8: #include "chrome/browser/extensions/extension_service.h" Won't need this or extension_system anymore. https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:41: service->GetExtensionById(embedder_extension_id, false); instead, use ExtensionRegistry::enabled_extensions().GetByID(). https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:58: // Get the extension's base URL . https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:72: // Get the options page URL for later use . https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:76: registry->GetExtensionById(extension_id, nit: ever-so-slightly-faster: registry->enabled_extensions().GetByID(id). https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:84: // Create a Webcontents using the extension URL . https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... File chrome/browser/guest_view/extension_options/extension_options_guest.h (right): https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.h:7: #include "base/macros.h" https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.h:9: #include "extensions/browser/extension_function_dispatcher.h" #include "url/gurl.h" https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.h:10: namespace content { class BrowserContext; } https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.h:20: // GuestViewBase implementation these need periods at the end (like line 34 has). https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.h:43: GURL options_page_; We probably want this to be DISALLOW_COPY_AND_ASSIGN, yes? https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... File chrome/browser/guest_view/guest_view_base.cc (right): https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... chrome/browser/guest_view/guest_view_base.cc:160: if (!base::CommandLine::ForCurrentProcess()->HasSwitch( Feature switch it up. https://codereview.chromium.org/378783002/diff/240001/chrome/renderer/extensi... File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc (right): https://codereview.chromium.org/378783002/diff/240001/chrome/renderer/extensi... chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:38: #include "extensions/common/switches.h" don't need this. https://codereview.chromium.org/378783002/diff/240001/chrome/renderer/extensi... chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:318: if (context_type == extensions::Feature::BLESSED_EXTENSION_CONTEXT) { now that there are no ors, this is perfectly readable as if (a && b && c) { } https://codereview.chromium.org/378783002/diff/240001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/extension_options.js (right): https://codereview.chromium.org/378783002/diff/240001/chrome/renderer/resourc... chrome/renderer/resources/extensions/extension_options.js:14: if (this.parseExtensionAttribute()) { no brackets. https://codereview.chromium.org/378783002/diff/240001/chrome/renderer/resourc... chrome/renderer/resources/extensions/extension_options.js:126: DocumentNatives.RegisterElement('extensionoptions', {prototype: proto}); this indentation is off by one. https://codereview.chromium.org/378783002/diff/240001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/embed_self/options.js (right): https://codereview.chromium.org/378783002/diff/240001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/options.js:9: for (var i = 0; i < windows.length; ++i) { no brackets. https://codereview.chromium.org/378783002/diff/240001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/embed_self/test.js (right): https://codereview.chromium.org/378783002/diff/240001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/test.js:13: for (var i = 0; i < windows.length; ++i) { no brackets.
https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... File chrome/browser/guest_view/extension_options/extension_options_apitest.cc (right): https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_apitest.cc:16: extensions::switches::kEnableEmbeddedExtensionOptions); On 2014/07/16 20:55:06, Devlin wrote: > Why do we need to modify the command line _and_ use the feature switch? The feature switch does not get copied over to the renderer, so the flag doesn't get set in the other process. https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... File chrome/browser/guest_view/extension_options/extension_options_guest.cc (right): https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:8: #include "chrome/browser/extensions/extension_service.h" On 2014/07/16 20:55:06, Devlin wrote: > Won't need this or extension_system anymore. Done. https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:41: service->GetExtensionById(embedder_extension_id, false); On 2014/07/16 20:55:06, Devlin wrote: > instead, use ExtensionRegistry::enabled_extensions().GetByID(). Done. https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:58: // Get the extension's base URL On 2014/07/16 20:55:06, Devlin wrote: > . Done. https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:72: // Get the options page URL for later use On 2014/07/16 20:55:06, Devlin wrote: > . Done. https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:76: registry->GetExtensionById(extension_id, On 2014/07/16 20:55:06, Devlin wrote: > nit: ever-so-slightly-faster: registry->enabled_extensions().GetByID(id). Done. https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:84: // Create a Webcontents using the extension URL On 2014/07/16 20:55:06, Devlin wrote: > . Done. https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... File chrome/browser/guest_view/extension_options/extension_options_guest.h (right): https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.h:7: On 2014/07/16 20:55:06, Devlin wrote: > #include "base/macros.h" Done. https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.h:9: #include "extensions/browser/extension_function_dispatcher.h" On 2014/07/16 20:55:07, Devlin wrote: > #include "url/gurl.h" Done. https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.h:10: On 2014/07/16 20:55:06, Devlin wrote: > namespace content { > class BrowserContext; > } Done. https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.h:20: // GuestViewBase implementation On 2014/07/16 20:55:06, Devlin wrote: > these need periods at the end (like line 34 has). Done. https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.h:43: GURL options_page_; On 2014/07/16 20:55:06, Devlin wrote: > We probably want this to be DISALLOW_COPY_AND_ASSIGN, yes? Done. https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... File chrome/browser/guest_view/guest_view_base.cc (right): https://codereview.chromium.org/378783002/diff/240001/chrome/browser/guest_vi... chrome/browser/guest_view/guest_view_base.cc:160: if (!base::CommandLine::ForCurrentProcess()->HasSwitch( On 2014/07/16 20:55:07, Devlin wrote: > Feature switch it up. Done. https://codereview.chromium.org/378783002/diff/240001/chrome/renderer/extensi... File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc (right): https://codereview.chromium.org/378783002/diff/240001/chrome/renderer/extensi... chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:38: #include "extensions/common/switches.h" On 2014/07/16 20:55:07, Devlin wrote: > don't need this. Done. https://codereview.chromium.org/378783002/diff/240001/chrome/renderer/extensi... chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:318: if (context_type == extensions::Feature::BLESSED_EXTENSION_CONTEXT) { On 2014/07/16 20:55:07, Devlin wrote: > now that there are no ors, this is perfectly readable as > if (a && b && c) { } Done. https://codereview.chromium.org/378783002/diff/240001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/extension_options.js (right): https://codereview.chromium.org/378783002/diff/240001/chrome/renderer/resourc... chrome/renderer/resources/extensions/extension_options.js:14: if (this.parseExtensionAttribute()) { On 2014/07/16 20:55:07, Devlin wrote: > no brackets. Done. https://codereview.chromium.org/378783002/diff/240001/chrome/renderer/resourc... chrome/renderer/resources/extensions/extension_options.js:126: DocumentNatives.RegisterElement('extensionoptions', {prototype: proto}); On 2014/07/16 20:55:07, Devlin wrote: > this indentation is off by one. Done. https://codereview.chromium.org/378783002/diff/240001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/embed_self/options.js (right): https://codereview.chromium.org/378783002/diff/240001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/options.js:9: for (var i = 0; i < windows.length; ++i) { On 2014/07/16 20:55:07, Devlin wrote: > no brackets. Done. https://codereview.chromium.org/378783002/diff/240001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/embed_self/test.js (right): https://codereview.chromium.org/378783002/diff/240001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/test.js:13: for (var i = 0; i < windows.length; ++i) { On 2014/07/16 20:55:07, Devlin wrote: > no brackets. Done.
extensions lgtm https://codereview.chromium.org/378783002/diff/260001/chrome/browser/guest_vi... File chrome/browser/guest_view/extension_options/extension_options_apitest.cc (right): https://codereview.chromium.org/378783002/diff/260001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_apitest.cc:17: // browser process . https://codereview.chromium.org/378783002/diff/260001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_apitest.cc:33: extensions::FeatureSwitch::embedded_extension_options()->IsEnabled()); nit: don't need extensions:: prefix. https://codereview.chromium.org/378783002/diff/260001/chrome/browser/guest_vi... File chrome/browser/guest_view/extension_options/extension_options_guest.cc (right): https://codereview.chromium.org/378783002/diff/260001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:37: extensions::ExtensionRegistry* registry = nit: to me, this looks a bit excessive with the variables. Personally, I find: return extensions::ExtensionRegistry::Get( browser_context())->enabled_extensions().GetByID(embedder_extension_id) ->permissions_data()->HasAPIPermission( extensions::APIPermission::kEmbeddedExtensionOptions); to be readable enough. I'm probably in the minority. But can we shorten it a touch? Maybe only cache the extension? https://codereview.chromium.org/378783002/diff/260001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:52: content::RenderProcessHost* embedder_render_process_host = nit: Can we inline these? content::BrowserContext* browser_context = content::RenderProcessHost::FromID(embedder_render_process_id) ->GetBrowserContext(); https://codereview.chromium.org/378783002/diff/260001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:63: } Maybe a DCHECK(Extension::IdIsValid(extension_id))? https://codereview.chromium.org/378783002/diff/260001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:111: IPC_MESSAGE_HANDLER(ExtensionHostMsg_Request, OnRequest) nit: git cl format will undo this, but I think that we try to indent these macros IPC_BEGIN... IPC_MESSAGE_HANDLER... IPC_MESSAGE_UNHANDLED... IPC_END... At least, that's what I've been told by the IPC folks. https://codereview.chromium.org/378783002/diff/260001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/extension_options.js (right): https://codereview.chromium.org/378783002/diff/260001/chrome/renderer/resourc... chrome/renderer/resources/extensions/extension_options.js:60: if (!this.instanceId && this.parseExtensionAttribute()) { if (IsOneLineIfStatement()) RemoveBrackets(); https://codereview.chromium.org/378783002/diff/260001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/embed_self/manifest.json (right): https://codereview.chromium.org/378783002/diff/260001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/manifest.json:6: "homepage_url": "http://example.com", nit: Is this needed?
https://codereview.chromium.org/378783002/diff/260001/chrome/browser/guest_vi... File chrome/browser/guest_view/extension_options/extension_options_apitest.cc (right): https://codereview.chromium.org/378783002/diff/260001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_apitest.cc:17: // browser process On 2014/07/17 20:05:09, Devlin wrote: > . Done. https://codereview.chromium.org/378783002/diff/260001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_apitest.cc:33: extensions::FeatureSwitch::embedded_extension_options()->IsEnabled()); On 2014/07/17 20:05:09, Devlin wrote: > nit: don't need extensions:: prefix. Done. https://codereview.chromium.org/378783002/diff/260001/chrome/browser/guest_vi... File chrome/browser/guest_view/extension_options/extension_options_guest.cc (right): https://codereview.chromium.org/378783002/diff/260001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:37: extensions::ExtensionRegistry* registry = On 2014/07/17 20:05:09, Devlin wrote: > nit: to me, this looks a bit excessive with the variables. Personally, I find: > return extensions::ExtensionRegistry::Get( > browser_context())->enabled_extensions().GetByID(embedder_extension_id) > ->permissions_data()->HasAPIPermission( > extensions::APIPermission::kEmbeddedExtensionOptions); > > to be readable enough. I'm probably in the minority. But can we shorten it a > touch? Maybe > only cache the extension? I'll create a variable just for the extension, and then return whether the extension has the permission. https://codereview.chromium.org/378783002/diff/260001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:52: content::RenderProcessHost* embedder_render_process_host = On 2014/07/17 20:05:09, Devlin wrote: > nit: Can we inline these? > > content::BrowserContext* browser_context = > content::RenderProcessHost::FromID(embedder_render_process_id) > ->GetBrowserContext(); Done. https://codereview.chromium.org/378783002/diff/260001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:63: } On 2014/07/17 20:05:09, Devlin wrote: > Maybe a DCHECK(Extension::IdIsValid(extension_id))? Done. https://codereview.chromium.org/378783002/diff/260001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:111: IPC_MESSAGE_HANDLER(ExtensionHostMsg_Request, OnRequest) On 2014/07/17 20:05:09, Devlin wrote: > nit: git cl format will undo this, but I think that we try to indent these > macros > IPC_BEGIN... > IPC_MESSAGE_HANDLER... > IPC_MESSAGE_UNHANDLED... > IPC_END... > > At least, that's what I've been told by the IPC folks. Yeah, it looks like I undid the indentation with git cl format last patch. https://codereview.chromium.org/378783002/diff/260001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/extension_options.js (right): https://codereview.chromium.org/378783002/diff/260001/chrome/renderer/resourc... chrome/renderer/resources/extensions/extension_options.js:60: if (!this.instanceId && this.parseExtensionAttribute()) { On 2014/07/17 20:05:09, Devlin wrote: > if (IsOneLineIfStatement()) > RemoveBrackets(); Agh sorry it's a habit https://codereview.chromium.org/378783002/diff/260001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/embed_self/manifest.json (right): https://codereview.chromium.org/378783002/diff/260001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/manifest.json:6: "homepage_url": "http://example.com", On 2014/07/17 20:05:09, Devlin wrote: > nit: Is this needed? Not really
just looking at the tests. https://codereview.chromium.org/378783002/diff/280001/chrome/browser/guest_vi... File chrome/browser/guest_view/extension_options/extension_options_apitest.cc (right): https://codereview.chromium.org/378783002/diff/280001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_apitest.cc:32: ASSERT_TRUE(FeatureSwitch::embedded_extension_options()->IsEnabled()); this check is unnecessary. you just enabled it on line 25. https://codereview.chromium.org/378783002/diff/280001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/embed_self/options.js (right): https://codereview.chromium.org/378783002/diff/280001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/options.js:6: // is has been successfully created, it adds {pass: true} to every extension it has https://codereview.chromium.org/378783002/diff/280001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/options.js:8: var windows = chrome.extension.getViews(); it could be nice to write this like chrome.extension.getViews().forEach(function(view) { view.pass = true; }); https://codereview.chromium.org/378783002/diff/280001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/options.js:11: chrome.runtime.sendMessage('hi'); nice test :) we should also add a test that the options page actually gets, and can call, privileged APIs. it can clearly call unprivileged APIs because sendMessage is unprivileged. a fairly simple way of doing that would be to: 1- make the extension request the "storage" permission 2- make the options page call var pass = chrome.test.callbackPass; chrome.storage.local.set({'test': 42}, pass(function() { chrome.storage.local.get('test', pass(function(storage) { chrome.test.assertEq(42, storage.test); })); }); chrome.test.listenOnce(chrome.storage.onChanged, function(change) { chrome.test.assertEq(42, change.test.newValue); }); https://codereview.chromium.org/378783002/diff/280001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/embed_self/test.js (right): https://codereview.chromium.org/378783002/diff/280001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/test.js:10: chrome.runtime.onMessage.addListener( nice to do this like chrome.test.listenOnce(chrome.runtime.onMessage, function() { ... }); https://codereview.chromium.org/378783002/diff/280001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/test.js:11: function(message, sender, sendResponse) { assert message is 'hi'? assert sender is the options page? may as well get some messaging sanity checks in here as well. https://codereview.chromium.org/378783002/diff/280001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/test.js:12: var windows = chrome.extension.getViews(); likewise with the forEach. and please also assert some things about these windows. for example, if there is no views then it's moot whether they have pass or not :) (mind you it would be crazy for there to be no views, since *this* is a view, but you know...) https://codereview.chromium.org/378783002/diff/280001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/test.js:15: chrome.test.assertTrue('hi' == message); assertEq('hi', message);
https://codereview.chromium.org/378783002/diff/280001/chrome/browser/guest_vi... File chrome/browser/guest_view/extension_options/extension_options_apitest.cc (right): https://codereview.chromium.org/378783002/diff/280001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_apitest.cc:32: ASSERT_TRUE(FeatureSwitch::embedded_extension_options()->IsEnabled()); On 2014/07/18 17:07:26, kalman wrote: > this check is unnecessary. you just enabled it on line 25. Removed. https://codereview.chromium.org/378783002/diff/280001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/embed_self/options.js (right): https://codereview.chromium.org/378783002/diff/280001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/options.js:6: // is has been successfully created, it adds {pass: true} to every extension On 2014/07/18 17:07:26, kalman wrote: > it has Done. https://codereview.chromium.org/378783002/diff/280001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/options.js:8: var windows = chrome.extension.getViews(); On 2014/07/18 17:07:26, kalman wrote: > it could be nice to write this like > > chrome.extension.getViews().forEach(function(view) { > view.pass = true; > }); Done. https://codereview.chromium.org/378783002/diff/280001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/embed_self/test.js (right): https://codereview.chromium.org/378783002/diff/280001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/test.js:10: chrome.runtime.onMessage.addListener( On 2014/07/18 17:07:26, kalman wrote: > nice to do this like > > chrome.test.listenOnce(chrome.runtime.onMessage, function() { > ... > }); Done. https://codereview.chromium.org/378783002/diff/280001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/test.js:11: function(message, sender, sendResponse) { On 2014/07/18 17:07:26, kalman wrote: > assert message is 'hi'? > assert sender is the options page? > > may as well get some messaging sanity checks in here as well. I can't see if the sender is the options page specifically, the only property I have access to in MessageSender is id. https://codereview.chromium.org/378783002/diff/280001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/test.js:12: var windows = chrome.extension.getViews(); On 2014/07/18 17:07:26, kalman wrote: > likewise with the forEach. > > and please also assert some things about these windows. for example, if there is > no views then it's moot whether they have pass or not :) > > (mind you it would be crazy for there to be no views, since *this* is a view, > but you know...) I can check how many there are, but I can't seem to do anything else because I'm having a weird bug where adding the "tabs" permissions prevents extension_browsertest from loading the extension. https://codereview.chromium.org/378783002/diff/280001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/test.js:15: chrome.test.assertTrue('hi' == message); On 2014/07/18 17:07:26, kalman wrote: > assertEq('hi', message); Done.
Looked at the tests and *_guest.cc, some nits... https://codereview.chromium.org/378783002/diff/300001/chrome/browser/guest_vi... File chrome/browser/guest_view/extension_options/extension_options_guest.cc (right): https://codereview.chromium.org/378783002/diff/300001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:41: return embedder_extension->permissions_data()->HasAPIPermission( if (embedder_extension) { .... } https://codereview.chromium.org/378783002/diff/300001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/embed_self/manifest.json (right): https://codereview.chromium.org/378783002/diff/300001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/manifest.json:5: "options_page": "options.html", add a "description" field to describe in short what this test does. Same for the other manifest.json https://codereview.chromium.org/378783002/diff/300001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/privileged_apis/test.js (right): https://codereview.chromium.org/378783002/diff/300001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/privileged_apis/test.js:6: // Tests if the <extensionoptions> guest view can access and write to storage. nit: Also add that chrome.storage is a privileged api
https://codereview.chromium.org/378783002/diff/300001/chrome/browser/guest_vi... File chrome/browser/guest_view/extension_options/extension_options_guest.cc (right): https://codereview.chromium.org/378783002/diff/300001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:41: return embedder_extension->permissions_data()->HasAPIPermission( On 2014/07/18 19:55:40, lazyboy wrote: > if (embedder_extension) { .... } Done. https://codereview.chromium.org/378783002/diff/300001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/embed_self/manifest.json (right): https://codereview.chromium.org/378783002/diff/300001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/manifest.json:5: "options_page": "options.html", On 2014/07/18 19:55:40, lazyboy wrote: > add a "description" field to describe in short what this test does. > > Same for the other manifest.json Done. https://codereview.chromium.org/378783002/diff/300001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/privileged_apis/test.js (right): https://codereview.chromium.org/378783002/diff/300001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/privileged_apis/test.js:6: // Tests if the <extensionoptions> guest view can access and write to storage. On 2014/07/18 19:55:40, lazyboy wrote: > nit: Also add that chrome.storage is a privileged api Done.
https://codereview.chromium.org/378783002/diff/280001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/embed_self/test.js (right): https://codereview.chromium.org/378783002/diff/280001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/test.js:11: function(message, sender, sendResponse) { On 2014/07/18 19:37:06, Eric Zeng wrote: > On 2014/07/18 17:07:26, kalman wrote: > > assert message is 'hi'? > > assert sender is the options page? > > > > may as well get some messaging sanity checks in here as well. > I can't see if the sender is the options page specifically, the only property I > have access to in MessageSender is id. ah. https://codereview.chromium.org/378783002/diff/280001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/test.js:12: var windows = chrome.extension.getViews(); On 2014/07/18 19:37:06, Eric Zeng wrote: > On 2014/07/18 17:07:26, kalman wrote: > > likewise with the forEach. > > > > and please also assert some things about these windows. for example, if there > is > > no views then it's moot whether they have pass or not :) > > > > (mind you it would be crazy for there to be no views, since *this* is a view, > > but you know...) > I can check how many there are, but I can't seem to do anything else because I'm > having a weird bug where adding the "tabs" permissions prevents > extension_browsertest from loading the extension. yeah just check how many there are. there should be exactly 2, really - this page, and the options page. since there's no background page that shouldn't exist (it'd be kinda flaky otherwise). not sure what's going on the with tabs permission there, but it shouldn't be necessary anyway. https://codereview.chromium.org/378783002/diff/320001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/privileged_apis/options.js (right): https://codereview.chromium.org/378783002/diff/320001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/privileged_apis/options.js:6: it'd be good to also test the event in here, since that's the interesting part. https://codereview.chromium.org/378783002/diff/320001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/privileged_apis/test.js (right): https://codereview.chromium.org/378783002/diff/320001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/privileged_apis/test.js:6: // Tests if the <extensionoptions> guest view can access the chrome.storage any reason why you couldn't make this part of the other test?
Couple more comments, guest_view/* stuff looks OK. https://codereview.chromium.org/378783002/diff/320001/chrome/browser/guest_vi... File chrome/browser/guest_view/extension_options/extension_options_guest.cc (right): https://codereview.chromium.org/378783002/diff/320001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:80: callback.Run(NULL); For extensions that don't have options page, we shouldn't even be coming this far, right?, i.e. "Options" link shouldn't be available to try to navigate to this *View... I'm trying to understand if we want to log some error if we hit this. https://codereview.chromium.org/378783002/diff/320001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:84: // Create a Webcontents using the extension URL. You also want to add a bit more doc here: "Options page's WebContents and the extension whose option it is showing, should live in the same process, so we use |extension_url| as site URL."
https://codereview.chromium.org/378783002/diff/320001/chrome/browser/guest_vi... File chrome/browser/guest_view/extension_options/extension_options_guest.cc (right): https://codereview.chromium.org/378783002/diff/320001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:80: callback.Run(NULL); On 2014/07/18 20:37:38, lazyboy wrote: > For extensions that don't have options page, we shouldn't even be coming this > far, right?, i.e. "Options" link shouldn't be available to try to navigate to > this *View... > I'm trying to understand if we want to log some error if we hit this. In this patchset, I don't think anything explicitly prevents extensions without options pages from making it this far. I'm adding a check to extension_options.js that should ensure that the guest is only created if the extension has an options page. https://codereview.chromium.org/378783002/diff/320001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:84: // Create a Webcontents using the extension URL. On 2014/07/18 20:37:38, lazyboy wrote: > You also want to add a bit more doc here: "Options page's WebContents and the > extension whose option it is showing, should live in the same process, so we use > |extension_url| as site URL." Done. https://codereview.chromium.org/378783002/diff/320001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/privileged_apis/options.js (right): https://codereview.chromium.org/378783002/diff/320001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/privileged_apis/options.js:6: On 2014/07/18 20:31:35, kalman wrote: > it'd be good to also test the event in here, since that's the interesting part. Done (moved to other file). https://codereview.chromium.org/378783002/diff/320001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/privileged_apis/test.js (right): https://codereview.chromium.org/378783002/diff/320001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/privileged_apis/test.js:6: // Tests if the <extensionoptions> guest view can access the chrome.storage On 2014/07/18 20:31:35, kalman wrote: > any reason why you couldn't make this part of the other test? Nope
getting there. https://codereview.chromium.org/378783002/diff/340001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/embed_self/options.js (right): https://codereview.chromium.org/378783002/diff/340001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/options.js:11: chrome.runtime.sendMessage('hi'); it's a bit odd to see these 2 tests intermingled here, but separate in test.js. how about this design: chrome.runtime.sendMessage('ready', function(command) { switch (test) { case 'canCreateExtensionOptionsGuest': // the chrome.extension.getViews() stuff. chrome.runtime.sendMessage('done'); break; case 'guestCanAccessStorage': // similar to the chrome.storage stuff. } }); and then the test listens for that 'ready' message each time, sends the test name, coordinates with the test to know when it's done (whether that be through a 'done' message or other). https://codereview.chromium.org/378783002/diff/340001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/options.js:21: chrome.storage.local.set({'test': 42}, passed(function() { be careful here actually - this options page runs in a different context than the test runner, and chrome.test.callbackPass automatically calls chrome.test.pass when it's done. that means you'll get multiple test-passing messages and undefined results. you should try to make all of the test assertions happen in the test runner, so here, send back to the test runner whether it was successful or not. https://codereview.chromium.org/378783002/diff/340001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/embed_self/test.js (right): https://codereview.chromium.org/378783002/diff/340001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/test.js:19: chrome.test.succeed(); remove the element from the DOM after this?
https://codereview.chromium.org/378783002/diff/340001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/embed_self/options.js (right): https://codereview.chromium.org/378783002/diff/340001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/options.js:11: chrome.runtime.sendMessage('hi'); On 2014/07/18 22:59:34, kalman wrote: > it's a bit odd to see these 2 tests intermingled here, but separate in test.js. > how about this design: > > chrome.runtime.sendMessage('ready', function(command) { > switch (test) { > case 'canCreateExtensionOptionsGuest': > // the chrome.extension.getViews() stuff. > chrome.runtime.sendMessage('done'); > break; > > case 'guestCanAccessStorage': > // similar to the chrome.storage stuff. > } > }); > > and then the test listens for that 'ready' message each time, sends the test > name, coordinates with the test to know when it's done (whether that be through > a 'done' message or other). Done. https://codereview.chromium.org/378783002/diff/340001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/options.js:21: chrome.storage.local.set({'test': 42}, passed(function() { On 2014/07/18 22:59:34, kalman wrote: > be careful here actually - this options page runs in a different context than > the test runner, and chrome.test.callbackPass automatically calls > chrome.test.pass when it's done. that means you'll get multiple test-passing > messages and undefined results. > > you should try to make all of the test assertions happen in the test runner, so > here, send back to the test runner whether it was successful or not. What about using chrome.test.callback instead? https://codereview.chromium.org/378783002/diff/340001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/embed_self/test.js (right): https://codereview.chromium.org/378783002/diff/340001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/test.js:19: chrome.test.succeed(); On 2014/07/18 22:59:34, kalman wrote: > remove the element from the DOM after this? Done.
https://codereview.chromium.org/378783002/diff/340001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/embed_self/options.js (right): https://codereview.chromium.org/378783002/diff/340001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/options.js:21: chrome.storage.local.set({'test': 42}, passed(function() { On 2014/07/19 00:43:44, Eric Zeng wrote: > On 2014/07/18 22:59:34, kalman wrote: > > be careful here actually - this options page runs in a different context than > > the test runner, and chrome.test.callbackPass automatically calls > > chrome.test.pass when it's done. that means you'll get multiple test-passing > > messages and undefined results. > > > > you should try to make all of the test assertions happen in the test runner, > so > > here, send back to the test runner whether it was successful or not. > > What about using chrome.test.callback instead? I think that has the same problem: https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere... which is callbackAdded, and has: https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere...
nice! just that callback stuff to go. https://codereview.chromium.org/378783002/diff/360001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/embed_self/options.js (right): https://codereview.chromium.org/378783002/diff/360001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/options.js:18: chrome.runtime.sendMessage('hi'); break; https://codereview.chromium.org/378783002/diff/360001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/embed_self/test.js (right): https://codereview.chromium.org/378783002/diff/360001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/test.js:23: document.body.removeChild(document.querySelector('extensionoptions')); document.body.removeChild(extensionoptions);
I refactored a lot of the test to move the test assertions to the test runner, hopefully this is what you were looking for. https://codereview.chromium.org/378783002/diff/360001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/embed_self/options.js (right): https://codereview.chromium.org/378783002/diff/360001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/options.js:18: chrome.runtime.sendMessage('hi'); On 2014/07/19 01:37:09, kalman wrote: > break; Done. https://codereview.chromium.org/378783002/diff/360001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/embed_self/test.js (right): https://codereview.chromium.org/378783002/diff/360001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/test.js:23: document.body.removeChild(document.querySelector('extensionoptions')); On 2014/07/19 01:37:09, kalman wrote: > document.body.removeChild(extensionoptions); Done.
https://codereview.chromium.org/378783002/diff/380001/chrome/browser/guest_vi... File chrome/browser/guest_view/extension_options/extension_options_guest.cc (right): https://codereview.chromium.org/378783002/diff/380001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:41: if (embedder_extension) { nit: you can avoid a block as follows (I feel this is slightly more readable): if (!embedder_extension) return false; return embedder_extension->permissions_data()->HasAPIPermission( extensions::APIPermission::kEmbeddedExtensionOptions); https://codereview.chromium.org/378783002/diff/380001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:53: content::BrowserContext* browser_context = Hmm, is this necessary? A GuestView should know about its BrowserContext at this point. Try browser_context() accessor. https://codereview.chromium.org/378783002/diff/380001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:84: // Create a Webcontents using the extension URL. The options page's s/Webcontents/WebContents
lgtm! a bunch more nits but they're easy to address. great work! https://codereview.chromium.org/378783002/diff/180001/chrome/browser/guest_vi... File chrome/browser/guest_view/extension_options/extension_options_guest.cc (right): https://codereview.chromium.org/378783002/diff/180001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:99: void ExtensionOptionsGuest::DidInitialize() { On 2014/07/16 01:52:01, Eric Zeng wrote: > On 2014/07/16 00:10:57, kalman wrote: > > you need to create an instance of a ChromeExtensionWebContentsObserver here > for > > messaging to work. > > > > extensions::ChromeExtensionWebContentsObserver::CreateForWebContents( > > guest_web_contents()); > > > > and #include > > "chrome/browser/extensions/chrome_extension_web_contents_observer.h" > > Done. you don't need this anymore, I fixed the bug which made it necessary :) https://codereview.chromium.org/378783002/diff/380001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/embed_self/options.js (right): https://codereview.chromium.org/378783002/diff/380001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/options.js:18: chrome.runtime.sendMessage('hi'); maybe send something less arbitrary-looking? like 'done'? https://codereview.chromium.org/378783002/diff/380001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/options.js:19: break; nit: new line after the break https://codereview.chromium.org/378783002/diff/380001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/options.js:24: chrome.storage.onChanged.addListener(function(change) { listenOnce https://codereview.chromium.org/378783002/diff/380001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/options.js:27: actual: change.test.newValue, nice! https://codereview.chromium.org/378783002/diff/380001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/options.js:28: description: 'onStorage' nit: 'onChanged' maybe? https://codereview.chromium.org/378783002/diff/380001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/options.js:37: description: 'onSetCallback' it would be more correct to call this 'onGetCallback' right? https://codereview.chromium.org/378783002/diff/380001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/embed_self/test.js (right): https://codereview.chromium.org/378783002/diff/380001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/test.js:14: nit: no blank line here https://codereview.chromium.org/378783002/diff/380001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/test.js:46: if (message == 'ready') nits: - if one block needs braces, they all do. - move the comments inside the bodies that they apply to. i.e. if (message == 'ready') { // Options page is waiting for a command. sendResponse('getCanAccessStorage'); } else if (message.hasOwnProperty('description')) { // Message from the options page containing test data. ... } https://codereview.chromium.org/378783002/diff/380001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/test.js:50: if (message.description == 'onStorage') { that said I don't think the outer message.hasOwnProperty(..) check is really necessary; message.description will be undefined here, and therefore not == 'onStorage'.
https://codereview.chromium.org/378783002/diff/380001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/embed_self/options.js (right): https://codereview.chromium.org/378783002/diff/380001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/options.js:37: description: 'onSetCallback' On 2014/07/21 16:30:43, kalman wrote: > it would be more correct to call this 'onGetCallback' right? or 'onSetAndGet'?
also I think that "Implement groundwork for <extensionoptions>" is selling yourself a bit short. I would say: Initial implementation of the <extensionoptions> guestview tag. Extensions can now use the <extensionoptions> page to embed another extension's options page with basic iframe-like behavior and features.
https://codereview.chromium.org/378783002/diff/180001/chrome/browser/guest_vi... File chrome/browser/guest_view/extension_options/extension_options_guest.cc (right): https://codereview.chromium.org/378783002/diff/180001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:99: void ExtensionOptionsGuest::DidInitialize() { On 2014/07/21 16:30:43, kalman wrote: > On 2014/07/16 01:52:01, Eric Zeng wrote: > > On 2014/07/16 00:10:57, kalman wrote: > > > you need to create an instance of a ChromeExtensionWebContentsObserver here > > for > > > messaging to work. > > > > > > extensions::ChromeExtensionWebContentsObserver::CreateForWebContents( > > > guest_web_contents()); > > > > > > and #include > > > "chrome/browser/extensions/chrome_extension_web_contents_observer.h" > > > > Done. > > you don't need this anymore, I fixed the bug which made it necessary :) Apparently not, I can't run any of the chrome.test.* functions without this line. https://codereview.chromium.org/378783002/diff/380001/chrome/browser/guest_vi... File chrome/browser/guest_view/extension_options/extension_options_guest.cc (right): https://codereview.chromium.org/378783002/diff/380001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:41: if (embedder_extension) { On 2014/07/21 12:48:56, Fady Samuel wrote: > nit: you can avoid a block as follows (I feel this is slightly more readable): > > if (!embedder_extension) > return false; > > return embedder_extension->permissions_data()->HasAPIPermission( > extensions::APIPermission::kEmbeddedExtensionOptions); Done. https://codereview.chromium.org/378783002/diff/380001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:53: content::BrowserContext* browser_context = On 2014/07/21 12:48:56, Fady Samuel wrote: > Hmm, is this necessary? A GuestView should know about its BrowserContext at this > point. > > Try browser_context() accessor. Done. https://codereview.chromium.org/378783002/diff/380001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:84: // Create a Webcontents using the extension URL. The options page's On 2014/07/21 12:48:56, Fady Samuel wrote: > s/Webcontents/WebContents Done. https://codereview.chromium.org/378783002/diff/380001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/embed_self/options.js (right): https://codereview.chromium.org/378783002/diff/380001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/options.js:18: chrome.runtime.sendMessage('hi'); On 2014/07/21 16:30:43, kalman wrote: > maybe send something less arbitrary-looking? like 'done'? Done. https://codereview.chromium.org/378783002/diff/380001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/options.js:19: break; On 2014/07/21 16:30:43, kalman wrote: > nit: new line after the break Done. https://codereview.chromium.org/378783002/diff/380001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/options.js:24: chrome.storage.onChanged.addListener(function(change) { On 2014/07/21 16:30:43, kalman wrote: > listenOnce For some reason, using listenOnce causes the the test to succeed from options.js. It displays "[SUCCESS] (no test)" and terminates the test before the rest of test.js can be executed. https://codereview.chromium.org/378783002/diff/380001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/options.js:28: description: 'onStorage' On 2014/07/21 16:30:43, kalman wrote: > nit: 'onChanged' maybe? onStorageChanged? https://codereview.chromium.org/378783002/diff/380001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/options.js:37: description: 'onSetCallback' On 2014/07/21 16:31:28, kalman wrote: > On 2014/07/21 16:30:43, kalman wrote: > > it would be more correct to call this 'onGetCallback' right? > > or 'onSetAndGet'? Let's do onSetAndGet since the test is kind of verifying both functions. https://codereview.chromium.org/378783002/diff/380001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/embed_self/test.js (right): https://codereview.chromium.org/378783002/diff/380001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/test.js:14: On 2014/07/21 16:30:43, kalman wrote: > nit: no blank line here Done. https://codereview.chromium.org/378783002/diff/380001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/test.js:46: if (message == 'ready') On 2014/07/21 16:30:43, kalman wrote: > nits: > - if one block needs braces, they all do. > - move the comments inside the bodies that they apply to. > i.e. > > if (message == 'ready') { > // Options page is waiting for a command. > sendResponse('getCanAccessStorage'); > } else if (message.hasOwnProperty('description')) { > // Message from the options page containing test data. > ... > } Done. https://codereview.chromium.org/378783002/diff/380001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/test.js:50: if (message.description == 'onStorage') { On 2014/07/21 16:30:43, kalman wrote: > that said I don't think the outer message.hasOwnProperty(..) check is really > necessary; message.description will be undefined here, and therefore not == > 'onStorage'. Done.
https://codereview.chromium.org/378783002/diff/380001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/extension_options/embed_self/options.js (right): https://codereview.chromium.org/378783002/diff/380001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/options.js:24: chrome.storage.onChanged.addListener(function(change) { On 2014/07/21 17:52:39, Eric Zeng wrote: > On 2014/07/21 16:30:43, kalman wrote: > > listenOnce > > For some reason, using listenOnce causes the the test to succeed from > options.js. It displays "[SUCCESS] (no test)" and terminates the test before the > rest of test.js can be executed. ah right. it probably runs some testing functions, which is exactly what moving the assertions out of this file was fixing. cool. https://codereview.chromium.org/378783002/diff/380001/chrome/test/data/extens... chrome/test/data/extensions/api_test/extension_options/embed_self/options.js:28: description: 'onStorage' On 2014/07/21 17:52:39, Eric Zeng wrote: > On 2014/07/21 16:30:43, kalman wrote: > > nit: 'onChanged' maybe? > > onStorageChanged? sure
https://codereview.chromium.org/378783002/diff/400001/chrome/browser/guest_vi... File chrome/browser/guest_view/guest_view_base.cc (right): https://codereview.chromium.org/378783002/diff/400001/chrome/browser/guest_vi... chrome/browser/guest_view/guest_view_base.cc:150: // we want a registry of GuestView types in the future. I've addressed this TODO: https://codereview.chromium.org/409603003/
guest_view lgtm otherwise.
https://codereview.chromium.org/378783002/diff/400001/chrome/browser/guest_vi... File chrome/browser/guest_view/guest_view_base.cc (right): https://codereview.chromium.org/378783002/diff/400001/chrome/browser/guest_vi... chrome/browser/guest_view/guest_view_base.cc:150: // we want a registry of GuestView types in the future. On 2014/07/21 19:44:47, Fady Samuel wrote: > I've addressed this TODO: https://codereview.chromium.org/409603003/ Done.
+jochen for chrome/renderer/chrome_content_renderer_client.cc, chrome/renderer/resources/renderer_resources.grd
https://codereview.chromium.org/378783002/diff/420001/chrome/browser/guest_vi... File chrome/browser/guest_view/guest_view_base.cc (right): https://codereview.chromium.org/378783002/diff/420001/chrome/browser/guest_vi... chrome/browser/guest_view/guest_view_base.cc:21: #include "extensions/common/feature_switch.h" nit: This line isn't necessary right?
https://codereview.chromium.org/378783002/diff/420001/chrome/browser/guest_vi... File chrome/browser/guest_view/guest_view_base.cc (right): https://codereview.chromium.org/378783002/diff/420001/chrome/browser/guest_vi... chrome/browser/guest_view/guest_view_base.cc:21: #include "extensions/common/feature_switch.h" On 2014/07/22 00:42:52, Fady Samuel wrote: > nit: This line isn't necessary right? Done.
ping jochen
lgtm
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/378783002/440001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel_swarming on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win8_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/32132) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/37010)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/32141)
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/378783002/460001
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/378783002/480001
still lgtm but something wonky has gone on here. at the very least I think you need to rebase (the win_gpu has rejected your patch). I suggest do that, run a try job, click submit
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/378783002/500001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_rel_swarming on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
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/378783002/500001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_rel_swarming on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
The CQ bit was checked by ericzeng@chromium.org
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/378783002/520001
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/378783002/520001
Message was sent while issue was closed.
Change committed as 285518 |
