220624 - [cookies] refactor prompting helper to where it belongs

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[[e-mail]]

currently, nsCookieService::SetCookieInternal does an extra cookielist traversal
that's not necessary in most cases: it needs to find whether a site has set any
cookies previously, in order to pass that info into the prompting code, to
display to the user. if the user has the "prompt me before setting cookies" pref
turned off, these cycles are wasted...

patch coming up, which pushes this code into nsCookiePermission::CanSetCookie,
to be executed only when necessary. this has the consequence that
nsCookieService::FindCookiesFromHost must now be exposed publicly, so i've done
that via nsICookieManager2.

the code for checking if the cookie has already expired (so we don't prompt the
user in that case) is ugly; i wish we could kill it... :/

Comment 1

[[e-mail]]

Attachment: el patcho

Comment 2

[[e-mail]]

Comment on attachment 132339 [details] [diff] [review]
el patcho

darin, mind taking a peek?

(this diff is against your v2.1 patch in bug 210561, btw... i didn't want to
wait until it landed ;)

Comment 3

[[e-mail]]

mvl commented that the method 'FindCookiesFromHost' was ill-named since it
returned bool. I've renamed it in my tree to 'FindMatchingCookie', and updated
comments to match.

Comment 4

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 132339 [details] [diff] [review]
el patcho

> NS_IMETHODIMP 
> nsCookiePermission::CanSetCookie(nsIURI     *aURI,

>+    // check if the cookie we're trying to set is already expired, and return;
>+    // but only if there's no previous cookie, because then we need to delete the previous
>+    // cookie. we need this check to avoid prompting the user for already-expired cookies.

>+NS_IMETHODIMP
>+nsCookieService::FindCookiesFromHost(nsICookie2 *aCookie,

>     // only count session or non-expired cookies

So, I'd guess the check in CanSetCookie is not needed.


Otherwise, looks good.

Comment 5

[Michiel van Leeuwen (email: mvl+moz@)]

Ignore my comment on the session cookie check. The two checks are on different
cookies.

Comment 6

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 132339 [details] [diff] [review]
el patcho

after irc discussion with dwitte:

> nsCookiePermission::CanSetCookie(nsIURI     *aURI,
>+    // check if the cookie we're trying to set is already expired, and return;
>+    // but only if there's no previous cookie, because then we need to delete the previous
>+    // cookie. we need this check to avoid prompting the user for already-expired cookies.
>+    if (!foundCookie) {
>+      PRBool isSession;
>+      aCookie->GetIsSession(&isSession);
>+      if (!isSession) {
>+        nsInt64 currentTime = NOW_IN_SECONDS;
>+        PRInt64 expiry;
>+        aCookie->GetExpiry(&expiry);
>+        if (nsInt64(expiry) <= currentTime)
>+          // the cookie has already expired - reject it
>+          return PR_FALSE;
>+      }
>+    }

the return PR_FALSE here should be optional, because expiry of cookies is
something that should work without nsCookiePermission being implemented. And it
actually is an optimization, no need to do more work when it is expired anyway.

>+++ extensions/cookie/nsCookieService.cpp	2003-09-29 01:01:59.164474680 -0700

>-  // check if the cookie we're trying to set is already expired, and return.
>-  // but we need to check there's no previous cookie first, because servers use
>-  // this as a trick for deleting previous cookies.
>-  if (!foundCookie && !cookie->IsSession() && cookie->Expiry() <= currentTime) {
>-    COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, cookieHeader, "cookie has already expired");
>-    return newCookie;
>-  }

It would be handy to put back this log somehow. It was useful at times.
Remove the optimization above, and you log in AddInternal.

Comment 7

[[e-mail]]

Attachment: dos patcho

nice comment about the optimization... thx!
(i removed it, so we still have sane logging... this change was needed for my
later notifications patch anyway ;)

Comment 8

[Darin Fisher]

Comment on attachment 132756 [details] [diff] [review]
dos patcho

sr=darin

Comment 9

[[e-mail]]

landed!