343232 - Camino should prefill passwords ASAP, and not after whole page finished loading

Status: RESOLVED INCOMPLETE

(Camino Graveyard :: General, defect)

Description

[Håkan Waara]

I think Camino needs its own fix for this. See bug 221634 for the Firefox/Core fix.

It involved loading "satchel" (which is toolkit's passwordmgr) on startup.

IIRC, our code needs to be hacked at least in CHBrowserListener.

Comment 1

[Chris Lawson (gone)]

Last time I filed this bug, it was WONTFIXed by pink, but if you've got an idea for how we can do it properly, I'd still be in favour of it.

For reference, see bug 286166.

cl

Comment 2

[Paul Borokhov (lensovet)]

the fix for this in firefox got checked in about five months back. basic strategy there, if i understood correctly, was to call password manager on DOMContentLoaded events, instead of waiting for nsIWebProgressListener's STATE_IS_DOCUMENT/STATE_STOP events.
any progress on this?

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #0)
> I think Camino needs its own fix for this. See bug 221634 for the Firefox/Core
> fix.

Note that this triggered lots of regressions in Firefox, so we should be very careful when implementing this.  See bug 221634 comment 50 and 51.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Ineffective patch

FWIW, changing (either|both) KeychainAutoCompleteSession and FormFillController from DOMAutoComplete to DOMContentLoaded doesn't fix this bug, so there's more plumbing to be done to fix this (let alone to prevent regressions it might cause).  Håkan mentioned CHBrowserListener (but that was well before Stuart and Bryan rewrote the Keychain stuff), but I don't see anything relevant there, given that both of those have their own listeners.

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

OK, I think I know why what I tried before wasn't working

1) FormFillController's |filledAutoCompleteFieldText| synthesizes a DOMAutoComplete event when (possibly among other things) a (e.g., username) form field is filled out, so that

2) KeychainAutoCompleteSession can listen for that event and then fill in the password.

(Reading comments and method names sometimes helps :P )

Based on that simple description, it looks like those two sites handle the multiple accounts case (select or fill in a username, then fill the matching password).

So I'm not sure yet what triggers the start of password autofill, but once we figure out where that starts, we need to have that place start listening for DOMContentLoaded instead of whatever home-grown method we're currently using (and then figure out who needs the listener and plumb it; maybe CHBrowserListener as Håkan suggested in comment 0).

This is something else I'd like to try to look at again for 2.1.1, because it really annoys me on a daily basis, at times when I'm most in a hurry!  If someone else wants to take a stab at it before me, though, or do some MXRing, feel free!

Also, while doing some MXRing in Gecko, I saw this comment:

          // Fastback doesn't fire DOMContentLoaded, so process forms now.
          if (aStateFlags & Ci.nsIWebProgressListener.STATE_RESTORING) {
              this._pwmgr.log("onStateChange: restoring document");
              return this._pwmgr._fillDocument(domDoc);
          }
http://mxr.mozilla.org/mozilla1.9.2/source/toolkit/components/passwordmgr/src/nsLoginManager.js#318

Something else to keep in mind…

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Oh, we call KeychainService's (infamous--how could I have forgotten it!) |fillDOMWindow:| from KS's |onLoadingCompleted:|; I'm really not sure how to replumb that :(  Maybe Stuart will have an idea?

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Oh, looking at the Firefox combo patch for the fix plus regressions (for 1.8.1, where it was stil C++; attachment 229625 [details] [diff] [review]) again, it looks like they used an nsIWebProgressListener's OnStateChange: to check a few state things, trigger fill on fastback, and set up the event listener for DOMContentLoaded (and pagehide).  Then in HandleEvent: they trigger filling on DOMContentLoaded and tear down stuff on pagehide.

So I'm not sure about replumbing an nsIWebProgressListener into KeychainService (CHBrowserListener has one, but I have no idea what it's doing), but the rest of the fix ought to be fairly easy.

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Also on my list of things to look at for 2.1.2…