Closed Bug 1295000 Opened 3 years ago Closed 3 years ago

Clean up for-loops in cookies.js

Categories

(Firefox :: Preferences, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 52
Tracking Status
firefox51 --- affected
firefox52 --- fixed

People

(Reporter: jaws, Assigned: schmittw, Mentored)

References

Details

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

Attachments

(1 file, 3 obsolete files)

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.
Attachment #8782153 - Flags: feedback+
Assignee: nobody → schmittw
Status: NEW → ASSIGNED
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.
Attachment #8782153 - Flags: feedback+ → review-
(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.
Summary: Clean up code in cookies.js and remove duplication between cookies.js and permissions.js → Clean up for-loops in cookies.js
Attached patch patch.txt for cookies.js (obsolete) — Splinter Review
Attachment #8782169 - Flags: review?(jaws)
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`
Attachment #8782169 - Flags: review?(jaws) → review-
Attached patch cookies.js patch (obsolete) — Splinter Review
Thank you for the guidance - I really appreciate it :)
This should be it though ~
Attachment #8782153 - Attachment is obsolete: true
Attachment #8782169 - Attachment is obsolete: true
Attachment #8783087 - Flags: review+
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.
Attachment #8783087 - Flags: review+ → review?(standard8)
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.
Attachment #8783087 - Flags: review?(standard8) → review+
Updated patch for the missing comment.
Attachment #8802534 - Flags: review+
Attachment #8783087 - Attachment is obsolete: true
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.
https://hg.mozilla.org/mozilla-central/rev/9f7b448a6b5c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Depends on: 1313634
You need to log in before you can comment on or make changes to this bug.