Closed Bug 221185 Opened 16 years ago Closed 12 years ago
[cookies] rework notifications
the current notifications in cookies/permissions are cumbersome. firing a notification that just says "hey, something changed, somewhere", and forcing the consumer to reload the entire cookie list (via getEnumerator), scales badly. the cookie manager UI doesn't help things by having a surprisingly terrible removal algorithm. the net result is that, with moderate sized cookie/permission lists (~100 items), it can take minutes to execute a 'remove all' operation in the cookie manager. see bug 210465 for an example. and i won't get started on the ridiculousness of the p3p "cookieIcon" notification. the proposal for fixing this, is to replace the current notification system in cookies/permissions with two notifications: 1) "cookie-rejected", whenever a cookie gets rejected as a result of user prefs (this includes default prefs, and the permission list, and prompting. it does not include actual failures to set a cookie, such as it being over the 4kb size limit). this is for bug 216743, so we can implement a statusbar cookie icon for easily seeing when cookies have been rejected. it will also replace the "cookieIcon" p3p-related notification. it will pass the cookie host URI as an nsIURI* subject to its observers, so that they may easily whitelist that URI via permmgr. 2) "cookie-changed"/"perm-changed", whenever the cookie/permlist changes. this will have four possible "data" categories, with an |nsICookie2* aCookie|/|nsIPermission* aPermission| subject as described: "deleted" means a cookie was deleted. aCookie is the deleted cookie. "added" means a cookie was added. aCookie is the added cookie. "changed" means a cookie was altered. aCookie is the new cookie. "cleared" means the entire cookie list was cleared. aCookie is null. this will allow for extremely fast UI operations (no list reloading, after the initial load), and easy "cookie snooping" for things that need it (such as cookie monitors, silly p3p icons etc). this can be implemented in two parts: first, add backend support; then rewrite the UI to take advantage of it. (so for a while, we will have a bit of compat code to fire the old notifications). any suggestions welcome ;)
this is the backend work... there's a tiny bit of compat code in cookies, to deal with the old notifications (which can die after the UI is fixed). the permissions patch is ugly because permissions doesn't store nsIPermission objects natively, so we have to make them just to notify. we might want to change that; it's up to mvl :)
oh; a benefit of this new system... it eliminates the need for the LazyNotify stuff darin is adding in, so it will end up bloat-neutral :)
just for laughs, i worked out the algorithmic complexity of the 'remove all' function in cookiemanager. it's O( n^3 log(n) ).
same as previous, but with some meaty comments in nsICookieService.idl and nsIPermissionManager.idl describing the new notifications (thanks for comments mvl!). it also adds a new method to nsICookie2, GetRawHost() - same as GetHost() but without the leading dot (in the case of domain cookies). this is useful for the cookiemgr UI, which i'm fixing to use the new notifications now. i think this is ready for review ;)
same as previous, updated to trunk and with a couple tweaks.
Comment on attachment 133126 [details] [diff] [review] backend patch v1.1 > nsresult > nsPermissionManager::AddInternal(const nsAFlatCString &aHost, >- PRInt32 aTypeIndex, >- PRUint32 aPermission) >+ PRInt32 aTypeIndex, >+ PRUint32 aPermission, >+ PRBool aNotify) Please add a comment why the notification can't be done in Add(), but must be in AddInternal(). I just can't remember it :) r=mvl with that.
Attachment #133126 - Flags: review?(mvl) → review+
Comment on attachment 133126 [details] [diff] [review] backend patch v1.1 >@@ -897,35 +912,24 @@ nsCookieService::Remove(const nsACString >- mCookieChanged = PR_TRUE; >- // we might want to eventually push this Write() call into the UI, >- // to just write once on cookiemanager close. > Write(); But don't forget the LazyWrite trick you mentioned, to speed the UI up a little bit.
>Please add a comment why the notification can't be done in Add(), but must be >in AddInternal(). I just can't remember it :) will do. (the reason is because we need to do the hash lookup before we know whether it's a removal, addition, or change). >But don't forget the LazyWrite trick you mentioned, to speed the UI up a little >bit. darin - i'll change the Write() in Remove() to a LazyWrite(). sound ok?
Comment on attachment 133126 [details] [diff] [review] backend patch v1.1 punting to bz, to take some load off darin. bz: please see comment 6 on for the current review comments that apply to this patch... thanks!
Attachment #133126 - Flags: superreview?(darin) → superreview?(bzbarsky)
Comment on attachment 133126 [details] [diff] [review] backend patch v1.1 >@@ -1316,48 +1305,49 @@ nsCookieService::AddInternal(nsCookie >+ // replace previous cookie >+ NotifyChanged(aCookie, NS_LITERAL_STRING("changed").get()); >+ mCookieList.ReplaceElementAt(aCookie, insertPosition); >+ NS_ADDREF(aCookie); You notify before you change the cookie. This means that if the observer still uses the enumerator to get all the cookies (as it does currently) it will get the old list. Same with the other notifications here. > nsPermissionManager::AddInternal(const nsAFlatCString &aHost, >+ // adding >+ NotifyObserversWithPermission(aHost, >+ mTypeArray[aTypeIndex], >+ aPermission, >+ NS_LITERAL_STRING("added").get()); > entry->SetPermission(aTypeIndex, aPermission); Same here, but for permissions. Maybe it isn't a huge problem for cookies, but you use the same notification here as before, so the sites tab for cookies now shows the situation before the last change.
>You notify before you change the cookie. This means that if the observer still >uses the enumerator to get all the cookies (as it does currently) it will get >the old list. Same with the other notifications here. yeah i wondered if anyone would catch that :) good spotting. i noticed that a few days ago and fixed in my local tree. you'll notice it's fixed in the hashtable patch in bug 143939.
addresses all previous review comments. mvl: i'll carry over your r= on this, but it would be super if you could doublecheck to make sure i have the order correct now, and also to make sure you're happy with the comment in nsPermissionManager:AddInternal()... thx!
Attachment #133126 - Attachment is obsolete: true
Changes look good to me.
Comment on attachment 133620 [details] [diff] [review] backend patch v1.2 [Checked in: Comment 15] sr=bzbarsky with the comment changes I pointed out on irc.
Attachment #133620 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 133620 [details] [diff] [review] backend patch v1.2 [Checked in: Comment 15] landed this one for alpha. i'll keep this bug open for the UI changes that go with it...
Attachment #133620 - Attachment description: backend patch v1.2 → backend patch v1.2 (checked in)
ui changes have long since been made; marking fixed.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.