Last Comment Bug 779626 - Javascript-global-constructor objects should be passed a window reference
: Javascript-global-constructor objects should be passed a window reference
Status: RESOLVED FIXED
: addon-compat, dev-doc-needed
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Anant Narayanan [:anant]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-01 13:56 PDT by Anant Narayanan [:anant]
Modified: 2012-08-07 07:34 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Pass the calling as the last argument to the constructor (1.78 KB, patch)
2012-08-02 14:56 PDT, Anant Narayanan [:anant]
no flags Details | Diff | Splinter Review
Pass the calling as the first argument to the constructor (1.51 KB, patch)
2012-08-02 21:51 PDT, Anant Narayanan [:anant]
no flags Details | Diff | Splinter Review
Pass the calling window as the first argument to the constructor (1.50 KB, patch)
2012-08-03 08:35 PDT, Anant Narayanan [:anant]
mrbkap: review+
Details | Diff | Splinter Review

Description Anant Narayanan [:anant] 2012-08-01 13:56:35 PDT
In bug 723206, we added the ability to receive arguments for global constructors implemented in IDL. Javascript initializers are passed a reference to the calling window, but constructors aren't, and is required in many cases.

The proposal is to append a window reference to the end of the argument list passed to the constructor in order to remain compatible with existing objects registered as Javascript-global-constructors.
Comment 1 :Ms2ger (⌚ UTC+1/+2) 2012-08-01 14:30:58 PDT
I understand you're working on this.
Comment 2 Anant Narayanan [:anant] 2012-08-02 14:56:54 PDT
Created attachment 648503 [details] [diff] [review]
Pass the calling as the last argument to the constructor
Comment 3 Blake Kaplan (:mrbkap) 2012-08-02 15:34:05 PDT
Comment on attachment 648503 [details] [diff] [review]
Pass the calling as the last argument to the constructor

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

This looks right, but I think we should pass the window as the first argument instead of the last. That way code can be written that knows where the window will be passed.

::: dom/base/nsDOMClassInfo.cpp
@@ +5738,5 @@
>            rooter.changeLength(i + 1);
>          }
>  
> +        // Append window to the end of arguments
> +        JS::Value win;

Why not wrap directly into args, i.e. why use the temporary?

@@ +5742,5 @@
> +        JS::Value win;
> +        nsCOMPtr<nsIXPConnectJSObjectHolder> holder;
> +        nsCOMPtr<nsIDOMWindow> currentWin(do_GetInterface(currentInner));
> +        rv = WrapNative(cx, obj, currentWin, &NS_GET_IID(nsIDOMWindow),
> +          true, &win, getter_AddRefs(holder));

Nit: Please make |true| line up with |cx|.
Comment 4 Anant Narayanan [:anant] 2012-08-02 16:27:33 PDT
(In reply to Blake Kaplan (:mrbkap) from comment #3)
> This looks right, but I think we should pass the window as the first
> argument instead of the last. That way code can be written that knows where
> the window will be passed.

I didn't want existing code that used Javascript-global-constructor to break. Should I look for existing instances and roll in the changes needed into this patch?
Comment 5 Anant Narayanan [:anant] 2012-08-02 17:09:50 PDT
Ok, turns out it's not really a big deal since there's only one user of Javascript-global-constructor currently (https://mxr.mozilla.org/mozilla-central/search?string=Javascript-global-constructor) and it doesn't use any arguments. I'll post an updated patch.
Comment 6 Alex Vincent [:WeirdAl] 2012-08-02 17:56:36 PDT
(In reply to Anant Narayanan [:anant] from comment #4)
> I didn't want existing code that used Javascript-global-constructor to
> break. Should I look for existing instances and roll in the changes needed
> into this patch?

Please keep add-ons and documentation for them in mind when you do this... I'm not using this currently in any add-on work, but in the past at other companies I might have.
Comment 7 Anant Narayanan [:anant] 2012-08-02 21:51:33 PDT
Created attachment 648610 [details] [diff] [review]
Pass the calling as the first argument to the constructor
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2012-08-03 00:09:24 PDT
Comment on attachment 648610 [details] [diff] [review]
Pass the calling as the first argument to the constructor

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

::: dom/base/nsDOMClassInfo.cpp
@@ +5726,5 @@
>          JSObject* thisObject = &thisValue.toObject();
>  
>          // wrap parameters in the target compartment
> +        // we also pass in the calling window as the first argument
> +        argc += 1;

++argc;

@@ +5738,5 @@
> +        rv = WrapNative(cx, obj, currentWin, &NS_GET_IID(nsIDOMWindow),
> +          true, &args[i], getter_AddRefs(holder));
> +        if (!JS_WrapValue(cx, &args[i]))
> +          return NS_ERROR_FAILURE;
> +        rooter.changeLength(i + 1);

I wouldn't use the i here; rather just use args[0].
Comment 9 Anant Narayanan [:anant] 2012-08-03 08:35:45 PDT
Created attachment 648719 [details] [diff] [review]
Pass the calling window as the first argument to the constructor
Comment 10 Blake Kaplan (:mrbkap) 2012-08-06 17:41:34 PDT
Comment on attachment 648719 [details] [diff] [review]
Pass the calling window as the first argument to the constructor

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

Thanks!
Comment 12 Ed Morley [:emorley] 2012-08-07 07:34:46 PDT
https://hg.mozilla.org/mozilla-central/rev/f8f98c855ecc

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