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

Status: RESOLVED FIXED

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

Description

[Tomislav Jovanovic :zombie]

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

Comment 1

[Anuj Agarwal [:anujagarwal464]]

I'm willing to work on it but need help.

Comment 2

[Tomislav Jovanovic :zombie]

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

Comment 3

[Anuj Agarwal [:anujagarwal464]]

(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

Comment 4

[Tomislav Jovanovic :zombie]

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.

Comment 5

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug986294.diff

Comment 6

[Tomislav Jovanovic :zombie]

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.

Comment 7

[Jordan Santell [:jsantell] [@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

Comment 9

[Erik Vold [:erikvold] (please needinfo? me)]

Irakli has a patch for this in bug 1110457

Comment 10

[Erik Vold [:erikvold] (please needinfo? me)]

Irakli's pull request depends on bug 783829 according to him in https://github.com/mozilla/addon-sdk/pull/1746

Comment 11

[Tomislav Jovanovic :zombie]

hey Irakli, bug 783829 was resolved, wanna commit it now? (SM guys are anxious to remove Proxy.create)

Comment 12

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(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.

Comment 13

[David Bruant]

(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?

Comment 14

[Tom S [:evilpie]]

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(...);

Comment 15

[Tom S [:evilpie]]

This code will now generate a warning.

Comment 16

[Tom S [:evilpie]]

Attachment: Remove Proxy.create from addon-sdk

Comment 17

[Tom S [:evilpie]]

Attachment: Remove Proxy.create from addon-sdk

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.

Comment 18

[Tom S [:evilpie]]

Attachment: Remove Proxy.create from addon-sdk

Sorry about the bug spam, just forgot the refresh for two times.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c138edad150f

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c138edad150f