Bug 27785 - [V8] [two-sided] Add a way to register V8 extensions for Isolated Worlds only
Summary: [V8] [two-sided] Add a way to register V8 extensions for Isolated Worlds only
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-28 15:32 PDT by Matt Perry
Modified: 2009-07-30 13:46 PDT (History)
1 user (show)

See Also:


Attachments
first attempt (8.05 KB, patch)
2009-07-28 15:35 PDT, Matt Perry
abarth: review-
Details | Formatted Diff | Diff
rework of above using an extensionGroup tag (11.97 KB, patch)
2009-07-29 12:44 PDT, Matt Perry
abarth: review-
Details | Formatted Diff | Diff
fixed up the naming (12.03 KB, patch)
2009-07-29 13:02 PDT, Matt Perry
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Perry 2009-07-28 15:32:25 PDT
Right now we can only register for all types of contexts (frames and isolated worlds).  We need to be able to control both separately.
Comment 1 Matt Perry 2009-07-28 15:35:26 PDT
Created attachment 33678 [details]
first attempt

There is a corresponding Chromium change here: http://codereview.chromium.org/159542
Comment 2 Adam Barth 2009-07-28 15:48:10 PDT
Comment on attachment 33678 [details]
first attempt

+        } else if (it->scheme.length() > 0 && (it->scheme != m_frame->loader()->activeDocumentLoader()->url().protocol() || it->scheme != m_frame->page()->mainFrame()->loader()->activeDocumentLoader()->url().protocol())) {

You don't need the "else" here because the "if" clause ends in continue.

+        v8::Persistent<v8::Context> createNewContext(v8::Handle<v8::Object> global, bool isolatedContext);

Adding bools to this function doesn't really scale.  Can we pass in the set of extensions somehow?  You might want a helper function that grabs the proper set of extensions for normal and for isolated worlds.

Also, there's is no such thing as an "isolated context".  There are isolated worlds, which eventually will hold more than one context each.
Comment 3 Matt Perry 2009-07-28 16:02:44 PDT
(In reply to comment #2)
> +        v8::Persistent<v8::Context> createNewContext(v8::Handle<v8::Object>
> global, bool isolatedContext);
> 
> Adding bools to this function doesn't really scale.  Can we pass in the set of
> extensions somehow?  You might want a helper function that grabs the proper set
> of extensions for normal and for isolated worlds.

Doesn't scale how? Do you foresee many other parameters being added here?

> Also, there's is no such thing as an "isolated context".  There are isolated
> worlds, which eventually will hold more than one context each.

I've been using "isolated context" as a generic term for contexts created by either "evaluateInNewContext" or "evaluateInNewWorld". Do you have a suggestion for a better term?
Comment 4 Adam Barth 2009-07-28 16:28:21 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > +        v8::Persistent<v8::Context> createNewContext(v8::Handle<v8::Object>
> > global, bool isolatedContext);
> > 
> > Adding bools to this function doesn't really scale.  Can we pass in the set of
> > extensions somehow?  You might want a helper function that grabs the proper set
> > of extensions for normal and for isolated worlds.
> 
> Doesn't scale how? Do you foresee many other parameters being added here?

There seems to be room for expansion in the rest of the patch:

+ enum IsolatedContextTag { IsolatedContextOnly };

struct V8ExtensionInfo {
    String scheme;
    bool isolatedContextOnly;
    v8::Extension* extension;
};

What if we want to add extensions only for contexts created with certain permissions?  etc.

> > Also, there's is no such thing as an "isolated context".  There are isolated
> > worlds, which eventually will hold more than one context each.
> 
> I've been using "isolated context" as a generic term for contexts created by
> either "evaluateInNewContext" or "evaluateInNewWorld". Do you have a suggestion
> for a better term?

evaluateInNewContext will eventually be removed.  We're keeping it around in case we need to audible back to the old design for some reason.
Comment 5 Matt Perry 2009-07-28 16:55:12 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > +        v8::Persistent<v8::Context> createNewContext(v8::Handle<v8::Object>
> > > global, bool isolatedContext);
> > > 
> > > Adding bools to this function doesn't really scale.  Can we pass in the set of
> > > extensions somehow?  You might want a helper function that grabs the proper set
> > > of extensions for normal and for isolated worlds.
> > 
> > Doesn't scale how? Do you foresee many other parameters being added here?
> 
> There seems to be room for expansion in the rest of the patch:
> 
> + enum IsolatedContextTag { IsolatedContextOnly };

That was more of a way to use the type system to register an extension for a different purpose.

> struct V8ExtensionInfo {
>     String scheme;
>     bool isolatedContextOnly;
>     v8::Extension* extension;
> };
> 
> What if we want to add extensions only for contexts created with certain
> permissions?  etc.

True.  I had another idea that would have addressed this use case.  What about:

  registerExtension(v8::Extension);
  registerExtension(v8::Extension, int contextTypeBitfield);
  evaluateInNewWorld(int contextType);

and then the consumer could group contexts as he pleases.  For content scripts, we would do:

  registerExtension(foo, CONTENT_SCRIPT_CONTEXT);
  registerExtension(bar, CONTENT_SCRIPT_CONTEXT | SOME_OTHER_TYPE);
  registerExtension(baz, SOME_OTHER_TYPE);
  ...
  evaluateInNewWorld(CONTENT_SCRIPT_CONTEXT);  // would enable foo and bar

Thoughts?
Comment 6 Adam Barth 2009-07-28 16:58:38 PDT
> Thoughts?

That seems better because it doesn't tie the isolated world mechanism directly to the extension system.  For example, you could imagine the Web Inspector using an isolated world and not wanting to see the v8extensions appropriate for content scripts.

We might be over engineering.  Perhaps you're right and we should use the minimal mechanism for our current use cases.
Comment 7 Matt Perry 2009-07-28 16:59:40 PDT
(In reply to comment #6)
> > Thoughts?
> 
> We might be over engineering.  Perhaps you're right and we should use the
> minimal mechanism for our current use cases.

That's why I changed my mind and eventually settled on the current patch :).
Comment 8 Matt Perry 2009-07-28 17:44:37 PDT
(In reply to comment #5)
> True.  I had another idea that would have addressed this use case.  What about:
> 
>   registerExtension(v8::Extension);
>   registerExtension(v8::Extension, int contextTypeBitfield);
>   evaluateInNewWorld(int contextType);

I'm going to give this approach a shot. The only weirdness is that the embedder (chromium) would be the one to define the bitfields. That means webkit can't use the bitfield feature on its own, or would have to reserve a bit range.
Comment 9 Matt Perry 2009-07-29 12:44:34 PDT
Created attachment 33733 [details]
rework of above using an extensionGroup tag

Here's the rework using a tag for contexts. You can see how it's used in the chromium code review I linked earlier.
Comment 10 Adam Barth 2009-07-29 12:53:26 PDT
Comment on attachment 33733 [details]
rework of above using an extensionGroup tag

This looks good.  The below are only minor style issues:

1) Sometimes we call the value extensionGroup and sometimes extensionFlags.  We should probably make this consistent.

2) We should put the parameter name in the headers.  Having a bare "int" is very ambiguous.

Thanks!
Comment 11 Matt Perry 2009-07-29 13:02:30 PDT
Created attachment 33734 [details]
fixed up the naming
Comment 12 Adam Barth 2009-07-29 13:12:22 PDT
Comment on attachment 33734 [details]
fixed up the naming

Great.  Thanks.
Comment 13 Adam Barth 2009-07-29 13:23:02 PDT
I believe this requires a simultaneous downstream change during landing.
Comment 14 Matt Perry 2009-07-30 13:46:35 PDT
Committed http://trac.webkit.org/changeset/46594.