Comment 77 for bug 398128

Revision history for this message
In , Dolske (dolske) wrote :

(In reply to comment #66)

> >+ // Don't trigger a 2nd master password prompt (it's
> >+ // unlikely we'll get here, because we normally don't
> >+ // attach this listener until after a MP entry).
>
> So how would we get to this case?

Hmm, I guess we can't. I was thinking some mix of loading a login page, canceling the resulting MP prompt, and then later revisiting the page to type in a login... But the MP cancelation would throw, and fillDoc() would thus bail out before attaching an autocomplete listener. So, not just edge-casy but shouldn't be possible. Removed. Ditto for the other case of this.

> > if (!this._remember)
> >- return false;
> >+ return null;
>
> What's this change for?

Noticed while checking code that autoCompleteSearch is supposed to return an nsIAutoCompleteResult (or null), not a bool.

> >+ doc.addEventListener("mozCleverClosureHack", observer, false);
>
> Trickyness indeed. Is this something we do elsewhere in the code?

Yeah, I first used it over in browser.js for some plugin stuff.

> Didn't we already do the hasLogins check when adding the prompt? Isn't it just
> |prompt.hasLogins| & we can avoid this additional DB hit?

Hmm, good catch. I think that's deadwood from an early version of the patch. I've removed the |prompt.hasLogins| setting, since it wasn't used. Better to check it right when needed (otherwise we could miss a recently-saved login).