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

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: arai, Assigned: sumi29, Mentored)

Tracking

Trunk
Firefox 51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 affected, firefox51 fixed)

Details

Attachments

(1 attachment)

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
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
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?
(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.
(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 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()) {
```
Attachment #8778726 - Flags: review?(arai.unmht)
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.
Attachment #8778726 - Flags: review?(dtownsend)
Comment on attachment 8778726 [details]
Bug 1290698 - Replace Iterator() with Object.{values, entries};

looks like I failed to redirect the review request.
Attachment #8778726 - Flags: review?(arai.unmht) → feedback+
(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. :)
Assignee: nobody → sumi29
Status: NEW → ASSIGNED
Comment on attachment 8778726 [details]
Bug 1290698 - Replace Iterator() with Object.{values, entries};

https://reviewboard.mozilla.org/r/69886/#review67710
Attachment #8778726 - Flags: review?(dtownsend) → review+
Pushed by arai_a@mac.com:
https://hg.mozilla.org/integration/autoland/rev/0a88d448eac1
Replace Iterator() with Object.{values, entries}; r=mossop
https://hg.mozilla.org/mozilla-central/rev/0a88d448eac1
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
You need to log in before you can comment on or make changes to this bug.