Closed
Bug 1145754
Opened 10 years ago
Closed 9 years ago
Allow per-site recipes to adjust the username/password field detection for capture
Categories
(Toolkit :: Password Manager, defect, P1)
Toolkit
Password Manager
Tracking
()
People
(Reporter: MattN, Assigned: MattN)
References
(Depends on 1 open bug, Blocks 2 open bugs, )
Details
User Story
As a user I want PM to correctly fill my favourite site that has incorrect or strange use of forms and fields.
Attachments
(1 file, 1 obsolete file)
Bug 1120129 implemented login field overrides for filling. We should do the same for capture (although it's less common of an issue since we already ignore fields with empty values at capture time).
Flags: qe-verify-
Flags: firefox-backlog+
Updated•10 years ago
|
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar
Updated•10 years ago
|
Iteration: 39.3 - 30 Mar → 40.1 - 13 Apr
Updated•10 years ago
|
Iteration: 40.1 - 13 Apr → 40.2 - 27 Apr
Updated•10 years ago
|
Iteration: 40.2 - 27 Apr → 40.3 - 11 May
Assignee | ||
Comment 1•10 years ago
|
||
/r/7825 - Bug 1145754 - Allow per-site recipes to adjust the username/password field detection for capture. f=dolske Pull down this commit: hg pull -r 0475d2d3edd3a2086b67a632c2977c98734f0b06 https://reviewboard-hg.mozilla.org/gecko/
Assignee | ||
Comment 2•10 years ago
|
||
Comment on attachment 8598997 [details] MozReview Request: bz://1145754/MattN /r/7825 - Bug 1145754 - Allow per-site recipes to adjust the username/password field detection for capture. f=dolske Pull down this commit: hg pull -r ea7085a7827a4b5d3dd210eeabd775378750b783 https://reviewboard-hg.mozilla.org/gecko/
Assignee | ||
Comment 3•10 years ago
|
||
Comment on attachment 8598997 [details] MozReview Request: bz://1145754/MattN Asking for feedback only since: A) I'm waiting to here back about bug 803783 as I have a workaround for the Set but it still doesn't handle the RegExp if pathRegex is used. B) I'm not really a fan of using LoginManagerParent._recipeManager since it's assuming that recipes are loaded before the first form submission (which is likely true due to auto-fill or autocomplete initializing it but still seems kinda wrong). The other reason why I think we should go ahead and implement copies/caches in the child is to implement bug 1159538 so we don't need to do a sync messages for recipes on every blur/autocomplete (even for non-username fields since we don't have the recipe to know which field is the username field).
Attachment #8598997 -
Flags: feedback?(dolske)
Comment 4•10 years ago
|
||
https://reviewboard.mozilla.org/r/7823/#review6667 ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:576 (Diff revision 2) > + formOrigin: formSubmitURL, I think this should be s/formSubmitURL/hostname/
Updated•10 years ago
|
Attachment #8598997 -
Flags: feedback?(dolske) → feedback+
Assignee | ||
Comment 5•10 years ago
|
||
Comment on attachment 8598997 [details] MozReview Request: bz://1145754/MattN /r/7825 - Bug 1145754 - Allow per-site recipes to adjust the username/password field detection for capture. r=dolske Pull down this commit: hg pull -r 19e0a5cf4176ad375206a9ddb06f6826f0a724cc https://reviewboard-hg.mozilla.org/gecko/
Attachment #8598997 -
Flags: feedback+
Assignee | ||
Comment 6•10 years ago
|
||
Comment on attachment 8598997 [details] MozReview Request: bz://1145754/MattN /r/7825 - Bug 1145754 - Allow per-site recipes to adjust the username/password field detection for capture. r=dolske Pull down this commit: hg pull -r 19e0a5cf4176ad375206a9ddb06f6826f0a724cc https://reviewboard-hg.mozilla.org/gecko/
Attachment #8598997 -
Flags: review?(dolske)
Assignee | ||
Comment 7•10 years ago
|
||
billm says he's working on bug 803783 so I'll get him to remove the [...…] and the comment in that bug. We can figure out what to do for bug 1159538 in its own bug as this works fine for now. (In reply to Justin Dolske [:Dolske] from comment #4) > I think this should be s/formSubmitURL/hostname/ Fixed.
Assignee | ||
Updated•10 years ago
|
status-firefox39:
--- → affected
status-firefox40:
--- → affected
Updated•9 years ago
|
Iteration: 40.3 - 11 May → 41.1 - May 25
Updated•9 years ago
|
Updated•9 years ago
|
User Story: (updated)
Comment 8•9 years ago
|
||
Comment on attachment 8598997 [details] MozReview Request: bz://1145754/MattN https://reviewboard.mozilla.org/r/7823/#review7675 Ship It! ::: toolkit/components/passwordmgr/LoginManagerParent.jsm:166 (Diff revision 3) > + * Synchronous reference to the default LoginRecipesParent. This is a temporary hack. Not really sure what a "synchronous reference" is. Local import / local copy? ::: toolkit/components/passwordmgr/LoginManagerParent.jsm:219 (Diff revision 3) > + // Convert to an array due to not using structured clone - bug 803783. Looks like that's fixed now.
Attachment #8598997 -
Flags: review?(dolske) → review+
Assignee | ||
Comment 9•9 years ago
|
||
https://reviewboard.mozilla.org/r/7823/#review7695 > Not really sure what a "synchronous reference" is. Local import / local copy? It was to contrast with the async use of the initialization promise mentioned in the comment.
Assignee | ||
Comment 10•9 years ago
|
||
Comment on attachment 8598997 [details] MozReview Request: bz://1145754/MattN /r/7825 - Bug 1145754 - Allow per-site recipes to adjust the username/password field detection for capture. r=dolske Pull down this commit: hg pull -r 2b33c0e061e9c8ec37b0fc5437d0906d93c442d0 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8598997 -
Flags: review+ → review?(dolske)
Assignee | ||
Updated•9 years ago
|
Attachment #8598997 -
Flags: review?(dolske) → review+
Comment 13•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/53248ab3c730 https://hg.mozilla.org/mozilla-central/rev/09dde9554781
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee | ||
Comment 14•9 years ago
|
||
Comment on attachment 8598997 [details] MozReview Request: bz://1145754/MattN Approval Request Comment [Feature/regressing bug #]: Works with bug 1120129 to implement password recipes [User impact if declined]: No login may be saved or incorrect information may be saved for sites in our recipe list [Describe test coverage new/current, TreeHerder]: Mozilla employee dogfooding on Nightly with a few sites & automated test [Risks and why]: The list of sites with recipes is currently small (3) so this is likely low risk from that perspective. Automated tests are fairly good so I think there is a low risk of this totally breaking aspects of the password manager in general. [String/UUID change made/needed]: None
Attachment #8598997 -
Flags: approval-mozilla-beta?
Attachment #8598997 -
Flags: approval-mozilla-aurora?
Comment 15•9 years ago
|
||
Comment on attachment 8598997 [details] MozReview Request: bz://1145754/MattN Approved for uplift to aurora. Looks like this should go along with bug 1120129 which is already on 40. Matt, can this wait and ride the train with 40?
Flags: needinfo?(MattN+bmo)
Attachment #8598997 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 16•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/c46dae02c46e
Flags: in-testsuite+
Backed out of aurora for mochitest-5 and mochitest-bc3 orange: https://hg.mozilla.org/releases/mozilla-aurora/rev/e0484c0cd499 https://treeherder.mozilla.org/logviewer.html#?job_id=841563&repo=mozilla-aurora https://treeherder.mozilla.org/logviewer.html#?job_id=841540&repo=mozilla-aurora
Updated•9 years ago
|
Comment 18•9 years ago
|
||
> As a user I want PM to correctly fill my favorate site that has incorrect or strange
...........................................^^^^^^^^
Dude, Firefox has a built in spill chicken. Do use it.
User Story: (updated)
Updated•9 years ago
|
tracking-firefox40:
--- → +
tracking-firefox41:
--- → +
Assignee | ||
Comment 19•9 years ago
|
||
(In reply to Wes Kocher (:KWierso) from comment #17) > Backed out of aurora for mochitest-5 and mochitest-bc3 orange: I just had to put the workaround for bug 803783 back in since that wasn't uplifted. https://hg.mozilla.org/releases/mozilla-aurora/rev/9f4d4ce255a1
Flags: needinfo?(MattN+bmo)
Assignee | ||
Comment 20•9 years ago
|
||
(In reply to Liz Henry (:lizzard) from comment #15) > Approved for uplift to aurora. Looks like this should go along with bug > 1120129 which is already on 40. Matt, can this wait and ride the train with > 40? I'm confused. Bug 1120129 got fixed in 39 and having them together in the same release makes things less confusing and more consistent.
Comment hidden (typo) |
Comment 22•9 years ago
|
||
Sounds like you are saying that it really, really should be in 39 then. Thanks for letting me know. Here is my perspective and context: 39 beta 1 went to build last Friday and because of some delays and problems with 38.0.5, 39 was delayed. Beta 1 should release next Tuesday. 39 Beta 2 will go to build next Thursday and release Friday. Because we've had quite a lot of uplift requests (including several entire features) in the two weeks since 39 was in aurora, I'm trying to be conservative about accepting beta requests. We are now past the point that we would normally think of as "early beta" since 39 will have only 3 and a half weeks to be live on the beta channel. So I was weighing that risk of against your statement about this being a very low user impact. We could potentially verify this on aurora and then look at uplifting it for beta 2. Matt and dolske: what do you think? Keeping in mind that it will be a very short beta cycle and some of that will be at the work week. Basically, if this breaks stuff are you confident that can you be on top of things and either fix it or back this out? Thanks.
Comment 23•9 years ago
|
||
tl;dr: here are the uplift guidelines for beta: https://wiki.mozilla.org/Release_Management/Uplift_rules#Beta_Uplift_.28approval-mozilla-beta.29 Nevertheless, I would probably have waved this through in early beta because it sounds like something pretty limited in scope. Problem is that this cycle we don't really *have* an early beta because of the special extra release.
Flags: needinfo?(dolske)
Flags: needinfo?(MattN+bmo)
Comment 24•9 years ago
|
||
I think it's reasonable to take this in beta, since the intended behavior change is limited to sites we have recipes for, and can fix some oddities with that.
Flags: needinfo?(dolske)
Comment 25•9 years ago
|
||
Comment on attachment 8598997 [details] MozReview Request: bz://1145754/MattN Approved for uplift to beta based on discussion in comment 20
Attachment #8598997 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Comment 28•9 years ago
|
||
Attachment #8598997 -
Attachment is obsolete: true
Attachment #8619832 -
Flags: review+
Assignee | ||
Comment 29•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•