Closed Bug 1145754 Opened 9 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)

defect
Points:
5

Tracking

()

RESOLVED FIXED
mozilla41
Iteration:
41.1 - May 25
Tracking Status
firefox39 --- fixed
firefox40 + fixed
firefox41 + fixed

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+
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar
Iteration: 39.3 - 30 Mar → 40.1 - 13 Apr
Iteration: 40.1 - 13 Apr → 40.2 - 27 Apr
Iteration: 40.2 - 27 Apr → 40.3 - 11 May
Attached file MozReview Request: bz://1145754/MattN (obsolete) —
/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/
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/
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)
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/
Attachment #8598997 - Flags: feedback?(dolske) → feedback+
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+
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)
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.
Iteration: 40.3 - 11 May → 41.1 - May 25
User Story: (updated)
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+
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.
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)
Attachment #8598997 - Flags: review?(dolske) → review+
https://hg.mozilla.org/mozilla-central/rev/53248ab3c730
https://hg.mozilla.org/mozilla-central/rev/09dde9554781
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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 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+
Depends on: 1167872
> 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)
Depends on: 1168768
(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)
(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.
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.
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)
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 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+
Thanks
Flags: needinfo?(MattN+bmo)
Attachment #8598997 - Attachment is obsolete: true
Attachment #8619832 - Flags: review+
Depends on: 1309057
You need to log in before you can comment on or make changes to this bug.