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

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Tooru Fujisawa (away) [:arai]]

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

The usage of non-standard Iterator() should be replaced with Object.entries or Object.values or something standard.

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

Comment 1

[Tooru Fujisawa (away) [: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

[Sumit Tiwari]

This is quite a suitable bug/enhancement for a beginner such as myself and I am willing to take it up. However, I don't really know which suite of tests to run for qualifying these changes. Any help?

Comment 3

[Tooru Fujisawa (away) [:arai]]

(In reply to Sumit Tiwari from comment #2)
> This is quite a suitable bug/enhancement for a beginner such as myself and I
> am willing to take it up. However, I don't really know which suite of tests
> to run for qualifying these changes. Any help?

Thank you for your response :)

There are several testsuites, and which one to use depends on each file:
  https://developer.mozilla.org/en-US/docs/Mozilla/QA/Automated_testing

There are 2 ways to find corresponding testcase.

1. In most case, test files should be added in the same changeset as the file was added or modified.
for example, browser/base/content/browser-gestureSupport.js was added by the following changeset:
  https://hg.mozilla.org/mozilla-central/rev/69c2332c6f77
and there browser_gestureSupport.js file was added under "test" directory:
  https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_gestureSupport.js
  https://dxr.mozilla.org/mozilla-central/rev/6b65dd49d4f045c0a9753ce60bdb4b7b4aaedcf8/browser/base/content/test/general/browser.ini#313
it's a testcase for Browser chrome tests:
  https://developer.mozilla.org/en-US/docs/Mozilla/Browser_chrome_tests

so, you can run it by the following command, after building Firefox:
  ./mach mochitest browser/base/content/test/general/browser_gestureSupport.js


2. find "test" or "tests" directory, or "mochitest.ini", "browser.ini", "chrome.ini", or "xpcshell.ini" file near the file.
each ini file corresponds to each testsuite:
  mochitest.ini -- Mochitest  https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Mochitest
  browser.ini   -- Browser chrome tests  https://developer.mozilla.org/en-US/docs/Mozilla/Browser_chrome_tests
  chrome.ini    -- Chrome tests  https://developer.mozilla.org/en-US/docs/Chrome_tests
  xpcshell.ini  -- xpcshell test  https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests

for example, browser/base/content/test/popupNotifications/head.js, there is browser.ini in the same directory (as head.js is a file for test)
  https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/popupNotifications/browser.ini
you can run entire testcases under this directory by the following command, after building Firefox:
  ./mach mochitest browser/base/content/test/popupNotifications/

another example, browser/components/places/PlacesUIUtils.jsm, there is "tests" directory in parent directory:
  https://dxr.mozilla.org/mozilla-central/source/browser/components/places
  https://dxr.mozilla.org/mozilla-central/source/browser/components/places/tests
"browser" directory contains browser.ini, so you can run them by the following command:
  ./mach mochitest browser/components/places/tests/browser
"chrome" directory contains chrome.ini, so you can run them by the following command:
  ./mach mochitest browser/components/places/tests/chrome
and "unit" directory contains xpcshell.ini, so you can run them by the following command:
  ./mach xpcshell-test browser/components/places/tests/unit


anyway, if it takes long time to run those tests, feel free to post the patch here and ping me, I'll push your patch to automation server that runs whole automated tests.

Comment 4

[Sumit Tiwari]

(In reply to Tooru Fujisawa [:arai] from comment #3)
 
> There are several testsuites, and which one to use depends on each file:
>   https://developer.mozilla.org/en-US/docs/Mozilla/QA/Automated_testing

That was the first link that I read when I started reading up on how I could contribute, and it still feels just as daunting.

Anyway, thanks a ton for all the information; I'll get started on it right away. Most of the changes would be simple string substitutions, but there's a few that might be a little more than that. I'll push out a review sometime tomorrow and include you on it.

Comment 5

[Sumit Tiwari]

Attachment: Bug 1290698 - Replace Iterator() with Object.{values, entries};

Review commit: https://reviewboard.mozilla.org/r/69886/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69886/

Comment 6

[Tooru Fujisawa (away) [:arai]]

Comment on attachment 8778726 [details]
Bug 1290698 - Replace Iterator() with Object.{values, entries};

https://reviewboard.mozilla.org/r/69886/#review67002

Thank you for your patch :)

This looks almost good, but needs some fix.
please address the following comments and ask review again.

::: browser/components/customizableui/CustomizableUI.jsm:1981
(Diff revision 1)
>        gRestoring = true;
>  
>        let restored = false;
>        if (placementsPreexisted) {
>          log.debug("Restoring " + aArea + " from pre-existing placements");
> -        for (let [position, id] in Iterator(gPlacements.get(aArea))) {
> +        for (let [position, id] of Object.entries(gPlacements.get(aArea))) {

`gPlacements.get(aArea)` returns an array.
so you can iterate over it by
```
  for (let [position, id] of gPlacements.get(aArea).entries()) {
    ...
  }
```

::: browser/components/downloads/content/allDownloadsViewOverlay.js:1031
(Diff revision 1)
>      // and adding it back when we're done.
>  
>      // Hack for bug 836283: reset xbl fields to their old values after the
>      // binding is reattached to avoid breaking the selection state
>      let xblFields = new Map();
> -    for (let [key, value] in Iterator(this._richlistbox)) {
> +    for (let [key, value] of Object.entries(this._richlistbox)) {

This case is somewhat tricky.

`this._richlistbox` is a XUL element, that is not a plain object, and calling `Iterator` with it returns a Generator that iterates over its enumerable own properties.
But caling `Object.entries` with it returns an Array with enumerable properties that includes not-owned properties.

So, this case, we need to iterate only over own property names, with:
```
  for (let key of Object.getOwnPropertyNames(this._richlistbox)) {
```
and then get value by
```
    let value = this._richlistbox[key];
```

::: browser/modules/BrowserUITelemetry.jsm:248
(Diff revision 1)
>     */
>    _ensureObjectChain: function(aKeys, aEndWith, aRoot) {
>      let current = aRoot;
>      let parent = null;
>      aKeys.unshift(this._bucket);
> -    for (let [i, key] of Iterator(aKeys)) {
> +    for (let [i, key] of Object.entries(aKeys)) {

aKeys is an array, so we can iterate over it by
```
  for (let [i, key] of aKeys.entries()) {
```

Comment 7

[Sumit Tiwari]

Comment on attachment 8778726 [details]
Bug 1290698 - Replace Iterator() with Object.{values, entries};

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69886/diff/1-2/

Comment 8

[Tooru Fujisawa (away) [:arai]]

Comment on attachment 8778726 [details]
Bug 1290698 - Replace Iterator() with Object.{values, entries};

https://reviewboard.mozilla.org/r/69886/#review67156

Thank you again :)
Looks good.

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

Comment 9

[Tooru Fujisawa (away) [:arai]]

Comment on attachment 8778726 [details]
Bug 1290698 - Replace Iterator() with Object.{values, entries};

looks like I failed to redirect the review request.

Comment 10

[Sumit Tiwari]

(In reply to Tooru Fujisawa [:arai] from comment #8)
> Comment on attachment 8778726 [details]
> Bug 1290698 - Replace Iterator() with Object.{values, entries};
> 
> https://reviewboard.mozilla.org/r/69886/#review67156
> 
> Thank you again :)
> Looks good.
> 
> Forwarding to mossop, as I'm not a module owner/peer here.

Thank you for all the feedback; I'll keep it in mind as I try to remove Iterator() from all the other folders as well. :)

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 8778726 [details]
Bug 1290698 - Replace Iterator() with Object.{values, entries};

https://reviewboard.mozilla.org/r/69886/#review67710

Comment 12

[Tooru Fujisawa (away) [:arai]]

Thanks!

here's try run
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e44fe871b9b1

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a88d448eac1
Replace Iterator() with Object.{values, entries}; r=mossop

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0a88d448eac1