Justin Dolske [:Dolske] Reporter
	Description • 16 years ago

Bug 359675 is introducing an login manger API which allows other code to request an HTML login form to be filled in. That's sufficient to implement an extension which provides, say, a toolbar button that fills in form(s) on the page when clicked.

But just that is a little clunky, ideally there should be a way to only show the button when the login manager found an eligible login form but did *not* automatically fill it in. Then an extension could only expose its UI when it's actually needed.

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 1 • 16 years ago

Do we have a message that we want to broadcast?  What I've put in for now is "passwordmgr-found-form" but I wanted some feedback on that. (Is there an etiquette I should follow for those?)

I've put this into _fillForm, and it only gets broadcast if both of the following:
1. we are not filling the form in already
2. one of the 3 cases where we have a login occur

No patch yet because it's pretty trivial and hardly seems worth it.

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 2 • 16 years ago

Settled on "passwordmgr-found-form" as the topic for the observer service.

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 3 • 16 years ago

Forgot to take some bad code out of the test.

	Justin Dolske [:Dolske] Reporter
	Comment 4 • 16 years ago

Comment on attachment 325883 [details] [diff] [review]
Patch v2

>+        // For when autofillForm is false, but we still have the information
>+        // to fill a form, we notify observers
>+        var canFillForm = (usernameField && usernameField.value) ||
>+                          (usernameField && logins.length == 2) ||
>+                          (logins.length == 1)

Duplicating the login from earlier in the function is prone to breakage. I think it would be better to have a "loginFilled" boolean, and set it to true when a field is filled. Then the observer code can just look more like:

if (!loginFilled && logins.length) { /* notify observers */ }

>+        if (!autofillForm && !isFormDisabled && canFillForm) {

Eventually I would like to have fillForm() take a argument to optionally ignore autocomplete=off.

>+            var os = Components.classes["@mozilla.org/observer-service;1"]
>+                     .getService(Components.interfaces.nsIObserverService);

Use a memoizing getter here, like this._logService and this._ioService.

Style nit: this would have been:

var os = Cc["blah"].
         getService(Ci.foo);

...but do the getter thing instead anyway. :)

>+                os.notifyObservers(form, "passwordmgr-found-form", null);

Let's use the last arg to indicate why the form wasn't filled. I think the two useful cases are (1) autofillForms was disabled and (2) autocomplete=off was found. Eventually I'd like to have fillForm() take an argument to optionally ignore autocomplete=off. (don't need to do that here :).

>+            catch (ex) { this.log("Something went wrong with the observer service"); }

Can the observer service throw? When? If we can do without the try/catch, that would be nice.

>+++ b/toolkit/components/passwordmgr/test/test_basic_form_observer.html

Maybe also try testing to make sure we don't fire a notification when fillForm() is explicitly invoked? Not sure if that makes sense or not though. :)

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 5 • 16 years ago

>if (!loginFilled && logins.length) { /* notify observers */ }

So while most of those changes are trivial, the problem comes in about what the notification really means. Is the notification supposed to fire if and only if we can actually fill a form? If so then what you're suggesting doesn't really cover the cases (and to be honest, my code didn't either).

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 6 • 16 years ago

This clears up the duplication, takes an argument for ignoring autocomplete=off, memoizes, and adds a message to the notification.

This also adds a return value to the public fillForm (boolean) so that developers will know of success/failure.

Added multiple tests (check for correct message and that no notification is sent when it shouldn't) & modified old test to check return value.

	Justin Dolske [:Dolske] Reporter
	Comment 7 • 16 years ago

Comment on attachment 326608 [details] [diff] [review]
Patch v3

>@@ -220,7 +220,7 @@
>      * @param aForm
>      *        The form to fill
>      */
>-    void fillForm(in nsIDOMHTMLFormElement aForm);
>+    boolean fillForm(in nsIDOMHTMLFormElement aForm);

Add a @return comment here indicating what the boolean is.

>-        var observerService = Cc["@mozilla.org/observer-service;1"].
>-                              getService(Ci.nsIObserverService);
>-        observerService.addObserver(this._observer, "earlyformsubmit", false);
>-        observerService.addObserver(this._observer, "xpcom-shutdown", false);
>+        this._observerService.addObserver(this._observer, "earlyformsubmit", false);
>+        this._observerService.addObserver(this._observer, "xpcom-shutdown", false);

Good catch. :)

>@@ -979,7 +986,7 @@
>      * passed in.  The logins are returned so they can be reused for
>      * optimization.
>      */
>-    _fillForm : function (form, autofillForm, foundLogins) {
>+    _fillForm : function (form, autofillForm, ignoreAutocomplete, foundLogins) {

Add comment briefly explaining the arguments here.

>-        if (autofillForm && !isFormDisabled) {

This kind of touches on the problem in bug 400680. But I guess it's not making things worse, and the fix for that bug will just be different now.

>+        // Variables such that we reduce code duplication and can be sure we
>+        // should be firing notifications if and only if we can fill the form
>+        var usernameValue = null;
>+        var passwordValue = null;

Hmm, I think this would be simpler to carry around a single "selectedLogin" variable (holding the nsILoginInfo we selected), instead of the actual username and password. Then, at the end, you'd just do "usernameField.value = selectedLogin.username" (ditto for the password)

>+        } else if (passwordValue && !autofillForm) {
>+            // For when autofillForm is false, but we still have the information
>+            // to fill a form, we notify observers.

I suppose this is slightly tricky when using "selectedLogin", because if there are multiple eligible logins (the last case in the block above), we won't select one (because we don't know which to use), but we'd still want to fire the notification. Maybe set a "haveMultipleLogins" flag for that case and check it here? Kind of messy, I suppose. Maybe it would help to have an early return here if "!selectedLogin && !haveMultipleLogins"?

>+            this._observerService.notifyObservers(form, "passwordmgr-found-form", "autofillForms=false");
>+            this._observerService.notifyObservers(form, "passwordmgr-found-form", "autocomplete=off");

Let's do "noAutofillForms" and "autocompleteOff", to avoid the implication that "foo=bar" could sometimes be "foo=baz" or some other value.


>--- a/toolkit/components/passwordmgr/test/Makefile.in
>+++ b/toolkit/components/passwordmgr/test/Makefile.in
>@@ -57,6 +57,9 @@
> 		test_basic_form_2pw_2.html \
> 		test_basic_form_3pw_1.html \
> 		test_basic_form_autocomplete.html \
>+		test_basic_form_observer_autofillForms.html \

I'd also add an extra form in this test, with autocomplete=off, to ensure we do the right thing in that combination.

>+		test_basic_form_observer_nofire.html \

I don't see this file in the diff? You could probably just roll that into the autocomplete=off test you're adding (one form disabled, the other form not, then ensure you get just 1 event).

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 8 • 16 years ago

(In reply to comment #7)
> I suppose this is slightly tricky when using "selectedLogin", because if there
> are multiple eligible logins (the last case in the block above), we won't
> select one (because we don't know which to use), but we'd still want to fire
> the notification. Maybe set a "haveMultipleLogins" flag for that case and check
> it here? Kind of messy, I suppose. Maybe it would help to have an early return
> here if "!selectedLogin && !haveMultipleLogins"?

I was under the impression we didn't want one to fire if this method couldn't actually fill the form. I guess we could fire a notification with the extra param being "haveMutipleLogins", but in that case even if they call this method, it won't fill the form (unless the form gets modified before the call).

	Justin Dolske [:Dolske] Reporter
	Comment 9 • 16 years ago

Oh, yeah, I think you're right. When there are multiple logins, we shouldn't fire the event, because there really isn't anything more the observer can do. (The autocomplete dropdown will have been attached, so manual user intervention is still possible and required).

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 10 • 16 years ago

uses nsILoginInfo instead of usernameValue and passwordValue, changed message values, fixed the test issue and consolidated into autocomplete and nofire into 1 test

	Justin Dolske [:Dolske] Reporter
	Comment 11 • 16 years ago

Comment on attachment 326980 [details] [diff] [review]
Patch v4

Just some minor nits. Attach an updated patch for checkin.

>+     * passed in. The logins are returned so they can be reused for
>+     * optimization. Success of action is also returned in format
>+     * [success, foundLogins]
>+     * @param form The form to fill
>+     * @param autofillForm Should fill the form in automatically
>+     * @param ignoreAutocomplete Ignore autocomplete=off attributes
>+     * @param foundLogins Array of nsILoginInfo for optimization

AFAIK we don't generate doxygen docs from source files, so the @param format isn't needed.

>+        // Variables such that we reduce code duplication and can be sure we
>+        // should be firing notifications if and only if we can fill the form
>+        var selectedLogin = null;

Comment is out of date now. :)

>+            if (found) {
>+                selectedLogin = matchingLogin;
>+            } else
>+                this.log("Password not filled. None of the stored " +
>+                         "logins match the username already present.");

Style nit: when the if/else blocks are just 1 statement, don't enclose them in {}.

>+            if (!logins[0].username && logins[1].username) {
>+                selectedLogin = logins[1];
>+            } else if (!logins[1].username && logins[0].username) {
>+                selectedLogin = logins[0];
>             }

Ditto.


>+++ b/toolkit/components/passwordmgr/test/test_basic_form_observer_autocomplete.html
>+  <form id="form2" action="formtest.js">
>+    <p>This is form 1.</p>

No, it's form 2. :-)

>+  // Check that form1 was filled
>+  is($_(2, "uname").value, "testuser", "Checking for filled username 2");
>+  is($_(2, "pword").value, "testpass", "Checking for filled password 2");

Ditto.

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 12 • 16 years ago

Should be final.

	Justin Dolske [:Dolske] Reporter
	Comment 13 • 16 years ago

Pushed http://hg.mozilla.org/mozilla-central/index.cgi/rev/039ec7b9e494

	Sebastian Tschan
	Comment 14 • 16 years ago

I know this has been marked "Resolved Fixed", but I'd like to throw in that it is kind of strange to only fire an event if exactly one login username is available.
Keeping silent if there are multiple usernames available makes this kind of useless to me.

	Justin Dolske [:Dolske] Reporter
	Comment 15 • 16 years ago

(In reply to comment #14)
> Keeping silent if there are multiple usernames available makes this kind of
> useless to me.

What are you trying to do?

	Sebastian Tschan
	Comment 16 • 16 years ago

I'm the author of the Secure Login add-on ( https://addons.mozilla.org/de/firefox/addon/4429 ). Currently, Secure Login adds its own progress listener to the browser object to search for available logins on loaded pages and highlight the form fields and its key icon.

Secure Login offers the possibility to login with a keyboard shortcut or via a toolbar icon. For multiple available logins a selection menu is shown:
http://securelogin.mozdev.org/graphics/screenshots/secure-login-02.png

I would like to make use of the observer service and the notification for found logins, but it would only make sense if the notification is sent everytime logins are found.

	Eric Shepherd [:sheppy]
	Comment 17 • 16 years ago

Documented here: https://developer.mozilla.org/En/Observer_Notifications

	Justin Dolske [:Dolske] Reporter
	Comment 18 • 15 years ago

(In reply to comment #16)

> I would like to make use of the observer service and the notification for found
> logins, but it would only make sense if the notification is sent everytime
> logins are found.

I think bug 492153 will be adding what you want, feel free to comment there.