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

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

[Tooru Fujisawa [:arai]]

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

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

Here's the rough list for Iterator() usage (not exhaustive)
https://dxr.mozilla.org/mozilla-central/search?q=%22+Iterator(%22+path%3Adevtools+-path%3Avendor%2Fimmutable.js+-path%3Adebugger%2Fnew%2Fbundle.js+-path%3Atest-bug-632347-iterators-generators.html&redirect=false

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

another example:
  for (let [, v] in Iterator(array)) {
    ...
  }
can be replaced to:
  for (let v of array) {
    ...
  }

There is one more usage in test-bug-632347-iterators-generators.html
https://dxr.mozilla.org/mozilla-central/rev/6b65dd49d4f045c0a9753ce60bdb4b7b4aaedcf8/devtools/client/webconsole/test/test-bug-632347-iterators-generators.html#23
> _container.iter1 = Iterator(obj);
but it looks like it's testing Iterator itself, from browser_webconsole_bug_632347_iterators_generators.js
https://dxr.mozilla.org/mozilla-central/rev/6b65dd49d4f045c0a9753ce60bdb4b7b4aaedcf8/devtools/client/webconsole/test/browser_webconsole_bug_632347_iterators_generators.js#46
>   result = container.iter1.next();
so I'd like to leave it for now (maybe, until we actually remove Iterator).

[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 1

[Divyanshu Vishwakarma]

Attachment: Replace Iterator() with Object.{entries,values}

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

Comment 2

[Tooru Fujisawa [:arai]]

Comment on attachment 8780820 [details] [diff] [review]
Replace Iterator() with Object.{entries,values}

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

Thank you for your patch :)

please address the following comments and attach a fixed patch.

::: devtools/client/performance/test/unit/test_marker-blueprint.js
@@ +18,4 @@
>    ok(Object.keys(TIMELINE_BLUEPRINT).length,
>      "The timeline blueprint has at least one entry.");
>  
> +  for (let [key, value] of Object.entries(TIMELINE_BLUEPRINT)) {

The change itself looks good, but this conflicts with the following change:

https://hg.mozilla.org/mozilla-central/diff/6e88bf1b9a41/devtools/client/performance/test/unit/test_marker-blueprint.js

so please rebase onto latest m-c.

::: devtools/client/performance/views/details.js
@@ +75,4 @@
>        button.removeEventListener("command", this._onViewToggle);
>      }
>  
> +    for (let [, component] of Object.entries(this.components)) {

If you don't need key, you can use Object.values:

for (let component of Object.values(this.components)) {

@@ +204,4 @@
>      let selectedPanel = this.el.selectedPanel;
>      let selectedId = selectedPanel.id;
>  
> +    for (let [, { id, view }] of Object.entries(this.components)) {

Here too, you can use Object.values.

Comment 3

[Divyanshu Vishwakarma]

Attachment: Replace Iterator() with Object.{entries,values}

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

Comment 4

[Tooru Fujisawa [:arai]]

Comment on attachment 8780866 [details] [diff] [review]
Replace Iterator() with Object.{entries,values}

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

Thank you again :)

This looks good.
Forwarding to jryans, as I'm not a module owner/peer here.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8780866 [details] [diff] [review]
Replace Iterator() with Object.{entries,values}

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

Looks good to me, thanks for working on this!

Comment 6

[Tooru Fujisawa [:arai]]

Here's try run for the patch.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdcb190c3fe8

Comment 7

[Tooru Fujisawa [:arai]]

Some test failure happens:

https://treeherder.mozilla.org/logviewer.html#?job_id=25745643&repo=try#L2782
> 156 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_charts-03.js | The first column of the first row exists. - Got 0, expected label1
> 157 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_charts-03.js | The second column of the first row exists. - Got 1, expected label2
> ...

Comment 8

[Tooru Fujisawa [:arai]]

Comment on attachment 8780866 [details] [diff] [review]
Replace Iterator() with Object.{entries,values}

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

::: devtools/client/shared/widgets/Chart.jsm
@@ +381,4 @@
>      boxNode.setAttribute("name", rowInfo.label);
>      rowNode.appendChild(boxNode);
>  
> +    for (let [key, value] in Object.entries(rowInfo)) {

sorry, I overlooked this.
please replace "in" with "of".

@@ +399,4 @@
>    let totalsNode = document.createElement("vbox");
>    totalsNode.className = "table-chart-totals";
>  
> +  for (let [key, value] in Object.entries(totals)) {

here too.

Comment 9

[Tooru Fujisawa [:arai]]

can you fix them?

Comment 10

[Patrick Brosset <:pbro>]

[good first bug] whiteboard -> keyword mass change

Comment 11

[Divyanshu Vishwakarma]

Attachment: Replace 'in' with 'of' in Object.entries in devtools

Replace 'in' with 'of' as suggested in devtools/client/shared/widgets/Chart.jsm

Comment 12

[Tooru Fujisawa [:arai]]

Comment on attachment 8782878 [details] [diff] [review]
Replace 'in' with 'of' in Object.entries in devtools

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

Thanks :)
Can you fold this and the previous patch into single patch?

Comment 13

[Divyanshu Vishwakarma]

Attachment: Replace Iterator() with Object.{entries,values}

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

Comment 14

[Tooru Fujisawa [:arai]]

Comment on attachment 8782889 [details] [diff] [review]
Replace Iterator() with Object.{entries,values}

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

Thank you again :)
I'll push this to try shortly.

Comment 15

[Tooru Fujisawa [:arai]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bab954b51017

Comment 16

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d99c3e55902f1a68a409f52126e99a9712439f9
Bug 1292998 - Replace Iterator() with Object.{entries,values}. r=jryans

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9d99c3e55902