Summary: | [V8] Shadow DOM should be per-window-configurable. | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hajime Morrita <morrita> | ||||||||||||||||||
Component: | WebCore JavaScript | Assignee: | Hajime Morrita <morrita> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, haraken, japhet, ojan, rakuco, webkit.review.bot | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||
Bug Blocks: | 86984, 87063, 87086 | ||||||||||||||||||||
Attachments: |
|
Description
Hajime Morrita
2012-05-15 18:21:22 PDT
I think you're going to need to do it on a v8::Context by v8::Context basis, when the context is created. The tricky part is going to be the boilerplate templates, which you're going to have to figure out a way to have more than on static copy of. Created attachment 142224 [details]
WIP
(In reply to comment #1) > I think you're going to need to do it on a v8::Context by v8::Context basis, when the context is created. The tricky part is going to be the boilerplate templates, which you're going to have to figure out a way to have more than on static copy of. Yeah exactly. I'm trying to factor per-context properties out to a function which injects such properties _after_ object creation. This will work only with DOMWindow. But it should be sufficient for our purpose. Is it possible for chrome extensions to host child frames of wild domains in their chrome:// frame? In that case, we cannot rely on this assumption which uses per-Settings based configuration. They can and many do. (In reply to comment #4) > They can and many do. Hmm. I need to change the direction... Created attachment 142944 [details]
Patch
Hi Adam and Dimitri, could you take a look? I'm going to extend WebPermissionClient to disclose shadow stuff only for chrome:// pages. Comment on attachment 142944 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142944&action=review So, this only works for DOMWindow objects? For other types of objects we'll get in trouble with object templates. > Source/WebCore/bindings/scripts/IDLAttributes.txt:114 > +V8EnabledAtContext=* Can you add a run-bindings-test for this attribute? This approach seems plausible. @haraken: Would you be willing to take a look? Comment on attachment 142944 [details] Patch Attachment 142944 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12746009 Comment on attachment 142944 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142944&action=review The approach looks good to me. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2876 > + foreach my $runtime_attr (@enabledAtContext) { Nit: $runtimeAttr > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2877 > + my $enable_function = GetContextEnableFunction($runtime_attr->signature); Nit: $enableFunction > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3989 > + # Otherwise return a function named {methodName}Allowed(). {methodName} is wrong. >> Source/WebCore/bindings/scripts/IDLAttributes.txt:114 >> +V8EnabledAtContext=* > > Can you add a run-bindings-test for this attribute? "V8EnabledPerDOMWindow" might be clearer? Created attachment 142965 [details]
Patch
Created attachment 142966 [details]
Patch
Created attachment 142979 [details]
Patch
Hi Adam, Haraken, Thanks for taking a look! I updated the patch. (In reply to comment #11) > (From update of attachment 142944 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=142944&action=review > > The approach looks good to me. > > > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2876 > > + foreach my $runtime_attr (@enabledAtContext) { > > Nit: $runtimeAttr Done. > > > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2877 > > + my $enable_function = GetContextEnableFunction($runtime_attr->signature); > > Nit: $enableFunction Done. > > > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3989 > > + # Otherwise return a function named {methodName}Allowed(). > > {methodName} is wrong. Sure. Clarified. > > >> Source/WebCore/bindings/scripts/IDLAttributes.txt:114 > >> +V8EnabledAtContext=* > > > > Can you add a run-bindings-test for this attribute? Sure. Added. > > "V8EnabledPerDOMWindow" might be clearer? I prefer "Context" since the concept isn't limited to the DOMWindow. I generalized the generator no to limit the attribute for DOMWindow to reflect the intension. Comment on attachment 142979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142979&action=review > Source/WebCore/ChangeLog:8 > + This change introduces an IDL attribute named "V8EnabledAtContext" Bikeshedding: V8EnabledPerContext? "at context" sounds awkward. > Source/WebCore/bindings/generic/RuntimeEnabledFeatures.h:229 > + static bool shadowDOMAllowed(DOMWindow*); Sounds like this is no longer a runtime-enabled feature? Should we have a new class ContextEnabledFeatures? Created attachment 143173 [details]
Patch
Hi Dimitri, thanks for the comment! I addressed your point. Haraken, could you rubberstamp this? I'd like to move to the next step during the PST night ;-) Comment on attachment 143173 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143173&action=review rs=me > Source/WebCore/bindings/scripts/IDLAttributes.txt:114 > +V8EnabledPerContext=* Please add the explanation to https://trac.webkit.org/wiki/WebKitIDL Created attachment 143178 [details]
Patch for landing
Created attachment 143179 [details]
Patch for landing
Thanks!
> > Source/WebCore/bindings/scripts/IDLAttributes.txt:114
> > +V8EnabledPerContext=*
>
> Please add the explanation to https://trac.webkit.org/wiki/WebKitIDL
Done.
Committed r117898: <http://trac.webkit.org/changeset/117898> |