The current checks in nsILoginManager.addLogin make it virtually impossible to use from c++ code. If I call it with (assume strings are of the correct type): AddLogin("hostname", EmptyString(), "hostname/path", ...); This sets the formSubmitUrl to "" and the httpRealm to "hostname/path". In addLogin there is the following code: if (login.formSubmitURL || login.formSubmitURL == "") { // We have a form submit URL. Can't have a HTTP realm. if (login.httpRealm != null) throw "Can't add a login with both a httpRealm and formSubmitURL."; } else if (login.httpRealm) { // We have a HTTP realm. Can't have a form submit URL. if (login.formSubmitURL) throw "Can't add a login with both a httpRealm and formSubmitURL."; } So we enter the first if, because login.formSubmitURL == "", and then login.httpRealm is not null, and hence it throws. I think if you do it the other way round, you'll get the same issues. I need to write some testcases, but I think some of the other if checks unnecessary/wrong in that function as well. We want this for TB 3 as its blocking password manager migration, but I'll try and come up with a patch before next week.

(In reply to comment #0) > The current checks in nsILoginManager.addLogin make it virtually impossible to > use from c++ code. Well, I wouldn't say impossible. We ran into this back in bug 380857. Two approaches: 1) Create a nsLoginInfo object, don't use Init(), and only set the fields you have values for (the others thus keep the default null value). 2) Now that bug 380783 has landed, shouldn't string->SetIsVoid() work? > I need to write some testcases, but I think some of the other if checks > unnecessary/wrong in that function as well. No, the null-vs-emptystring is an important distinction in the password manager API and logic. Can't be removed.

(In reply to comment #1) > 1) Create a nsLoginInfo object, don't use Init(), and only set the fields you > have values for (the others thus keep the default null value). That's not really a nice work around and would imply the function should be noc++ (or whatever the wording is). > 2) Now that bug 380783 has landed, shouldn't string->SetIsVoid() work? I didn't know about this. The string APIs and changes are really badly documented. > > I need to write some testcases, but I think some of the other if checks > > unnecessary/wrong in that function as well. > > No, the null-vs-emptystring is an important distinction in the password manager > API and logic. Can't be removed. I think it would be very useful to provide documentation as to what the distinction is. At the moment, I have only a hint of an idea why it may be and that I'm definitely not certain about. I guess I'll probably have to use the individual functions, if it works, then I think we should morph this bug into documenting the code and idl better.

I just gave it a quick try, I can get around it, though it means I have to call most (all but one) of the other attribute set functions individually. Therefore morphing to a documentation bug as I think the interface docs need improving: - nsILoginInfo::Init needs to explain or refer to the other fields so that implementers are made aware of the possible null implementations. - Somewhere on nsILoginInfo I think we should have something explaining what NULL is in c++ terms. As ns(C)String->SetIsVoid() isn't very logical. - Null versus empty string also needs better documentation for what it actually means and why the difference is required.

There's https://developer.mozilla.org/En/NsILoginManager and https://developer.mozilla.org/en/NsILoginInfo, based off the IDL. The docs could call out this distinction as a special note, I guess.

Err, https://developer.mozilla.org/en/Using_nsILoginManager is what I meant for the first link. And since I'm commenting again, I also meant to add: (In reply to comment #3) > As ns(C)String->SetIsVoid() isn't very logical. Our whole string API isn't very logical. :-)