Closed Bug 1220565 Opened 6 years ago Closed 6 years ago

Remove non-standard comprehension from addon-sdk/.

Categories

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

defect
Not set
normal

Tracking

(firefox45 fixed)

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: arai, Assigned: bmanojkumar24, Mentored)

References

Details

Attachments

(1 file, 6 obsolete files)

Before fixing bug 1220564, we should replace in-tree consumers of array/generator comprehension with map, filter, and some other things.
Attached patch Bug1220565.patch (obsolete) — Splinter Review
Please review the patch :)
Attachment #8681951 - Flags: review?(arai.unmht)
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/
Attachment #8681951 - Flags: review?(arai.unmht) → feedback+
Attached patch bug1220565.patch (obsolete) — Splinter Review
Please review the patch.Thank you very much..!! :D
Attachment #8681951 - Attachment is obsolete: true
Attachment #8682096 - Flags: review?(arai.unmht)
Attached patch bug1220565.patch (obsolete) — Splinter Review
Please review the patch :)
Attachment #8682096 - Attachment is obsolete: true
Attachment #8682096 - Flags: review?(arai.unmht)
Attachment #8682105 - Flags: review?(arai.unmht)
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.
Attachment #8682105 - Flags: review?(arai.unmht) → feedback+
Attached patch bug1220565.patch (obsolete) — Splinter Review
Please review the patch.Thanks
Attachment #8682105 - Attachment is obsolete: true
Attachment #8682142 - Flags: review?(arai.unmht)
Comment on attachment 8682142 [details] [diff] [review]
bug1220565.patch

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

Thanks, looks good :)

:mossop, can you review?
Attachment #8682142 - Flags: review?(dtownsend)
Attachment #8682142 - Flags: review?(arai.unmht)
Attachment #8682142 - Flags: feedback+
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.
Attachment #8682142 - Flags: review?(dtownsend)
Attached patch bug-1220565.patch (obsolete) — Splinter Review
Hi, please review the patch.Thanks :)
Attachment #8682650 - Flags: review?(arai.unmht)
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.
Attachment #8682650 - Flags: review?(dtownsend)
Attachment #8682650 - Flags: review?(arai.unmht)
Attachment #8682650 - Flags: feedback+
Comment on attachment 8682650 [details] [diff] [review]
bug-1220565.patch

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

Looks good, thanks
Attachment #8682650 - Flags: review?(dtownsend) → review+
Attachment #8682142 - Attachment is obsolete: true
Keywords: checkin-needed
oops, I forgot to push it to try :P
Keywords: checkin-needed
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?
Flags: needinfo?(bmanojkumar24)
arai:  I'm looking into it :)
Attached patch bug1220565.patch (obsolete) — Splinter Review
Please review my patch.Thanks!
Attachment #8682650 - Attachment is obsolete: true
Flags: needinfo?(bmanojkumar24)
Attachment #8687905 - Flags: review?(dtownsend)
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)
Attachment #8687905 - Flags: review?(dtownsend) → review+
Attached patch bug1220565.patchSplinter Review
Hi, I have made the appropriate changes. Please review the patch :) Thanks.
Attachment #8687905 - Attachment is obsolete: true
Attachment #8690402 - Flags: review?(dtownsend)
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())
Attachment #8690402 - Flags: review?(dtownsend) → review+
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 ;)
okay, this time try run is green :)
thank you for your patch!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/63e6e11fae5b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.