Last Comment Bug 759275 - Specialize unwrapping to HTML elements in dom bindings
: Specialize unwrapping to HTML elements in dom bindings
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Peter Van der Beken [:peterv]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-29 03:26 PDT by Peter Van der Beken [:peterv]
Modified: 2012-06-02 11:56 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (9.15 KB, patch)
2012-05-29 03:26 PDT, Peter Van der Beken [:peterv]
bzbarsky: review+
Details | Diff | Review

Description Peter Van der Beken [:peterv] 2012-05-29 03:26:41 PDT
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.
Comment 1 :Ms2ger 2012-05-29 03:51:45 PDT
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 [:bz] 2012-05-29 17:51:44 PDT
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] 2012-05-30 05:58:51 PDT
(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] 2012-05-30 07:07:19 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/867941a48b80
Comment 5 Ed Morley [:emorley] 2012-05-31 06:18:56 PDT
https://hg.mozilla.org/mozilla-central/rev/867941a48b80
Comment 6 Boris Zbarsky [:bz] 2012-05-31 15:08:34 PDT
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 :Ehsan Akhgari (busy, don't ask for review please) 2012-06-02 11:56:01 PDT
Bustage fix: https://hg.mozilla.org/mozilla-central/rev/a3d080bb3dc7

Note You need to log in before you can comment on or make changes to this bug.