Specialize unwrapping to HTML elements in dom bindings

RESOLVED FIXED in mozilla15

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: peterv, Assigned: peterv)

Tracking

Trunk
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 627893 [details] [diff] [review]
v1

We can add unwrapping to concrete implementations for HTML elements without any QI or refcounting by using the nsIContent bit and calling IsHTML(tag) on the nsIContent. It's pretty nice, because we can start using the concrete classes and when we switch them to new bindings we don't have to change the native APIs that take these as arguments.
Attachment #627893 - Flags: review?(bzbarsky)
Comment on attachment 627893 [details] [diff] [review]
v1

Review of attachment 627893 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCQuickStubs.h
@@ +490,5 @@
>  xpc_qsUnwrapArgImpl(JSContext *cx, jsval v, const nsIID &iid, void **ppArg,
>                      nsISupports **ppArgRef, jsval *vp);
>  
>  /** Convert a jsval to an XPCOM pointer. */
> +template <class T, class S>

It might be useful to use more descriptive names than T and S here...
Comment on attachment 627893 [details] [diff] [review]
v1

>@@ -3845,15 +3846,15 @@ struct PrototypeIDMap;
>+        curr = CGHeaders([], ["nsDebug.h", "mozilla/dom/UnionTypes.h", "nsDOMQS.h"], [], curr)

Why do we need that in the prototype header?

>+++ b/js/xpconnect/src/XPCQuickStubs.h
>+template <class T, class S>

Yeah, better template arg names might be nice here.

I assume there are later patches adding uses of DEFINE_UNWRAP_CAST_HTML?

r=me
Attachment #627893 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 3

5 years ago
(In reply to Boris Zbarsky (:bz) from comment #2)
> Why do we need that in the prototype header?

This was part of the union stuff, so ignore.

> Yeah, better template arg names might be nice here.

I've used Interface and StrongRefType. This should be going away at some point, so I don't really care about the names.

> I assume there are later patches adding uses of DEFINE_UNWRAP_CAST_HTML?

Yes, I've added them as I needed them. Feel free to do the same :-).
(Assignee)

Comment 4

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/867941a48b80
Target Milestone: --- → mozilla15

Comment 5

5 years ago
https://hg.mozilla.org/mozilla-central/rev/867941a48b80
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
There's actually a bug in the second function defined by DEFINE_UNWRAP_CAST_HTML.  Specifically, argRef in there is not initialized, but is unconditionally assigned to *ppArgRef.  This means that if the unwrap fails the caller will have garbage in a place where they thing they need to call Release() and we'll crash.  This bit me in bug 748266 because some of our tests take the "failed to unwrap" path here.

Peter, do you want a check for success rv there, or do you want to just initialize argRef to null?  Either one fixes the issue for me.
Bustage fix: https://hg.mozilla.org/mozilla-central/rev/a3d080bb3dc7
You need to log in before you can comment on or make changes to this bug.