LoginManagerContent._fillForm should take an options argument

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: Paolo, Assigned: squib, Mentored)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox41 affected, firefox55 fixed)

Details

(Whiteboard: [passwords:tech-debt][good first bug][lang=js])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
LoginManagerContent._fillForm is an internal JavaScript function that accepts various arguments:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginManagerContent.jsm#636

The function is called in other places from the same file, but the meaning of each of the arguments is unclear from the context.

To make the code that calls that function clearer, this function should be updated to use a single "options" argument, for example using a destructuring assignment like this:

_fillForm : function ({ form, autofillForm, clobberPassword,
                        userTriggered, foundLogins, recipes }) {
  // ...
}

See <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment#Function_argument_defaults> for details on this syntax.
Paolo, I would like to work on this bug. Can you guide me through it?
(In reply to :Paolo Amadini from comment #0)
> _fillForm : function ({ form, autofillForm, clobberPassword,
>                         userTriggered, foundLogins, recipes }) {
>   // ...
> }

Nit: I personally wouldn't make the required arguments part of the object.

(In reply to Akilan Elango [:falcon] from comment #1)
> Paolo, I would like to work on this bug. Can you guide me through it?

Hello, have you got your Firefox build setup? Are you able to find the file Paolo pointed to (LoginManagerContent.jsm)? You can join #fx-team on IRC to get live help.
Ya I have, and I did spot that file......
(Reporter)

Comment 4

4 years ago
Thanks for signing up to work on this, and sorry for the delay! For some reason I didn't get the usual e-mail notification from Bugzilla.

I think the next step, in general, would be to run the automated tests associated with the Password Manager to check that the build works properly before doing any changes. This would be done by using the "./mach test" command with the folder where the Password Manager component is located.

At this point you would have to change the code (callers and function definition) to use the new syntax, and see that all the tests still pass. Let me know when you are at this stage, or if you have any questions!
A Clarification : The js content from the page mentioned is not functioning in the scratchpad of firefox..... When I ran the exact code in that page it showed something like 
/*
Exception: SyntaxError: missing : after property id
@Scratchpad/1:3
*/
Attachment #8613241 - Flags: review?(paolo.mozmail)
Comment on attachment 8613241 [details] [diff] [review]
patch for the bug ID 1164469 : to be reviewed

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

Paolo, have you considered that perhaps the obvious required arguments should be left as regular arguments (e.g. form, foundLogins, & recipes)?

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ +393,5 @@
>            "interface.");
>        return;
>      }
> +    this._fillForm({form : topState.loginFormForFill, autofillForm : true, clobberUsername : true, clobberPassword : true, userTriggered : true,
> +                       foundLogins : loginsFound, recipes : recipes});

Please put one object property per line please and not space before the colon.

@@ +400,5 @@
>    loginsFound: function({ form, loginsFound, recipes }) {
>      let doc = form.ownerDocument;
>      let autofillForm = gAutofillForms && !PrivateBrowsingUtils.isContentWindowPrivate(doc.defaultView);
>  
> +    this._fillForm({form : form, autofillForm : autofillForm, clobberUsername : false, clobberPassword : false, userTriggered : false, foundLogins : loginsFound, recipes : recipes});

We should probably have sane defaults for many of these so we don't need to specify them all.

@@ +756,5 @@
>     *                             the user
>     * @param {nsILoginInfo[]} foundLogins is an array of nsILoginInfo that could be used for the form
>     * @param {Set} recipes that could be used to affect how the form is filled
>     */
> +  _fillForm : function (options) {

Nit: Use the new method syntax (without the `function`) while you're change thing line:

_fillForm(…)

@@ +758,5 @@
>     * @param {Set} recipes that could be used to affect how the form is filled
>     */
> +  _fillForm : function (options) {
> +    options = options === undefined ? {} : options ;
> +    var form = options.form ;

Please use the new syntax where the argument looks like an object which avoids the need to define all of the properties below.

Also, in-general, please use `let` instead of `var` especially where the code right below is using `let`.
Attachment #8613241 - Flags: feedback+
(Reporter)

Comment 8

4 years ago
(In reply to Akilan Elango [:falcon] from comment #5)
> A Clarification : The js content from the page mentioned is not functioning
> in the scratchpad of firefox.....

This may not be a problem, since the Scratchpad environment is different than the environment where the internal scripts are executed. The important bit is whether the tests you execute with mach still pass after these changes. Did you check that they still pass?

Now that you have received feedback from Matthew, it may be worth uploading a new version of the patch with those comments addressed. If you have any questions on that feedback, feel free to ask on the bug!

(In reply to Matthew N. [:MattN] from comment #7)
> Paolo, have you considered that perhaps the obvious required arguments
> should be left as regular arguments (e.g. form, foundLogins, & recipes)?

Hm, some of this required arguments may become optional in the future (you mentioned sending the recipes out of band for example) so it might be worth sending them inside the object to avoid another signature change later. What do you think?
(Reporter)

Updated

4 years ago
Attachment #8613241 - Flags: review?(paolo.mozmail)
Can you assign this to me? I'll finish this.
(Reporter)

Comment 10

4 years ago
Done!
Assignee: nobody → akilan1997
(In reply to :Paolo Amadini from comment #8)
> (In reply to Akilan Elango [:falcon] from comment #5)
> > A Clarification : The js content from the page mentioned is not functioning
> > in the scratchpad of firefox.....
> 
> This may not be a problem, since the Scratchpad environment is different
> than the environment where the internal scripts are executed. The important
> bit is whether the tests you execute with mach still pass after these
> changes. Did you check that they still pass?
> 
> Now that you have received feedback from Matthew, it may be worth uploading
> a new version of the patch with those comments addressed. If you have any
> questions on that feedback, feel free to ask on the bug!


While declaring the function _fillForm(), Which version of assignment should I use? ES5 or ES6?

(In reply to Matthew N. [:MattN] from comment #7)
> Nit: Use the new method syntax (without the `function`) while you're change
> thing line:
> 
> _fillForm(…)
> 
> @@ +758,5 @@
> >     * @param {Set} recipes that could be used to affect how the form is filled
> >     */
> > +  _fillForm : function (options) {
> > +    options = options === undefined ? {} : options ;
> > +    var form = options.form ;
> 
> Please use the new syntax where the argument looks like an object which
> avoids the need to define all of the properties below.

like in C/C++ 
> > functionname(args){
> >  //function statements
> > }
(In reply to :Paolo Amadini from comment #8)
> Hm, some of this required arguments may become optional in the future (you
> mentioned sending the recipes out of band for example) so it might be worth
> sending them inside the object to avoid another signature change later. What
> do you think?

Yeah, I'm not 100% sure about recipes but I think the form argument will remain to pass a FormLike as I think the method should stay focused on filling a single "form".

(In reply to Akilan Elango [:falcon] from comment #11)
> While declaring the function _fillForm(), Which version of assignment should
> I use? ES5 or ES6?

I don't understand what you mean. Can you post examples or a new patch?

> > Please use the new syntax where the argument looks like an object which
> > avoids the need to define all of the properties below.
> 
> like in C/C++ 
> > > functionname(args){
> > >  //function statements
> > > }

Is this a question?
Status: NEW → ASSIGNED
Flags: needinfo?(akilan1997)
@MattN, @falcon was asking if they should use the ES6 syntax for defining function signatures, and from Paolo's initial request, I think that is correct. 

@falcon, can you update the patch to reflect the comments from the review.

@MattN, will you be taking over from Paolo as Mentor?
Flags: needinfo?(MattN+bmo)
ES6 syntax would be preferred.
Mentor: MattN+bmo
Flags: needinfo?(MattN+bmo)
Whiteboard: [good first bug][lang=js] → [passwords:tech-debt][good first bug][lang=js]
Assignee: akilan1997 → nobody
Mentor: dolske
Status: ASSIGNED → NEW
(Assignee)

Updated

2 years ago
Assignee: nobody → squibblyflabbetydoo
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Comment 16

2 years ago
I think this should address all the comments from the previous review. Notable bits:

1) `form`, `foundLogins`, and `recipes` are all required positional arguments.
2) Other args go in the `options` object to make them keyword args and all default to false (or null) since I didn't see a compelling reason for any of them to be true.
3) I could see some arguments for changing how I indent the `options` object in the definition of `_fillForm` but I went with minimizing vertical space since the block comment above is already pretty tall.
4) I tried to use the "standard" jsdoc way of commenting property values in an `options` object; hopefully that looks good to you.

I kicked off a Try build to make sure I didn't mess anything up, but it's a pretty straightforward patch.
Flags: needinfo?(akilan1997)
Comment on attachment 8849350 [details]
Bug 1164469 - LoginManagerContent._fillForm should take an options argument;

https://reviewboard.mozilla.org/r/122142/#review126558

Sorry for the delay.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:998
(Diff revision 1)
> -   *                               field value is not found in foundLogins, it will not fill the password.
> -   * @param {bool} clobberPassword controls if an existing password value can be
> -   *                               overwritten
> -   * @param {bool} userTriggered is an indication of whether this filling was triggered by
> -   *                             the user
> -   * @param {nsILoginInfo[]} foundLogins is an array of nsILoginInfo that could be used for the form
> -   * @param {Set} recipes that could be used to affect how the form is filled
> -   * @param {Object} [options = {}] is a list of options for this method.
> -            - [inputElement] is an optional target input element we want to fill
> -   */
> -  _fillForm(form, autofillForm, clobberUsername, clobberPassword,
> -            userTriggered, foundLogins, recipes, {inputElement} = {}) {
> +   * @param {HTMLInputElement} [options.inputElement = null] an optional target
> +   *        input element we want to fill
> +   * @param {bool} [options.autofillForm = false] denotes if we should fill the
> +   *        form in automatically
> +   * @param {bool} [options.clobberUsername = false] controls if an existing
> +   *        username can be overwritten. If this is false and an inputElement
> +   *        of type password is also passed, the username field will be ignored.
> +   *        If this is false and no inputElement is passed, if the username
> +   *        field value is not found in foundLogins, it will not fill the
> +   *        password.
> +   * @param {bool} [options.clobberPassword = false] controls if an existing
> +   *        password value can be overwritten
> +   * @param {bool} [options.userTriggered = false] an indication of whether
> +   *        this filling was triggered by the user
> +   */
> +  _fillForm(form, foundLogins, recipes, {
> +    inputElement = null, userTriggered = false, autofillForm = false,
> +    clobberUsername = false, clobberPassword = false,

Nit: I personally would sort the properties alphabetically (and the @param to match) and would put one property per line (mostly to help with blame when adding new args later) but I don't feel strongly about it so feel free to leave as-is.
Attachment #8849350 - Flags: review?(MattN+bmo) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 19

2 years ago
mozreview-review
Comment on attachment 8849350 [details]
Bug 1164469 - LoginManagerContent._fillForm should take an options argument;

https://reviewboard.mozilla.org/r/122142/#review128338

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:998
(Diff revision 1)
> -   *                               field value is not found in foundLogins, it will not fill the password.
> -   * @param {bool} clobberPassword controls if an existing password value can be
> -   *                               overwritten
> -   * @param {bool} userTriggered is an indication of whether this filling was triggered by
> -   *                             the user
> -   * @param {nsILoginInfo[]} foundLogins is an array of nsILoginInfo that could be used for the form
> -   * @param {Set} recipes that could be used to affect how the form is filled
> -   * @param {Object} [options = {}] is a list of options for this method.
> -            - [inputElement] is an optional target input element we want to fill
> -   */
> -  _fillForm(form, autofillForm, clobberUsername, clobberPassword,
> -            userTriggered, foundLogins, recipes, {inputElement} = {}) {
> +   * @param {HTMLInputElement} [options.inputElement = null] an optional target
> +   *        input element we want to fill
> +   * @param {bool} [options.autofillForm = false] denotes if we should fill the
> +   *        form in automatically
> +   * @param {bool} [options.clobberUsername = false] controls if an existing
> +   *        username can be overwritten. If this is false and an inputElement
> +   *        of type password is also passed, the username field will be ignored.
> +   *        If this is false and no inputElement is passed, if the username
> +   *        field value is not found in foundLogins, it will not fill the
> +   *        password.
> +   * @param {bool} [options.clobberPassword = false] controls if an existing
> +   *        password value can be overwritten
> +   * @param {bool} [options.userTriggered = false] an indication of whether
> +   *        this filling was triggered by the user
> +   */
> +  _fillForm(form, foundLogins, recipes, {
> +    inputElement = null, userTriggered = false, autofillForm = false,
> +    clobberUsername = false, clobberPassword = false,

I put all the properties on separate lines and sorted them in the same order as the comment. It's not quite alphabetical, but I was trying to put the most-interesting properties first so people don't have to read quite as much.

Comment 20

2 years ago
Pushed by squibblyflabbetydoo@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fa52ac5563f3
LoginManagerContent._fillForm should take an options argument; r=MattN

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fa52ac5563f3
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.