156922 - Cookie/Image Manager dialog not being updated

Status: VERIFIED FIXED

(Core :: Networking: Cookies, defect, P2)

Description

[Stephen P. Morse]

Now that cookie manager dialog has been changed from modal to non-modal, the 
display needs to be updated dynamically whenever the cookie list changes (e.g., 
when new cookies are accepted).  This applies to its sister dialogs as well, 
such as password manager, image manager, and probably form manager.

Could have sworn there was a bug on this already but I can't find it.  So I'm 
opening this one.

Comment 1

[Stephen P. Morse]

Opening separate bug reports on Password Manager and Form Manager.  This bug is 
just for Cookie/Image manager

See bug 157038 for Password Manager.

Comment 2

[Stephen P. Morse]

And see bug 157039 for Form Manager.

Comment 3

[Stephen P. Morse]

Attachment: patch for cookie and image manager dialogs

Comment 4

[Stephen P. Morse]

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

Comment 5

[Samir Gehani]

Comment on attachment 91053 [details] [diff] [review]
patch for cookie and image manager dialogs

>+close = Close

Is there an extra leading space before Close here?  I can see that the strings
above it needed an extra leading space since they were in a sentence.

Comment 6

[Stephen P. Morse]

Attachment: remove space preceding " Close"

Comment 7

[Stephen P. Morse]

Preceding patch had a severe performance penalty.

Specifically, each time a cookie was removed from the cookie manager front end 
(dialog), the cookie was removed from the tree being displayed and a call was 
made to the back end to remove the cookie from the cookie list.  The back end in 
turn sent a notify back to the front end to remove the cookie from the tree 
being displayed.

The notify is required if a change not originating from the front-end itself is 
made to the cookie list.  But if the change originated from the front end, the 
notify is redundant.  Worse yet, it was responsible for a big performance hit.  
Specifically, I took a list of 200 cookies and did a remove-all from the front 
end.  This would normally take a couple of seconds to complete.  Instead it was 
taking several minutes.

I'll attach a revised patch shortly that does not send the notify for front-end 
originated changes.

Comment 8

[Stephen P. Morse]

Attachment: Significant performance gain

Comment 9

[jag (Peter Annema)]

Comment on attachment 91396 [details] [diff] [review]
Significant performance gain

>Index: extensions/cookie/nsCookies.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/cookie/nsCookies.cpp,v
>retrieving revision 1.62
>diff -u -r1.62 nsCookies.cpp
>--- extensions/cookie/nsCookies.cpp	11 Jul 2002 21:58:43 -0000	1.62
>+++ extensions/cookie/nsCookies.cpp	15 Jul 2002 20:27:12 -0000
>@@ -1704,6 +1709,15 @@
>   cookie_changed = PR_FALSE;
>   strm.flush();
>   strm.close();
>+
>+  /* Notify cookie manager dialog to update its display */
>+  if (notify) {
>+    nsCOMPtr<nsIObserverService> os(do_GetService("@mozilla.org/observer-service;1"));
>+    if (os) {
>+      os->NotifyObservers(nsnull, "cookieChanged", NS_ConvertASCIItoUCS2("cookies").get());

os->NotifyObservers(nsnull, "cookieChanged",
NS_LITERAL_STRING("cookies").get());

NS_LITERAL_STRING is slightly more efficient when the compiler supports
unsigned 16-bit wchar_t.

Same applies to the other NotifyObservers call.

sr=jag with that change.

Comment 10

[Stephen P. Morse]

Attachment: NS_ConvertASCIItoUCS2 changed to NS_LITERAL_STRING

Comment 11

[Stephen P. Morse]

Comment on attachment 91587 [details] [diff] [review]
NS_ConvertASCIItoUCS2 changed to NS_LITERAL_STRING

r=jag (brought forward)

Comment 12

[Stephen P. Morse]

Comment on attachment 91587 [details] [diff] [review]
NS_ConvertASCIItoUCS2 changed to NS_LITERAL_STRING

oops, I meant sr=jag (brought forward)

Comment 13

[[e-mail]]

[Patch looks good to me.] Do you want other perf gains in cookie/wallet code?

Comment 14

[Stephen P. Morse]

Attachment: last patch had some stuff in it for another bug by accident

Comment 15

[Stephen P. Morse]

Comment on attachment 91698 [details] [diff] [review]
last patch had some stuff in it for another bug by accident

sr=jag (brought forward)

Comment 16

[Stephen P. Morse]

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

Comment 17

[Samir Gehani]

Comment on attachment 91698 [details] [diff] [review]
last patch had some stuff in it for another bug by accident

r=sgehani

Comment 18

[Stephen P. Morse]

Checked in on trunk.

Comment 19

[Stephen P. Morse]

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

Comment 20

[Tom Everingham]

verified - cookie display updated dynamically now - 09/16/08 trunk - winNT4,
linux rh6, mac osX