Closed
Bug 661980
Opened 14 years ago
Closed 14 years ago
Add support for scriptable-but-not-script-implementable interfaces
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
People
(Reporter: sicking, Assigned: sicking)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, Whiteboard: [sg:want P3])
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
Currently we have no way of making an interface scriptable while not letting script implementing it.
There are several reasons why we would want to do that:
* It's nice to be able to rely on that all instances of the interface is a
"real" implementation. This is very much the case for nsIDOMNode for example
where we constantly have to check that it's one of our nodes.
* The interfaces contains some [notxpcom] methods which the xptstub can't
implement and so if they are called from C++ we'll end up crashing. This
will very soon be the case for nsIDOMEventTarget
* Other. nsIAtom contains inline members and so implementing from script is
guaranteed to crash.
Patch coming up.
Assignee | ||
Comment 1•14 years ago
|
||
I included marking nsIDOMEventTarget as a test (which will be needed by the changes in bug 658714). And I marked nsIAtom since we really need to.
Assignee: nobody → jonas
Attachment #537276 -
Flags: review?(khuey)
How about instead of having [scriptable, scriptcanuseonly] you'd just have [script_usable] ?
Assignee | ||
Comment 3•14 years ago
|
||
I like that idea. It'll be a somewhat bigger patch, but should be ok.
Assignee | ||
Comment 4•14 years ago
|
||
Implements script_usable instead.
Attachment #537276 -
Attachment is obsolete: true
Attachment #537276 -
Flags: review?(khuey)
Attachment #537395 -
Flags: review?(khuey)
Comment on attachment 537395 [details] [diff] [review]
Patch v2
This looks pretty good. A couple of nits, and one test suggestion.
>diff --git a/js/src/xpconnect/src/xpcwrappedjsclass.cpp b/js/src/xpconnect/src/xpcwrappedjsclass.cpp
>- if(NS_SUCCEEDED(info->IsScriptable(&canScript)) && canScript &&
>+ if(NS_SUCCEEDED(info->IsScriptimplementable(&canScript)) && canScript &&
80 lines?
>@@ -291,17 +291,17 @@ nsXPCWrappedJSClass::CallQueryInterfaceO
>- if(NS_FAILED(info->IsScriptable(&canScript)) || !canScript)
>+ if(NS_FAILED(info->IsScriptimplementable(&canScript)) || !canScript)
80 lines?
>diff --git a/xpcom/reflect/xptinfo/public/nsIInterfaceInfo.idl b/xpcom/reflect/xptinfo/public/nsIInterfaceInfo.idl
> /* this is NOT intended to be scriptable */
>-[uuid(215DBE04-94A7-11d2-BA58-00805F8A5DD7)]
>+[uuid(7de126a2-ef4b-4e3b-a952-78ce4c133e38)]
> interface nsIInterfaceInfo : nsISupports
> {
> readonly attribute string name;
> readonly attribute nsIIDPtr InterfaceIID;
>
> PRBool isScriptable();
>+ PRBool isScriptimplementable();
isScriptImplementable, with a capital I.
>diff --git a/xpcom/reflect/xptinfo/src/xptiprivate.h b/xpcom/reflect/xptinfo/src/xptiprivate.h
>@@ -283,16 +287,20 @@ public:
> const nsID& IID() const { return mIID; }
>
> //////////////////////
> // These non-virtual methods handle the delegated nsIInterfaceInfo methods.
>
> nsresult GetName(char * *aName);
> nsresult GetIID(nsIID * *aIID);
> nsresult IsScriptable(PRBool *_retval);
>+ nsresult IsScriptimplementable(PRBool *_retval) {
>+ *_retval = GetScriptimplementableFlag();
>+ return NS_OK;
>+ }
Move this one to wherever the rest of these are implemented.
Also, don't we want XPIDL to enforce that an iface with a [notxpcom] method is at most script_usable? I didn't see that in the tests.
Other than that, looks great. r=me. You probably should check with bsmedberg to see if my review is enough here. I'm not a peer ... but I don't think anyone active is ...
Attachment #537395 -
Flags: review?(khuey) → review+
Comment 6•14 years ago
|
||
I'm not sure I like this name, actually. What we really mean by this is not merely that *script* cannot implement the interface, we mean that nobody except the builtin Mozilla platform should implement the interface (at least I think that's what we want). This lets us cast to concrete classes without lots of extra checking.
I think the clearest way to express this would be [scriptable, builtinclass].
And we can then do things like have xptcall itself refuse to reflect builtin classes.
[notxpcom] means that this method isn't xptcall-compatible, and so it's certainly not scriptable in any way or form.
Assignee | ||
Comment 7•14 years ago
|
||
Won't there be situations where we'll want addons to be able to implement interfaces which are exposed to script, without also making it possible for script to implement those interfaces?
Comment 8•14 years ago
|
||
By "addons" do you mean JS addons, or C++ addons? I don't think we should design any of this for the case of C++ addons. If you want JS addons to be able to implement these, then the utility is a little different, and I think we need to be explicit that they are only implementable by chrome but not content (and the name should reflect that).
Assignee | ||
Comment 9•14 years ago
|
||
I meant C++ addons. JS addons should not be able to implement these interfaces since it can't implement the [notxpcom] methods on them.
I really don't care about the syntax enough to push one way or another. If
[scriptable, builtinclass] is preferred over [script_usable] then I'm happy to go back to the first patch in this bug.
I didn't quite understand the "And we can then do things like have xptcall itself refuse to reflect builtin classes" comment. Do you mean anything beyond what the patch already does?
Comment 10•14 years ago
|
||
Yes, I mean that NS_GetXPTCallStub (http://mxr.mozilla.org/mozilla-central/source/xpcom/reflect/xptcall/public/xptcall.h#198) should refuse to implement "builtinclass" interfaces. This would affect not only JS scripting, but also XPCOM proxies, JavaXPCOM, and PyXPCOM. I think the first version is probably better.
Assignee | ||
Comment 11•14 years ago
|
||
This should work. Even includes a test!
Attachment #537395 -
Attachment is obsolete: true
Attachment #539858 -
Flags: review?(benjamin)
Comment 12•14 years ago
|
||
Comment on attachment 539858 [details] [diff] [review]
Patch v3
I'd love to also check this in xptcall also, but this seems good.
Attachment #539858 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 13•14 years ago
|
||
Checked in. Thanks for the quick review!
Assignee | ||
Comment 14•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 15•14 years ago
|
||
This was backed out in cset:
http://hg.mozilla.org/mozilla-central/rev/995f2c3e7a78
shouldn't this be re-opened ?
After this lands, can we go through and make a whole raft of DOM APIs builtinclass? That would help with issues like bug 664708.
Comment 18•14 years ago
|
||
Yup, that's the (maybe unwritten) plan!
Updated•14 years ago
|
Keywords: dev-doc-needed
Updated•14 years ago
|
Whiteboard: [sg:want P3]
Assignee | ||
Comment 19•14 years ago
|
||
Checked in! http://hg.mozilla.org/mozilla-central/rev/8e30eba8ff64
We should definitely start marking interfaces as [builtinonly]. Please do file separate bugs on that, and I'm happy to review :)
Assignee | ||
Updated•14 years ago
|
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
Comment 20•14 years ago
|
||
What semantics did we settle on here?
Can you send a message to dev-platform or planet explaining when [builtinonly] should be used, perhaps encouraging people to file bugs in their modules?
Is it possible to get a list of functions exposed to web content through IDL, so we can find out which classes are most important to consider marking as [builtinonly]?
Did this patch implement the "make casting to concrete classes free" part, or will that be a followup?
Assignee | ||
Comment 21•13 years ago
|
||
(In reply to comment #20)
> What semantics did we settle on here?
>
> Can you send a message to dev-platform or planet explaining when
> [builtinonly] should be used, perhaps encouraging people to file bugs in
> their modules?
Mail sent to dev-platform. Subject was "[builtinclass] in idl".
> Is it possible to get a list of functions exposed to web content through
> IDL, so we can find out which classes are most important to consider marking
> as [builtinonly]?
The only thing I can think of is to grep our tree for .idl files with [scriptable] but not [builtinclass] interfaces.
> Did this patch implement the "make casting to concrete classes free" part,
> or will that be a followup?
That's not really a separate feature. Once you know that your pointer points to an interface with only a single implementation, you can safely use static_cast<> to cast to that implementation.
Comment 22•13 years ago
|
||
> > Can you send a message to dev-platform or planet explaining when
> > [builtinonly] should be used, perhaps encouraging people to file bugs in
> > their modules?
>
> Mail sent to dev-platform. Subject was "[builtinclass] in idl".
http://groups.google.com/group/mozilla.dev.platform/msg/b9b000850191e659 thanks
> > Did this patch implement the "make casting to concrete classes free" part,
> > or will that be a followup?
>
> That's not really a separate feature. Once you know that your pointer points
> to an interface with only a single implementation, you can safely use
> static_cast<> to cast to that implementation.
Filed bug 672389 requesting a static analysis ;)
Comment 23•13 years ago
|
||
jcranmer already updated the documentation:
https://developer.mozilla.org/en/XPIDL#Interfaces
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•