759275 - Specialize unwrapping to HTML elements in dom bindings

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Peter Van der Beken [:peterv]]

Attachment: 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.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 2

[Boris Zbarsky [:bzbarsky]]

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

Comment 3

[Peter Van der Beken [:peterv]]

(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 :-).

Comment 4

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/867941a48b80

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/867941a48b80

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[(no longer active)]

Bustage fix: https://hg.mozilla.org/mozilla-central/rev/a3d080bb3dc7