130306 - Freeze nsIPasswordManager and nsIPassword

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Judson Valeski]

nsIPasswordManager needs to be frozen.

Comment 1

[Stephen P. Morse]

Attachment: freeze for nsIPasswordManager.idl

Comment 2

[Alec Flett]

we still have the issue of UTF8 hostnames/URIs - we need to clarify that in this
interface, it's possible that we'll have to tweak findPasswordEntry. 

I say we should move findPasswordEntry into a seperate interface
(nsIPasswordManagerInternal) and fix up the callers to use this new interface..
esp since it wasn't really part of the original wants-to-be-frozen interface.
then we can freeze nsIPasswordManager without it.

Comment 3

[timeless]

um. i had a patch to one of these interfaces. please don't freeze these interfaces without making sure that they're fully functional. and please give me a weekend over which to comment on them.

is there an announcement to npm.xpcom warning that these interfaces are freezing and asking for comments? (I know we haven't but i'm tired of learning about freezes by clicking on what bugs are reported today and we really should seek input from the community instead of quietly freezing)

Comment 4

[Judson Valeski]

timeless, where's your patch and what does it do? Is there a bug associated with it?

The announcement was to npm.embedding. grep for "API Review Agenda 2/22/01"
(yes, that's last year :-)), we're just getting around to this now, not sure why
it's taken so long.

Community input is sought, _nothing_ is being done in the dark here. Determining
which aliases are the right aliases to spam, and when, isn't always perfect.

Comment 5

[timeless]

A quick search for bugs filed by timeless assigned to morse turns up the bug 
that's dependent on bug 109800.

I'm futuring this bug until someone addresses my bug. It has been waiting for a 
while with an unreasonable 'do we need this' question.

Comment 6

[Judson Valeski]

resetting target milestone. only bug owners can change the target milestone.
let's get some traction on 109800.

Comment 7

[Judson Valeski]

should we be using AUTF8String in here?

Comment 8

[Stephen P. Morse]

Yes we should.  I plan on making the same change here as I made for the 
cookiemanager idl.  Will be posting a revised patch shortly.

Comment 9

[Stephen P. Morse]

*** Bug 130317 has been marked as a duplicate of this bug. ***

Comment 10

[Stephen P. Morse]

Modify summary to reflect the fact that this bug now refers to freezing both 
nsIPasswordManager and nsIPassword.

Comment 11

[Stephen P. Morse]

Attachment: freeze for nsIPassword.idl and nsIPasswordManager.idl

Comment 12

[Stephen P. Morse]

darin,alecf please reviw.  Thanks.

Comment 13

[Alec Flett]

Comment on attachment 74791 [details] [diff] [review]
freeze for nsIPassword.idl and nsIPasswordManager.idl

why do we need nsPasswordManagerInternal? You can just implement
nsIPasswordManagerInternal on nsPasswordManager! no need for extra bloat, extra
contract IDs, etc.

Comment 14

[timeless]

if you are adding new files, they should be MPL/LGPL/GPL, not NPL

Comment 15

[Stephen P. Morse]

Attachment: implementing nsIPasswordManagerInternal on nsIPasswordManager

Comment 16

[Stephen P. Morse]

Comment on attachment 74791 [details] [diff] [review]
freeze for nsIPassword.idl and nsIPasswordManager.idl

r=morse

Comment 17

[Alec Flett]

Comment on attachment 75405 [details] [diff] [review]
implementing nsIPasswordManagerInternal on nsIPasswordManager

woah. no need to mark an "Internal" interface as FROZEN. The whole point of an
internal interface is that we can continue to tweak it. If in future releases
we find methods that we want to freeze, we make a new interface,
(nsIPasswordManager2) move the methods there, and freeze THAT interface.

other than that, the code looks good. sr=alecf with the "@status FROZEN" line
removed from nsIPasswordManagerInternal

Comment 18

[Charles Manske]

Comment on attachment 75405 [details] [diff] [review]
implementing nsIPasswordManagerInternal on nsIPasswordManager

r=cmanske for changes
to publishprefs.js

Comment 19

[timeless]

I don't know how to say this but please STOP NOW.

This bug is *BLOCKED* by my bug, which you acceeded to, and which received an 
SR.  And according to a conversation we've had elsewhere, an interface is *NOT* 
frozen until it says it is.  And there are clearly open issues here.

I'd like to propose a teleconference to discuss this interface. Schedule it for 
Monday-Wednesday of next week.  I can't attend one earlier.

Comment 20

[Stephen P. Morse]

Please ignore comment 16 above.  I was puzzled just now when I saw it, and then 
I realized what happened -- I checked off the "review" box when I meant 
to check off the "obsolete".

Comment 21

[Stephen P. Morse]

timeless: your proposed extension can go into nsPasswordManagerInternal which is 
an extension that is not being frozen.  This would be consistent with how we 
could handle cookie rejects for example -- there is no add-reject method for it 
in the already-frozen nsCookieManager interface but one could be put into an 
internal extension.

Comment 22

[Darin Fisher]

Comment on attachment 75405 [details] [diff] [review]
implementing nsIPasswordManagerInternal on nsIPasswordManager

>Index: nsIPasswordManager.idl

>+  /**
>+   * Called to add an individual login to the list of saved logins
>+   *
>+   * @param aHost The host for which the login is being remembered
>+   * @param aUser The usernam portion of the login

"usernam" (sp)


>Index: nsIPasswordManagerInternal.idl

as timeless is fond of pointing out, new files should use the mixed license...

http://mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c
or
http://mozilla.org/MPL/boilerplate-1.1/npl-tri-license-c

the NPL one is probably what you should use since you are a netscape employee,
but i really don't know what our rules are for such things.


>+ * @status FROZEN

as alecf said: don't freeze an internal interface!


>Index: nsIPassword.idl

>+/** 
>+ * An optional interface for clients wishing to access a
>+ * password object
>+ * 
>+ * @status FROZEN
>+ */

>+    /**
>+     * the name of the host for whom the password is being saved

"for whom"?

how about "the name of the host corresponding to the login being saved"

in fact, why is this called nsIPassword... and not nsILogin or nsILoginInfo??
calling it nsIPassword seems odd since there is a password attribute.


>+     * - if it was created as a result of submitting a form to a site, then the
>+     *   host is the url of the site, as obtained from a call to GetSpec
>+     *
>+     * - if it was created as a result of another app (e.g., mailnews) calling a
>+     *   prompt routine such at PromptUsernameAndPassword, then the host is whatever
>+     *   arbitrary string the app decided to pass in.
>+     *
>+     * Whatever form it is in, it will be used by the password manager to uniquely
>+     * identify hosts, so that "newsserver:119" is not the same thing as "newsserver".

since the meaning of the |host| attribute is so generic, why call it |host|? 
this 
has always bothered me.  it seems like it should be called |domain| or |realm|
or
|loginRealm|... or something, anything but |host|.  but, maybe that's just me
:-/


>+     * the user name that is being saved for this host
>+     * the password that is being saved for this host

again, this wording seems awkward, given that |host| isn't necessarily a
hostname.


>Index: nsFtpConnectionThread.cpp

>+                    pm->RemoveUser(prePath, nsAutoString());

instead of nsAutoString(), use NS_LITERAL_STRING("") or even nsString()...
no reason to waste stack space on an empty string.


>Index: nsHttpChannel.cpp

>+        passWordManager->RemoveUser(domain, nsString(user));

you want nsDependentString(user) instead... that'll avoid this heap
allocation.


>Index: publishprefs.js

>+    var passwordManagerInternal = Components.classes["@mozilla.org/passwordmanager;1"].createInstance();
>+    if (passwordManagerInternal)
>+      gPasswordManagerInternal = passwordManagerInternal.QueryInterface(Components.interfaces.nsIPasswordManagerInternal);

this can be simplified to:

   var passwordManagerInternal =
     Component.classes["@mozilla.org/passwordmanager;1"].createInstance(
       Components.interfaces.nsIPasswordManagerInternal);

>Index: nsMsgIncomingServer.cpp

>       // Obtain the server URI which is in the format <protocol>://<userid>@<hostname>.
>       // Password manager uses the same format when it stores the password on user's request.
>-      char* hostURI;
>-      hostURI = ToNewCString(currServerUri);
>+      nsCAutoString hostURI(currServerUri);

it looks like |hostURI| is not modified... therefore, it is just a needless
copy of
|currServerUri|.  how about eliminating |hostURI| then?


so, basically just a bunch of nits... fix these and r/sr=darin.

Comment 23

[Stephen P. Morse]

Attachment: address issues in comment 22

Comment 24

[timeless]

function GetPasswordManagerInternal() will throw if there is nothing implementing nsIPasswordManagerInternal. You should wrap that. (either caller or callee)

Beyond that. I've repeatedly read the API review notes. It does not mention nsIPasswordManager. 

it mentions nsISingleSignonManager [public: yes. status <Does not yet exist>] and nsIWallet [public: no. status: <empty>] -- the newsgroup contains similar content.

I still don't understand what functionality you're trying to freeze. Clearly any embedders actually using the methods that were supposedly frozen feb 2001 will be just as broken by the changes you're making as the MRJ/OJI's LiveConnect functionality was broken.

My crazy view is that if you aren't freezing for Binary compat (which we aren't, and which we didn't for Mac) then you should be freezing for useful functionality. which means that if it makes sense to have a remove method, it probably makes sense to have an add method.

Oh and yes, i'm just as annoyed at cookie manager. So why am I complaining here? because you already froze that one.

Afaik, netscape policy is to use MPL instead of NPL for any new code, so the only reason to use NPL/LGPL/GPL would be if you're transplanting NPL code and need to relicense to satisfy the trilicense for new files requirement.

You still added a file with the NPL single license.

If findPasswordEntry isn't frozen, then perhaps you could make it useful and convert it to return an Enumerator.  Personally, i'd prefer a single function that returns an enumerator. and if someone wants to enumerate over everything, they would pass in nulls.

I know that I in nsI usually stands for Interface. and that nsPI usually stands for Private Interface. and that nsA doesn't actually stand for Abstract class (although that's probably a bug). What does nsC stand for?

I agree that we shouldn't use host, realm sounds correct to me.
* - if it was created as a result of another app (e.g., mailnews) calling a
*   prompt routine such at PromptUsernameAndPassword, then the host is whatever
*   arbitrary string the app decided to pass in.
It's a host, unless it's a consumer filled value, which makes it an abused api.

Comment 25

[Stephen P. Morse]

timeless wrote:

> function GetPasswordManagerInternal() will throw if there is nothing
> implementing nsIPasswordManagerInternal. You should wrap that. (either
> caller or callee)

Will do.  New patch coming.

> It does not mention nsIPasswordManager.
> it mentions nsISingleSignonManager

Those are the same thing.  There never was a file named nsISingleSignonManager.

> You still added a file with the NPL single license.

If you are referring to nsIPassword.idl, that was fixed in the most recent patch 
posted.  Or is there another one?

> If findPasswordEntry isn't frozen, then perhaps you could make it useful and
> convert it to return an Enumerator.

That function already exists in nsIPasswordManager.  However the mail/news team 
didn't want to have to enumerate, but for efficiency want to obtain the result 
in one call.  So they are the ones who added the findPasswordEntry routine.  
Since then, the composer team has decided to use it as well rather than 
enumerating.

> What does nsC stand for?

Component.  See bug 130304 comment 28.  Also comments 30 and 32.

Comment 26

[Stephen P. Morse]

Attachment: catch exceptions in GetPasswordManagerInternal

Comment 27

[Stephen P. Morse]

Latest patch is essentially the same as the preceding two, so r/sr=darin and 
r/sr=alecf carry forward.

Comment 28

[timeless]

NPL: Index: nsIPasswordManagerInternal.idl

ah yes, I knew i saw nsC somewhere...

Comment 29

[Stephen P. Morse]

> NPL: Index: nsIPasswordManagerInternal.idl

Where are you looking?  There is no NPL in nsIPasswordManagerInternal.idl.  At 
least not in the previous two patches that were posted.

Comment 30

[Stephen P. Morse]

Oh, I see what you mean.  It's not NPL (which I was searching for) but rather it 
has it written out -- Netscape Public License.

New patch coming right up.

Comment 31

[Stephen P. Morse]

Attachment: Netscape Public License -> Mozilla Public License

Comment 32

[Stephen P. Morse]

Comment on attachment 75637 [details] [diff] [review]
Netscape Public License -> Mozilla Public License

carrying reviews forward (alecf and darin)

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why don't we just add timeless's 'add' method while we still have the chance? It
sure looks odd having a removeReject without an addReject, especially with
addUser/removeUser just above. Consistency with the cookie manager doesn't seem
like a strong argument.

Comment 34

[Stephen P. Morse]

Let's try to move this along and not get bogged down over the frozen issue.  
There are many other good changes here that will bit-rot if this doesn't get 
checked in.

Therefore removing the @status FROZEN indicator from nsIPasswordManager at this 
time (but leaving it on nsIPassword).  This will allow timeless to continue 
working on his patch for bug 109800 after this checkin is made.  Then, in the 
future, when we are ready to freeze this we can add back the @status FROZEN 
without having to go through all the other changes that were done here.

Posting a new patch with FROZEN removed.

Comment 35

[Stephen P. Morse]

Attachment: same as above but without FROZEN for nsIPasswordManager

Comment 36

[Stephen P. Morse]

Comment on attachment 75790 [details] [diff] [review]
same as above but without FROZEN for nsIPasswordManager

carrying r and sr forward from prevous patches

Comment 37

[Alec Flett]

Comment on attachment 75790 [details] [diff] [review]
same as above but without FROZEN for nsIPasswordManager

sr=alecf for the record

Comment 38

[Asa Dotzler [:asa]]

Comment on attachment 75790 [details] [diff] [review]
same as above but without FROZEN for nsIPasswordManager

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 39

[Stephen P. Morse]

Fix checked in.

Not closing this out yet, because we still have to add the FROZEN indicator to 
the nsIPasswordManager.idl file.  That will be the next step.

So now this bug is blocked by 109800.

Comment 40

[timeless]

I'm still very confused, can you please explain why we need to freeze this 
internal interface against the advice of darin and alecf?

I can have an updated patch to internal in 30 mins.

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

He's not proposing to freeze the internal interface. Steve is proposing to
freeze nsIPasswordManager.

Having said that, I would like to see your method in the public interface. Go
for it!

Comment 42

[Gervase Markham [:gerv]]

Steve: nsIPasswordManagerInternal.idl , which you created in the previous patch,
was checked in with the MPL license only. Please could you fix this?

Gerv

Comment 43

[Stephen P. Morse]

Gerv, in comment 24 timeless wrote:

   Afaik, netscape policy is to use MPL instead of NPL for any new code,
   so the only reason to use NPL/LGPL/GPL would be if you're transplanting
   NPL code and need to relicense to satisfy the trilicense for new files
   requirement.

Comment 44

[Gervase Markham [:gerv]]

Timeless is being unclear again :-)

What he meant to say was:

"AFAIK, Netscape policy is to use MPL-based licenses instead of NPL-based
licenses for any new code. So the only reason to use NPL/LGPL/GPL instead of
MPL/LGPL/GPL would be if you're transplanting NPL code. Otherwise, you should
use MPL/LGPL/GPL."

The mozilla.org licensing policy
(http://www.mozilla.org/MPL/license-policy.html) requires that new code that is
checked into the tree be MPL/LGPL/GPL tri-licensed. If you need more info on
what license to use in more complex situations, that document is the place to look.

Gerv

Comment 45

[Stephen P. Morse]

I'm really confused by all this.  I just clicked on the link you cited above and 
it directed me to the following url for the tri-license boilerplate for idl 
code:

  http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c

That page looks like what I have in the nsIPasswordManagerInternal.idl file.  So 
please tell me explicitly what it should say instead.

Comment 46

[Daniel Veditz [:dveditz]]

nsIPasswordManagerInternal.idl (and probably nsIPasswordManager.idl based on
what little of the license appears in the patch) is only the plain MPL... you
don't have the license block "envelope"

  /* ***** BEGIN LICENSE BLOCK *****
   * Version: MPL 1.1/GPL 2.0/LGPL 2.1
   *
      <license>
   *
   * ***** END LICENSE BLOCK ***** */

and you don't have the final paragraph ("Alternatively, ...") that makes this
other than the plain MPL.

Comment 47

[Stephen P. Morse]

Oops, I misunderstood the boilerplate.  I thought that paragraph starting with 
"alternatively" was an alternative to the preceding wording and that only one of 
the two forms would be used in the file.  Now that I read the paragraph more 
carefully, I can see that was a dumb thing for me to conclude.

OK, I'll fix this up.

Comment 48

[Stephen P. Morse]

Attachment: use correct license

Comment 49

[jag (Peter Annema)]

Comment on attachment 76687 [details] [diff] [review]
use correct license

sr=jag

Comment 50

[Brian Ryner (not reading)]

Comment on attachment 76687 [details] [diff] [review]
use correct license

r=bryner

Comment 51

[Asa Dotzler [:asa]]

Comment on attachment 76687 [details] [diff] [review]
use correct license

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 52

[Stephen P. Morse]

license update checked in.

Comment 53

[John Keiser (jkeiser)]

License change was backed out; compilation problems.

Comment 54

[Asa Dotzler [:asa]]

We're late in the 1.0 cycle. Can we please not use tinderbox as our compiler.
Thanks.

Comment 55

[Stephen P. Morse]

Fixed and checked back in.