[cookies] refactor prompting helper to where it belongs

RESOLVED FIXED

Status

()

Core
Networking: Cookies
--
minor
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: dwitte@gmail.com, Assigned: dwitte@gmail.com)

Tracking

({perf})

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

14.88 KB, patch
Michiel van Leeuwen (email: mvl+moz@)
: review+
Darin Fisher
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

15 years ago
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... :/
(Assignee)

Comment 1

15 years ago
Created attachment 132339 [details] [diff] [review]
el patcho
(Assignee)

Comment 2

15 years ago
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 ;)
Attachment #132339 - Flags: superreview?(darin)
(Assignee)

Updated

15 years ago
Keywords: perf
(Assignee)

Comment 3

15 years ago
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.
(Assignee)

Updated

15 years ago
Attachment #132339 - Flags: review?(mvl)
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.
Ignore my comment on the session cookie check. The two checks are on different
cookies.
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.
(Assignee)

Updated

15 years ago
Attachment #132339 - Flags: superreview?(darin)
Attachment #132339 - Flags: review?(mvl)
(Assignee)

Comment 7

15 years ago
Created attachment 132756 [details] [diff] [review]
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 ;)
Attachment #132339 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #132756 - Flags: superreview?(darin)

Comment 8

15 years ago
Comment on attachment 132756 [details] [diff] [review]
dos patcho

sr=darin
Attachment #132756 - Flags: superreview?(darin) → superreview+
(Assignee)

Comment 9

15 years ago
landed!
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.