Matthew N. [:MattN] Assignee
	Description • 10 years ago

The module will have the recipes in memory and eventually various methods to get overrides for specific tasks for a specific login form. e.g. bug 1120129 will have a method to get overrides for username/password fields.

	Matthew N. [:MattN] Assignee
	Comment 1 • 10 years ago

/r/4881 - Bug 1134846 - Add helpers to create loggers that follow the signon.debug pref. r=dolske
/r/4883 - Bug 1134846 - Add a module to support per-site password manager recipes.

Pull down these commits:

hg pull review -r a19d54a35fe47ff0c03d4ca18ed37fd2ec4aa9b0

	Matthew N. [:MattN] Assignee
	Comment 2 • 10 years ago

I'm just asking for review on the logger helper for now. I'll file a follow-up to use the logger helper in the existing modules.

I'd like feedback on LoginRecipes.jsm while I write unit tests.

	Matthew N. [:MattN] Assignee
	Comment 5 • 10 years ago

Comment on attachment 8573605 [details]
MozReview Request: bz://1134846/MattN

/r/5053 - Bug 1134846 - Add helpers to create loggers that follow the signon.debug pref. r=dolske
/r/4881 - Bug 1134846 - Add a module to support per-site password manager recipes. r=dolske

Pull down these commits:

hg pull review -r 03bb184d12e28cf194817c3789cbae4b20e9a8f3

	Matthew N. [:MattN] Assignee
	Comment 6 • 10 years ago

Now with unit tests so both commits are ready for review.

	Justin Dolske [:Dolske]
	Comment 7 • 10 years ago

https://reviewboard.mozilla.org/r/5053/#review4135

::: toolkit/components/passwordmgr/LoginHelper.jsm
(Diff revision 1)
> +    let logger = this._loggers.get(logPrefix);

Is this actually useful? (The Map, and retrieving an already-created logger). Seems like logging consumers will all just create a persistent logger on startup.

Instead of _onPrefChange, each logger could just have its own pref listener setup in _createLogger, operating on consoleOptions via a closure?

That would seem to keep this all simpler.

	Justin Dolske [:Dolske]
	Comment 8 • 10 years ago

https://reviewboard.mozilla.org/r/4881/#review4137

::: toolkit/components/passwordmgr/LoginManagerParent.jsm
(Diff revision 2)
> -        this.findLogins(data.options.showMasterPassword,
> +        this.sendLoginDataToChild(data.options.showMasterPassword,

Thank you for renaming this!

Worth changing "RemoteLogins:findLogins" too?

::: toolkit/components/passwordmgr/LoginManagerParent.jsm
(Diff revision 2)
> +      recipes = [...recipeManager.getRecipesForHost(formHost)];

Why the array clone? Seems like either getRecipesForHost() should handle that, or just not bother (since it just immediately goes to sendAsychMessage where it's cloned again.0

::: toolkit/components/passwordmgr/LoginManagerParent.jsm
(Diff revision 2)
> +      return parent.init();

Hmm, what about making this more RAIIish... Kill init() and just return parent.initalizationPromise?

::: toolkit/components/passwordmgr/test/unit/test_recipes.js
(Diff revision 2)
> +  let recipesParent = yield RecipeHelpers.initNewParent();

yield (new LoginRecipesParent()).initializaionPromize ?

	Matthew N. [:MattN] Assignee
	Comment 9 • 10 years ago

https://reviewboard.mozilla.org/r/5053/#review4217

> Is this actually useful? (The Map, and retrieving an already-created logger). Seems like logging consumers will all just create a persistent logger on startup.
> 
> Instead of _onPrefChange, each logger could just have its own pref listener setup in _createLogger, operating on consoleOptions via a closure?
> 
> That would seem to keep this all simpler.

I like the closure idea but I was trying to avoid having so many pref listeners that all do the same thing and I was also making LoginHelper.debug accessible to get rid of the existing gDebug in a few files. I suppose I could have every closure set .debug repeatedly (once for each listener) which isn't ideal. The one theoretical problem is that LoginHelper.debug could be stale if there are no loggers created (since they add the pref observer) unless I also add a top-level one in the JSM still just for updating .debug? That is going to be rare in practice I think so maybe I can just note it and ignore that edge case.

	Matthew N. [:MattN] Assignee
	Comment 10 • 10 years ago

https://reviewboard.mozilla.org/r/4881/#review4225

> Thank you for renaming this!
> 
> Worth changing "RemoteLogins:findLogins" too?

I had a TODO to rename the message in both directions but removed it since I didn't really know what to name it while keeping symmetry between the two directions and didn't want to slow down this bug. I can rename if you have feedback: e.g. "LoginDataRequest" for the request and "LoginDataResponse" for the response.

Btw. I don't like the RemoteLogin prefix but for consistency all 5 of the messages should be changed then (which sounded like a new bug).

> Why the array clone? Seems like either getRecipesForHost() should handle that, or just not bother (since it just immediately goes to sendAsychMessage where it's cloned again.0

This was a leftover from before bug 1140242 was fixed but I didn't update the commit in mozreview yet

> Hmm, what about making this more RAIIish... Kill init() and just return parent.initalizationPromise?

What were you thinking would actually start the init? Making .initalizationPromise a getter which kicks it off or the constructor?

Note that for bug 1120129 I was adding an argument to init to indicate whether app-default recipes should be loaded (eventually from disk) so that consumers (e.g. tests) can indicate that they want to start with an empty set of recipes. I'm thinking I should make a separate method that would have to be called for consumers who want the defaults.

> yield (new LoginRecipesParent()).initializaionPromize ?

Do you want me to inline and get rid of the helper? I'm fine with that. At one point I was thinking there was going to be another method call in the helper and didn't want to have to update every test function.

	Matthew N. [:MattN] Assignee
	Comment 11 • 10 years ago

Comment on attachment 8573605 [details]
MozReview Request: bz://1134846/MattN

/r/5053 - Bug 1134846 - Add helpers to create loggers that follow the signon.debug pref. r=dolske
/r/4881 - Bug 1134846 - Add a module to support per-site password manager recipes. r=dolske

Pull down these commits:

hg pull review -r d2aebd6999901ab6ba4b7625f4bd6bdd6a9789a9

	Matthew N. [:MattN] Assignee
	Comment 12 • 10 years ago

I made the suggested changes to both patches other than renaming the messages and the test change as I had questions in comment 10.


(In reply to Matthew N. [:MattN] from comment #10)
> I'm thinking I should make a separate method that would have to be called for consumers who want the 
> defaults.

I'll just add this to the constructor instead actually as it makes more sense there IMO.

	Justin Dolske [:Dolske]
	Comment 13 • 10 years ago

https://reviewboard.mozilla.org/r/5053/#review4277

Ship It!

	Justin Dolske [:Dolske]
	Comment 14 • 10 years ago

(Still looking at the 2nd patch, may not finish before my flight.)

	Justin Dolske [:Dolske]
	Comment 15 • 10 years ago

https://reviewboard.mozilla.org/r/4881/#review4279

Ship It!

::: toolkit/components/passwordmgr/LoginManagerParent.jsm
(Diff revision 3)
> +    });

Hmm. Do we want actually want a lazy getter here? I'm sorta thinking we should eagerly initialize LoginRecipesParent, so that it (eventually) has time to load up the recipees from disk before they're actually needed. Even if we aggressively deferred the json load, I'd expect most sessions to trigger it eventually.

Although there is some value it avoiding IO during startup if we can do so.

::: toolkit/components/passwordmgr/LoginManagerParent.jsm
(Diff revision 3)
> +    if (formOrigin) {

How can formOrigin be empty here? *looks at code* Oh. Huh. I... Wonder if we shoud have _getPasswordOrigin callers just bail out early when they get formOrigin == null. After all we know right then that we're not going to save/fill anything, right?

::: toolkit/components/passwordmgr/LoginRecipes.jsm
(Diff revision 3)
> +this.EXPORTED_SYMBOLS = ["LoginRecipesContent", "LoginRecipesParent"];

LoginRecipesContent? Unused leftover?

::: toolkit/components/passwordmgr/LoginRecipes.jsm
(Diff revision 3)
> +      return new Set();

I hate to suggest microoptimizations, but...

Could have a static EmptySet (frozen/immutable?) to avoid allocation for the common case of not having any recipes? Or return null and make callers deal with it? (Dunno if that's a pain, and airport wifi is slow enough I can't load your other bug to see how it's used.)

::: toolkit/components/passwordmgr/LoginRecipes.jsm
(Diff revision 3)
> +        };

Why not just use rawRecipe directly? (ie this.add(rawRecipe)? [Is this another temporary 1140242 workaround?]

	Justin Dolske [:Dolske]
	Comment 16 • 10 years ago

https://reviewboard.mozilla.org/r/4881/#review4279

Ship It!

::: toolkit/components/passwordmgr/LoginManagerParent.jsm
(Diff revision 3)
> +    });

Hmm. Do we want actually want a lazy getter here? I'm sorta thinking we should eagerly initialize LoginRecipesParent, so that it (eventually) has time to load up the recipees from disk before they're actually needed. Even if we aggressively deferred the json load, I'd expect most sessions to trigger it eventually.

Although there is some value it avoiding IO during startup if we can do so.

::: toolkit/components/passwordmgr/LoginManagerParent.jsm
(Diff revision 3)
> +    if (formOrigin) {

How can formOrigin be empty here? *looks at code* Oh. Huh. I... Wonder if we shoud have _getPasswordOrigin callers just bail out early when they get formOrigin == null. After all we know right then that we're not going to save/fill anything, right?

::: toolkit/components/passwordmgr/LoginRecipes.jsm
(Diff revision 3)
> +this.EXPORTED_SYMBOLS = ["LoginRecipesContent", "LoginRecipesParent"];

LoginRecipesContent? Unused leftover?

::: toolkit/components/passwordmgr/LoginRecipes.jsm
(Diff revision 3)
> +      return new Set();

I hate to suggest microoptimizations, but...

Could have a static EmptySet (frozen/immutable?) to avoid allocation for the common case of not having any recipes? Or return null and make callers deal with it? (Dunno if that's a pain, and airport wifi is slow enough I can't load your other bug to see how it's used.)

::: toolkit/components/passwordmgr/LoginRecipes.jsm
(Diff revision 3)
> +        };

Why not just use rawRecipe directly? (ie this.add(rawRecipe)? [Is this another temporary 1140242 workaround?]

	Matthew N. [:MattN] Assignee
	Comment 17 • 10 years ago

https://reviewboard.mozilla.org/r/4881/#review4285

> Why not just use rawRecipe directly? (ie this.add(rawRecipe)? [Is this another temporary 1140242 workaround?]

It's not a workaround for 1140242, it's because we're expecting callers to be getting the argument from parsing JSON (e.g. some recipe JSON file in the profile) and JSON doesn't support RegExp so we convert from "/foo/" to a real RegExp (so we can catch invalid patterns at load time rather than on-demand so it's easier to know if an invalid recipe exists in the default rules.

> I hate to suggest microoptimizations, but...
> 
> Could have a static EmptySet (frozen/immutable?) to avoid allocation for the common case of not having any recipes? Or return null and make callers deal with it? (Dunno if that's a pain, and airport wifi is slow enough I can't load your other bug to see how it's used.)

I also wondered about the overhead of `new Set` but figured it wasn't worth the microoptimization and I like the consistency of being able to rely on a Set being returned. Handling null at this time wouldn't be too bad but it is a small amount of added complexity.

> LoginRecipesContent? Unused leftover?

It was supposed to be in the next patch (bug 1120129).

> Hmm. Do we want actually want a lazy getter here? I'm sorta thinking we should eagerly initialize LoginRecipesParent, so that it (eventually) has time to load up the recipees from disk before they're actually needed. Even if we aggressively deferred the json load, I'd expect most sessions to trigger it eventually.
> 
> Although there is some value it avoiding IO during startup if we can do so.

I'm indifferent on this. It's a tradeoff between startup perf and first auto-fill speed.

	Matthew N. [:MattN] Assignee
	Comment 18 • 10 years ago

Comment on attachment 8573605 [details]
MozReview Request: bz://1134846/MattN

/r/5053 - Bug 1134846 - Add helpers to create loggers that follow the signon.debug pref. r=dolske
/r/4881 - Bug 1134846 - Add a module to support per-site password manager recipes. r=dolske

Pull down these commits:

hg pull review -r 0484c1e7d873dc53a1c579108f6ea9b506c04ce8

	Matthew N. [:MattN] Assignee
	Comment 19 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/438e73cdb242
https://hg.mozilla.org/integration/fx-team/rev/1cb5e49440ed
https://hg.mozilla.org/integration/fx-team/rev/1375c8688cc4

	Carsten Book [:Tomcat]
	Comment 20 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/438e73cdb242
https://hg.mozilla.org/mozilla-central/rev/1cb5e49440ed
https://hg.mozilla.org/mozilla-central/rev/1375c8688cc4

	Justin Dolske [:Dolske]
	Comment 21 • 10 years ago

Comment on attachment 8573605 [details]
MozReview Request: bz://1134846/MattN

(Not sure why there's still a review flag here, I thought I ShipIt'd everything previously. And we already talked about the last few issues, so everything was good here.)