In browser/components/preferences/cookies.js, there are some loops that walk arrays by getting an index to the array and incrementing that index as it walks through the array. This can be improved by using for..of. Both browser/components/preferences/cookies.js and browser/components/preferences/permissions.js share the same code for onCookieKeyPress and onPermissionKeyPress, respectively. This code can be refactored to remove the duplication. There may be other things you may find while looking at these files. Do not hesitate to ask about other things you can do :)

Hi, I'm new to open source development, trying to get out of my .NET webdev comfort zone, could I take a crack at this one? If so, how does it get assigned to me? Thanks!

Hey Ryan, thanks and welcome. I usually wait until a patch is attached to the bug before I mark it as assigned.

So I refactored the respective for-loops as mentioned. However, I'm confused as to how to refactor onPermissionKeyPress/onCookieKeyPress other than adding something to Services.jsm, but that seems out of the scope of this bug/a bit extreme.

Comment on attachment 8782153 [details] [diff] [review] cookies.js with updated for loops Thanks for the patch but you need to upload a diff of what you changed. If you're using Mercurial, you can run `hg diff > patch.txt` which will write out the changes you made to a file named patch.txt. You can then attach patch.txt to this bug and set the review flag to "?" and type in :jaws as the reviewer.

(In reply to schmittw from comment #3) > Created attachment 8782153 [details] [diff] [review] > cookies.js with updated for loops > > So I refactored the respective for-loops as mentioned. However, I'm confused > as to how to refactor onPermissionKeyPress/onCookieKeyPress other than > adding something to Services.jsm, but that seems out of the scope of this > bug/a bit extreme. onPermissionKeyPress and onCookieKeyPress have some of the same code within them but they don't share a JS file and it's probably not worth creating a new shared file just for this simple code. I'll update this bug to just be about the for-loops.

Comment on attachment 8782169 [details] [diff] [review] patch.txt for cookies.js Review of attachment 8782169 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, just one more pass through and this should be ready. ::: browser/components/preferences/cookies.js @@ +132,2 @@ > ++rowIndex; > + var hostItem = this._hosts[host]; You should run this file through our eslint tool to look for any JS issues that it finds. There's some trailing whitespace here that it will complain about for example. To run the tool, type: mach eslint browser/components/preferences/cookies.js @@ +242,4 @@ > // We are looking for an entry within this host's children, > // enumerate them looking for the index. > ++count; > + for(let cookie in currHost.cookies){ for (let cookie of currHost.cookies) { @@ +601,4 @@ > var blockFutureCookies = false; > if (psvc.prefHasUserValue("network.cookie.blockFutureCookies")) > blockFutureCookies = psvc.getBoolPref("network.cookie.blockFutureCookies"); > + for (var item of deleteItems) { please use `let` instead of `var`

Thank you for the guidance - I really appreciate it :) This should be it though ~

Comment on attachment 8783087 [details] [diff] [review] cookies.js patch Hi William, we're sorry for the delay in this - Jared was expecting a review request (as he'd set it to r-), rather than just re-attaching. The good news is the patch still applies, so I'll look at it for you in a few minutes.

Comment on attachment 8783087 [details] [diff] [review] cookies.js patch Review of attachment 8783087 [details] [diff] [review]: ----------------------------------------------------------------- There was just one change you'd missed for Jared's comments. As we missed getting back to you, I'll upload a new patch in a moment, then get this landed for you. ::: browser/components/preferences/cookies.js @@ +242,4 @@ > // We are looking for an entry within this host's children, > // enumerate them looking for the index. > ++count; > + for (let cookie in currHost.cookies) { This change was missed from Jared's comment.

Updated patch for the missing comment.

https://hg.mozilla.org/integration/fx-team/rev/9f7b448a6b5ce5dfcffbcde6eed50052f3105a26 Bug 1295000 - Clean up for-loops in cookies.js. r=Standard8

William: the patch has landed & this will get merged into the main development tree in the next day at so (at which point this bug will be marked fixed). Thank you for working on this patch, sorry there was a delay in getting it landed.