443183 - <nsLoginManager.js>, |_fillForm()|: optimize/remove |var isFormDisabled|

Status: RESOLVED WONTFIX

(Toolkit :: Password Manager, defect)

Description

[Serge Gautherie (:sgautherie)]

(Noticed while looking into bug 425145.)

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) <nsLoginManager.js>

*The code optimization.
*1 comment addition.
*Blank line removals.

Comment 2

[Serge Gautherie (:sgautherie)]

Comment on attachment 327809 [details] [diff] [review]
(Av1) <nsLoginManager.js>

Oh, well: just noticed that  mozilla-central has (2) new (related) checkins...

Comment 3

[Justin Dolske [:Dolske]]

The optimization is no longer relevant on trunk, and I don't think there's good reason to take this on branch.

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) <nsLoginManager.js>

Av1, with comment 2 suggestion(s). [Unified diff; no(t) Hg.]

Do we really need |this.log("form not filled, has autocomplete=off");| ?
Otherwise, that code could be optimized too.

Comment 5

[Justin Dolske [:Dolske]]

Comment on attachment 327905 [details] [diff] [review]
(Av2) <nsLoginManager.js>

I don't see the point of this.

Comment 6

[Serge Gautherie (:sgautherie)]

(In reply to comment #5)
> (From update of attachment 327905 [details] [diff] [review])
> I don't see the point of this.

Isn't it easier to read/execute ?

Comment 7

[Justin Dolske [:Dolske]]

No, I don't think so. You've added an early return, shuffled the logic, and made a large pile of unneeded whitespace changes.

Comment 8

[Serge Gautherie (:sgautherie)]

(In reply to comment #7)
> No, I don't think so. You've added an early return, shuffled the logic, and
> made a large pile of unneeded whitespace changes.

Reading a test once instead of three times is easier to me, at least.
Early return: yes, that's the point !
Which logic did I shuffle ?
Blank line removals shouldn't bother anything...

If you prefer, I can:
*use an |if (selectedLogin) { ... }|.
*leave the blank lines as they are.