Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
138563005:
<webview>: Inject denyWebView into all extensions (Closed)
|
Created:
6 years, 11 months ago by Fady Samuel Modified:
6 years, 10 months ago Reviewers:
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 Visibility:
Public. |
Description<webview>: Inject denyWebView into all extensions
In the denyWebView module, we now create a custom element so the comment in dispatcher.cc
is out of date.
This CL removes the comment and the if statement in question.
BUG=196453
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=248554
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressed comments #
Total comments: 8
Patch Set 3 : Updated comments #Messages
Total messages: 17 (0 generated)
https://codereview.chromium.org/138563005/diff/1/chrome/renderer/extensions/d... File chrome/renderer/extensions/dispatcher.cc (left): https://codereview.chromium.org/138563005/diff/1/chrome/renderer/extensions/d... chrome/renderer/extensions/dispatcher.cc:1195: context_type == Feature::UNBLESSED_EXTENSION_CONTEXT) { it's premature to delete this altogether, you probably just want to replace these two checks with an "if (extension)" https://codereview.chromium.org/138563005/diff/1/chrome/renderer/extensions/d... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/138563005/diff/1/chrome/renderer/extensions/d... chrome/renderer/extensions/dispatcher.cc:1209: if (includeExperimental) include_experimental, though, why isn't this checked first and the code above executed in an "else"? and include_experimental just inlined?
PTAL https://codereview.chromium.org/138563005/diff/1/chrome/renderer/extensions/d... File chrome/renderer/extensions/dispatcher.cc (left): https://codereview.chromium.org/138563005/diff/1/chrome/renderer/extensions/d... chrome/renderer/extensions/dispatcher.cc:1195: context_type == Feature::UNBLESSED_EXTENSION_CONTEXT) { On 2014/01/14 23:58:49, kalman wrote: > it's premature to delete this altogether, you probably just want to replace > these two checks with an "if (extension)" Done. https://codereview.chromium.org/138563005/diff/1/chrome/renderer/extensions/d... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/138563005/diff/1/chrome/renderer/extensions/d... chrome/renderer/extensions/dispatcher.cc:1209: if (includeExperimental) On 2014/01/14 23:58:49, kalman wrote: > include_experimental, though, why isn't this checked first and the code above > executed in an "else"? and include_experimental just inlined? I think I capture what you're suggesting here. Does this look OK to you? Thanks.
sorry I lost this one. https://codereview.chromium.org/138563005/diff/60001/chrome/renderer/extensio... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/138563005/diff/60001/chrome/renderer/extensio... chrome/renderer/extensions/dispatcher.cc:1197: if (extension->HasAPIPermission(APIPermission::kWebView)) { If I'm reading this correctly... you can merge this condition with the outer conditional? if (extension && extension->HasAPIPermission(APIPermission::kWebView)) { module_system->Require("webView"); } https://codereview.chromium.org/138563005/diff/60001/chrome/renderer/extensio... chrome/renderer/extensions/dispatcher.cc:1200: module_system->Require("webViewExperimental"); (is there any way to (eventually, not now obviously) only set up the webview bindings after a <webview> element is detected? all this setup is pretty wasteful in the common case.) https://codereview.chromium.org/138563005/diff/60001/chrome/renderer/extensio... chrome/renderer/extensions/dispatcher.cc:1206: id_hash.length()); indentation https://codereview.chromium.org/138563005/diff/60001/chrome/renderer/extensio... chrome/renderer/extensions/dispatcher.cc:1213: module_system->Require("denyWebView"); err I don't understand. this code is executed even after requiring 'webView', which is weird, why is webview *and* denyWebView being injected?
PTAL https://codereview.chromium.org/138563005/diff/60001/chrome/renderer/extensio... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/138563005/diff/60001/chrome/renderer/extensio... chrome/renderer/extensions/dispatcher.cc:1197: if (extension->HasAPIPermission(APIPermission::kWebView)) { On 2014/01/28 19:08:45, kalman wrote: > If I'm reading this correctly... you can merge this condition with the outer > conditional? > > if (extension && extension->HasAPIPermission(APIPermission::kWebView)) { > module_system->Require("webView"); > } Done. https://codereview.chromium.org/138563005/diff/60001/chrome/renderer/extensio... chrome/renderer/extensions/dispatcher.cc:1200: module_system->Require("webViewExperimental"); On 2014/01/28 19:08:45, kalman wrote: > (is there any way to (eventually, not now obviously) only set up the webview > bindings after a <webview> element is detected? all this setup is pretty > wasteful in the common case.) I'm not aware of anyway to do that. <webview> is a custom element so I would imagine it's fairly light-weight to inject (versus using MutationObservers which is what we were doing previously). https://codereview.chromium.org/138563005/diff/60001/chrome/renderer/extensio... chrome/renderer/extensions/dispatcher.cc:1206: id_hash.length()); On 2014/01/28 19:08:45, kalman wrote: > indentation Done. https://codereview.chromium.org/138563005/diff/60001/chrome/renderer/extensio... chrome/renderer/extensions/dispatcher.cc:1213: module_system->Require("denyWebView"); On 2014/01/28 19:08:45, kalman wrote: > err I don't understand. this code is executed even after requiring 'webView', > which is weird, why is webview *and* denyWebView being injected? Wow, that's a silly refactor bug :P Thanks for catching that.
lgtm
The CQ bit was checked by fsamuel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fsamuel@chromium.org/138563005/110001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_clang for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
The CQ bit was checked by fsamuel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fsamuel@chromium.org/138563005/110001
Message was sent while issue was closed.
Change committed as 248554
Message was sent while issue was closed.
CQ bit was unchecked on CL. Ignoring.
Message was sent while issue was closed.
CQ bit was unchecked on CL. Ignoring. |
