1290701 - Replace in-tree consumer of non-standard Iterator() with Object.{values,entries} in toolkit/

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Tooru Fujisawa [:arai]]

separated from bug 1290637.
see bug 1290637 for the details.

https://dxr.mozilla.org/mozilla-central/search?q=%22+Iterator(%22+path%3Atoolkit%2F&redirect=false

Comment 1

[Tooru Fujisawa [:arai]]

Quick Summary

Required code changes are following:
  * Check each usage of non-standard Iterator [1] function,
    and replace it with Object.entries [2] or Object.values [3],
    or something appropriate for the specific usage

for example:
  for (let [k, v] in Iterator(obj)) {
    ...
  }
can be replaced to:
  for (let [k, v] of Object.entries(obj)) {
    ...
  }

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Iterator
[2] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/entries
[3] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/values

Comment 2

[kashyap]

Attachment: mypatch.patch

Comment 3

[Tooru Fujisawa [:arai]]

Comment on attachment 8777220 [details] [diff] [review]
mypatch.patch

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

Thank you for your patch :D

Please address the following comments, and attach updated patch and ask review again.

::: toolkit/components/passwordmgr/test/notification_common.js
@@ +1,2 @@
>  /*
> + * Initialization: for each test, remove any prior notifications. define window:true;

Is this accidentally added?
If so please remove this.

@@ +91,4 @@
>      ok(true, "is popup panel open? " + container.isPanelOpen);
>      var notes = container._currentNotifications;
>      ok(true, "Found " + notes.length + " popup notifications.");
> +    for (i=0; i < notes.length; i++) {

what's the reason of this change?

@@ +104,4 @@
>      var nb = chromeWin.getNotificationBox(window.top);
>      notes = nb.allNotifications;
>      ok(true, "Found " + notes.length + " notification bars.");
> +    for ( i = 0; i < notes.length; i++) {

here too.

::: toolkit/components/places/PlacesRemoteTabsAutocompleteProvider.jsm
@@ +49,4 @@
>    if (weaveXPCService.ready) {
>      let engine = Weave.Service.engineManager.get("tabs");
>  
> +    for (let [guid, client] in Object.entries(engine.getAllClients())) {

replace "in" with "of".
Object.entries returns an Array, so it should iterate over it with for-of.
here and most of remaining files.

::: toolkit/components/places/tests/autocomplete/head_autocomplete.js
@@ +268,4 @@
>  
>    Task.spawn(function* () {
>      // Iterate over all tasks and execute them
> +    for (let [, [fn, args]] in Object.entries(gNextTestSetupTasks)) {

gNextTestSetupTasks is an Array, so this can be re-written to:
  for (let [fn, args] of gNextTestSetupTasks) {

::: toolkit/components/places/tests/queries/test_async.js
@@ +356,4 @@
>  
>  add_task(function* test_async()
>  {
> +  for (let [, test] in Object.entries(tests)) {

here too, tests is an Array, so
 for (let test of tests) {

::: toolkit/components/places/tests/queries/test_sorting.js
@@ +1271,4 @@
>  
>  add_task(function* test_sorting()
>  {
> +  for (let [, test] in Object.entries(tests)) {

here Array too

::: toolkit/components/places/tests/unit/test_463863.js
@@ +48,4 @@
>  add_task(function* test_execute()
>  {
>    // add visits, one for each transition type
> +  for (let [, transition] in Object.entries(transitions)) {

Here Array too

::: toolkit/components/places/tests/unit/test_adaptive.js
@@ +384,4 @@
>    // Disable autoFill for this test.
>    Services.prefs.setBoolPref("browser.urlbar.autoFill", false);
>    do_register_cleanup(() => Services.prefs.clearUserPref("browser.urlbar.autoFill"));
> +  for (let [, test] in Object.entries(tests)) {

here Array too

::: toolkit/components/places/tests/unit/test_preventive_maintenance.js
@@ +1327,4 @@
>    stmt.finalize();
>    do_check_true(defaultBookmarksMaxId > 0);
>  
> +  for (let [, test] in Object.entries(tests)) {

here Array too

::: toolkit/content/tests/unit/test_contentAreaUtils.js
@@ +62,4 @@
>      "SaveLinkTitle",
>    ];
>  
> +  for (let [, filePickerTitleKey] in Object.entries(validFilePickerTitleKeys)) {

here Array too

::: toolkit/modules/tests/xpcshell/test_sqlite_shutdown.js
@@ +1,1 @@
> +f/* Any copyright is dedicated to the Public Domain.

please remove this change

Comment 4

[kashyap]

Attachment: updated-patch

Comment 5

[Tooru Fujisawa [:arai]]

Comment on attachment 8777668 [details] [diff] [review]
updated-patch

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

Thank you again.

::: toolkit/components/passwordmgr/test/notification_common.js
@@ +1,2 @@
>  /*
> + * Initialization: for each test, remove any prior notifications. */

Please remove whole changes to this file.
This patch is still changing style without good reason.

::: toolkit/components/places/tests/autocomplete/head_autocomplete.js
@@ +268,4 @@
>  
>    Task.spawn(function* () {
>      // Iterate over all tasks and execute them
> +    for (let [, [fn, args]] of Object.entries(gNextTestSetupTasks)) {

gNextTestSetupTasks is an Array, so this can be re-written to:
  for (let [fn, args] of gNextTestSetupTasks) {

::: toolkit/components/places/tests/queries/test_async.js
@@ +342,4 @@
>     */
>    _makeDataWithDefaults: function DH__makeDataWithDefaults(aData, aDefaults) {
>      let dat = {};
> +    for (let [prop, val] in Object.entries(aDefaults)) {

please use for-of
  for (let [prop, val] of Object.entries(aDefaults)) {

@@ +356,4 @@
>  
>  add_task(function* test_async()
>  {
> +  for (let [, test] of Object.entries(tests)) {

here too, tests is an Array, so
  for (let test of tests) {

::: toolkit/components/places/tests/queries/test_sorting.js
@@ +1271,4 @@
>  
>  add_task(function* test_sorting()
>  {
> +  for (let [, test] of Object.entries(tests)) {

here Array too

::: toolkit/components/places/tests/queries/test_tags.js
@@ +300,4 @@
>    };
>  
>    do_print("Add visits and tag the URIs");
> +  for (let [pURI, tags] in Object.entries(urisAndTags)) {

please use "of"

@@ +335,4 @@
>                     "http://example.com/3"]);
>  
>    // Clean up.
> +  for (let [pURI, tags] in Object.entries(urisAndTags)) {

of

@@ +352,4 @@
>    };
>  
>    do_print("Add bookmarks and tag the URIs");
> +  for (let [pURI, tags] in Object.entries(urisAndTags)) {

of

@@ +392,4 @@
>                     "http://example.com/3"]);
>  
>    // Clean up.
> +  for (let [pURI, tags] in Object.entries(urisAndTags)) {

of

@@ +472,4 @@
>    }
>  
>    do_print("Add visits and tag the URIs");
> +  for (let [pURI, tags] in Object.entries(urisAndTags)) {

of

@@ +519,4 @@
>    queryResultsAre(root, ["http://example.com/1", "http://example.com/2"]);
>  
>    // Clean up.
> +  for (let [pURI, tags] in Object.entries(urisAndTags)) {

of

::: toolkit/components/places/tests/unit/test_000_frecency.js
@@ +70,4 @@
>    // generate a date within the cutoff period
>    var dateInPeriod = (now - ((cutoff - 1) * 86400 * 1000)) * 1000;
>  
> +  for (let [bonusName, visitType] in Object.entries(bonusPrefs)) {

of

::: toolkit/components/places/tests/unit/test_463863.js
@@ +48,4 @@
>  add_task(function* test_execute()
>  {
>    // add visits, one for each transition type
> +  for (let [, transition] in Object.entries(transitions)) {

Here Array too

::: toolkit/components/places/tests/unit/test_adaptive.js
@@ +384,4 @@
>    // Disable autoFill for this test.
>    Services.prefs.setBoolPref("browser.urlbar.autoFill", false);
>    do_register_cleanup(() => Services.prefs.clearUserPref("browser.urlbar.autoFill"));
> +  for (let [, test] of Object.entries(tests)) {

here Array too

::: toolkit/components/places/tests/unit/test_preventive_maintenance.js
@@ +1327,4 @@
>    stmt.finalize();
>    do_check_true(defaultBookmarksMaxId > 0);
>  
> +  for (let [, test] of Object.entries(tests)) {

here Array too

::: toolkit/content/tests/chrome/test_bug253481.xul
@@ +71,4 @@
>                         .getService(Components.interfaces.nsIClipboardHelper);
>    clip.copyString(testString);
>  }, function() {
> +  for (let [item, expected] in Object.entries(expectedResults)) {

of

::: toolkit/content/tests/unit/test_contentAreaUtils.js
@@ +62,4 @@
>      "SaveLinkTitle",
>    ];
>  
> +  for (let [, filePickerTitleKey] of Object.entries(validFilePickerTitleKeys)) {

here Array too

::: toolkit/crashreporter/CrashSubmit.jsm
@@ +277,4 @@
>      let formData = Cc["@mozilla.org/files/formdata;1"]
>                     .createInstance(Ci.nsIDOMFormData);
>      // add the data
> +    for (let [name, value] in Object.entries(this.extraKeyVals)) {

of

::: toolkit/modules/Preferences.jsm
@@ +101,4 @@
>   */
>  Preferences.set = function(prefName, prefValue) {
>    if (isObject(prefName)) {
> +    for (let [name, value] in Object.entries(prefName))

of

::: toolkit/modules/tests/xpcshell/test_session_recorder.js
@@ +241,4 @@
>    let sessions = recorder.getPreviousSessions();
>    do_check_eq(Object.keys(sessions).length, 10);
>  
> +  for (let [i, session] in Object.entries(sessions)) {

of

::: toolkit/modules/tests/xpcshell/test_sqlite.js
@@ +43,4 @@
>  function getConnection(dbName, extraOptions={}) {
>    let path = dbName + ".sqlite";
>    let options = {path: path};
> +  for (let [k, v] in Object.entries(extraOptions)) {

of

@@ +59,4 @@
>    let c = yield getConnection(name, extraOptions);
>    c._initialStatementCount = 0;
>  
> +  for (let [k, v] in Object.entries(TABLES)) {

of

@@ +76,4 @@
>    let c = yield getConnection(name, extraOptions);
>    c._initialStatementCount = 0;
>  
> +  for (let [k, v] in Object.entries(TABLES)) {

of

::: toolkit/modules/tests/xpcshell/test_sqlite_shutdown.js
@@ +18,4 @@
>  function getConnection(dbName, extraOptions={}) {
>    let path = dbName + ".sqlite";
>    let options = {path: path};
> +  for (let [k, v] in Object.entries(extraOptions)) {

of

@@ +34,4 @@
>    let c = yield getConnection(name, extraOptions);
>    c._initialStatementCount = 0;
>  
> +  for (let [k, v] in Object.entries(TABLES)) {

of

::: toolkit/mozapps/downloads/content/downloads.js
@@ +415,4 @@
>    // convert strings to those in the string bundle
>    let sb = document.getElementById("downloadStrings");
>    let getStr = string => sb.getString(string);
> +  for (let [name, value] in Object.entries(gStr))

of

::: toolkit/mozapps/downloads/tests/chrome/test_unknownContentType_dialog_layout.xul
@@ +82,4 @@
>  }
>  
>  function checkWindow(win) {
> +  for (let [id, props] in Object.entries(tests[testIndex].elements)) {

of

@@ +86,2 @@
>      let elem = win.dialog.dialogElement(id);
> +    for (let [prop, value] in Object.entries(props)) {

of

::: toolkit/mozapps/downloads/tests/unit/test_syncedDownloadUtils.js
@@ +21,4 @@
>    // Pretend we're a download status bar also asking for a time left, but we're
>    // using a different "last sec". We need to make sure we get the same time.
>    let lastSec = 314;
> +  for (let [time, text] in Object.entries(downloadTimes))

of

Comment 6

[kashyap]

Attachment: patch3.patch

Comment 7

[Tooru Fujisawa [:arai]]

Comment on attachment 8778081 [details] [diff] [review]
patch3.patch

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

Thanks!
It's almost there.

::: toolkit/components/passwordmgr/test/notification_common.js
@@ +91,4 @@
>      ok(true, "is popup panel open? " + container.isPanelOpen);
>      var notes = container._currentNotifications;
>      ok(true, "Found " + notes.length + " popup notifications.");
> +    for (let i=0; i < notes.length; i++) {

please revert this.

you can check the diff with the following commands:
  if it's committed:
    hg log -pr . toolkit/components/passwordmgr/test/notification_common.js
  if it's not yet committed:
    hg diff toolkit/components/passwordmgr/test/notification_common.js

::: toolkit/components/places/tests/autocomplete/head_autocomplete.js
@@ +268,4 @@
>  
>    Task.spawn(function* () {
>      // Iterate over all tasks and execute them
> +    for (let [fn, args]] of gNextTestSetupTasks) {

please remove extra "]"
  for (let [fn, args] of gNextTestSetupTasks) {

Comment 8

[kashyap]

Attachment: patch4.patch

Comment 9

[Tooru Fujisawa [:arai]]

Comment on attachment 8778132 [details] [diff] [review]
patch4.patch

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

Thank you again! Looks fine.
and it passes try run
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbf9b4f8a1fe

Forwarding to mossop, as I'm not a module owner/peer here.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/94a0f0b6dadd
Replace in-tree consumer of non-standard Iterator() with Object.{values,entries} in toolkit/. r=dtownsend

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/94a0f0b6dadd