Bug 86555 – [V8] Shadow DOM should be per-window-configurable.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Hajime Morrita]

Currently constructors can be turned on/off based on RuntimeEnabledFeatures. 
But sometimes it is desirable if the such switch can be pulled from Settings instead of RuntimeEnabledFeatures.

A rough idea is that we could interrupt the constructor creation to see the flag.
I'm taking a closer look.

[Adam Barth]

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.

[Hajime Morrita]

Created attachment 142224 [details]
WIP

[Hajime Morrita]

(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.

[Adam Barth]

They can and many do.

[Hajime Morrita]

(In reply to comment #4)
> They can and many do.
Hmm. I need to change the direction...

[Hajime Morrita]

Created attachment 142944 [details]
Patch

[Hajime Morrita]

Hi Adam and Dimitri, could you take a look?
I'm going to extend WebPermissionClient to disclose shadow stuff only for chrome:// pages.

[Adam Barth]

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?

[Adam Barth]

This approach seems plausible.  @haraken: Would you be willing to take a look?

[Build Bot]

Comment on attachment 142944 [details]
Patch

Attachment 142944 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12746009

[Kentaro Hara]

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?

[Hajime Morrita]

Created attachment 142965 [details]
Patch

[Hajime Morrita]

Created attachment 142966 [details]
Patch

[Hajime Morrita]

Created attachment 142979 [details]
Patch

[Hajime Morrita]

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.

[Dimitri Glazkov (Google)]

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?

[Hajime Morrita]

Created attachment 143173 [details]
Patch

[Hajime Morrita]

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 ;-)

[Kentaro Hara]

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

[Hajime Morrita]

Created attachment 143178 [details]
Patch for landing

[Hajime Morrita]

Created attachment 143179 [details]
Patch for landing

[Hajime Morrita]

Thanks!

> > Source/WebCore/bindings/scripts/IDLAttributes.txt:114
> > +V8EnabledPerContext=*
> 
> Please add the explanation to https://trac.webkit.org/wiki/WebKitIDL

Done.

[Hajime Morrita]

Committed r117898: <http://trac.webkit.org/changeset/117898>