Closed Bug 986294 Opened 7 years ago Closed 5 years ago

replace uses of Proxy.create() with new Direct Proxy API

Categories

(Add-on SDK Graveyard :: General, defect, P2)

defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: zombie, Assigned: evilpie)

References

Details

Attachments

(1 file, 3 obsolete files)

SpiderMonkey folks are trying to align with the ES6 soon-to-be spec on proxies, and they would be grateful if we could move from the old, unspecced Proxy.create() API to the new, ES6 standards-track "direct proxies".

i believe Erik is going to update preferences/service.js in bug 980565, and that leaves just system/environment.js, util/iteration.js and one content-script test.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy
Whiteboard: [good first bug]
I'm willing to work on it but need help.
Flags: needinfo?(tomica+amo)
hi Anuj, that's great.

what exactly do you need help with?

here is a page describing how to contribute to the Jetpack project:

https://github.com/mozilla/addon-sdk/wiki/contribute
Flags: needinfo?(tomica+amo)
(In reply to Tomislav Jovanovic [:zombie] from comment #2)

> what exactly do you need help with?

Can you tell me how I should replace Proxy.create() with new Proxy() in this line-
http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/util/iteration.js#15
that line is a bit special, as it's basically a "hack" to get to the ES6 @@iterator symbol (even before it's implemented). i believe this should be:

    for (let _ of new Proxy({}, {get: (_, name) => { throw name; } }))

but you should try doing the other, more regular uses of Proxy.create(). to help with that, i suggest you study both the documentation for new direct proxies [1] and how they differ from the old API [2].

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy
[2] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Old_Proxy_API

and ask if you get stuck.
Attached patch bug986294.diff (obsolete) — Splinter Review
Assignee: nobody → anujagarwal464
Attachment #8400024 - Flags: feedback?(tomica+amo)
Comment on attachment 8400024 [details] [diff] [review]
bug986294.diff

hey Jordan, can you take this review please? 

i don't feel comfortable doing it, especially after talking with Eric Faust, who said he didn't do the whole conversion to direct proxies because the semantics seemed different enough.
Attachment #8400024 - Flags: feedback?(tomica+amo) → feedback?(jsantell)
the test string overload (test-content-script.js) test fails, as do the environment tests. the `iteratorSymbol` works, and things that use it are working (sequence, list), but the other two conversions aren't currently. I would recommend checking out the master repo (mozilla/addon-sdk) on GH to run the tests, and also submit a PR through there, rather than a patch for m-c. Let me know if you have any questions
Attachment #8400024 - Flags: feedback?(jsantell) → feedback-
Assignee: anujagarwal464 → nobody
Duplicate of this bug: 1110457
Irakli has a patch for this in bug 1110457
Assignee: nobody → rFobic
Whiteboard: [good first bug]
Irakli's pull request depends on bug 783829 according to him in https://github.com/mozilla/addon-sdk/pull/1746
Depends on: 783829
hey Irakli, bug 783829 was resolved, wanna commit it now? (SM guys are anxious to remove Proxy.create)
Flags: needinfo?(rFobic)
Depends on: 1110457
Flags: needinfo?(rFobic)
(In reply to Tomislav Jovanovic [:zombie] from comment #11)
> hey Irakli, bug 783829 was resolved, wanna commit it now? (SM guys are
> anxious to remove Proxy.create)

That change has landed now.
(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #12)
> (In reply to Tomislav Jovanovic [:zombie] from comment #11)
> > hey Irakli, bug 783829 was resolved, wanna commit it now? (SM guys are
> > anxious to remove Proxy.create)
> 
> That change has landed now.

So this bug is RESOLVED FIXED or is there something missing?
Flags: needinfo?(rFobic)
I still see the following three uses of Proxy.create:

addon-sdk/source/lib/sdk/system/environment.js
15:exports.env = Proxy.create({

addon-sdk/source/test/addons/e10s-content/lib/test-content-script.js
264:  let p = Proxy.create({
329:      //     return obj._proxy = Proxy.create(...);

addon-sdk/source/test/test-content-script.js
264:  let p = Proxy.create({
329:      //     return obj._proxy = Proxy.create(...);
This code will now generate a warning.
Attachment #8727107 - Attachment is obsolete: true
Attachment #8727107 - Flags: review?(dtownsend)
This is not necessarily a perfect match for the old semantics, but it passes the tests and I assume it's good enough for most reasonable uses of this API.
Attachment #8727108 - Flags: review?(dtownsend)
Sorry about the bug spam, just forgot the refresh for two times.
Attachment #8727108 - Attachment is obsolete: true
Attachment #8727108 - Flags: review?(dtownsend)
Attachment #8727109 - Flags: review?(dtownsend)
Assignee: rFobic → evilpies
Attachment #8400024 - Attachment is obsolete: true
Attachment #8727109 - Flags: review?(dtownsend) → review+
https://hg.mozilla.org/mozilla-central/rev/c138edad150f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Flags: needinfo?(rFobic)
You need to log in before you can comment on or make changes to this bug.