Bug 86555 - [V8] Shadow DOM should be per-window-configurable.
Summary: [V8] Shadow DOM should be per-window-configurable.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on:
Blocks: 86984 87063 87086
  Show dependency treegraph
 
Reported: 2012-05-15 18:21 PDT by Hajime Morrita
Modified: 2012-05-21 23:29 PDT (History)
7 users (show)

See Also:


Attachments
WIP (8.14 KB, patch)
2012-05-16 04:41 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (17.87 KB, patch)
2012-05-20 22:13 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (17.92 KB, patch)
2012-05-21 01:34 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (17.94 KB, patch)
2012-05-21 01:36 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (24.26 KB, patch)
2012-05-21 03:06 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (36.26 KB, patch)
2012-05-21 19:38 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch for landing (36.26 KB, patch)
2012-05-21 20:18 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch for landing (36.26 KB, patch)
2012-05-21 20:20 PDT, Hajime Morrita
morrita: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2012-05-15 18:21:22 PDT
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.
Comment 1 Adam Barth 2012-05-15 18:31:19 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.
Comment 2 Hajime Morrita 2012-05-16 04:41:55 PDT
Created attachment 142224 [details]
WIP
Comment 3 Hajime Morrita 2012-05-16 04:47:24 PDT
(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.
Comment 4 Adam Barth 2012-05-16 10:32:21 PDT
They can and many do.
Comment 5 Hajime Morrita 2012-05-20 18:02:26 PDT
(In reply to comment #4)
> They can and many do.
Hmm. I need to change the direction...
Comment 6 Hajime Morrita 2012-05-20 22:13:23 PDT
Created attachment 142944 [details]
Patch
Comment 7 Hajime Morrita 2012-05-20 22:15:18 PDT
Hi Adam and Dimitri, could you take a look?
I'm going to extend WebPermissionClient to disclose shadow stuff only for chrome:// pages.
Comment 8 Adam Barth 2012-05-20 22:24:44 PDT
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?
Comment 9 Adam Barth 2012-05-20 22:25:35 PDT
This approach seems plausible.  @haraken: Would you be willing to take a look?
Comment 10 Build Bot 2012-05-20 22:44:37 PDT
Comment on attachment 142944 [details]
Patch

Attachment 142944 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12746009
Comment 11 Kentaro Hara 2012-05-20 22:45:04 PDT
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?
Comment 12 Hajime Morrita 2012-05-21 01:34:32 PDT
Created attachment 142965 [details]
Patch
Comment 13 Hajime Morrita 2012-05-21 01:36:58 PDT
Created attachment 142966 [details]
Patch
Comment 14 Hajime Morrita 2012-05-21 03:06:17 PDT
Created attachment 142979 [details]
Patch
Comment 15 Hajime Morrita 2012-05-21 03:07:57 PDT
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 16 Dimitri Glazkov (Google) 2012-05-21 09:01:32 PDT
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?
Comment 17 Hajime Morrita 2012-05-21 19:38:14 PDT
Created attachment 143173 [details]
Patch
Comment 18 Hajime Morrita 2012-05-21 19:39:45 PDT
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 19 Kentaro Hara 2012-05-21 19:44:19 PDT
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
Comment 20 Hajime Morrita 2012-05-21 20:18:51 PDT
Created attachment 143178 [details]
Patch for landing
Comment 21 Hajime Morrita 2012-05-21 20:20:52 PDT
Created attachment 143179 [details]
Patch for landing
Comment 22 Hajime Morrita 2012-05-21 20:27:58 PDT
Thanks!

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

Done.
Comment 23 Hajime Morrita 2012-05-21 21:18:32 PDT
Committed r117898: <http://trac.webkit.org/changeset/117898>