Comment 21 for bug 182519

(From update of attachment 295350)
*sigh* I still think propagating the nsIPromptService/nsIPromptService2 mess further up into consumers isn't the right thing to do, and is likely to cause more headaches down the road. I guess if bsmedberg says this is the lesser of two evils, then I'm probably grudgingly willing to defer to him, and grind my teeth until Mozilla 2 can save us. :-/

>Index: toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js
>===================================================================

>+ _makeDialogText : function (aChannel,
>+ aAuthInfo) {

Nit: 1 line.

>+ var stringBundleSvc = Cc["@mozilla.org/intl/stringbundle;1"].
>+ getService(Ci.nsIStringBundleService);
>+ var stringBundle = stringBundleSvc.createBundle("chrome://global/locale/prompts.properties");

Nit: Align "Cc" and "getService" to same column. stringBundle assignment line is > 80 columns. Maybe s/stringBundleSvc/bundleSvc/ to help keep things short?

>+ var port = aChannel.URI.hostPort;
>+ var host = aChannel.URI.host;
>+ var scheme = aChannel.URI.scheme;
>+ var username = aAuthInfo.username;
>+ var password = aAuthInfo.password;
>+ var proxyAuth = aAuthInfo.AUTH_PROXY & aAuthInfo.flags;
>+ var realm = aAuthInfo.realm;
>+ var text = "";
>
>+ if (port > -1)
>+ host = host + ":" + port;

This can all be replaced with:

var [hostname, httpRealm] = this._getAuthTarget(aChannel, aAuthInfo);

>+ if (proxyAuth) {
>+ text = "EnterUserPasswordForProxy";
>+ } else {
>+ text = "EnterUserPasswordForRealm";
>+ host = scheme + "://" + host;
>+ }

Tweaking host not needed here with _getAuthTarget, can also then remove brackets.

>+ if (aAuthInfo.flags & aAuthInfo.ONLY_PASSWORD) {
>+ text = password;

This should be |text = "EnterPasswordFor";|

>+ } else if (!proxyAuth && !realm) {
>+ text = "EnterUserPasswordFor";
>+ count--;
>+ strings[0] = strings[1];
>+ }

This case goes away with _getAuthTarget(). It returns the hostname as the realm when there's no realm.

>+ _promptPasswordAdapter : function (aPromptService,
...
>+ if(flags & aAuthInfo.ONLY_PASSWORD) {
>+ rv = aPromptService.promptPassword(aParentDOMWindow, null, message, defaultPass, aCheckLabel, aCheckValue);
>+ }

This isn't right, see next comment. [Sorry, was reviewing bits in a random order. :-)]

>+ else {
>+ var defaultUserObject = new Object();
>+ defaultUserObject.value = defaultUser;
>+ var defaultPassObject = new Object();
>+ defaultPassObject.value = defaultPass;
>+ rv = aPromptService.promptUsernameAndPassword(aParentDOMWindow, null, message, defaultUserObject, defaultPassObject, aCheckLabel, aCheckValueInOut);

This isn't right, the values entered by the user will be ignored.

You want:

           var usernameInOut = { value : defaultUser }
           var passwordInOut = { value : defaultPass }
           rv = aPromptService.promptUsernameAndPassword(...)
           if (!rv)
             this._SetAuthInfo(aAuthInfo, usernameInOut.value, passwordInOut.value);

(Check that rv works as expected there)

Also, nit, line is > 80 columns.

>- var ok = this._promptService.promptAuth(this._window, aChannel,
>- aLevel, aAuthInfo, checkboxLabel, checkbox);
>+ var ok = false;
>+ try {
>+ ok = this._promptService2.promptAuth(this._window, aChannel,
>+ aLevel, aAuthInfo, checkboxLabel, checkbox);
>+ } catch (e) {
>+ try {
>+ ok = this._promptPasswordAdapter (this._promptService, this._window, aChannel,
>+ aLevel, aAuthInfo, checkboxLabel, checkbox);
>+ } catch(e) { dump("exception in _promptPasswordAdapter ::: "+e+" \n"); throw e; }
>+ }
>+

I'd rather contain all this in promptPasswordAdapter()... Just s/promptAuth/promptPasswordAdapter/ in the original code, and have promptPasswordAdapter() handle the try/catch fallback.

Is there some to determine which promptService to use, without a try/catch? I'm a bit concerned that we could get into a case where promptService2.promptAuth() is what we really do want to be calling, it throws an exception [on purpose or accidentally]. Now we would be ignoring the failure and trying again with promptService1.