Closed Bug 1293002 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox for Android :: General, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: arai, Assigned: tuhinatwyla, Mentored)

References

Details

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

Attachments

(1 file, 1 obsolete file)

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%3Amobile+-path%3Anews.bbcimg.co.uk&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(obj)) {
    ...
  }
can be replaced to:
  for (let v of Object.values(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
Hi I would like to work on this bug. It will be my first bug.
Thank you for your comment :)

If you have any question related to the required fix, building, or testing, feel free to ask here,
or ping me (:arai) in IRC #introduction channel
Priority: -- → P3
I found two occurrences of Iterator(obj) in the code. I have replaced them with Object.entries and Object.values as mentioned above. How do I test it? Do I write automated tests? The build is successful.
Edit : I have changed all the occurences of Iterator(obj). I have replaced them with Object.entries and Object.values as mentioned above. How do I test it? Do I write automated tests? The build is successful.
(In reply to tuhina chatterjee from comment #4)
> Edit : I have changed all the occurences of Iterator(obj). I have replaced
> them with Object.entries and Object.values as mentioned above. How do I test
> it? Do I write automated tests? The build is successful.

Unfortunately, there seems to be no existing automated test that tests the related code.
It would be great if you can write a testcase for it, but for this case, this code seems to be tested manually for previous changes.

according to bug 630341 and bug 1035439 (that added those Iterator), here are steps for manual test

steps for bug 630341
1. run Firefox for Android
2. open "Setting"
3. in "Advanced" pane, "Restore tabs" option, choose "Always restore"
4. close "Setting"
5. open slashdot.org
6. hit Home button to suspend Firefox for Android
7. terminate Firefox for Android process
8. restart Firefox for Android
   (the old tab is opened as part of session restore)
9. close the tab
10. confirm Slashdot is in "RECENT TABS" in home screen
11. open "Setting"
12. in "Clear Private Data", choose "Clear now"
13. choose everithing , "Clear now" dialog, and hit "CLEAR DATA"
14. hit Home button to suspend Firefox for Android
15. terminate Firefox for Android process
16. restart Firefox for Android
17. confirm Slashdot is *not* in "RECENT TABS" in home screen

steps for bug 1035439
1. run Firefox for Android
2. open bugzilla.mozilla.org
3. close the tab
4. create a "New Private Tab"
5. click URL bar
6. confirm bugzilla.mozilla.org is not in "RECENT TABS" in home screen


If the result with those operations doesn't change after applying patch, the code change should be okay.
Attached patch Changes (obsolete) — Splinter Review
Bug fix replacing Iterator() with Object.entries or Object.values in mobile/ directory.
Please review this.
Comment on attachment 8779504 [details] [diff] [review]
Changes

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

Thank you for your patch :)

The change looks fine.
Can you export it as a patch file? ("hg export" command, or "git format-patch" command)
this file doesn't have commit message, nor author information.
Attachment #8779504 - Flags: feedback+
Attached patch hg_exportSplinter Review
Added patch (created using hg export)
Comment on attachment 8779653 [details] [diff] [review]
hg_export

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

Thanks again :)
It's perfect.

for the next time posting a patch, please set "review" flag to "?" and choose reviewer (Requestee) from suggested reviewers,
and also mark previous patch "obsolete".

anyway, forwarding to liuche, as I'm not a module owner/peer here.
Attachment #8779653 - Flags: review?(liuche)
Attachment #8779504 - Attachment is obsolete: true
Can I be assigned to the bug ?
yes
thanks :)
Assignee: nobody → tuhinatwyla
Status: NEW → ASSIGNED
Comment on attachment 8779653 [details] [diff] [review]
hg_export

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

Looks fine to me.

Please upload a version of the patch with the addition of r=reviewer (in this case, me, liuche) as the patch message. That way, when looking at commits in-tree, it's possible to tell who reviewed the code.
Attachment #8779653 - Flags: review?(liuche) → review+
Sure liuche. I will keep that in mind the next time.
Thank you :)

Sheriff will add "r=reviewer" part when landing a patch.
Keywords: checkin-needed
oops, I forgot try run, sorry.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d0a625ee469

will add checkin-needed again when it finishes.
Keywords: checkin-needed
okay, it passed try run.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/e336054c4c85
Replace in-tree consumer of non-standard Iterator() with Object.{values,entries} in mobile/. r=liuche
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e336054c4c85
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.