Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
453613002:
Implement support for <extensionoptions> embedding in WebUI (Closed)
DescriptionImplement support for <extensionoptions> embedding in WebUI
Modify permissions and checks in GuestView, WebUI, and
extensions to allow <extensionoptions> to be used within WebUI.
In the GuestView infrastructure, allow guest views in WebUI to
be created without extension ids. In extensions, allow extension
options-related APIs to be used in WebUI contexts. In WebUI,
relax the CSP to allow internal browser plugins.
BUG=386842
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289233
Patch Set 1 #
Total comments: 47
Patch Set 2 : Address comments #Patch Set 3 : Fix object-src CSP override #
Total comments: 3
Patch Set 4 : Rebase onto most recent extensionoptions CL #Patch Set 5 : Rebase onto Feature refactor, address comments #Patch Set 6 : Fix build issue #Patch Set 7 : Remove unneccessary whitespace changes #Patch Set 8 : (again) #
Total comments: 15
Patch Set 9 : Address kalman's comments #
Total comments: 6
Patch Set 10 : Address thestig's comments #Patch Set 11 : Rebase #Patch Set 12 : Rebase #Patch Set 13 : Add left brace #Messages
Total messages: 37 (0 generated)
PTAL, this is my initial attempt at enabling my guest view in WebUI. fsamuel - I ended up ignoring or skipping checks for extension ids in multiple places, can you confirm if those were safe to do? Also, I thought I might need to make changes to ExtensionMessageFilter to allow WebUI to access the listeners, but it seems to work without making changes there. Will it be neccesary to modify that class? kalman - Still working on the CSP problem. So far are the API/Feature/permissions changes are ok?
https://codereview.chromium.org/453613002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/guest_view/guest_view_internal_api.cc (right): https://codereview.chromium.org/453613002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/guest_view/guest_view_internal_api.cc:38: // there is no extension, and extension() will be null. Use an empty string this shouldn't be necessary after https://codereview.chromium.org/426593007/ goes in https://codereview.chromium.org/453613002/diff/1/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_webui_apitest.cc (right): https://codereview.chromium.org/453613002/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_webui_apitest.cc:183: FeatureSwitch::embedded_extension_options(), true); might as well do this in a member variable as well, since you're overriding the command line. odd to have one but not the other? https://codereview.chromium.org/453613002/diff/1/chrome/browser/guest_view/gu... File chrome/browser/guest_view/guest_view_base.cc (left): https://codereview.chromium.org/453613002/diff/1/chrome/browser/guest_view/gu... chrome/browser/guest_view/guest_view_base.cc:396: but you'll still need this. it's not an important check because the event router makes it anyway. https://codereview.chromium.org/453613002/diff/1/chrome/browser/guest_view/gu... File chrome/browser/guest_view/guest_view_base.cc (right): https://codereview.chromium.org/453613002/diff/1/chrome/browser/guest_view/gu... chrome/browser/guest_view/guest_view_base.cc:109: if (!embedder_extension_id.empty()) { this should also be unnecessary https://codereview.chromium.org/453613002/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/453613002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/_api_features.json:374: "matches": ["chrome://*/*"] can you make this more specific than all of chrome? https://codereview.chromium.org/453613002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/_api_features.json:454: }], ideally this API rule would actually be: "guestViewInternal": [{ "internal": true, "dependencies": ["api:appViewInternal"] }, { "internal": true, "dependencies": ["api:extensionOptionsInternal"] }, { "internal": true, "dependencies": ["api:webViewInternal"] }] curious whether that passes all the tests. in fact, I'm going to try it now. I wouldn't try making it work yourself, though, what you have is fine for now. https://codereview.chromium.org/453613002/diff/1/chrome/renderer/chrome_conte... File chrome/renderer/chrome_content_renderer_client.cc (left): https://codereview.chromium.org/453613002/diff/1/chrome/renderer/chrome_conte... chrome/renderer/chrome_content_renderer_client.cc:594: probably should revert this change. https://codereview.chromium.org/453613002/diff/1/chrome/renderer/chrome_conte... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/453613002/diff/1/chrome/renderer/chrome_conte... chrome/renderer/chrome_content_renderer_client.cc:516: void GetScriptContextForGuestView(extensions::ScriptContext** out, should be in an anonymous namespace. and "Find" might be a better term? https://codereview.chromium.org/453613002/diff/1/chrome/renderer/chrome_conte... chrome/renderer/chrome_content_renderer_client.cc:533: "webViewInternal"}; hm, in https://codereview.chromium.org/426593007/ I add constants for these. we should really keep them in sync, but I'm not sure how - there doesn't appear to be any shared files between the browser and renderer relating to guestview. fady? https://codereview.chromium.org/453613002/diff/1/chrome/renderer/chrome_conte... chrome/renderer/chrome_content_renderer_client.cc:539: you need to check whether it actually found a context. though really a better check would be to instead of finding an appropriate script context, to just look through them and see if any have the guestview API available. and for *that* - I suppose you could have all of these constants here. but even easier would to just check for the availability of the guestViewInternal API. that sort of thing would be: FindScriptContextWithGuestView(bool* found, ScriptContext* context) { if (context->GetAvailability("guestViewInternal").is_available()) *found = true; } ... bool found = true; extension_dispatcher_->script_context_set().ForEach( render_frame->GetRenderView(), base::Bind(&FindScriptContextWithGuestView, &found)); if (found) return false; https://codereview.chromium.org/453613002/diff/1/chrome/renderer/resources/ex... File chrome/renderer/resources/extensions/extension_options.js (left): https://codereview.chromium.org/453613002/diff/1/chrome/renderer/resources/ex... chrome/renderer/resources/extensions/extension_options.js:146: // Only allow extensions to embed their own options page (if it has one). was this security check important? because now there is no check, obviously. https://codereview.chromium.org/453613002/diff/1/chrome/test/data/extensions/... File chrome/test/data/extensions/webui/can_embed_extension_options.js (right): https://codereview.chromium.org/453613002/diff/1/chrome/test/data/extensions/... chrome/test/data/extensions/webui/can_embed_extension_options.js:14: var extensionoptions = document.createElement('extensionoptions'); I remember we moved this out of here as a speculative fix, but it's probably better inside the function scope if possible. https://codereview.chromium.org/453613002/diff/1/content/browser/webui/web_ui... File content/browser/webui/web_ui_data_source_impl.cc (right): https://codereview.chromium.org/453613002/diff/1/content/browser/webui/web_ui... content/browser/webui/web_ui_data_source_impl.cc:92: return "object-src chrome://chrome/extensions/ chrome://extensions-frame/;"; you must get somebody else to look at this. I can't remember who the person was to ask, maybe you can find it. https://codereview.chromium.org/453613002/diff/1/extensions/renderer/event_bi... File extensions/renderer/event_bindings.cc (right): https://codereview.chromium.org/453613002/diff/1/extensions/renderer/event_bi... extensions/renderer/event_bindings.cc:214: context()->context_type() != Feature::Context::WEBUI_CONTEXT) { I'm surprised any of the filtered event logic works, I never implemented the dispatch-to-URL logic for filtered events: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... just non-filtered events. in any case, just delete this check rather than adding the WEBUI_CONTEXT part. https://codereview.chromium.org/453613002/diff/1/extensions/renderer/event_bi... extensions/renderer/event_bindings.cc:261: context()->context_type() != Feature::Context::WEBUI_CONTEXT) { likewise
https://codereview.chromium.org/453613002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/guest_view/guest_view_internal_api.cc (right): https://codereview.chromium.org/453613002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/guest_view/guest_view_internal_api.cc:40: std::string embedder_extension_id = ""; no need for initializer. https://codereview.chromium.org/453613002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/guest_view/guest_view_internal_api.cc:41: if (extension()) { No need for braces. https://codereview.chromium.org/453613002/diff/1/chrome/browser/guest_view/gu... File chrome/browser/guest_view/guest_view_base.cc (left): https://codereview.chromium.org/453613002/diff/1/chrome/browser/guest_view/gu... chrome/browser/guest_view/guest_view_base.cc:396: On 2014/08/07 23:06:02, kalman wrote: > but you'll still need this. it's not an important check because the event router > makes it anyway. Will the event router work in WebUI? Will WebUI receive extension events? https://codereview.chromium.org/453613002/diff/1/chrome/renderer/chrome_conte... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/453613002/diff/1/chrome/renderer/chrome_conte... chrome/renderer/chrome_content_renderer_client.cc:538: base::Bind(&GetScriptContextForGuestView, &context)); If I'm reading this correctly, context will be NULL if it's neither extension nor WebUI, right? Then we access it below? May we don't need this perm check at all here? The browser process will not allow the creation of guests of these types without the appropriate permission anyway. Do we need an extra check in the renderer? https://codereview.chromium.org/453613002/diff/1/chrome/renderer/extensions/c... File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc (right): https://codereview.chromium.org/453613002/diff/1/chrome/renderer/extensions/c... chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:326: context->GetAvailability("extensionOptionsInternal").is_available()) { Can we refer to a constant here?
https://codereview.chromium.org/453613002/diff/1/chrome/browser/guest_view/gu... File chrome/browser/guest_view/guest_view_base.cc (left): https://codereview.chromium.org/453613002/diff/1/chrome/browser/guest_view/gu... chrome/browser/guest_view/guest_view_base.cc:396: On 2014/08/07 23:09:29, Fady Samuel wrote: > On 2014/08/07 23:06:02, kalman wrote: > > but you'll still need this. it's not an important check because the event > router > > makes it anyway. > > Will the event router work in WebUI? Will WebUI receive extension events? yes, this is one of the things I made work. however I didn't make filtered events work.
https://codereview.chromium.org/453613002/diff/1/chrome/renderer/chrome_conte... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/453613002/diff/1/chrome/renderer/chrome_conte... chrome/renderer/chrome_content_renderer_client.cc:533: "webViewInternal"}; On 2014/08/07 23:06:03, kalman wrote: > hm, in https://codereview.chromium.org/426593007/ I add constants for these. we > should really keep them in sync, but I'm not sure how - there doesn't appear to > be any shared files between the browser and renderer relating to guestview. > > fady? Nope, perhaps something in chrome/common/guest_view?
https://codereview.chromium.org/453613002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/guest_view/guest_view_internal_api.cc (right): https://codereview.chromium.org/453613002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/guest_view/guest_view_internal_api.cc:38: // there is no extension, and extension() will be null. Use an empty string On 2014/08/07 23:06:02, kalman wrote: > this shouldn't be necessary after https://codereview.chromium.org/426593007/ > goes in I'm not sure if that's the case. ExtensionFunction::extension_id() gets the id by calling extension_->id(), but extension_ will be NULL if I embed my guest view in WebUI, which causes it to crash in here. Did you make any modifications to ExtensionFunction elsewhere? https://codereview.chromium.org/453613002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/guest_view/guest_view_internal_api.cc:40: std::string embedder_extension_id = ""; On 2014/08/07 23:09:29, Fady Samuel wrote: > no need for initializer. Done. https://codereview.chromium.org/453613002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/guest_view/guest_view_internal_api.cc:41: if (extension()) { On 2014/08/07 23:09:29, Fady Samuel wrote: > No need for braces. Done. https://codereview.chromium.org/453613002/diff/1/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_webui_apitest.cc (right): https://codereview.chromium.org/453613002/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_webui_apitest.cc:183: FeatureSwitch::embedded_extension_options(), true); On 2014/08/07 23:06:02, kalman wrote: > might as well do this in a member variable as well, since you're overriding the > command line. odd to have one but not the other? Done. https://codereview.chromium.org/453613002/diff/1/chrome/browser/guest_view/gu... File chrome/browser/guest_view/guest_view_base.cc (left): https://codereview.chromium.org/453613002/diff/1/chrome/browser/guest_view/gu... chrome/browser/guest_view/guest_view_base.cc:396: On 2014/08/07 23:13:54, kalman wrote: > On 2014/08/07 23:09:29, Fady Samuel wrote: > > On 2014/08/07 23:06:02, kalman wrote: > > > but you'll still need this. it's not an important check because the event > > router > > > makes it anyway. > > > > Will the event router work in WebUI? Will WebUI receive extension events? > > yes, this is one of the things I made work. > > however I didn't make filtered events work. What do you mean by I'll still need it? Won't it crash every time I try to embed extensionoptions outside of an extension? https://codereview.chromium.org/453613002/diff/1/chrome/browser/guest_view/gu... File chrome/browser/guest_view/guest_view_base.cc (right): https://codereview.chromium.org/453613002/diff/1/chrome/browser/guest_view/gu... chrome/browser/guest_view/guest_view_base.cc:109: if (!embedder_extension_id.empty()) { On 2014/08/07 23:06:02, kalman wrote: > this should also be unnecessary I'll leave it in until your patch lands. https://codereview.chromium.org/453613002/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/453613002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/_api_features.json:374: "matches": ["chrome://*/*"] On 2014/08/07 23:06:02, kalman wrote: > can you make this more specific than all of chrome? Done, but I'm not sure why this combination in particular makes it work. I tried to do the same to guestViewInternal but my test becomes extremely flaky. Probably due to that bug you were talking about earlier. https://codereview.chromium.org/453613002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/_api_features.json:454: }], On 2014/08/07 23:06:02, kalman wrote: > ideally this API rule would actually be: > > "guestViewInternal": [{ > "internal": true, > "dependencies": ["api:appViewInternal"] > }, { > "internal": true, > "dependencies": ["api:extensionOptionsInternal"] > }, { > "internal": true, > "dependencies": ["api:webViewInternal"] > }] > > curious whether that passes all the tests. in fact, I'm going to try it now. I > wouldn't try making it work yourself, though, what you have is fine for now. Acknowledged. https://codereview.chromium.org/453613002/diff/1/chrome/renderer/chrome_conte... File chrome/renderer/chrome_content_renderer_client.cc (left): https://codereview.chromium.org/453613002/diff/1/chrome/renderer/chrome_conte... chrome/renderer/chrome_content_renderer_client.cc:594: On 2014/08/07 23:06:03, kalman wrote: > probably should revert this change. Done. https://codereview.chromium.org/453613002/diff/1/chrome/renderer/chrome_conte... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/453613002/diff/1/chrome/renderer/chrome_conte... chrome/renderer/chrome_content_renderer_client.cc:516: void GetScriptContextForGuestView(extensions::ScriptContext** out, On 2014/08/07 23:06:03, kalman wrote: > should be in an anonymous namespace. > > and "Find" might be a better term? Done. https://codereview.chromium.org/453613002/diff/1/chrome/renderer/chrome_conte... chrome/renderer/chrome_content_renderer_client.cc:538: base::Bind(&GetScriptContextForGuestView, &context)); On 2014/08/07 23:09:29, Fady Samuel wrote: > If I'm reading this correctly, context will be NULL if it's neither extension > nor WebUI, right? Then we access it below? May we don't need this perm check at > all here? The browser process will not allow the creation of guests of these > types without the appropriate permission anyway. Do we need an extra check in > the renderer? Fixed the null pointer issue - see kalman's comment below. It looks like we need an early escape from the code below in order to create the guest view browser plugin. If not, I think this method just creates some kind of web plugin, and never creates a BrowserPlugin in RenderFrameImpl. https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... https://codereview.chromium.org/453613002/diff/1/chrome/renderer/chrome_conte... chrome/renderer/chrome_content_renderer_client.cc:539: On 2014/08/07 23:06:03, kalman wrote: > you need to check whether it actually found a context. > > though really a better check would be to instead of finding an appropriate > script context, to just look through them and see if any have the guestview API > available. > > and for *that* - I suppose you could have all of these constants here. but even > easier would to just check for the availability of the guestViewInternal API. > > that sort of thing would be: > > FindScriptContextWithGuestView(bool* found, ScriptContext* context) { > if (context->GetAvailability("guestViewInternal").is_available()) > *found = true; > } > > ... > > bool found = true; > extension_dispatcher_->script_context_set().ForEach( > render_frame->GetRenderView(), > base::Bind(&FindScriptContextWithGuestView, &found)); > if (found) > return false; Done. https://codereview.chromium.org/453613002/diff/1/chrome/renderer/extensions/c... File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc (right): https://codereview.chromium.org/453613002/diff/1/chrome/renderer/extensions/c... chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:326: context->GetAvailability("extensionOptionsInternal").is_available()) { On 2014/08/07 23:09:29, Fady Samuel wrote: > Can we refer to a constant here? Yes, when this CL lands: https://codereview.chromium.org/426593007/ https://codereview.chromium.org/453613002/diff/1/chrome/renderer/resources/ex... File chrome/renderer/resources/extensions/extension_options.js (left): https://codereview.chromium.org/453613002/diff/1/chrome/renderer/resources/ex... chrome/renderer/resources/extensions/extension_options.js:146: // Only allow extensions to embed their own options page (if it has one). On 2014/08/07 23:06:03, kalman wrote: > was this security check important? because now there is no check, obviously. No, the checks will be unnecessary when I rebase onto this CL: https://codereview.chromium.org/429763002/. The CL handles all of the invalid cases at the C++ level in ExtensionOptionsGuest. The change was a little premature but I had to get rid of the checks so that it doesn't depend on being inside an extension context. https://codereview.chromium.org/453613002/diff/1/chrome/test/data/extensions/... File chrome/test/data/extensions/webui/can_embed_extension_options.js (right): https://codereview.chromium.org/453613002/diff/1/chrome/test/data/extensions/... chrome/test/data/extensions/webui/can_embed_extension_options.js:14: var extensionoptions = document.createElement('extensionoptions'); On 2014/08/07 23:06:03, kalman wrote: > I remember we moved this out of here as a speculative fix, but it's probably > better inside the function scope if possible. Done. https://codereview.chromium.org/453613002/diff/1/content/browser/webui/web_ui... File content/browser/webui/web_ui_data_source_impl.cc (right): https://codereview.chromium.org/453613002/diff/1/content/browser/webui/web_ui... content/browser/webui/web_ui_data_source_impl.cc:92: return "object-src chrome://chrome/extensions/ chrome://extensions-frame/;"; On 2014/08/07 23:06:03, kalman wrote: > you must get somebody else to look at this. I can't remember who the person was > to ask, maybe you can find it. Acknowledged. https://codereview.chromium.org/453613002/diff/1/extensions/renderer/event_bi... File extensions/renderer/event_bindings.cc (right): https://codereview.chromium.org/453613002/diff/1/extensions/renderer/event_bi... extensions/renderer/event_bindings.cc:214: context()->context_type() != Feature::Context::WEBUI_CONTEXT) { On 2014/08/07 23:06:03, kalman wrote: > I'm surprised any of the filtered event logic works, I never implemented the > dispatch-to-URL logic for filtered events: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... > > just non-filtered events. > > in any case, just delete this check rather than adding the WEBUI_CONTEXT part. Done. Are you absolutely sure that the check isn't necessary? https://codereview.chromium.org/453613002/diff/1/extensions/renderer/event_bi... extensions/renderer/event_bindings.cc:261: context()->context_type() != Feature::Context::WEBUI_CONTEXT) { On 2014/08/07 23:06:03, kalman wrote: > likewise Done.
I think I figured out how to do the CSP properly, and I made it work with "object-src 'self'" instead of using multiple chrome://extensions-related URL. The existing test passes, and I am also able to insert a new <extensionoptions> element into chrome://extensions-frame manually with the Chrome console. Better yet, there is precedent for using this method of CSP overriding in another WebUI page: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
some follow-up comments but looks fine, wait for my other CL to land. but I'm still extra puzzled by the - guest view API stuff. - filtered events working. https://codereview.chromium.org/453613002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/guest_view/guest_view_internal_api.cc (right): https://codereview.chromium.org/453613002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/guest_view/guest_view_internal_api.cc:38: // there is no extension, and extension() will be null. Use an empty string On 2014/08/08 00:23:46, ericzeng wrote: > On 2014/08/07 23:06:02, kalman wrote: > > this shouldn't be necessary after https://codereview.chromium.org/426593007/ > > goes in > > I'm not sure if that's the case. ExtensionFunction::extension_id() gets the id > by calling extension_->id(), but extension_ will be NULL if I embed my guest > view in WebUI, which causes it to crash in here. Did you make any modifications > to ExtensionFunction elsewhere? ah you're right, my bad sorry. 1 last nit: the last sentence isn't needed. https://codereview.chromium.org/453613002/diff/1/chrome/browser/guest_view/gu... File chrome/browser/guest_view/guest_view_base.cc (left): https://codereview.chromium.org/453613002/diff/1/chrome/browser/guest_view/gu... chrome/browser/guest_view/guest_view_base.cc:396: On 2014/08/08 00:23:46, ericzeng wrote: > On 2014/08/07 23:13:54, kalman wrote: > > On 2014/08/07 23:09:29, Fady Samuel wrote: > > > On 2014/08/07 23:06:02, kalman wrote: > > > > but you'll still need this. it's not an important check because the event > > > router > > > > makes it anyway. > > > > > > Will the event router work in WebUI? Will WebUI receive extension events? > > > > yes, this is one of the things I made work. > > > > however I didn't make filtered events work. > > What do you mean by I'll still need it? Won't it crash every time I try to embed > extensionoptions outside of an extension? I meant you'll still need to make this change :) bad wording sorry. https://codereview.chromium.org/453613002/diff/1/chrome/browser/guest_view/gu... File chrome/browser/guest_view/guest_view_base.cc (right): https://codereview.chromium.org/453613002/diff/1/chrome/browser/guest_view/gu... chrome/browser/guest_view/guest_view_base.cc:109: if (!embedder_extension_id.empty()) { On 2014/08/08 00:23:46, ericzeng wrote: > On 2014/08/07 23:06:02, kalman wrote: > > this should also be unnecessary > > I'll leave it in until your patch lands. stay tuned! https://codereview.chromium.org/453613002/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/453613002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/_api_features.json:374: "matches": ["chrome://*/*"] On 2014/08/08 00:23:46, ericzeng wrote: > On 2014/08/07 23:06:02, kalman wrote: > > can you make this more specific than all of chrome? > Done, but I'm not sure why this combination in particular makes it work. - the second rule chrome://chrome/extensions should have the wildcard at the end. - the third rule shouldn't be necessary since that's covered by the first rule (chrome-extension:// URLs are blessed extensions... unless they live in iframes) > > I tried to do the same to guestViewInternal but my test becomes extremely flaky. > Probably due to that bug you were talking about earlier. your test becomes flaky making that change to guestviewinternal? how odd. in what way? It doesn't sound like the bug I was seeing, since these are all running in the same context. https://codereview.chromium.org/453613002/diff/1/extensions/renderer/event_bi... File extensions/renderer/event_bindings.cc (right): https://codereview.chromium.org/453613002/diff/1/extensions/renderer/event_bi... extensions/renderer/event_bindings.cc:214: context()->context_type() != Feature::Context::WEBUI_CONTEXT) { On 2014/08/08 00:23:47, ericzeng wrote: > On 2014/08/07 23:06:03, kalman wrote: > > I'm surprised any of the filtered event logic works, I never implemented the > > dispatch-to-URL logic for filtered events: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... > > > > just non-filtered events. > > > > in any case, just delete this check rather than adding the WEBUI_CONTEXT part. > > Done. Are you absolutely sure that the check isn't necessary? yes, it will get caught up on the browser. https://codereview.chromium.org/453613002/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extensions_ui.cc (right): https://codereview.chromium.org/453613002/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extensions_ui.cc:88: "object-src 'self';"); awesome! https://codereview.chromium.org/453613002/diff/40001/chrome/renderer/chrome_c... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/453613002/diff/40001/chrome/renderer/chrome_c... chrome/renderer/chrome_content_renderer_client.cc:252: context->GetAvailability("guestViewInternal").is_available()) { why is just the guestViewInternal check not enough?
https://codereview.chromium.org/453613002/diff/40001/chrome/renderer/chrome_c... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/453613002/diff/40001/chrome/renderer/chrome_c... chrome/renderer/chrome_content_renderer_client.cc:252: context->GetAvailability("guestViewInternal").is_available()) { On 2014/08/08 14:08:15, kalman wrote: > why is just the guestViewInternal check not enough? I think it is enough.
https://codereview.chromium.org/453613002/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/453613002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/_api_features.json:374: "matches": ["chrome://*/*"] On 2014/08/08 14:08:15, kalman wrote: > On 2014/08/08 00:23:46, ericzeng wrote: > > On 2014/08/07 23:06:02, kalman wrote: > > > can you make this more specific than all of chrome? > > Done, but I'm not sure why this combination in particular makes it work. > > - the second rule chrome://chrome/extensions should have the wildcard at the > end. > - the third rule shouldn't be necessary since that's covered by the first rule > (chrome-extension:// URLs are blessed extensions... unless they live in iframes) > > > > > I tried to do the same to guestViewInternal but my test becomes extremely > flaky. > > Probably due to that bug you were talking about earlier. > > your test becomes flaky making that change to guestviewinternal? how odd. in > what way? It doesn't sound like the bug I was seeing, since these are all > running in the same context. I think I fixed it actually, it was easier to find out which matches I need using Feature https://codereview.chromium.org/453613002/diff/1/chrome/renderer/extensions/c... File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc (right): https://codereview.chromium.org/453613002/diff/1/chrome/renderer/extensions/c... chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:326: context->GetAvailability("extensionOptionsInternal").is_available()) { On 2014/08/08 00:23:47, ericzeng wrote: > On 2014/08/07 23:09:29, Fady Samuel wrote: > > Can we refer to a constant here? > > Yes, when this CL lands: https://codereview.chromium.org/426593007/ Actually I can't because the constants are in browser, but this is in renderer.
ping
guest_view lgtm
nit, but lgtm. https://codereview.chromium.org/453613002/diff/140001/chrome/browser/guest_vi... File chrome/browser/guest_view/extension_options/extension_options_guest.cc (right): https://codereview.chromium.org/453613002/diff/140001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:68: if (extension_id != embedder_extension_id) { how about: if (extensions::Extension::IdIsValid(embedder_extension_id) && extension_id != embedder_extension_id) { // Extensions cannot embed other extensions' options pages. callback.Run(NULL); return; } https://codereview.chromium.org/453613002/diff/140001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extensions_ui.cc (right): https://codereview.chromium.org/453613002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extensions_ui.cc:86: mention why you need this https://codereview.chromium.org/453613002/diff/140001/chrome/common/extension... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/453613002/diff/140001/chrome/common/extension... chrome/common/extensions/api/_api_features.json:384: "channel": "trunk", no much point having this channel restriction really, it's internal. https://codereview.chromium.org/453613002/diff/140001/chrome/common/extension... chrome/common/extensions/api/_api_features.json:463: "channel": "trunk", ditto https://codereview.chromium.org/453613002/diff/140001/chrome/renderer/extensi... File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc (right): https://codereview.chromium.org/453613002/diff/140001/chrome/renderer/extensi... chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:292: if (extension->permissions_data()->HasAPIPermission( Add TODO(fsamuel): Use context->GetAvailability("webViewInternal"). https://codereview.chromium.org/453613002/diff/140001/chrome/renderer/extensi... chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:316: if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableAppView) && Add TODO(fsamuel): Use context->GetAvailability("appViewInternal"). https://codereview.chromium.org/453613002/diff/140001/chrome/test/data/extens... File chrome/test/data/extensions/webui/can_embed_extension_options.js (right): https://codereview.chromium.org/453613002/diff/140001/chrome/test/data/extens... chrome/test/data/extensions/webui/can_embed_extension_options.js:21: delete this line
https://codereview.chromium.org/453613002/diff/140001/chrome/browser/guest_vi... File chrome/browser/guest_view/extension_options/extension_options_guest.cc (right): https://codereview.chromium.org/453613002/diff/140001/chrome/browser/guest_vi... chrome/browser/guest_view/extension_options/extension_options_guest.cc:68: if (extension_id != embedder_extension_id) { On 2014/08/12 00:08:23, kalman wrote: > how about: > > if (extensions::Extension::IdIsValid(embedder_extension_id) && > extension_id != embedder_extension_id) { > // Extensions cannot embed other extensions' options pages. > callback.Run(NULL); > return; > } Done. https://codereview.chromium.org/453613002/diff/140001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extensions_ui.cc (right): https://codereview.chromium.org/453613002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extensions_ui.cc:86: On 2014/08/12 00:08:23, kalman wrote: > mention why you need this Done. https://codereview.chromium.org/453613002/diff/140001/chrome/common/extension... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/453613002/diff/140001/chrome/common/extension... chrome/common/extensions/api/_api_features.json:384: "channel": "trunk", On 2014/08/12 00:08:23, kalman wrote: > no much point having this channel restriction really, it's internal. If I remove the channel restriction, it gives me this error: "extensionOptionsInternal: Must supply a value for channel or dependencies." Somehow it doesn't stop the guest view from loading though. https://codereview.chromium.org/453613002/diff/140001/chrome/common/extension... chrome/common/extensions/api/_api_features.json:463: "channel": "trunk", On 2014/08/12 00:08:23, kalman wrote: > ditto ditto https://codereview.chromium.org/453613002/diff/140001/chrome/renderer/extensi... File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc (right): https://codereview.chromium.org/453613002/diff/140001/chrome/renderer/extensi... chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:292: if (extension->permissions_data()->HasAPIPermission( On 2014/08/12 00:08:23, kalman wrote: > Add TODO(fsamuel): Use context->GetAvailability("webViewInternal"). Done. https://codereview.chromium.org/453613002/diff/140001/chrome/renderer/extensi... chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:316: if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableAppView) && On 2014/08/12 00:08:24, kalman wrote: > Add TODO(fsamuel): Use context->GetAvailability("appViewInternal"). Done. https://codereview.chromium.org/453613002/diff/140001/chrome/test/data/extens... File chrome/test/data/extensions/webui/can_embed_extension_options.js (right): https://codereview.chromium.org/453613002/diff/140001/chrome/test/data/extens... chrome/test/data/extensions/webui/can_embed_extension_options.js:21: On 2014/08/12 00:08:24, kalman wrote: > delete this line Done.
https://codereview.chromium.org/453613002/diff/140001/chrome/common/extension... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/453613002/diff/140001/chrome/common/extension... chrome/common/extensions/api/_api_features.json:384: "channel": "trunk", On 2014/08/12 00:32:26, ericzeng wrote: > On 2014/08/12 00:08:23, kalman wrote: > > no much point having this channel restriction really, it's internal. > > If I remove the channel restriction, it gives me this error: > "extensionOptionsInternal: Must supply a value for channel or dependencies." > > Somehow it doesn't stop the guest view from loading though. oh damn it. ok, put it back then... I'll fix that some time.
+thestig PTAL at chrome/renderer/chrome_content_renderer_client.cc
lgtm with some nits. https://codereview.chromium.org/453613002/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/api/guest_view/guest_view_internal_api.cc (right): https://codereview.chromium.org/453613002/diff/160001/chrome/browser/extensio... chrome/browser/extensions/api/guest_view/guest_view_internal_api.cc:37: // If this the guest is an <extensionoptions> to be embedded in a WebUI, then "if this the guest is ..." <- did you mean "If the guest is ... " ? https://codereview.chromium.org/453613002/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/extension_webui_apitest.cc (right): https://codereview.chromium.org/453613002/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_webui_apitest.cc:121: extensions::switches::kEnableEmbeddedExtensionOptions); nit: no need for extensions:: in namespace extensions. https://codereview.chromium.org/453613002/diff/160001/chrome/renderer/extensi... File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc (right): https://codereview.chromium.org/453613002/diff/160001/chrome/renderer/extensi... chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:317: // Add TODO(fsamuel): Use context->GetAvailability("appViewInternal"). s/Add // ?
https://codereview.chromium.org/453613002/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/api/guest_view/guest_view_internal_api.cc (right): https://codereview.chromium.org/453613002/diff/160001/chrome/browser/extensio... chrome/browser/extensions/api/guest_view/guest_view_internal_api.cc:37: // If this the guest is an <extensionoptions> to be embedded in a WebUI, then On 2014/08/12 20:00:46, Lei Zhang wrote: > "if this the guest is ..." <- did you mean "If the guest is ... " ? Done. https://codereview.chromium.org/453613002/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/extension_webui_apitest.cc (right): https://codereview.chromium.org/453613002/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_webui_apitest.cc:121: extensions::switches::kEnableEmbeddedExtensionOptions); On 2014/08/12 20:00:46, Lei Zhang wrote: > nit: no need for extensions:: in namespace extensions. Done. https://codereview.chromium.org/453613002/diff/160001/chrome/renderer/extensi... File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc (right): https://codereview.chromium.org/453613002/diff/160001/chrome/renderer/extensi... chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:317: // Add TODO(fsamuel): Use context->GetAvailability("appViewInternal"). On 2014/08/12 20:00:47, Lei Zhang wrote: > s/Add // ? Done.
The CQ bit was checked by ericzeng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ericzeng@chromium.org/453613002/180001
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/453613002/200001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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/453613002/220001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: 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/40898) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/46123) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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/453613002/240001
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/453613002/240001
Message was sent while issue was closed.
Change committed as 289233 |
