Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23653012:
Support webview tag in component extension (Closed)
|
Created:
7 years, 3 months ago by guohui Modified:
7 years, 3 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, Roger Tawa OOO till Jul 10th Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSupport webview tag in component extension
This CL only implements the case when the extension is loaded in the main frame. It does not yet support the case when the extension is embedded in a subframe.
https://docs.google.com/a/google.com/document/d/1LcriMmLY76IUhFO5rWh3Tz6xDFkGPNnL4MYSIBD4Jbc/edit#
BUG=285151
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=224894
Patch Set 1 #
Total comments: 7
Patch Set 2 : allow webview in unblessed ext context #Patch Set 3 : comments added #Patch Set 4 : add whitelist #
Total comments: 2
Patch Set 5 : Restored check for blessed ontext #
Total comments: 12
Patch Set 6 : only inject denyWebView for platform apps #
Total comments: 2
Patch Set 7 : Remove duplicate call for enableCustomElement #
Messages
Total messages: 33 (0 generated)
Hello guys, This CL is ready for initial review. Thanks, Hui
I'm not comfortable landing this until we have a solid plan for how to safely support webviews in WebUI pages. Once that's in place, this CL will probably be fine. https://codereview.chromium.org/23653012/diff/1/chrome/renderer/extensions/di... File chrome/renderer/extensions/dispatcher.cc (left): https://codereview.chromium.org/23653012/diff/1/chrome/renderer/extensions/di... chrome/renderer/extensions/dispatcher.cc:481: if (IsWithinPlatformApp()) Removing this seems concerning. We don't want the whitelist in every extension, do we?
https://codereview.chromium.org/23653012/diff/1/chrome/renderer/extensions/di... File chrome/renderer/extensions/dispatcher.cc (left): https://codereview.chromium.org/23653012/diff/1/chrome/renderer/extensions/di... chrome/renderer/extensions/dispatcher.cc:481: if (IsWithinPlatformApp()) On 2013/09/04 17:47:29, creis wrote: > Removing this seems concerning. We don't want the whitelist in every extension, > do we? In an earlier prototype CL, Fady mentioned that it may be ok to remove the check (and the one below), because document.register is no longer exposed. @fady, could you please confirm?
https://codereview.chromium.org/23653012/diff/1/chrome/renderer/extensions/di... File chrome/renderer/extensions/dispatcher.cc (left): https://codereview.chromium.org/23653012/diff/1/chrome/renderer/extensions/di... chrome/renderer/extensions/dispatcher.cc:481: if (IsWithinPlatformApp()) On 2013/09/05 19:35:04, guohui wrote: > On 2013/09/04 17:47:29, creis wrote: > > Removing this seems concerning. We don't want the whitelist in every > extension, > > do we? > > In an earlier prototype CL, Fady mentioned that it may be ok to remove the check > (and the one below), because document.register is no longer exposed. > > @fady, could you please confirm? Yea, document_natives is not exposed to third party developers.
https://codereview.chromium.org/23653012/diff/1/chrome/renderer/extensions/di... File chrome/renderer/extensions/dispatcher.cc (left): https://codereview.chromium.org/23653012/diff/1/chrome/renderer/extensions/di... chrome/renderer/extensions/dispatcher.cc:481: if (IsWithinPlatformApp()) On 2013/09/09 16:30:49, Fady Samuel wrote: > On 2013/09/05 19:35:04, guohui wrote: > > On 2013/09/04 17:47:29, creis wrote: > > > Removing this seems concerning. We don't want the whitelist in every > > extension, > > > do we? > > > > In an earlier prototype CL, Fady mentioned that it may be ok to remove the > check > > (and the one below), because document.register is no longer exposed. > > > > @fady, could you please confirm? > > Yea, document_natives is not exposed to third party developers. thanks fady, then is there any other unwanted side effect by removing this check?
https://codereview.chromium.org/23653012/diff/1/chrome/renderer/extensions/di... File chrome/renderer/extensions/dispatcher.cc (left): https://codereview.chromium.org/23653012/diff/1/chrome/renderer/extensions/di... chrome/renderer/extensions/dispatcher.cc:481: if (IsWithinPlatformApp()) On 2013/09/09 16:43:00, guohui wrote: > On 2013/09/09 16:30:49, Fady Samuel wrote: > > On 2013/09/05 19:35:04, guohui wrote: > > > On 2013/09/04 17:47:29, creis wrote: > > > > Removing this seems concerning. We don't want the whitelist in every > > > extension, > > > > do we? > > > > > > In an earlier prototype CL, Fady mentioned that it may be ok to remove the > > check > > > (and the one below), because document.register is no longer exposed. > > > > > > @fady, could you please confirm? > > > > Yea, document_natives is not exposed to third party developers. > > thanks fady, then is there any other unwanted side effect by removing this > check? None come to mind, but I'm not entirely certain. It's worth confirming with dominicc@. WebKit::WebRuntimeFeatures::enableEmbedderCustomElements(true); ought to be OK even in extensions. It doesn't enable document.register. It just enables creating custom elements via custom bindings: DocumentNatives.RegisterElement.
On 2013/09/09 17:35:04, Fady Samuel wrote: > https://codereview.chromium.org/23653012/diff/1/chrome/renderer/extensions/di... > File chrome/renderer/extensions/dispatcher.cc (left): > > https://codereview.chromium.org/23653012/diff/1/chrome/renderer/extensions/di... > chrome/renderer/extensions/dispatcher.cc:481: if (IsWithinPlatformApp()) > On 2013/09/09 16:43:00, guohui wrote: > > On 2013/09/09 16:30:49, Fady Samuel wrote: > > > On 2013/09/05 19:35:04, guohui wrote: > > > > On 2013/09/04 17:47:29, creis wrote: > > > > > Removing this seems concerning. We don't want the whitelist in every > > > > extension, > > > > > do we? > > > > > > > > In an earlier prototype CL, Fady mentioned that it may be ok to remove the > > > check > > > > (and the one below), because document.register is no longer exposed. > > > > > > > > @fady, could you please confirm? > > > > > > Yea, document_natives is not exposed to third party developers. > > > > thanks fady, then is there any other unwanted side effect by removing this > > check? > > None come to mind, but I'm not entirely certain. It's worth confirming with > dominicc@. > > WebKit::WebRuntimeFeatures::enableEmbedderCustomElements(true); ought to be OK > even in extensions. It doesn't enable document.register. It just enables > creating custom elements via custom bindings: DocumentNatives.RegisterElement. +dominicc as suggested by Fady, could you please take a look at Fady's comment in dispatcher.cc? Thanks!
I think that this is probably fine, see comments inline. https://codereview.chromium.org/23653012/diff/1/chrome/renderer/extensions/di... File chrome/renderer/extensions/dispatcher.cc (left): https://codereview.chromium.org/23653012/diff/1/chrome/renderer/extensions/di... chrome/renderer/extensions/dispatcher.cc:481: if (IsWithinPlatformApp()) On 2013/09/09 17:35:05, Fady Samuel wrote: > On 2013/09/09 16:43:00, guohui wrote: > > On 2013/09/09 16:30:49, Fady Samuel wrote: > > > On 2013/09/05 19:35:04, guohui wrote: > > > > On 2013/09/04 17:47:29, creis wrote: > > > > > Removing this seems concerning. We don't want the whitelist in every > > > > extension, > > > > > do we? > > > > > > > > In an earlier prototype CL, Fady mentioned that it may be ok to remove the > > > check > > > > (and the one below), because document.register is no longer exposed. > > > > > > > > @fady, could you please confirm? > > > > > > Yea, document_natives is not exposed to third party developers. > > > > thanks fady, then is there any other unwanted side effect by removing this > > check? > > None come to mind, but I'm not entirely certain. It's worth confirming with > dominicc@. > > WebKit::WebRuntimeFeatures::enableEmbedderCustomElements(true); ought to be OK > even in extensions. It doesn't enable document.register. It just enables > creating custom elements via custom bindings: DocumentNatives.RegisterElement. The side effect will be that any element names you're using (eg webview) will have HTMLElement as their interface instead of HTMLUnknownElement. This is detectable (by examining the prototype or doing instanceof) but since HTMLUnknownElement doesn't add any extra operations or attributes, it probably won't break much. Just out of curiosity, is there any guarantee that the setup code runs before web content?
https://codereview.chromium.org/23653012/diff/1/chrome/renderer/extensions/di... File chrome/renderer/extensions/dispatcher.cc (left): https://codereview.chromium.org/23653012/diff/1/chrome/renderer/extensions/di... chrome/renderer/extensions/dispatcher.cc:481: if (IsWithinPlatformApp()) On 2013/09/10 02:50:49, dominicc wrote: > On 2013/09/09 17:35:05, Fady Samuel wrote: > > On 2013/09/09 16:43:00, guohui wrote: > > > On 2013/09/09 16:30:49, Fady Samuel wrote: > > > > On 2013/09/05 19:35:04, guohui wrote: > > > > > On 2013/09/04 17:47:29, creis wrote: > > > > > > Removing this seems concerning. We don't want the whitelist in every > > > > > extension, > > > > > > do we? > > > > > > > > > > In an earlier prototype CL, Fady mentioned that it may be ok to remove > the > > > > check > > > > > (and the one below), because document.register is no longer exposed. > > > > > > > > > > @fady, could you please confirm? > > > > > > > > Yea, document_natives is not exposed to third party developers. > > > > > > thanks fady, then is there any other unwanted side effect by removing this > > > check? > > > > None come to mind, but I'm not entirely certain. It's worth confirming with > > dominicc@. > > > > WebKit::WebRuntimeFeatures::enableEmbedderCustomElements(true); ought to be OK > > even in extensions. It doesn't enable document.register. It just enables > > creating custom elements via custom bindings: DocumentNatives.RegisterElement. > > The side effect will be that any element names you're using (eg webview) will > have HTMLElement as their interface instead of HTMLUnknownElement. > > This is detectable (by examining the prototype or doing instanceof) but since > HTMLUnknownElement doesn't add any extra operations or attributes, it probably > won't break much. > > Just out of curiosity, is there any guarantee that the setup code runs before > web content? today EnableCustomElementWhiteList is called in two places WebKitInitialized and OnActivateExtension. I think WebKitInitialized is always called before OnActivateExtension and before any web content is rendered, thus it is guaranteed that the setup code runs b4 web content and we don't really need to call EnableCustomElementWhiteList in OnActivateExtension. @fady, what do you think?
On 2013/09/12 20:27:26, guohui wrote: > https://codereview.chromium.org/23653012/diff/1/chrome/renderer/extensions/di... > File chrome/renderer/extensions/dispatcher.cc (left): > > https://codereview.chromium.org/23653012/diff/1/chrome/renderer/extensions/di... > chrome/renderer/extensions/dispatcher.cc:481: if (IsWithinPlatformApp()) > On 2013/09/10 02:50:49, dominicc wrote: > > On 2013/09/09 17:35:05, Fady Samuel wrote: > > > On 2013/09/09 16:43:00, guohui wrote: > > > > On 2013/09/09 16:30:49, Fady Samuel wrote: > > > > > On 2013/09/05 19:35:04, guohui wrote: > > > > > > On 2013/09/04 17:47:29, creis wrote: > > > > > > > Removing this seems concerning. We don't want the whitelist in > every > > > > > > extension, > > > > > > > do we? > > > > > > > > > > > > In an earlier prototype CL, Fady mentioned that it may be ok to remove > > the > > > > > check > > > > > > (and the one below), because document.register is no longer exposed. > > > > > > > > > > > > @fady, could you please confirm? > > > > > > > > > > Yea, document_natives is not exposed to third party developers. > > > > > > > > thanks fady, then is there any other unwanted side effect by removing this > > > > check? > > > > > > None come to mind, but I'm not entirely certain. It's worth confirming with > > > dominicc@. > > > > > > WebKit::WebRuntimeFeatures::enableEmbedderCustomElements(true); ought to be > OK > > > even in extensions. It doesn't enable document.register. It just enables > > > creating custom elements via custom bindings: > DocumentNatives.RegisterElement. > > > > The side effect will be that any element names you're using (eg webview) will > > have HTMLElement as their interface instead of HTMLUnknownElement. > > > > This is detectable (by examining the prototype or doing instanceof) but since > > HTMLUnknownElement doesn't add any extra operations or attributes, it probably > > won't break much. > > > > Just out of curiosity, is there any guarantee that the setup code runs before > > web content? > today EnableCustomElementWhiteList is called in two places WebKitInitialized and > OnActivateExtension. I think WebKitInitialized is always called before > OnActivateExtension and before any web content is rendered, thus it is > guaranteed that the setup code runs b4 web content and we don't really need to > call EnableCustomElementWhiteList in OnActivateExtension. > > @fady, what do you think? ping?
> ping? At least from my perspective, I still think we need to finish https://codereview.chromium.org/23530029/ first, since it's not safe yet to allow webview in component extensions. For example, we'll need to enforce that the extension is storage isolated, or else we'll start creating guest partitions in the default profile directory. I defer on the custom element whitelist stuff, since I'm not familiar with that.
On 2013/09/13 21:19:59, creis wrote: > > ping? > > At least from my perspective, I still think we need to finish > https://codereview.chromium.org/23530029/ first, since it's not safe yet to > allow webview in component extensions. For example, we'll need to enforce that > the extension is storage isolated, or else we'll start creating guest partitions > in the default profile directory. BrowserPlugins always use an isolated storage under path Storage/ext/[ext-id]/[partition-id], regardless of the isolation setting of the embedder extension. As discussed with Charlie offline, it would cause issues when garbage collection runs and deletes all partitions that are not associated with an isolated app. This is not a concern for our use case, since GC for isolated storage only runs upon startup and we do not plan to persist any data. However, if we do want to support this case, i wonder if we could simply modify ExtensionService::GarbageCollectIsolatedStorage to retain partitions for an extension with webview permission. > > I defer on the custom element whitelist stuff, since I'm not familiar with that. Both fady and dominic have approved the whitelist stuff, so we should be fine.
On 2013/09/16 22:25:50, guohui wrote: > On 2013/09/13 21:19:59, creis wrote: > > > ping? > > > > At least from my perspective, I still think we need to finish > > https://codereview.chromium.org/23530029/ first, since it's not safe yet to > > allow webview in component extensions. For example, we'll need to enforce > that > > the extension is storage isolated, or else we'll start creating guest > partitions > > in the default profile directory. > BrowserPlugins always use an isolated storage under path > Storage/ext/[ext-id]/[partition-id], regardless of the isolation setting of the > embedder extension. > > As discussed with Charlie offline, it would cause issues when garbage collection > runs and deletes all partitions that are not associated with an isolated app. > This is not a concern for our use case, since GC for isolated storage only runs > upon startup and we do not plan to persist any data. Glad it works in your use case, but it still poses a risk if other component extensions try to use this new support. We filed http://crbug.com/293722 to make sure it can work in general. > However, if we do want to > support this case, i wonder if we could simply modify > ExtensionService::GarbageCollectIsolatedStorage to retain partitions for an > extension with webview permission. Yep, sounds about right. (Not sure whether StoragePartitionImpl's DataDeletionHelper is also relevant.) From our discussion, it sounded like this might be just a backup approach in case https://codereview.chromium.org/23530029/ runs into problems. If that one lands, do we need this at all?
On 2013/09/18 17:47:56, creis wrote: > On 2013/09/16 22:25:50, guohui wrote: > > On 2013/09/13 21:19:59, creis wrote: > > > > ping? > > > > > > At least from my perspective, I still think we need to finish > > > https://codereview.chromium.org/23530029/ first, since it's not safe yet to > > > allow webview in component extensions. For example, we'll need to enforce > > that > > > the extension is storage isolated, or else we'll start creating guest > > partitions > > > in the default profile directory. > > BrowserPlugins always use an isolated storage under path > > Storage/ext/[ext-id]/[partition-id], regardless of the isolation setting of > the > > embedder extension. > > > > As discussed with Charlie offline, it would cause issues when garbage > collection > > runs and deletes all partitions that are not associated with an isolated app. > > This is not a concern for our use case, since GC for isolated storage only > runs > > upon startup and we do not plan to persist any data. > > Glad it works in your use case, but it still poses a risk if other component > extensions try to use this new support. We filed http://crbug.com/293722 to > make sure it can work in general. > > > However, if we do want to > > support this case, i wonder if we could simply modify > > ExtensionService::GarbageCollectIsolatedStorage to retain partitions for an > > extension with webview permission. Since you have already filed a bug for this, and we don't need it for our use case, can we address this issue in a separate CL? Uploaded a new patch that allows this feature for whitelisted component exts only until all the caveats are addressed. > > Yep, sounds about right. (Not sure whether StoragePartitionImpl's > DataDeletionHelper is also relevant.) > > From our discussion, it sounded like this might be just a backup approach in > case https://codereview.chromium.org/23530029/ runs into problems. If that one > lands, do we need this at all? we always need this CL regardless of whether 23530029 is landed. This CL alone supports webview in a standalone component ext, combined with 23530029 it support webview in an iframed component ext.
On 2013/09/19 13:19:12, guohui wrote: > On 2013/09/18 17:47:56, creis wrote: > > On 2013/09/16 22:25:50, guohui wrote: > > > On 2013/09/13 21:19:59, creis wrote: > > > > > ping? > > > > > > > > At least from my perspective, I still think we need to finish > > > > https://codereview.chromium.org/23530029/ first, since it's not safe yet > to > > > > allow webview in component extensions. For example, we'll need to enforce > > > that > > > > the extension is storage isolated, or else we'll start creating guest > > > partitions > > > > in the default profile directory. > > > BrowserPlugins always use an isolated storage under path > > > Storage/ext/[ext-id]/[partition-id], regardless of the isolation setting of > > the > > > embedder extension. > > > > > > As discussed with Charlie offline, it would cause issues when garbage > > collection > > > runs and deletes all partitions that are not associated with an isolated > app. > > > This is not a concern for our use case, since GC for isolated storage only > > runs > > > upon startup and we do not plan to persist any data. > > > > Glad it works in your use case, but it still poses a risk if other component > > extensions try to use this new support. We filed http://crbug.com/293722 to > > make sure it can work in general. > > > > > However, if we do want to > > > support this case, i wonder if we could simply modify > > > ExtensionService::GarbageCollectIsolatedStorage to retain partitions for an > > > extension with webview permission. > Since you have already filed a bug for this, and we don't need it for our use > case, can we address this issue in a separate CL? > > Uploaded a new patch that allows this feature for whitelisted component exts > only until all the caveats are addressed. Yes, this CL can proceed without the fix http://crbug.com/293722 because it now only applies to the whitelisted extension. Note that you may still see unexpected StoragePartition problems until 293722 is fixed, but it seems safe enough to proceed. > > > > > Yep, sounds about right. (Not sure whether StoragePartitionImpl's > > DataDeletionHelper is also relevant.) > > > > From our discussion, it sounded like this might be just a backup approach in > > case https://codereview.chromium.org/23530029/ runs into problems. If that > one > > lands, do we need this at all? > > we always need this CL regardless of whether 23530029 is landed. This CL alone > supports webview in a standalone component ext, combined with 23530029 it > support webview in an iframed component ext. Ah, sorry for the misunderstanding. https://codereview.chromium.org/23653012/diff/24001/chrome/renderer/extension... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/23653012/diff/24001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:1118: // top frame. To support webview in iframed extensions, we have to drop the I don't understand why this is in this CL. The CL description and your most recent comments say that this CL is about supporting webview within a component extension in a main frame, not within iframes. That makes me think that this particular change belongs in https://codereview.chromium.org/23530029/.
https://codereview.chromium.org/23653012/diff/24001/chrome/renderer/extension... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/23653012/diff/24001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:1118: // top frame. To support webview in iframed extensions, we have to drop the On 2013/09/19 17:04:41, creis wrote: > I don't understand why this is in this CL. The CL description and your most > recent comments say that this CL is about supporting webview within a component > extension in a main frame, not within iframes. That makes me think that this > particular change belongs in https://codereview.chromium.org/23530029/. agree, Fady has pointed out the same thing in a offline chat, will refactor this into a new CL. Thanks!
On 2013/09/19 17:50:32, guohui1 wrote: > https://codereview.chromium.org/23653012/diff/24001/chrome/renderer/extension... > File chrome/renderer/extensions/dispatcher.cc (right): > > https://codereview.chromium.org/23653012/diff/24001/chrome/renderer/extension... > chrome/renderer/extensions/dispatcher.cc:1118: // top frame. To support webview > in iframed extensions, we have to drop the > On 2013/09/19 17:04:41, creis wrote: > > I don't understand why this is in this CL. The CL description and your most > > recent comments say that this CL is about supporting webview within a > component > > extension in a main frame, not within iframes. That makes me think that this > > particular change belongs in https://codereview.chromium.org/23530029/. > > agree, Fady has pointed out the same thing in a offline chat, will refactor this > into a new CL. > > Thanks! @charlie, any remaining concern about this CL?
LGTM
On 2013/09/20 16:00:50, creis wrote: > LGTM +kalman for owner review
what's the gaia component extension? https://codereview.chromium.org/23653012/diff/31001/chrome/renderer/extension... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/23653012/diff/31001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:482: EnableCustomElementWhiteList(); could you guard this with the webview permission check? https://codereview.chromium.org/23653012/diff/31001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:1142: module_system->Require("denyWebView"); I particularly like that we're injecting this into every extension context now; do we still need denyWebView or does the custom element whitelist cover that? @fady https://codereview.chromium.org/23653012/diff/31001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:1253: EnableCustomElementWhiteList(); likewise
https://codereview.chromium.org/23653012/diff/31001/chrome/renderer/extension... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/23653012/diff/31001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:1142: module_system->Require("denyWebView"); On 2013/09/20 16:44:34, kalman wrote: > I particularly like that we're injecting this into every extension context now; > do we still need denyWebView or does the custom element whitelist cover that? > > @fady Err. *don't* particularly.
https://codereview.chromium.org/23653012/diff/31001/chrome/renderer/extension... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/23653012/diff/31001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:482: EnableCustomElementWhiteList(); On 2013/09/20 16:44:34, kalman wrote: > could you guard this with the webview permission check? But we don't have the extension ID here to check for webview perission. And as commented by dominic, the only side effect is that webview "will have HTMLElement as their interface instead of HTMLUnknownElement".
https://codereview.chromium.org/23653012/diff/31001/chrome/renderer/extension... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/23653012/diff/31001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:1142: module_system->Require("denyWebView"); On 2013/09/20 16:45:25, kalman wrote: > On 2013/09/20 16:44:34, kalman wrote: > > I particularly like that we're injecting this into every extension context > now; > > do we still need denyWebView or does the custom element whitelist cover that? > > > > @fady > > Err. *don't* particularly. as discussed offline with Fady, the purpose of denyWebView module is to show a console error when webview tag is used without proper permission. Since it does have performance issue, we should only inject it for platform apps instead of all extensions, until the perf issue is fixed with custom tag. Fixed in a new patch.
https://codereview.chromium.org/23653012/diff/31001/chrome/renderer/extension... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/23653012/diff/31001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:416: kInitialExtensionIdleHandlerDelayMs); ... however, if you do want to just always set the custom element whitelist, you might as well do it here. https://codereview.chromium.org/23653012/diff/31001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:482: EnableCustomElementWhiteList(); On 2013/09/23 18:34:10, guohui wrote: > On 2013/09/20 16:44:34, kalman wrote: > > could you guard this with the webview permission check? > > But we don't have the extension ID here to check for webview perission. > > And as commented by dominic, the only side effect is that webview "will have > HTMLElement as their interface instead of HTMLUnknownElement". A few lines above we loop over the extensions; you could initialize the custom element whitelist from within there. https://codereview.chromium.org/23653012/diff/31001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:1253: EnableCustomElementWhiteList(); On 2013/09/20 16:44:34, kalman wrote: > likewise ... and here, we do know what the extension is.
https://codereview.chromium.org/23653012/diff/31001/chrome/renderer/extension... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/23653012/diff/31001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:416: kInitialExtensionIdleHandlerDelayMs); On 2013/09/23 20:59:12, kalman wrote: > ... however, if you do want to just always set the custom element whitelist, you > might as well do it here. i think this will only work for an active extension, but not for extensions iframed in WebUI. I understand the iframe case is covered in other CLs, but prefer not to introduce code that we know will break for iframe case unless necessary. https://codereview.chromium.org/23653012/diff/31001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:482: EnableCustomElementWhiteList(); On 2013/09/23 20:59:12, kalman wrote: > On 2013/09/23 18:34:10, guohui wrote: > > On 2013/09/20 16:44:34, kalman wrote: > > > could you guard this with the webview permission check? > > > > But we don't have the extension ID here to check for webview perission. > > > > And as commented by dominic, the only side effect is that webview "will have > > HTMLElement as their interface instead of HTMLUnknownElement". > > A few lines above we loop over the extensions; you could initialize the custom > element whitelist from within there. but we only loop over "active" extensions above, and thus it will not work for extensions iframed in webUI. https://codereview.chromium.org/23653012/diff/31001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:1253: EnableCustomElementWhiteList(); On 2013/09/23 20:59:12, kalman wrote: > On 2013/09/20 16:44:34, kalman wrote: > > likewise > > ... and here, we do know what the extension is. yes i could add webview check here, though i think if we always call EnableCustomElementWhiteList in WebkitInitialized above, then we don't need to call it again here.
https://codereview.chromium.org/23653012/diff/41001/chrome/renderer/extension... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/23653012/diff/41001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:1120: module_system->Require("webView"); (answering all comments here) Ah I see, you want it for iframes as well. Sorry for not realising given the other CL I saw about this. Could you just put the enablement here then? Given that it seems that webkit doesn't care whether that method is being called more than once?
https://codereview.chromium.org/23653012/diff/41001/chrome/renderer/extension... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/23653012/diff/41001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:1120: module_system->Require("webView"); On 2013/09/23 21:24:26, kalman wrote: > (answering all comments here) > > Ah I see, you want it for iframes as well. Sorry for not realising given the > other CL I saw about this. > > Could you just put the enablement here then? Given that it seems that webkit > doesn't care whether that method is being called more than once? unfortunately it is too late here, the setup code for webview element must be executed before any web content is shown, so WebKitInitialized does seem to be the right place to put the call. Do you have any concern why it should not be put there? According to dominicc and fady, there is not much side effect of always calling enableCustomElement except that the webview tag will always have an interface of HTMLElement rather than HTMLUnknownElement, the tag will still be blocked if not embedded in an extension with proper permission.
cool. no I have no problems with it. why does it need to be in activateextension though? is webkit not enough?
On 2013/09/23 22:42:44, kalman wrote: > cool. no I have no problems with it. why does it need to be in activateextension > though? is webkit not enough? right, if you agree with the change that always calls enableCustomElement in webkitInitailized, then there is no need to call it again in activateExtension.
lgtm
On 2013/09/23 22:48:50, kalman wrote: > lgtm thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/guohui@chromium.org/23653012/52001
Message was sent while issue was closed.
Change committed as 224894 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
