1134850 - Move password manager recipes to its own file

Status: RESOLVED FIXED

(Toolkit :: Password Manager, enhancement)

Description

[Matthew N. [:MattN]]

Bug 1134846 has recipes inline in the JSM but we will want these in their own file (with the path possibly specified by a pref) so that it's easier to maintain and update (over the wire).

Comment 1

[Riadh Chtara[:rchtara]]

Attachment: MozReview Request: Bug 1134850 - Move password manager recipes to its own file

Bug 1134850 - Move password manager recipes to its own file

Comment 2

[Riadh Chtara[:rchtara]]

Comment on attachment 8630734 [details]
MozReview Request: Bug 1134850 - Move password manager recipes to its own file

https://reviewboard.mozilla.org/r/12791/#review11331

Ship It\!

Comment 3

[Riadh Chtara[:rchtara]]

Comment on attachment 8630734 [details]
MozReview Request: Bug 1134850 - Move password manager recipes to its own file

Bug 1134850 - Move password manager recipes to its own file

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8630734 [details]
MozReview Request: Bug 1134850 - Move password manager recipes to its own file

https://reviewboard.mozilla.org/r/12791/#review11333

This is looking pretty good. Please ensure the tests pass locally. I'd like to take another look after the changes below.

::: toolkit/components/passwordmgr/LoginRecipes.jsm:263
(Diff revision 1)
>  const DEFAULT_RECIPES = {
>    "siteRecipes": [
>      {
>        "description": "okta uses a hidden password field to disable filling",
>        "hosts": ["mozilla.okta.com"],
>        "passwordSelector": "#pass-signin"
>      },

You forgot to delete the DEFAULT_RECIPES object from this file.

::: toolkit/components/passwordmgr/content/recipes.json
(Diff revision 1)
> -/* This Source Code Form is subject to the terms of the Mozilla Public
> - * License, v. 2.0. If a copy of the MPL was not distributed with this
> - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Are you able to leave the MPL 2.0 license at the top of the file or does that make the file invalid JSON?

::: toolkit/components/passwordmgr/LoginRecipes.jsm:16
(Diff revision 1)
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/NetUtil.jsm");

Nit: keep these alphabetical

::: toolkit/components/passwordmgr/LoginRecipes.jsm:94
(Diff revision 1)
> -        return this;
> +      let prefs = Components.classes["@mozgilla.org/preferences-service;1"]
> +                  .getService(Ci.nsIPrefService).getBranch("signon.");
> +      let path = prefs.getComplexValue("recipes.path", Ci.nsISupportsString).data;

You can get the preference service via Services.prefs. It's discouraged to get a branch of the preferences nowadays so you should just do:
`Services.prefs.getComplexValue("signon.recipes.path", Ci.nsISupportsString).data;`

::: toolkit/components/passwordmgr/LoginRecipes.jsm:105
(Diff revision 1)
> +            var recipes = JSON.parse(data);

Nit: We almost always use `let` instead of `var`.

::: toolkit/components/passwordmgr/LoginRecipes.jsm:97
(Diff revision 1)
> +      let channel = NetUtil.newChannel({uri: NetUtil.newURI(path, "UTF-8"),
> +                                        loadUsingSystemPrincipal: true});

Can you give a content-type hint for better performance as described at https://developer.mozilla.org/en-US/Add-ons/Code_snippets/File_I_O#Read_with_content_type_hint

::: toolkit/components/passwordmgr/LoginRecipes.jsm:102
(Diff revision 1)
> +          if (Components.isSuccessCode(result)) {
> +            let count = stream.available();

We prefer to use early returns when an error is encountered as it makes the code more readable (less nesting). See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Return_from_errors_immediately
`
if (!Components.isSuccessCode(result)) {
  log.error("Error loading recipes:", e);
  return;
}
let count = stream.available();
…
`

::: toolkit/components/passwordmgr/LoginRecipes.jsm:113
(Diff revision 1)
> +      }
> +      catch (e) {

Brace on the same line for JS please

::: toolkit/components/passwordmgr/LoginRecipes.jsm:115
(Diff revision 1)
> +        log.error("Error loading recipes: ", e);

Nit: no space before the second double-quote as the logging code adds spaces itself. This applies to both log.error calls.

::: toolkit/components/passwordmgr/LoginRecipes.jsm:110
(Diff revision 1)
> +            log.error("Error loading recipes: ", result);

The two log.error messages should be different so the specific failure case can be distinguished in bug reports.

Comment 5

[Riadh Chtara[:rchtara]]

Comment on attachment 8630734 [details]
MozReview Request: Bug 1134850 - Move password manager recipes to its own file

Bug 1134850 - Move password manager recipes to its own file

Comment 6

[Riadh Chtara[:rchtara]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b887832e524

Comment 7

[Riadh Chtara[:rchtara]]

https://reviewboard.mozilla.org/r/12791/#review11333

> Are you able to leave the MPL 2.0 license at the top of the file or does that make the file invalid JSON?

I tried to do it, but it breaks the json

> Can you give a content-type hint for better performance as described at https://developer.mozilla.org/en-US/Add-ons/Code_snippets/File_I_O#Read_with_content_type_hint

when I add 
channel.contentType = "application/json";
an exception is going to be thrown

Comment 8

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/12791/#review11333

> when I add 
> channel.contentType = "application/json";
> an exception is going to be thrown

What is the exception message?

Comment 9

[Riadh Chtara[:rchtara]]

Comment on attachment 8630734 [details]
MozReview Request: Bug 1134850 - Move password manager recipes to its own file

Bug 1134850 - Move password manager recipes to its own file

Comment 10

[Riadh Chtara[:rchtara]]

https://reviewboard.mozilla.org/r/12791/#review11333

> What is the exception message?

I have tried it again and it worked, I have just a small issue with the debegger, I was testing this patch by ading a breakpoint into .asyncFetch callback. But, it wasnt stop. But today it worked :  magic :)

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8630734 [details]
MozReview Request: Bug 1134850 - Move password manager recipes to its own file

https://reviewboard.mozilla.org/r/12791/#review11427

::: toolkit/components/passwordmgr/LoginRecipes.jsm:92
(Diff revision 3)
>        // XXX: Bug 1134850 will handle reading recipes from a file.

Delete this comment

::: toolkit/components/passwordmgr/LoginRecipes.jsm:31
(Diff revision 3)
>   * @param {boolean} [aOptions.defaults=true] whether to load default application recipes.
>   */
>  function LoginRecipesParent(aOptions = { defaults: true }) {

My original vision was that aOptions.defaults would be the URI to load the recipes from. I still think that makes sense and is easier for unit testing. Can you changing the default value of `aOptions.defaults` to `null` and change the 3 callers[1] for this. The test 2 callers can pass no argument and the "LoginManagerParent.jsm" caller can pass the result of `let path = Services.prefs.getComplexValue("signon.recipes.path", Ci.nsISupportsString).data;`

[1] https://mxr.mozilla.org/mozilla-central/search?string=new+LoginRecipesParent

::: toolkit/components/passwordmgr/LoginRecipes.jsm:95
(Diff revision 3)
> +      let channel = NetUtil.newChannel({uri: NetUtil.newURI(path, "UTF-8"),
> +                                        loadUsingSystemPrincipal: true});

replace `path` with `this._defaults`

::: toolkit/components/passwordmgr/LoginRecipes.jsm:93
(Diff revision 3)
> -      this.initializationPromise = this.load(DEFAULT_RECIPES).then(resolve => {
> +      let self = this;

We generally avoid assigning `this` to temporary variables in most cases since we have modern JS methods like Function.bind instead.

::: toolkit/components/passwordmgr/LoginRecipes.jsm:100
(Diff revision 3)
> +        NetUtil.asyncFetch(channel, function (stream, result) {

Bind this function instead of using `self` and give the function a name for stack traces e.g. `fetchedRecipes`

::: toolkit/components/passwordmgr/LoginRecipes.jsm:102
(Diff revision 3)
> +            log.error("Error fetching the file of the recipes:", result);

Nit: s/file of the recipes/recipe file/

(Same for the other log.error)

I think we should throw instead of continuing execution:
`throw new Error("Error fetching recipe file:" + result);`

::: toolkit/components/passwordmgr/LoginRecipes.jsm:113
(Diff revision 3)
> +        log.error("Error reading the file of the recipes:", e);

log.error("Error reading the recipe file:", e);
throw new Error("Error reading recipe file:" + e);

Comment 12

[Riadh Chtara[:rchtara]]

Comment on attachment 8630734 [details]
MozReview Request: Bug 1134850 - Move password manager recipes to its own file

Bug 1134850 - Move password manager recipes to its own file

Comment 13

[Riadh Chtara[:rchtara]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=24ca37fd60ff

Comment 14

[Matthew N. [:MattN]]

Comment on attachment 8630734 [details]
MozReview Request: Bug 1134850 - Move password manager recipes to its own file

https://reviewboard.mozilla.org/r/12791/#review11471

It looks like you have 2 sets of test failures to deal with still.

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:156
(Diff revision 4)
> +        defaults: Services.prefs.getComplexValue("signon.recipes.path", Ci.nsISupportsString).data

Nit: Object literals should have trailing commas at the end of the line so that blame doesn't change when a property gets added at the end afterwards.

::: toolkit/components/passwordmgr/LoginRecipes.jsm:60
(Diff revision 4)
>    _recipesByHost: null,
> -
>    /**

Nit: revert this

Comment 15

[Riadh Chtara[:rchtara]]

Comment on attachment 8630734 [details]
MozReview Request: Bug 1134850 - Move password manager recipes to its own file

Bug 1134850 - Move password manager recipes to its own file

Comment 16

[Riadh Chtara[:rchtara]]

Comment on attachment 8630734 [details]
MozReview Request: Bug 1134850 - Move password manager recipes to its own file

Bug 1134850 - Move password manager recipes to its own file

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 8630734 [details]
MozReview Request: Bug 1134850 - Move password manager recipes to its own file

https://reviewboard.mozilla.org/r/12791/#review11597

This can be simplified a bit: https://mattn.pastebin.mozilla.org/8839213

::: toolkit/components/passwordmgr/LoginRecipes.jsm:99
(Diff revision 6)
> +        var dataPromise = new Promise(function(resolve) {

Nit: s/var/let/

::: toolkit/components/passwordmgr/LoginRecipes.jsm:110
(Diff revision 6)
> +        var self = this;

This is unused.

Comment 18

[Riadh Chtara[:rchtara]]

Comment on attachment 8630734 [details]
MozReview Request: Bug 1134850 - Move password manager recipes to its own file

Bug 1134850 - Move password manager recipes to its own file

Comment 19

[Matthew N. [:MattN]]

Comment on attachment 8630734 [details]
MozReview Request: Bug 1134850 - Move password manager recipes to its own file

https://reviewboard.mozilla.org/r/12791/#review11607

Ship It!

Comment 20

[Riadh Chtara[:rchtara]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=045bb8b6c7a0

Comment 21

[Riadh Chtara[:rchtara]]

try job
https://treeherder.mozilla.org/#/jobs?repo=try&revision=77c613f97b3c

Comment 22

[Riadh Chtara[:rchtara]]

Comment on attachment 8630734 [details]
MozReview Request: Bug 1134850 - Move password manager recipes to its own file

Bug 1134850 - Move password manager recipes to its own file: fix broken tests

Comment 23

[Riadh Chtara[:rchtara]]

Comment on attachment 8630734 [details]
MozReview Request: Bug 1134850 - Move password manager recipes to its own file

Bug 1134850 - Move password manager recipes to its own file

Comment 24

[Riadh Chtara[:rchtara]]

The try job failed. I have an issue which I hope its fixed .
the new try jon is here
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e29070db5ae4

Comment 25

[Matthew N. [:MattN]]

Comment on attachment 8630734 [details]
MozReview Request: Bug 1134850 - Move password manager recipes to its own file

https://reviewboard.mozilla.org/r/12791/#review11863

Ship It!

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d1b29ca08199

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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