872576 - Stop using deprecated for each ... in

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

We should stop using for each ... in because it's deprecated:
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Statements/for_each...in

A quick search shows we use it in a bunch of places:
http://mxr.mozilla.org/mozilla-central/search?string=for+each+%28&find=%2Fmobile%2Fandroid%2F&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

For objects, we should should replace this with for ... of. For arrays, we should just use the built-in Array.forEach method.

Comment 1

[Sushant Hiray [:sushant]]

Hello,
I'm new here and well this bug seems to be a good way for me to start contributing. :)
I have some experience of Android coding and web-applications.
Although this bug essentially requires to change for each... in to for ... of, so it is pretty easy I guess.
Could you guide me regarding the same?

Comment 2

[:Margaret Leibovic]

Hi Sushant! First of all, do you have a Fennec build environment set up? If not, you should follow the directions here:
https://wiki.mozilla.org/Mobile/Fennec/Android

If you have questions, you should #mobile on IRC (irc.mozilla.org). It's also a good place to hang out and learn more about Fennec development :)

There are also more resources available here:
https://wiki.mozilla.org/Mobile/Get_Involved

With regards to this bug, you should only use for ... of when iterating over objects. For arrays you should use the built-in forEach method. This is a good bug to learn about some of the nuances of iteration features of JavaScript!

Comment 3

[Sushant Hiray [:sushant]]

Hi Margaret,

I finally managed to set up the environment!
Sigh! It takes a lot of time! :P

So now that this is done, could you guide me further on how to create a patch?
I'm pretty used to git but I haven't used Mercurial, so maybe some more inputs on this, if possible! :)

Apart from this am I ready to submit a pull request once I change the for loops for objects and arrays correspondingly?

Thanks!

Comment 4

[:Margaret Leibovic]

Glad you got things working! Here's some documentation on creating a patch:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

When you're ready to submit a patch, attach it to this bug and put my email address in the review? field.

And as always, let me know if you need any help!

Comment 5

[Nicolas Carlo [:nickecarlo]]

Is this bug assigned to someone or can I work on it? If I want to pick this bug up to try and work on it, what do I need to do apart from setting up Fennec (which I have done so)? If I understand correctly, this bug refers to all the .js files in the android folder. Is that correct? Apologies, I'm completely new and a complete noob.

Comment 6

[:Margaret Leibovic]

I believe Sushant is working on this, so I'll update the assignee field to reflect that.

Sushant, let us know if you're not working on this anymore, in which case Nicolas can take over.

Nicolas, let me know if you need help finding another bug to work on! You can find me in #mobile on irc.mozilla.org.

Comment 7

[Nicolas Carlo [:nickecarlo]]

Thanks Margaret. I am going to roam around the bugs area to see if there's something that I think I can work on, otherwise I'll find you in the IRC channel.

Comment 8

[Masatoshi Kimura [:emk]]

(In reply to :Margaret Leibovic from comment #0)
> For objects, we should should replace this with for ... of. For arrays, we
> should just use the built-in Array.forEach method.

Actually for ... of will work only with iterable objects (such as arrays).

[12:39:33.905] for (let o of {}) {}
[12:39:33.909] TypeError: ({}) is not iterable

That said, it is a bad practice to use for each ... in with random objects, so it should be rewritten anyway.

Comment 9

[Sushant Hiray [:sushant]]

@Margaret: Sorry for being inactive, I am out of town. I have created patches according to each file. I will attach them once I return back this evening. :)

Comment 10

[Sushant Hiray [:sushant]]

Attachment: This includes a small part of the complete patch from the file android/crome/content/browser.js

As @Masatoshi said, I felt the same thing after reading the documentation. I'm not entirely sure if those changes work correctly.So are there any testcases to check the correct functionality of for loops?

Comment 11

[:Margaret Leibovic]

Comment on attachment 755390 [details] [diff] [review]
This includes a small part of the complete patch from the file android/crome/content/browser.js

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

General comment: Please make sure to set your text editor to use two-space indentation (not tabs).

::: mobile/android/chrome/content/browser.js
@@ +1935,5 @@
>            this._addHTMLContextMenuItems(contextmenu, null);
>          }
>  
>          // then check for any context menu items registered in the ui
> +         items.forEach(function(item){

This won't work, since items isn't defined here. I think you meant to use |this.items|. Also, forEach won't work here, since this.items is an object:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1688

You probably want to iterate over this.items.keys. See:
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Object/keys?redirect=no

@@ +1941,1 @@
>              this.menuitems.push(item);

Now that you're wrapping this code in a function |this| isn't the same as it used to be. To use the same context, you should bind |this| to the function. See:
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Function/bind?redirect=no

@@ +1941,3 @@
>              this.menuitems.push(item);
>            }
> +	    });

To test this code, you should try to open a context menu and make sure it works.

@@ +6067,5 @@
>          case "textarea":
>            formData.push({ name: escapedName, value: escapedValue });
>            break;
>          case "select-one":
> +          for each (let option of el.options) {

for each .. of isn't a JS construct... I think you meant to switch this to for ... of as described in the bug.

Testing this code will probably be hard... we only go through this code path when adding a search engine by long tapping on an input. To test this specific test case, you should find (or create) a test case that has a select element in the form (where select.multiple = false).

Comment 12

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #0)

> For objects, we should should replace this with for ... of. For arrays, we
> should just use the built-in Array.forEach method.

Oops, this comment is wrong (as Masatoshi kindly pointed out above). We can use for ... of for arrays. For objects, we'll need to do something more complicated, such as iterating over the keys in the object.

Comment 13

[Sushant Hiray [:sushant]]

@Margaret Thanks for the review :)
I have got the errors I've made. Now I've a general doubt. Do I create a new patch which is in series with this patch or revert back to the original version of the code (i.e the cloned version).

I've read about the patches in series from the Mercurial Wiki here 
https://developer.mozilla.org/en-US/docs/Mercurial_Queues
but I could not find a way to revert back to the original code.

Comment 14

[Sushant Hiray [:sushant]]

Attachment: Corrected previous changes, used Object.keys() and for...of

I have corrected the changes you mentioned earlier.
I'm still a bit confused in the difference between using for...of and iterating over the Object keys. I'm still looking into it for a clearer perspective! :)

Comment 15

[:Margaret Leibovic]

Comment on attachment 757085 [details] [diff] [review]
Corrected previous changes, used Object.keys() and for...of

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

I'm worried that this bug may have been too challenging for a first patch... It seems that you don't totally understand the code that you're modifying here, and I'd encourage you to take some time to search through the code to figure out what kinds of values will be in these variables that you're using. I'd also encourage you to make sure you find a way to test this code as you're developing it. A simple way to do that is to add some logging with dump() statements and check to make sure things being printed to the console are what you expect.

Before reviewing another patch, I'd like to hear that you tested these changes locally. If it's too hard to test all of them, feel free to break these into smaller patches, and just upload ones that you know work correctly.

::: mobile/android/chrome/content/browser.js
@@ -974,5 @@
>  
>    getPreferences: function getPreferences(aPrefsRequest, aListen) {
>      let prefs = [];
> -
> -    for each (let prefName in aPrefsRequest.preferences) {

I believe aPrefsRequest.preferences is an array, so we should just be able to change this to:

for (let prefName of aPrefsRequest.preferences) {

You can verify this for yourself by following the code path to find how getPreferences() ends up being called. (I like using mxr.mozilla.org to do investigations like this.) You can also add some logging here to verify assumptions locally. This code gets called pretty frequently, but a sure way to test this would be to open the settings screen.

@@ +1971,5 @@
>          // then check for any context menu items registered in the ui
> +        var keys=Object.keys(this.items);
> +         for (var i=0; i < keys.length() ; i++){
> +          if (!this._getMenuItemForId(keys[i].id) && keys[i].matches(element, aX, aY)) {
> +            this.menuitems.push(keys[i]);

I think you're confused here... keys[i] is just going to return an item id (since keys is just an array of item ids). You still need to get the item out of this.items.

You could actually use for ... of to iterate through the array of ids. If you do that, you actually don't even need a local variable for keys. Something like this:

for (let itemId of Object.keys(this.items)) {

Then you can declare a local variable for the item:

let item = this.items[itemId];

Does that make sense?

Comment 16

[Masatoshi Kimura [:emk]]

> for (let itemId of Object.keys(this.items)) {

Neither for...of nor Object.keys is required.

  for (let itemId in this.items) {

Comment 17

[:Margaret Leibovic]

(In reply to Masatoshi Kimura [:emk] from comment #16)
> > for (let itemId of Object.keys(this.items)) {
> 
> Neither for...of nor Object.keys is required.
> 
>   for (let itemId in this.items) {

This is subtly different, in that it will iterate over inherited properties. I was debating whether or not we needed the extra safety of using Object.keys, and I thought it safe to err on the side of caution, although I now feel like I was probably being paranoid, since we do control the items object. I guess I was just trying to avoid a potential future foot-gun.

Comment 18

[Masatoshi Kimura [:emk]]

Oh, I didn't notice the difference. Although |for each...in| will also iterate over inherited properties, I agree Object.keys would be safer. Sorry for the confusion.

Comment 19

[:Margaret Leibovic]

(In reply to Masatoshi Kimura [:emk] from comment #18)
> Oh, I didn't notice the difference. Although |for each...in| will also
> iterate over inherited properties, I agree Object.keys would be safer. Sorry
> for the confusion.

It's okay, thanks for helping out with this bug :)

Comment 20

[Sushant Hiray [:sushant]]

@Margaret Thanks for explaining. I was not aware of MXR and was finding it difficult to figure out the exact type of the variables. I will now try to fix a small part and test it and then get back to you. 

I have a minor doubt though, how do I compile the code and generate an apk so that I can test it. If you could redirect me to some reference that will be great! :)

ps Thanks for being so patient! ^_^

(In reply to :Margaret Leibovic from comment #15)
> Comment on attachment 757085 [details] [diff] [review]
> Corrected previous changes, used Object.keys() and for...of
> 
> Review of attachment 757085 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm worried that this bug may have been too challenging for a first patch...
> It seems that you don't totally understand the code that you're modifying
> here, and I'd encourage you to take some time to search through the code to
> figure out what kinds of values will be in these variables that you're
> using. I'd also encourage you to make sure you find a way to test this code
> as you're developing it. A simple way to do that is to add some logging with
> dump() statements and check to make sure things being printed to the console
> are what you expect.
> 
> Before reviewing another patch, I'd like to hear that you tested these
> changes locally. If it's too hard to test all of them, feel free to break
> these into smaller patches, and just upload ones that you know work
> correctly.
> 
> ::: mobile/android/chrome/content/browser.js
> @@ -974,5 @@
> >  
> >    getPreferences: function getPreferences(aPrefsRequest, aListen) {
> >      let prefs = [];
> > -
> > -    for each (let prefName in aPrefsRequest.preferences) {
> 
> I believe aPrefsRequest.preferences is an array, so we should just be able
> to change this to:
> 
> for (let prefName of aPrefsRequest.preferences) {
> 
> You can verify this for yourself by following the code path to find how
> getPreferences() ends up being called. (I like using mxr.mozilla.org to do
> investigations like this.) You can also add some logging here to verify
> assumptions locally. This code gets called pretty frequently, but a sure way
> to test this would be to open the settings screen.
> 
> @@ +1971,5 @@
> >          // then check for any context menu items registered in the ui
> > +        var keys=Object.keys(this.items);
> > +         for (var i=0; i < keys.length() ; i++){
> > +          if (!this._getMenuItemForId(keys[i].id) && keys[i].matches(element, aX, aY)) {
> > +            this.menuitems.push(keys[i]);
> 
> I think you're confused here... keys[i] is just going to return an item id
> (since keys is just an array of item ids). You still need to get the item
> out of this.items.
> 
> You could actually use for ... of to iterate through the array of ids. If
> you do that, you actually don't even need a local variable for keys.
> Something like this:
> 
> for (let itemId of Object.keys(this.items)) {
> 
> Then you can declare a local variable for the item:
> 
> let item = this.items[itemId];
> 
> Does that make sense?

Comment 21

[:Margaret Leibovic]

(In reply to Sushant from comment #20)
> @Margaret Thanks for explaining. I was not aware of MXR and was finding it
> difficult to figure out the exact type of the variables. I will now try to
> fix a small part and test it and then get back to you. 
> 
> I have a minor doubt though, how do I compile the code and generate an apk
> so that I can test it. If you could redirect me to some reference that will
> be great! :)

:( You haven't been testing these patches?

The documentation I linked to in comment 2 explains how to build and install an APK. I thought that you said you got that working in comment 3?

Comment 22

[:Margaret Leibovic]

Sushant, are you still working on this? If not, we should reset the assignee in case someone else wants to pick this up.

Comment 23

[Sushant Hiray [:sushant]]

@Margaret I guess I won't be able to work on this bug. You can assign it to somebody else. Good luck. :)

Comment 24

[:Margaret Leibovic]

(In reply to Sushant from comment #23)
> @Margaret I guess I won't be able to work on this bug. You can assign it to
> somebody else. Good luck. :)

It's okay, thanks for the work you put in! I'm sure it will help the next person who wants to pick this up.

Comment 25

[Geoffrey Thomas]

By the way, I see a bunch more uses of "for each" outside the mobile directory (per your MXR search). Is there a corresponding bug for doing this in other products / are patches welcome to get rid of it elsewhere? Bug 791348 looks like it might be this bug in general, but I can't quite tell.

Comment 26

[:Margaret Leibovic]

(In reply to Geoffrey Thomas from comment #25)
> By the way, I see a bunch more uses of "for each" outside the mobile
> directory (per your MXR search). Is there a corresponding bug for doing this
> in other products / are patches welcome to get rid of it elsewhere? Bug
> 791348 looks like it might be this bug in general, but I can't quite tell.

Yes, that bug looks like a more general bug (or a bug about desktop Firefox in particular). This bug is just about Firefox for Android, and I think it makes sense to keep it that way, since the reviewers for these different modules are different.

Comment 27

[Michael Boon]

Hey,

I don't have much experience with JS and I'd be aiming to do some Java but the guys in IRC said that this would be a good first bug for me. If you're OK with this then I'd like to give this a try (otherwise I'm open to suggestions for other bugs).

I'm almost done with setting up the environment so I'll spend some time over the weekend.

Comment 28

[:Margaret Leibovic]

Welcome, Michael! I'll assign this bug to you.

Have you made any progress? I think following the comments in here should give you pretty good guidance, but you can find me on IRC if you have questions!

Comment 29

[Michael Boon]

Attachment: Feedback for first attempt

Comment 30

[Michael Boon]

Looking at that recent patch it seems like only 1 file has changed. I actually did 3/5 of the changes over 2 files. Am I right in thinking I've messed up here?

Comment 31

[:Margaret Leibovic]

Comment on attachment 773584 [details] [diff] [review]
Feedback for first attempt

To request feedback, you should set the feedback flag with a ? and a person you'd like feedback from. Sorry I didn't explain that clearly before!

Giving a patch a feedback+ means you yourself think it's good and on its way to review+ :)

Comment 32

[:Margaret Leibovic]

(In reply to Michael from comment #30)
> Looking at that recent patch it seems like only 1 file has changed. I
> actually did 3/5 of the changes over 2 files. Am I right in thinking I've
> messed up here?

I only see one change in the patch. What steps did you follow to generate the patch? We usually use mercurial queues to manage/generate patches. There are directions for that here:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Maybe you forgot to qref before exporting the patch?

Comment 33

[:Margaret Leibovic]

Comment on attachment 773584 [details] [diff] [review]
Feedback for first attempt

It looks like this is a patch on top of another patch... feel free to hop on IRC if you need some help figuring out how to generate a patch.

Comment 34

[:Margaret Leibovic]

Comment on attachment 757085 [details] [diff] [review]
Corrected previous changes, used Object.keys() and for...of

Marking this old patch as obsolete.

Comment 35

[Michael Boon]

Attachment: Updated patch for feedback

Comment 36

[:Margaret Leibovic]

Comment on attachment 774156 [details] [diff] [review]
Updated patch for feedback

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

Looking good!

::: mobile/android/chrome/content/browser.js
@@ +1968,5 @@
>            this._addHTMLContextMenuItems(contextmenu, null);
>          }
>  
>          // then check for any context menu items registered in the ui
> +         for (let itemId of Object.keys(this.items)) {

It looks like the indentation here is currently one space too much. Would you mind fixing that?

Comment 37

[Michael Boon]

Attachment: Patch for review

Comment 38

[:Margaret Leibovic]

Comment on attachment 777219 [details] [diff] [review]
Patch for review

Looks great, thanks!

Comment 39

[:Margaret Leibovic]

Adding the checkin-needed keyword to this bug, so that someone will come along and land it for you!

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0097d05cceea

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0097d05cceea