Closed
Bug 1134846
Opened 10 years ago
Closed 10 years ago
Create a module to support per-site password manager recipes
Categories
(Toolkit :: Password Manager, defect, P1)
Toolkit
Password Manager
Tracking
()
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: MattN, Assigned: MattN)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 1 obsolete file)
MozReview Request: Bug 1134846 - Add a module to support per-site password manager recipes. r=dolske
(deleted),
text/x-review-board-request
|
Details | |
(deleted),
text/x-review-board-request
|
Details | |
(deleted),
text/x-review-board-request
|
Details |
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.
Flags: qe-verify-
Flags: firefox-backlog+
Assignee | ||
Updated•10 years ago
|
Priority: -- → P1
Updated•10 years ago
|
Iteration: 38.3 - 23 Feb → 39.1 - 9 Mar
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
Attachment #8573605 -
Flags: review?(dolske)
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.
Comment hidden (obsolete) |
Comment hidden (obsolete) |
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
Assignee | ||
Comment 6•10 years ago
|
||
Now with unit tests so both commits are ready for review.
Updated•10 years ago
|
Iteration: 39.1 - 9 Mar → 39.2 - 23 Mar
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.
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 ?
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.
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.
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
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.
Comment 13•10 years ago
|
||
Comment 14•10 years ago
|
||
(Still looking at the 2nd patch, may not finish before my flight.)
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?]
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?]
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.
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
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
Whiteboard: [fixed-in-fx-team]
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla39
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.)
Attachment #8573605 -
Flags: review?(dolske)
Assignee | ||
Comment 22•9 years ago
|
||
Attachment #8573605 -
Attachment is obsolete: true
Assignee | ||
Comment 23•9 years ago
|
||
Assignee | ||
Comment 24•9 years ago
|
||
Assignee | ||
Comment 25•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•