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

RESOLVED FIXED in Firefox 51

Status

P2
normal
RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: arai, Assigned: divyanshu.divix, Mentored)

Tracking

({good-first-bug})

Trunk
Firefox 51
good-first-bug

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [good first bug] [lang=js])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

2 years ago
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
Priority: -- → P2
(Assignee)

Comment 1

2 years ago
Created attachment 8780820 [details] [diff] [review]
Replace Iterator() with Object.{entries,values}

Replace simple in-tree consumer of non-standard Iterator() with Object.{values,entries} in devtools/
Flags: needinfo?(arai.unmht)
Attachment #8780820 - Flags: review?(arai.unmht)
(Reporter)

Comment 2

2 years ago
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.
Attachment #8780820 - Flags: review?(arai.unmht) → feedback+
(Reporter)

Updated

2 years ago
Flags: needinfo?(arai.unmht)
(Assignee)

Comment 3

2 years ago
Created attachment 8780866 [details] [diff] [review]
Replace Iterator() with Object.{entries,values}

Replace simple in-tree consumer of non-standard Iterator() with Object.{values,entries} in devtools/
Attachment #8780820 - Attachment is obsolete: true
Attachment #8780866 - Flags: review?(arai.unmht)
(Reporter)

Comment 4

2 years ago
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.
Attachment #8780866 - Flags: review?(jryans)
Attachment #8780866 - Flags: review?(arai.unmht)
Attachment #8780866 - Flags: feedback+
(Reporter)

Updated

2 years ago
Assignee: nobody → divyanshu.divix
Status: NEW → ASSIGNED
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!
Attachment #8780866 - Flags: review?(jryans) → review+
(Reporter)

Comment 7

2 years ago
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
> ...
(Reporter)

Comment 8

2 years ago
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.
(Reporter)

Comment 9

2 years ago
can you fix them?
Flags: needinfo?(divyanshu.divix)
[good first bug] whiteboard -> keyword mass change
Keywords: good-first-bug
(Assignee)

Comment 11

2 years ago
Created attachment 8782878 [details] [diff] [review]
Replace 'in' with 'of' in Object.entries in devtools

Replace 'in' with 'of' as suggested in devtools/client/shared/widgets/Chart.jsm
Flags: needinfo?(divyanshu.divix) → needinfo?(arai.unmht)
Attachment #8782878 - Flags: review?(arai.unmht)
(Reporter)

Comment 12

2 years ago
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?
Attachment #8782878 - Flags: review?(arai.unmht) → feedback+
(Reporter)

Updated

2 years ago
Flags: needinfo?(arai.unmht)
(Assignee)

Comment 13

2 years ago
Created attachment 8782889 [details] [diff] [review]
Replace Iterator() with Object.{entries,values}

Replace simple in-tree consumer of non-standard Iterator() with Object.{values,entries} in devtools/
Attachment #8780866 - Attachment is obsolete: true
Attachment #8782878 - Attachment is obsolete: true
Flags: needinfo?(arai.unmht)
Attachment #8782889 - Flags: review?(arai.unmht)
(Reporter)

Comment 14

2 years ago
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.
Attachment #8782889 - Flags: review?(jryans)
Attachment #8782889 - Flags: review?(arai.unmht)
Attachment #8782889 - Flags: feedback+
Attachment #8782889 - Flags: review?(jryans) → review+

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9d99c3e55902
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51

Updated

6 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.