109800 - nsIPasswordManager.idl doesn't allow you to manually add a site to the never saved list

Status: VERIFIED FIXED

(SeaMonkey :: Passwords & Permissions, defect)

Description

[timeless]

Comment 1

[timeless]

Attachment: add interface and implementation

Comment 2

[Stephen P. Morse]

Patch looks fine to me.  But you are adding an API and never calling it.  So 
that raises the following questions:

1. If it's not being called, why is it needed?  If you plan on calling it in the 
future, shouldn't that be the time that you add this API?  Or, at the very 
least, shouldn't you mention in this bug report why this API is going to be 
needed.

2. How can you test it out if it's not being called?

Comment 3

[timeless]

Yeah i intend to call it, but if i got struck down at this point i wasn't going
to dig into the chrome to add the text box and button that would consume it,
besides the patches are separately reviewable and kinda ask for different
reviewers (maybe).

Since you haven't objected, i'll try to attach the ui pair tonight after i munch.

Comment 4

[timeless]

sorry, too complicated for tonight, the second half is now bug 109810.

Comment 5

[Judson Valeski]

Comment on attachment 57527 [details] [diff] [review]
add interface and implementation

seems reasonable to me. in fact, the existing nsIPasswordManager api has a
"removeReject" w/ out a corresponding "addReject" which indeed seems deficient
from an API standpoint.

Comment 6

[Judson Valeski]

adding topembed+ to this since it blocks another topembed+ bug.

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 57527 [details] [diff] [review]
add interface and implementation

>+PUBLIC nsresult
>+SINGSIGN_AddReject(const char *host /*, const char *userName*/) {
>+  si_PutReject(host, nsAutoString(/*thisParameter_isObsolete*/), PR_TRUE);
>+#ifdef SOMEONE_WANTS_TO_KNOW_WHY_I_DID_THIS
>+  http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/extensions/wallet/src/singsign.cpp&rev=1.202&mark=1698#1650
>+#endif
>+  return NS_OK;
>+}

This kind of #ifdef is bogus, it's not even code inside it.
Please make this a comment instead

A doxygen comment in the idl file would be nice. I know the
current entries don't have them, but the patch in the blocked
bug does. when both this and 130306 lands your new API won't
have a comment like the others.

with those comment changes, sr=dveditz

Comment 8

[Stephen P. Morse]

Please hold off on this.  The interface was supposedly frozen last march 
(although the @frozen was inadvertently not added to the file).  Since then 
another routine has crept into it (findPasswordEntry) and, as you know, we have 
been asked to remove it (bug 130305 and bug 130317)

Comment 9

[timeless]

these bugs don't seem relevant, could you provide more detail

Comment 10

[Stephen P. Morse]

Sorry, I meant to say bug 130306 (not 130305) above.

Comment 11

[Judson Valeski]

after more thinking/talking/emailing/discussing... reverting to topembed- here.
I don't think this should be a blocker for 130306. Although this method
complementing seems good, it breaks symmetry with similair ifaces
(nsICookieManager for example). There aren't any consumers of this, and the
other bug this blocks is marked as an enhancement and futured.

timeless, can we remove 130306 from the blocking list?

Comment 12

[timeless]

Attachment: idl comments, fixed ifdef

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 76438 [details] [diff] [review]
idl comments, fixed ifdef

marking r=valeski sr=dveditz

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

taking bug for checking in when it has approval

Comment 15

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

Comment on attachment 76438 [details] [diff] [review]
idl comments, fixed ifdef

a=roc+moz

Comment 16

[Stephen P. Morse]

Why don't you add the frozen line as part of this checkin?

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in

morse, I'll leave adding that line to timeless as part of bug 130306

Comment 18

[Tom Everingham]

verified