Closed Bug 1322124 Opened 8 years ago Closed 7 years ago

Remove javascript Array generics

Categories

(MailNews Core :: Backend, defect)

Product:
MailNews Core
Component:
Backend
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox53 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: aryx, Assigned: aryx)

References

Details

Attachments

(5 files, 1 obsolete file)

Bug 1322124 - Remove javascript Array generics: calendar.
58 bytes, text/x-review-board-request
Fallen
: review+
Details
Bug 1322124 - Remove javascript Array generics: chat and im.
58 bytes, text/x-review-board-request
aleth
: review+
Details
Bug 1322124 - Remove javascript Array generics: mail.
58 bytes, text/x-review-board-request
aceman
: review+
Details
Bug 1322124 - Remove javascript Array generics: mailnews.
58 bytes, text/x-review-board-request
aceman
: review+
Details
Bug 1322124 - Remove javascript Array generics: suite.
58 bytes, text/x-review-board-request
philip.chee
: review+
Details 
From bug 1222547 comment 0:

Array generics are non-standard SpiderMonkey extension and they should be removed.
  * Array.concat
  * Array.every
  * Array.filter
  * Array.forEach
  * Array.indexOf
  * Array.join
  * Array.lastIndexOf
  * Array.map
  * Array.pop
  * Array.push
  * Array.reduce
  * Array.reduceRight
  * Array.reverse
  * Array.shift
  * Array.slice
  * Array.some
  * Array.sort
  * Array.splice
  * Array.unshift
Attachment #8817788 - Attachment mime type: text/x-review-board-request → text/plain
Attachment #8817788 - Attachment mime type: text/plain → text/x-moz-url-data
Attachment #8817788 - Attachment mime type: text/x-moz-url-data → text/x-moz-url
Attachment #8817788 - Attachment mime type: text/x-moz-url → text/plain
Comment on attachment 8817785 [details]
Bug 1322124 - Remove javascript Array generics: chat and im.

https://reviewboard.mozilla.org/r/98046/#review98284

Surprised there aren't more instances - thanks!
Attachment #8817785 - Flags: review?(aleth) → review+
Comment on attachment 8817788 [details] [diff] [review]
Bug 1322124 - Remove javascript Array generics: suite.

Round 1: code inspection only:

> -                   (this._browsers = Array.map(this.tabs, tab => tab.linkedBrowser));
> +                   (this._browsers = Array.from(this.tabs).map(tab => tab.linkedBrowser));
I think you can do this:

(this._browsers = Array.from(this.tabs, tab => tab.linkedBrowser;));

> -    // Array.concat) is faster than resizing the array (by splice) on each loop
> +    // <Array>.concat) is faster than resizing the array (by splice) on each loop
Fixing comments is pretty pointless, but if you want to do it do something like:
Array.prototype.concat) is faster...

> -  var addedLang = Array.map(languages, function(e) { return e.value; });
> +  var addedLang = Array.from(languages).map(function(e) { return e.value; });
Does the following work?
var addedLang = Array.from(languages, e => e.value;);

> -  return Array.map(gLanguages, function(e) { return e.value; }).join(",");
> +  return Array.from(gLanguages).map(function(e) { return e.value; }).join(",");
==ditto==

> -               Array.every(fileNames, aFile => aValue.indexOf(aFile) >= 0);
> +               Array.from(fileNames).every(aFile => aValue.indexOf(aFile) >= 0);
Is aValue an Array? Does aValue.includes(aFile) work?

> -    return Array.every(node.options, (aOpt, aIx) =>
> -                                       (aValue.indexOf(aIx) > -1) == aOpt.selected);
> +    return Array.from(node.options).every((aOpt, aIx) =>
> +                                          (aValue.indexOf(aIx) > -1) == aOpt.selected);
==ditto==

> +        let states = Array.from(newTab.linkedBrowser.contentDocument.styleSheets)
> +                          .map(aSS => !aSS.disabled);
==ditto==

> -  return Array.map(domains, function(e) { return e.label; }).join(",");
> +  return Array.from(domains).map(e => e.label).join(",");
==ditto==

> -          if (Array.indexOf(types, modtype) >= 0) {
> +          if (Array.from(types).indexOf(modtype)) {
I think you meant:
Array.from(types).includes(modtype)
Attachment #8817788 - Flags: review?(philip.chee) → review-
Comment on attachment 8817786 [details]
Bug 1322124 - Remove javascript Array generics: mail.

https://reviewboard.mozilla.org/r/98048/#review98286

::: mail/base/content/folderPane.js:789
(Diff revision 1)
>      // should be handled; try to extract a url from non moz apps as well.
>      let feedUri = targetFolder.server.type == "rss" && count == 1 ?
>                      FeedUtils.getFeedUriFromDataTransfer(dt) : null;
>  
>      // we only support drag of a single flavor at a time.
>      let types = dt.mozTypesAt(0);

Does this need the Array.from() treatment?

::: mail/base/content/nsDragAndDrop.js:460
(Diff revision 1)
>          for (var j = 0; j < flavourSet.flavours.length; j++) {
>            var type = flavourSet.flavours[j].contentType;
>            // dataTransfer uses text/plain but older code used text/unicode, so
>            // switch this for compatibility
>            var modtype = (type == "text/unicode") ? "text/plain" : type;
> -          if (Array.indexOf(types, modtype) >= 0) {
> +          if (Array.from(types).includes(modtype)) {

At the other place you have made types an array globally. Is that not wanted in this function? Is it needed only in this single 'if' ?

::: mail/test/resources/mozmill/mozmill/extension/resource/modules/controller.js:287
(Diff revision 1)
>     *        Top menu node whose elements have to be populated
>     */
>    _buildMenu : function(menu) {
>      var items = menu ? menu.childNodes : null;
>  
> -    Array.forEach(items, function(item) {
> +    Array.from(items).forEach(function(item) {

Array.from() does not work on null. Maybe you need to let items = menu ? menu.childNodes : \[\]; Or just use menu.childNodes, unless items is used elsewhere than just this loop.
Attachment #8817786 - Flags: review?(acelists) → review-
Comment on attachment 8817787 [details]
Bug 1322124 - Remove javascript Array generics: mailnews.

https://reviewboard.mozilla.org/r/98050/#review98288

Thanks, looks good, just check the 2 places if the conversions are useful. Maybe see the history of those loops.

::: mailnews/db/gloda/modules/facet.js:569
(Diff revision 1)
>        let dayItemList = aMonthObj[key];
>        if (typeof(key) == "string" && key.startsWith('_'))
>          continue;
>        dayItemLists.push(dayItemList);
>      }
> -    return Array.concat.apply([], dayItemLists);
> +    return Array.from(dayItemLists);

What does this do? dayItemLists is already an array.

::: mailnews/db/gloda/modules/facet.js:580
(Diff revision 1)
>        let monthObj = aYearObj[key];
>        if (typeof(key) == "string" && key.startsWith('_'))
>          continue;
>        monthItemLists.push(this._unionMonth(monthObj));
>      }
> -    return Array.concat.apply([], monthItemLists);
> +    return Array.from(monthItemLists);

What does this do? monthItemLists is already an array.
Attachment #8817787 - Flags: review?(acelists) → review+
Comment on attachment 8817787 [details]
Bug 1322124 - Remove javascript Array generics: mailnews.

https://reviewboard.mozilla.org/r/98050/#review98288

> What does this do? dayItemLists is already an array.

Indeed, looks like someone made Array copies during the prototyping stage. Removed the Array.froms and returned the already generated arrays.
Comment on attachment 8817786 [details]
Bug 1322124 - Remove javascript Array generics: mail.

https://reviewboard.mozilla.org/r/98048/#review98286

> Array.from() does not work on null. Maybe you need to let items = menu ? menu.childNodes : \[\]; Or just use menu.childNodes, unless items is used elsewhere than just this loop.

Replaced null with [].
Comment on attachment 8817786 [details]
Bug 1322124 - Remove javascript Array generics: mail.

https://reviewboard.mozilla.org/r/98048/#review98294

Looks good now.
Attachment #8817786 - Flags: review?(acelists) → review+
Comment on attachment 8817787 [details]
Bug 1322124 - Remove javascript Array generics: mailnews.

>  PromiseTestUtils.promiseFolderNotification = function(folder, listenerMethod) {
>    return new Promise( (resolve, reject) => {
>      let mfnListener = {};
>      mfnListener[listenerMethod] = function() {
> -      let args = Array.prototype.slice.call(arguments);
> +      let args = Array.from(arguments);

If you use the rest operator:
     mfnListener[listenerMethod] = function(...args) {
Then args becomes a real array automatically.
Comment on attachment 8817784 [details]
Bug 1322124 - Remove javascript Array generics: calendar.

> -                        return this.promiseOperation(target, name, Array.slice(arguments));
> +                      let args;
> +                      if (Array.from) {
> +                        // Gecko 32+
> +                        args = Array.from(arguments);
I don't understand this. You've set the minimum version in install.rdf to Gecko 32 so Array.from is available irrespective of the version of Lightning installed.

> +                      } else {
> +                        // Array.slice as Array generic got removed.
> +                        args = Array.slice(arguments);
Or just give up and unconditionally use:
    let args = Array.prototype.slice.call(arguments);
This should work all the way back to Mozilla Suite 1.7

> +                      }
> +                      return this.promiseOperation(target, name, args);
Comment on attachment 8817817 [details]
Bug 1322124 - Remove javascript Array generics: suite.

Looks good thanks.
Attachment #8817817 - Flags: review?(philip.chee) → review+
Attachment #8817788 - Attachment is obsolete: true
Attachment #8817788 - Attachment is patch: true
Comment on attachment 8817784 [details]
Bug 1322124 - Remove javascript Array generics: calendar.

https://reviewboard.mozilla.org/r/98044/#review98328

::: calendar/providers/gdata/content/gdata-calendar-event-dialog.js:15
(Diff revision 2)
>      if (!("gOldEndTimezone" in window)) {
>          window.gOldEndTimezone = null;
>      }
>  
>      monkeyPatch(window, "updateCalendar", function(protofunc /*, ...args */) {
> -        let rv = protofunc.apply(this, Array.slice(arguments, 1));
> +        let rv = protofunc.apply(this, Array.from(arguments).slice(1));

This was just a hack since rest args were not available. You could change this to use rest args instead (and in similar locations)

::: calendar/providers/gdata/modules/shim/Calendar.jsm:265
(Diff revision 2)
> +                        args = Array.from(arguments);
> +                      } else {
> +                        // Array.slice as Array generic got removed.
> +                        args = Array.slice(arguments);
> +                      }
> +                      return this.promiseOperation(target, name, args); 

Trailing space

::: calendar/providers/gdata/modules/shim/Loader.jsm:252
(Diff revision 2)
>                                "#006600", "#336666", "#000099", "#333399", "#663366",
>                                "#000000", "#330000", "#663300", "#663333", "#333300",
>                                "#003300", "#003333", "#000066", "#330099", "#330033"];
>  
> -        let sum = Array.map(str || " ", function(e) { return e.charCodeAt(0); }).reduce(function(a, b) { return a + b; });
> +        let sum = Array.from(str || " ", function(e) { return e.charCodeAt(0); })
> +                       .reduce(function(a, b) { return a + b; });

Feel free to do it like in calUtils, or just remove this shim.
Attachment #8817784 - Flags: review?(philipp) → review+
Depends on: 1323219
Depends on: 1338264
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: