Summary: | [V8] [two-sided] Add a way to register V8 extensions for Isolated Worlds only | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Perry <mpcomplete> | ||||||||
Component: | WebCore JavaScript | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
Matt Perry
2009-07-28 15:32:25 PDT
Created attachment 33678 [details] first attempt There is a corresponding Chromium change here: http://codereview.chromium.org/159542 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.
(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? (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. (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? > 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.
(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 :). (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. 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 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!
Created attachment 33734 [details]
fixed up the naming
Comment on attachment 33734 [details]
fixed up the naming
Great. Thanks.
I believe this requires a simultaneous downstream change during landing. Committed http://trac.webkit.org/changeset/46594. |