Closed Bug 1147211 Opened 5 years ago Closed 4 years ago

Replace usages of var with let in LoginManagerPrompter.

Categories

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

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: liuche, Assigned: dolcinis, Mentored)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 2 obsolete files)

This should wait until anything we ending up needing to uplift for 39 has been done.
Mentor: liuche
Whiteboard: [good first bug][lang=js]
Chenxia, is this good to go?
Flags: needinfo?(liuche)
Oh, yeah - no more changes being made to LoginManagerPrompter.
Flags: needinfo?(liuche)
Assignee: nobody → dolcinis
Comment on attachment 8672999 [details] [diff] [review]
This patch replaces some vars inside if blocks.

Thanks for the patch Stanislas! One thing I'd recommend is flagging a reviewer when uploading a patch (click the "Details" link next to the patch to find the option). I've flagged the mentor of this bug for reviewer and they should get back to you soon.
Attachment #8672999 - Flags: review?(liuche)
Any news ? :)
Status: NEW → ASSIGNED
Comment on attachment 8672999 [details] [diff] [review]
This patch replaces some vars inside if blocks.

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

This looks good to me! I do notice that there are other `var` variables declared in the file, so we should look into fixing those while we're here.

Bug 1147197 would also be another good bug for you to work on!
Attachment #8672999 - Flags: review?(liuche) → review+
I do not always remember when to remove var by let 

for me it's

for(let) stuff
all the variables in if

Is there something else ?

If you point me the vars I need to replace i'll make another patch tonight :)

Regards
(In reply to Stanislas Daniel Claude Dolcini from comment #7)
> I do not always remember when to remove var by let 
> 
> for me it's
> 
> for(let) stuff
> all the variables in if
> 
> Is there something else ?
> 
> If you point me the vars I need to replace i'll make another patch tonight :)
> 
> Regards

Sorry for the slow response! In the future, you can use the needinfo? flag to make sure questions don't slip through the cracks :)

Here is some documentation about how `let` works:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let

Generally, people like to use `let` because it helps you avoid mistakes that can be caused by the way `var` "hoists" variable declarations.

Thinking more about this bug, I'm not sure it's worth digging into this in too much detail, unless we see places where we might be making mistakes with `var`.

You can update this patch to apply on top of the patch for bug 1147197, and see if there are any other places it would be worth making this change. Then I can re-review and land this.
Attached patch bug_1147211.patch (obsolete) — Splinter Review
This patches replace the use of var in nested conditions. There is not really a point of replacing vars at function level according to the documentation.
Attachment #8672999 - Attachment is obsolete: true
Attachment #8681321 - Flags: review?(margaret.leibovic)
Comment on attachment 8681321 [details] [diff] [review]
bug_1147211.patch

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

This looks good to me. Thanks for investigating.

::: mobile/android/components/LoginManagerPrompter.js
@@ +428,2 @@
>      if (port != handler.defaultPort)
>        hostname += ":" + port;

It looks like the indentation in here got a bit messed up... could you fix this while you're here?
Attachment #8681321 - Flags: review?(margaret.leibovic) → review+
Done :)
Attachment #8681321 - Attachment is obsolete: true
Attachment #8682516 - Flags: review?(margaret.leibovic)
Attachment #8682516 - Flags: review?(margaret.leibovic) → review+
Margaret, can this land?
Flags: needinfo?(margaret.leibovic)
(In reply to Michael Comella (:mcomella) from comment #12)
> Margaret, can this land?

Yes! Sorry I let this slip through the cracks :(
Flags: needinfo?(margaret.leibovic)
Keywords: checkin-needed
Needs rebasing :(
Keywords: checkin-needed
I'll fix it and land it.
Flags: needinfo?(margaret.leibovic)
https://hg.mozilla.org/integration/fx-team/rev/b6e38de09b1a8f6e5a04c899006943c810761354
Bug 1147211 - Replace usages of var with let in LoginManagerPrompter.js in nested code. r=margaret
Flags: needinfo?(margaret.leibovic)
https://hg.mozilla.org/mozilla-central/rev/b6e38de09b1a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.