Before fixing bug 1220564, we should replace in-tree consumers of array/generator comprehension with map, filter, and some other things.

Please review the patch :)

Comment on attachment 8681951 [details] [diff] [review] Bug1220565.patch Review of attachment 8681951 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for the patch :) The header is not formatted correctly, I think (maybe you copied from |hg log| ?) please use |hg export| to generate a patch. ::: addon-sdk/source/lib/sdk/base64.js @@ +37,5 @@ > exports.encode = function (data, charset) { > if (isUTF8(charset)) > return btoa(unescape(encodeURIComponent(data))) > > + data = String.fromCharCode(...Array.from(data,c => (c.charCodeAt(0) & 0xff))); please put a space between |data,| and |c|. ::: addon-sdk/source/lib/sdk/deprecated/unit-test.js @@ +379,5 @@ > this.testRunSummary.push({ > name: this.test.name, > passed: this.test.passed, > failed: this.test.failed, > + errors: this.test.errors.map((element,error) => error).join(",") https://dxr.mozilla.org/mozilla-central/rev/96377bdbcdf3e444a22aeaa677da696243b00d98/addon-sdk/source/lib/sdk/deprecated/unit-test.js#553 |this.test.errors| is an Object, so it doesn't have .map. you could use |Object.keys| to get an array of property keys. ::: addon-sdk/source/lib/sdk/test/harness.js @@ +192,5 @@ > if (ref !== null) { > var data = ref.__url__ ? ref.__url__ : ref; > var warning = data == "[object Object]" > ? "[object " + data.constructor.name + "(" + > + data.map((element,p) => p).join(",") + ")]" looks like the type of |data| is not Array (or at least it's not guaranteed to be). it would be better to use |Object.keys()| (I don't see any reference to gWeakrefInfo tho...) @@ +463,5 @@ > } > + POINTLESS_ERRORS.map(err => { > + if (message.indexOf(err) >= 0) > + return err; > + }); |if (...)| should be replaced with filter. Also, the function can be written like |err => message.indexOf(err) >= 0|. ::: addon-sdk/source/test/test-file.js @@ +64,5 @@ > }; > > exports.testList = function(assert) { > let list = file.list(profilePath); > + let found = list.map(name => name === fileNameInProfile); s/map/filter/

Please review the patch.Thank you very much..!! :D

Please review the patch :)

Comment on attachment 8682105 [details] [diff] [review] bug1220565.patch Review of attachment 8682105 [details] [diff] [review]: ----------------------------------------------------------------- ::: addon-sdk/source/lib/sdk/base64.js @@ +37,5 @@ > exports.encode = function (data, charset) { > if (isUTF8(charset)) > return btoa(unescape(encodeURIComponent(data))) > > + data = String.fromCharCode(...Array.from(data,c => (c.charCodeAt(0) & 0xff))); put a space after ",", like |Array.from(data, c => ...)| please.

Please review the patch.Thanks

Comment on attachment 8682142 [details] [diff] [review] bug1220565.patch Review of attachment 8682142 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, looks good :) :mossop, can you review?

Comment on attachment 8682142 [details] [diff] [review] bug1220565.patch Review of attachment 8682142 [details] [diff] [review]: ----------------------------------------------------------------- There are a few things to fix here and please run the automated tests before submitting a patch for review. ::: addon-sdk/source/lib/sdk/deprecated/unit-test.js @@ +379,5 @@ > this.testRunSummary.push({ > name: this.test.name, > passed: this.test.passed, > failed: this.test.failed, > + errors: Object.keys(this.test.errors).join(",") You lost a space here ::: addon-sdk/source/lib/sdk/test/harness.js @@ +192,5 @@ > if (ref !== null) { > var data = ref.__url__ ? ref.__url__ : ref; > var warning = data == "[object Object]" > ? "[object " + data.constructor.name + "(" + > + Object.keys(data).join(",") + ")]" Here too ::: addon-sdk/source/lib/sdk/timers.js @@ +61,5 @@ > // Take a snapshot of timer `id`'s that have being present before > // starting a dispatch loop, in order to ignore timers registered > // in side effect to dispatch while also skipping immediates that > // were removed in side effect. > + let ids = [...immediates.keys()]; Isn't this just immediates.keys()? In which case drop the variable entirely and use that in the for block below. ::: addon-sdk/source/test/test-file.js @@ +64,5 @@ > }; > > exports.testList = function(assert) { > let list = file.list(profilePath); > + let found = list.filter(name => name === fileNameInProfile); I don't think you've run tests, this will make the assert on line 72 fail. Change the following assertions to just check that the length of the resulting array is 1.

Hi, please review the patch.Thanks :)

Comment on attachment 8682650 [details] [diff] [review] bug-1220565.patch Review of attachment 8682650 [details] [diff] [review]: ----------------------------------------------------------------- seems good now (btw, have you run the automated tests?) I'd like to forward a review about the assertion message in test-file.js.

Comment on attachment 8682650 [details] [diff] [review] bug-1220565.patch Review of attachment 8682650 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks

oops, I forgot to push it to try :P

try is running https://treeherder.mozilla.org/#/jobs?repo=try&revision=b56fe3b35f68

2 errors in test-ui-toolbar.js, on all OS. https://treeherder.mozilla.org/logviewer.html#?job_id=13401858&repo=try TEST-UNEXPECTED-FAIL | jetpack-package/addon-sdk/source/test/test-ui-toolbar.js.test toolbar persistence | toolbar persisted state & ignored option - true == false TEST-UNEXPECTED-FAIL | jetpack-package/addon-sdk/source/test/test-ui-toolbar.js.test toolbar persistence | window.document.getElementById(...) is null simplyblue24, can you have a look?

arai: I'm looking into it :)

Please review my patch.Thanks!

Comment on attachment 8687905 [details] [diff] [review] bug1220565.patch Review of attachment 8687905 [details] [diff] [review]: ----------------------------------------------------------------- ::: addon-sdk/source/lib/sdk/content/context-menu.js @@ +306,5 @@ > // content scripts when clicked. > var RemoteItem = Class({ > initialize: function(options, manager) { > this.id = options.id; > + this.contexts = options.contexts.map(c => instantiateContext(c)); Can this just be options.contexts.map(instantiateContext)? ::: addon-sdk/source/lib/sdk/content/utils.js @@ +89,5 @@ > function makeChildOptions(options) { > function makeStringArray(arrayOrValue) { > if (!arrayOrValue) > return []; > + return [].concat(arrayOrValue).map(v => String(v)); And this [].concat(arrayOrValue).map(String)

Hi, I have made the appropriate changes. Please review the patch :) Thanks.

Comment on attachment 8690402 [details] [diff] [review] bug1220565.patch Review of attachment 8690402 [details] [diff] [review]: ----------------------------------------------------------------- ::: addon-sdk/source/lib/sdk/timers.js @@ +66,1 @@ > for (let id of ids) { Looks like you missed an earlier comment. Please just make this: for (let id of immediates.keys())

Hi, the above code change is causing a failure in try. That is why i had to revert back.Thanks! https://treeherder.mozilla.org/logviewer.html#?job_id=13401858&repo=try

(In reply to simplyblue24 from comment #20) > Hi, the above code change is causing a failure in try. That is why i had to > revert back.Thanks! > https://treeherder.mozilla.org/logviewer.html#?job_id=13401858&repo=try Ok, you should reply to the reviewer when something like that happens so they don't keep calling you on it ;)

try is running: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2025095544e4

okay, this time try run is green :) thank you for your patch!