Closed
Bug 1292998
Opened 6 years ago
Closed 6 years ago
Replace simple in-tree consumer of non-standard Iterator() with Object.{values,entries} in devtools/
Categories
(DevTools :: General, defect, P2)
DevTools
General
Tracking
(firefox51 fixed)
RESOLVED
FIXED
Firefox 51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: arai, Assigned: divyanshu.divix, Mentored)
References
Details
(Keywords: good-first-bug, Whiteboard: [good first bug] [lang=js])
Attachments
(1 file, 3 obsolete files)
9.34 KB,
patch
|
jryans
:
review+
arai
:
feedback+
|
Details | Diff | Splinter Review |
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•6 years ago
|
||
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•6 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•6 years ago
|
Flags: needinfo?(arai.unmht)
Assignee | ||
Comment 3•6 years ago
|
||
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•6 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•6 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 6•6 years ago
|
||
Here's try run for the patch. https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdcb190c3fe8
Reporter | ||
Comment 7•6 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•6 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.
Assignee | ||
Comment 11•6 years ago
|
||
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•6 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•6 years ago
|
Flags: needinfo?(arai.unmht)
Assignee | ||
Comment 13•6 years ago
|
||
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•6 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+
Reporter | ||
Comment 15•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bab954b51017
Flags: needinfo?(arai.unmht)
Attachment #8782889 -
Flags: review?(jryans) → review+
Reporter | ||
Comment 16•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d99c3e55902f1a68a409f52126e99a9712439f9 Bug 1292998 - Replace Iterator() with Object.{entries,values}. r=jryans
Comment 17•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9d99c3e55902
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Updated•4 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•