Refactor transplant code and improve comments

RESOLVED FIXED in mozilla15

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 1 obsolete attachment)

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.
Created attachment 622351 [details] [diff] [review]
Factor out CCW remapping from JS_TransplantObject and add comments. v1

Attaching a patch. Flagging mrbkap for review.
Attachment #622351 - Flags: review?(mrbkap)
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|?
Attachment #622351 - Flags: review?(mrbkap) → review+
(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. ;-)
Created attachment 623073 [details] [diff] [review]
Part 1 - Factor out CCW remapping from JS_TransplantObject and add comments. v2 r=mrbkap
Attachment #622351 - Attachment is obsolete: true
Attachment #623073 - Flags: review+
Created attachment 623074 [details] [diff] [review]
Part 2 - Rename |obj| to |newIdentity| in JS_TransplantObject. v1 r=mrbkap
Attachment #623074 - Flags: review+
Created attachment 623076 [details] [diff] [review]
Part 3 - Make js_TransplantObjectWithWrapper use RemapWrappers. v1
Attachment #623076 - Flags: review?(mrbkap)
Created attachment 623077 [details] [diff] [review]
Part 4 - Rename |obj| to |newWrapper| in js_TransplantObjectWithWrapper. v1
Attachment #623077 - Flags: review?(mrbkap)
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.
Attachment #623078 - Flags: review?(mrbkap)
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.
Attachment #623079 - Flags: review?(mrbkap)
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?
(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.

Updated

5 years ago
Attachment #623076 - Flags: review?(mrbkap) → review+

Updated

5 years ago
Attachment #623077 - Flags: review?(mrbkap) → review+

Updated

5 years ago
Attachment #623078 - Flags: review?(mrbkap) → review+
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.
Attachment #623079 - Flags: review?(mrbkap) → review+
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=1e91648bba69
Looks green. Pushed to m-i:

http://hg.mozilla.org/integration/mozilla-inbound/rev/6678097e44e5
http://hg.mozilla.org/integration/mozilla-inbound/rev/86ce1810ab6f
http://hg.mozilla.org/integration/mozilla-inbound/rev/2330abd18337
http://hg.mozilla.org/integration/mozilla-inbound/rev/d3af846d8bc9
http://hg.mozilla.org/integration/mozilla-inbound/rev/2d846fdb9ddc
http://hg.mozilla.org/integration/mozilla-inbound/rev/a8bdf0315130
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/a8bdf0315130
https://hg.mozilla.org/mozilla-central/rev/2d846fdb9ddc
https://hg.mozilla.org/mozilla-central/rev/d3af846d8bc9
https://hg.mozilla.org/mozilla-central/rev/2330abd18337
https://hg.mozilla.org/mozilla-central/rev/86ce1810ab6f
https://hg.mozilla.org/mozilla-central/rev/6678097e44e5
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.