Last Comment Bug 753277 - Refactor transplant code and improve comments
: Refactor transplant code and improve comments
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-09 04:26 PDT by Bobby Holley (:bholley) (busy with Stylo)
Modified: 2012-05-14 14:14 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Factor out CCW remapping from JS_TransplantObject and add comments. v1 (10.91 KB, patch)
2012-05-09 05:35 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 1 - Factor out CCW remapping from JS_TransplantObject and add comments. v2 r=mrbkap (10.89 KB, patch)
2012-05-11 02:18 PDT, Bobby Holley (:bholley) (busy with Stylo)
bobbyholley: review+
Details | Diff | Splinter Review
Part 2 - Rename |obj| to |newIdentity| in JS_TransplantObject. v1 r=mrbkap (3.02 KB, patch)
2012-05-11 02:18 PDT, Bobby Holley (:bholley) (busy with Stylo)
bobbyholley: review+
Details | Diff | Splinter Review
Part 3 - Make js_TransplantObjectWithWrapper use RemapWrappers. v1 (3.02 KB, patch)
2012-05-11 02:18 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 4 - Rename |obj| to |newWrapper| in js_TransplantObjectWithWrapper. v1 (3.60 KB, patch)
2012-05-11 02:19 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 5 - Rename tobj to wrapperGuts, and avoid going through the SCSW on the other side unnecessarily. v1 (1.31 KB, patch)
2012-05-11 02:19 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 6 - Assert that all cross-scope wrapper reparenting operations are cross-compartment, and remove the conditional. v1 (12.37 KB, patch)
2012-05-11 02:20 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review

Description Bobby Holley (:bholley) (busy with Stylo) 2012-05-09 04:26:37 PDT
Some of this stuff is more confusing than it needs to be. I'm about to fix a bug in this stuff (I think), and I felt like clarifying some things first. Patches soon.
Comment 1 Bobby Holley (:bholley) (busy with Stylo) 2012-05-09 05:35:32 PDT
Created attachment 622351 [details] [diff] [review]
Factor out CCW remapping from JS_TransplantObject and add comments. v1

Attaching a patch. Flagging mrbkap for review.
Comment 2 Blake Kaplan (:mrbkap) 2012-05-09 07:44:37 PDT
Comment on attachment 622351 [details] [diff] [review]
Factor out CCW remapping from JS_TransplantObject and add comments. v1

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

::: js/src/jsapi.cpp
@@ +1516,5 @@
> + * identities by altering wrappers.
> + *
> + * The |origobj| argument should be the object whose identity needs to be
> + * remapped, usually to another compartment. The contents of |origobj| are
> + * destroyed, unless |origobj == target|.

Here and below, you refer to the |origobj == target| case, while it's now been outlawed (in favor of JS_RefreshCrossCompartmentWrappers).

@@ +1544,4 @@
>      JSCompartment *destination = target->compartment();
>      WrapperMap &map = destination->crossCompartmentWrappers;
>      Value origv = ObjectValue(*origobj);
>      JSObject *obj;

Is there anything we can do about this name? I could never think of anything better here, but |obj| is pretty generic, especially in the face of |origobj| and |target|... Perhaps |newObj|?
Comment 3 Bobby Holley (:bholley) (busy with Stylo) 2012-05-10 06:37:17 PDT
(In reply to Blake Kaplan (:mrbkap) from comment #2)

> Is there anything we can do about this name? I could never think of anything
> better here, but |obj| is pretty generic, especially in the face of
> |origobj| and |target|... Perhaps |newObj|?

newIdentity. I'm making that a separate patch with r=mrbkap. Let me know if you disagree. ;-)
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2012-05-11 02:18:04 PDT
Created attachment 623073 [details] [diff] [review]
Part 1 - Factor out CCW remapping from JS_TransplantObject and add comments. v2 r=mrbkap
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2012-05-11 02:18:25 PDT
Created attachment 623074 [details] [diff] [review]
Part 2 - Rename |obj| to |newIdentity| in JS_TransplantObject. v1 r=mrbkap
Comment 6 Bobby Holley (:bholley) (busy with Stylo) 2012-05-11 02:18:56 PDT
Created attachment 623076 [details] [diff] [review]
Part 3 - Make js_TransplantObjectWithWrapper use RemapWrappers. v1
Comment 7 Bobby Holley (:bholley) (busy with Stylo) 2012-05-11 02:19:31 PDT
Created attachment 623077 [details] [diff] [review]
Part 4 - Rename |obj| to |newWrapper| in js_TransplantObjectWithWrapper. v1
Comment 8 Bobby Holley (:bholley) (busy with Stylo) 2012-05-11 02:19:55 PDT
Created attachment 623078 [details] [diff] [review]
Part 5 - Rename tobj to wrapperGuts, and avoid going through the SCSW on the other side unnecessarily. v1

It's not wrong, but it's not necessary either, since we'll just unwrap directly to the underlying object during wrapping. And this is confusing enough as-is.
Comment 9 Bobby Holley (:bholley) (busy with Stylo) 2012-05-11 02:20:27 PDT
Created attachment 623079 [details] [diff] [review]
Part 6 - Assert that all cross-scope wrapper reparenting operations are cross-compartment, and remove the conditional. v1

The reindentation is pure. No snakes.
Comment 10 :Ms2ger 2012-05-11 03:14:49 PDT
Comment on attachment 623078 [details] [diff] [review]
Part 5 - Rename tobj to wrapperGuts, and avoid going through the SCSW on the other side unnecessarily. v1

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

Only the rename happened, no?
Comment 11 Bobby Holley (:bholley) (busy with Stylo) 2012-05-11 03:32:10 PDT
(In reply to Ms2ger from comment #10)

> Only the rename happened, no?

No, it's subtle. Note that we previously were pointing to newWrapper, and now we point to targetobj.
Comment 12 Blake Kaplan (:mrbkap) 2012-05-11 06:40:05 PDT
Comment on attachment 623079 [details] [diff] [review]
Part 6 - Assert that all cross-scope wrapper reparenting operations are cross-compartment, and remove the conditional. v1

A diff -w would have been nice here.
Comment 13 Bobby Holley (:bholley) (busy with Stylo) 2012-05-14 01:57:38 PDT
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=1e91648bba69

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