129753 - Freeze interface for cookie manager and password manager

Status: RESOLVED INVALID

(Core :: Networking: Cookies, defect)

Description

[Stephen P. Morse]

Do appropriate things to indicate that the following interfaces are frozen:

   nsICookieManager.idl
   nsIPasswordManager.idl

Comment 1

[Stephen P. Morse]

Attachment: First draft of patch to get files into "frozen" format

Comment 2

[Judson Valeski]

adding topembed+ keyword. I'm guessing nsbeta1 should be minused though.

Comment 3

[Judson Valeski]

Comment on attachment 73289 [details] [diff] [review]
First draft of patch to get files into "frozen" format

please provide param comments for nsICookieManager::Remove(...) and move the
CID out into a private header file.

Comment 4

[Stephen P. Morse]

Attachment: patch to address valeski's concerns in comment #3

Comment 5

[Chak Nanga]

Comment on attachment 73544 [details] [diff] [review]
patch to address valeski's concerns in comment #3

r=chak

Comment 6

[Chak Nanga]

Cc'ing Alec for sr=

Comment 7

[Alec Flett]

Comment on attachment 73544 [details] [diff] [review]
patch to address valeski's concerns in comment #3

yahoo! an opportunity to comment on this interface :)

>+  /**
>+   * Called to enumerate through each cookie in the cookie list
>+   */
>   readonly attribute nsISimpleEnumerator enumerator;

This needs a better name. what are we enumerating? What objects are in the
enumerator? 
This should be called something like "cookies" or "cookieEnumerator" (thought I
don't
see a reason to have "enumerator" in the name.

Also, document that the objects are nsICookie objects (is that frozen? wouldn't 
it need to be?)

>-  // Takes hostname, username and password as input parameters and returns 
>-  // set of filled-in hostname, username and password for the first
>-  // password element match. Empty string is treated as a wild
>-  // card entry and will be considered as a match for any of the input
>-  // parameters. 
>+  /* A Call to find a specific login in the password manager list.
>+   * Empty input parameters are treated as wild cards.
>+   *
>+   * @param hostURI The uri for which the login was saved
>+   * @param username The username part of the saved login
>+   * @param password The password part of the saved login
>+   *
>+   * upon return, the blank input parameters are replaced with their
>+   * actual values
>+   */
>   void findPasswordEntry(inout string hostURI, inout wstring username, inout wstring password);

ack..You're removing details about this API! read the comment you're
removing...
all that info belongs in the new comment.

> 
>+  /**
>+   * Called to enumerate through each login in the password-manager list
>+   */
>   readonly attribute nsISimpleEnumerator enumerator;

again, what are we enumerating? this needs both a new name and documentation
for the 
type of objects we're going to get from the enumerator.

>+
>+  /**
>+   * Called to enumerate through each rejected site in the password-manager list
>+   */
>   readonly attribute nsISimpleEnumerator rejectEnumerator;
> };

Again, what objects are we getting back? Can this get a better name? 
How about "rejectedSites"?
Document what a "rejected site" is - rejected by who? the web site, the user,
or the 
password manager?

Comment 8

[Stephen P. Morse]

> This needs a better name. what are we enumerating? What objects are in
> the enumerator?

Jud held a meeting a year ago (March 2001) to freeze these interfaces.  The 
names appearing in the module now are those names that Jud requested at that 
time (see comment #4 in bug 46783).  And you super-reviewed these names at that 
time.

These interfaces were supposedly frozen since that time.  However last steps of 
the freezing process (putting in appropriate comments, adding @frozen) was 
overlooked at that time.  This bug report is to rectify that, rather than change 
the interface.  (Racham made a recent change to the interface which I agreed to 
because I had forgotten that they were frozen.)

> ack..You're removing details about this API! read the comment you're
> removing...  all that info belongs in the new comment.

All those details are there, only rewritten.  Those details are as follows:

1. Takes username, hostname, and password as input parameter.

   This is now conveyed in even more detail than before in the three
   @param lines that I added.

2. Returns set of filled in username, hostname, and password

   Well that's not really stated accurately.  These three items were supposedly
   supplied as inputs.  If any were left blank, they are filled in by the
   routine.  That is stated in the comment that I added at the end which says
   "upon return, the blank input parameters are replaced with their actual
   values"

3. Empty string is treated as wild-card entry and will be treated as a match
   for any input parameter

   That is covered in my added comment of "empty input parameters are treated
   as wild cards"

Comment 9

[Stephen P. Morse]

> Also, document that the objects are nsICookie objects (is that frozen?
> wouldn't it need to be?)

Jud, can you answer this?

Comment 10

[Judson Valeski]

morse wrote:
> (Racham made a recent change to the interface which I agreed to 
> because I had forgotten that they were frozen.)

Ouch! What was the modification? We might be able to let it slide because the
idl didn't actually have "@status FROZEN" in it, but, we should understand the
change and it's necessity first.

"nsICookie" needs to be frozen as well.

Comment 11

[Stephen P. Morse]

The change was the addition of the findPasswordEntry routine.

Comment 12

[Stephen P. Morse]

racham, can you comment on the necessity of your change (jud's question in 
comment #10).

Comment 13

[Stephen P. Morse]

Attachment: Freeze nsICookie and nsIPassword as well, add comment to indicate what object types are being enumerated

Comment 14

[Alec Flett]

> Jud held a meeting a year ago (March 2001) to freeze these interfaces.  The 
> names appearing in the module now are those names that Jud requested at that 
> time (see comment #4 in bug 46783).  And you super-reviewed these names at that 
> time.

What I super-reviewed was changing of .h into .idl, and there was no mention of
anyting freezing at the time (look in the bug, I can't find any mention of the
interfaces being frozen!)

Freezing an interface is a whole different beast than tweaking it for our own
use, the standards are higher as far as usability. If I had realized at the time
that these were for an eventual freeze, I might have suggested something better.

So for the record, I still prefer something less-generic than "enumerator" but
I'm willing to agree to how it is now since it went through one review process
already)

As for the comment bit - sorry I was mentally grepping for "a match for any
input parameter" and totally missed the line that said "Empty input parameters
are treated as wild cards."
That said, I think the comments should be corrected w.r.t. your comments in
comment 8, part 2, and in addition I think that we need to bring back the bit
about "a match for any input parameter" and drop the "wildcard" term because
"wildcard" usually refers to a magic character or set of characters that
represent a match...i.e. I don't want someone to think "oh, an empty string is
the same as a wildcard, therefor it accepts wildcards, like '*')

Also, specify what we mean by "empty input parameters" (i.e. a null string, or
null itself?)

Comment 15

[Alec Flett]

Comment on attachment 73574 [details] [diff] [review]
Freeze nsICookie and nsIPassword as well, add comment to indicate what object types are being enumerated

in nsICookie/nsIPassword, the comments need to start with /** not /*

Comment 16

[Stephen P. Morse]

> What I super-reviewed was changing of .h into .idl, and there was no mention
> of anyting freezing at the time (look in the bug, I can't find any mention of
> the interfaces being frozen!)

You are absolutely correct.  At that time I didn't know it had anything to do 
with freezing either.  But last week I got e-mail from Jud saying that it had 
everything to do with freezing.  So apparently we were freezing it at that time, 
and I was just as confused about that as you.

I'll make the comment changes you just requested and upload a new patch.

Comment 17

[Stephen P. Morse]

Attachment: Clarified comment for findPasswordEntry, changed /* to /**

Comment 18

[timeless]

i don't have time to critique this now, but if this interface doesn't allow js 
things to add/remove items at will then this is not ready to be frozen.  Morse 
has a bug with patch to allow one of those.

Comment 19

[Judson Valeski]

These interfaces aren't frozen until they have the @status FROZEN marker. The
intention was to get that marker in them *many* moons ago (month or two after
the first rev of mods), but, that never happened.

I'm filing new bugs individually for each interface so improve focus here (looks
like some of these might not be ready). Feel free to snuff this bug (or morph
it) out if desired.

http://bugzilla.mozilla.org/show_bug.cgi?id=130304
http://bugzilla.mozilla.org/show_bug.cgi?id=130305
http://bugzilla.mozilla.org/show_bug.cgi?id=130306

Steve, can you push these patches to those bugs?

Comment 20

[Stephen P. Morse]

Need to add the following to Jud's list above.

   http://bugzilla.mozilla.org/show_bug.cgi?id=130317

With that, I'm closing this report out as invalid (it's actually now a dup of 
the four reports above, but I can't mark it a dup to all four).

Comment 21

[Alec Flett]

the convention is to make this a tracking bug, and make this bug dependent on
the other 4.

Comment 22

[Peter Trudelle]

nsbeta1- per  Nav triage team, in case this gets turned into a tracking bug as
per convention.