1147211 - Replace usages of var with let in LoginManagerPrompter.

Status: RESOLVED FIXED

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

Description

[Chenxia Liu [:liuche]]

This should wait until anything we ending up needing to uplift for 39 has been done.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Chenxia, is this good to go?

Comment 2

[Chenxia Liu [:liuche]]

Oh, yeah - no more changes being made to LoginManagerPrompter.

Comment 3

[Stanislas Daniel Claude Dolcini]

Attachment: This patch replaces some vars inside if blocks.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 5

[Stanislas Daniel Claude Dolcini]

Any news ? :)

Comment 6

[:Margaret Leibovic]

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!

Comment 7

[Stanislas Daniel Claude Dolcini]

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

Comment 8

[:Margaret Leibovic]

(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.

Comment 9

[Stanislas Daniel Claude Dolcini]

Attachment: bug_1147211.patch

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.

Comment 10

[: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?

Comment 11

[Stanislas Daniel Claude Dolcini]

Attachment: bug_1147211.patch

Done :)

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Margaret, can this land?

Comment 13

[:Margaret Leibovic]

(In reply to Michael Comella (:mcomella) from comment #12)
> Margaret, can this land?

Yes! Sorry I let this slip through the cracks :(

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing :(

Comment 15

[:Margaret Leibovic]

I'll fix it and land it.

Comment 16

[: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

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b6e38de09b1a