210561 - [minimo][m1] move cookies into necko

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[[e-mail]]

Two objectives here: (a) remove nsCookieHTTPNotify, and just let the
nsHttpChannels set cookies directly; (b) move the cookie backend into
netwerk/cookie.

Patch forthcoming for part (a).

Comment 1

[[e-mail]]

Attachment: patch v1

ok. explanations ;)

- added some GetCookie() & SetCookie() helper fns where appropriate, to both
nsHttpChannel and nsMultiMixedConv

- added a member + helper to fetch the cookieservice to nsHttpHandler. this
allows nsHttpChannels to fetch a cookieservice without calling getService()
every time.

- in nsMultiMixedConv, we had a super-ugly-gross-evil-hack to force setting of
a cookie, by making nsHttpChannel re-throw notifications. I removed this in
favor of setting the cookie directly, but this may not be best - maybe we want
an interface wrapper method on nsIHttpInternal to say "take a cookie header
string I provide, but use your own URI's and prompters and stuff"? at any rate,
what we had just had to go.

- in nsHttpChannel::DoAuthRetry, we had a semi-hackish call to notify observers
in order to force cookies to be re-fetched. Since this seemed cookie-specific,
I removed the existing call to the notifier, and replaced it directly with a
GetCookie call. Not sure if this is correct.

- in the SetCookie() wrapper I provide in nsMultiMixedConv, it's basically the
same as the previous impl in nsCookieHTTPNotify except for the nsIPrompt fu. In
nsCookieHTTPNotify, we take two routes: (a) try to get the "parent" channel
from the loadgroup, and query that for a prompter; (b) if failed, get a
prompter from the httpchannel we were given. I wasn't sure if step (a) was
really necessary, so I just removed it in my new impl, but I don't know enough
about how channels inherit parent values to be sure this is right.

So, I consider this proof-of-concept until the above issues are hashed out. ;)

Comment 2

[[e-mail]]

Note that I'm also not too worried about the prompter issue, since that all
needs reworking anyway. at the moment, cookies totally ignores its nsIPrompt
inparam. :)

Comment 3

[[e-mail]]

Comment on attachment 126407 [details] [diff] [review]
patch v1

requesting review to get things started.

Comment 4

[Darin Fisher]

Comment on attachment 126407 [details] [diff] [review]
patch v1

>Index: netwerk/protocol/http/src/nsHttpChannel.cpp

>+    if (mResponseHead->PeekHeader(nsHttp::Set_Cookie)) {
>+      SetCookie();
>+    }

would be good to lookup the value of the "Set-Cookie" header only once.


>+nsHttpChannel::GetCookie()
>+{
>+    nsCOMPtr<nsICookieService> cookieService;
>+    nsresult rv = gHttpHandler->GetCookieService(getter_AddRefs(cookieService));

since each channel owns a hard reference to gHttpHandler, it
seems like this code shouldn't need to have an owning ref to
the cookie service.  it should be able to just get a pointer
from the nsHttpHandler.

the remainder of the function could be within a |if (cookieServ) {|
block or something like that.

>+    if (!cookie.IsEmpty()) {
>+        // overwrite any existing cookie headers
>+        mRequestHead.SetHeader(nsHttp::Cookie, cookie, PR_FALSE);
>+    }

hmm... maybe we should clear the Cookie header otherwise.  the
reason is that GetCookie might get called a subsequent time in
the case of authentication.  so, for consistency with the way
things happen today, it is probably a good idea to clear any
existing Cookie header even though it is unlikely that such a
header exists.	IOW, you should just call SetHeader uncondition-
ally.

>+nsHttpChannel::SetCookie()
>+{
>+    nsCOMPtr<nsICookieService> cookieService;
>+    nsresult rv = gHttpHandler->GetCookieService(getter_AddRefs(cookieService));
>+    if (NS_FAILED(rv)) return;
>+
>+    nsCOMPtr<nsIPrompt> prompt;
>+    if (mCallbacks) {
>+      mCallbacks->GetInterface(NS_GET_IID(nsIPrompt), getter_AddRefs(prompt));

i think there is a helper function for getting an interface from
the notification callbacks... the underlying point is that if
an interface implementation is not available from mCallbacks,
we should try asking mLoadGroup's notification callbacks.


>-    // notify nsIHttpNotify implementations.. the server response could
>-    // have included cookies that must be sent with this authentication
>-    // attempt (bug 84794).
>-    rv = gHttpHandler->OnModifyRequest(this);
>-    NS_ASSERTION(NS_SUCCEEDED(rv), "OnModifyRequest failed");
>+    // fetch cookies, and add them to the request header.
>+    // the server response could have included cookies that must be sent with
>+    // this authentication attempt (bug 84794).
>+    GetCookie();

hmm... i worry a bit since it is changing the semantics of when
OnModifyRequest gets called.  in fact, since we call OnExamineResponse
for each and every response received, i think it seems correct that
we call OnModifyRequest for each and every request we send out as well.

in fact, i imagine not doing so will effect/break the livehttpheaders
extension.


>Index: netwerk/streamconv/converters/nsMultiMixedConv.cpp

>+static inline void
>+SetCookie(const char *aHeader, nsIHttpChannel *aHttpChannel)
>+{
...
>+}

inline?!?  probably not, right?  and, yuck... it would be nice to
reuse some of the code in protocols/http here.	maybe we could
expose something on nsIHttpChannelInternal.  i'm not sure...

in fact, why not just stick with SetResponseHeader?  i know it is
a bit of a hack, but it certainly works.  less pain perhaps? ;-)


thank you very very much btw for working on this bug!!!!

Comment 5

[Darin Fisher]

oh.. didn't see your comments before making my own.  some of mine are just
redundant i guess.

Comment 6

[[e-mail]]

>would be good to lookup the value of the "Set-Cookie" header only once.

thanks, good catch; I meant to do that but forgot.

>since each channel owns a hard reference to gHttpHandler, it
>seems like this code shouldn't need to have an owning ref to
>the cookie service.  it should be able to just get a pointer
>from the nsHttpHandler.

yep, that sounds better

>IOW, you should just call SetHeader uncondition-
>ally.

very good point; thanks.

>i think there is a helper function for getting an interface from
>the notification callbacks... the underlying point is that if
>an interface implementation is not available from mCallbacks,
>we should try asking mLoadGroup's notification callbacks.

Okay - I just wanted to make sure it really was necessary first. I'll put that
part back in.

>hmm... i worry a bit since it is changing the semantics of when
>OnModifyRequest gets called.  in fact, since we call OnExamineResponse
>for each and every response received, i think it seems correct that
>we call OnModifyRequest for each and every request we send out as well.

fair enough ;)

>inline?!?  probably not, right?  and, yuck... it would be nice to
>reuse some of the code in protocols/http here.	maybe we could
>expose something on nsIHttpChannelInternal.  i'm not sure...

yeah; inline because it's only called once, so there's not much point having it
stick around. it matters not, though. :)

I'll toy with the nsIHttpChannelInternal idea. or maybe SetResponseHeader if you
want it that way... you're the owner ;)

thanks for the comments!

Comment 7

[[e-mail]]

Attachment: patch v1.5

This addresses all of darin's comments. I opted to go for the
nsIHttpChannelInternal::SetCookie(const char *) interface, since it fits neatly
with the existing nsHttpChannel::SetCookie() method I had.

In particular - does the interface requestor fu look okay now?

(I'll evaluate other cookie consumers sometime to see if they can be similarly
factored, but I doubt it.)

Comment 8

[[e-mail]]

Attachment: patch v1.5a (supplement)

forgot the nsIHttpChannelInternal changes in the last patch.

Comment 9

[[e-mail]]

Attachment: patch v1.6 (checked in)

ack... slight mistake in the previous patch... corrected.

Comment 10

[Darin Fisher]

NS_ENSURE_TRUE emits a warning... but it seems like it should be quite common
for SetCookie to be called with a null string.

Comment 11

[[e-mail]]

I've been toying around with shifting cookies into core necko. At the moment,
extensions/cookie is a pretty mixed bag:

a) the core backend (nsCookieService, nsCookie)
b) the permission backend (nsPermissionManager)
c) the cookie prompt, cookie manager, and cookie pref UI (the chrome living in
extensions/cookie/resources, and extensions/wallet/cookieviewer. the popupmgr
and imgmgr pref UI appears to be mixed in with the former). by "cookie manager"
I mean only the cookie viewing UI, not the 'cookie sites' stuff which relates to
the permissions backend. (I'm making the conceptual distinction here, but in
practice they will be difficult to separate).
d) the cookie permissions manager (the rest of the chrome living in
extensions/wallet/cookieviewer). this is the 'cookie sites' tab (and related
tabs for images & popups).

There are two obviously separate components here - cookie & permission backends.
The rest also needs separating out, but how to do that is less clear. After
chatting with mvl about it, we've come up with the following rough proposal for
the items mentioned above:

a) netwerk/cookie. since this is the core backend, it should add the least
amount of dependencies possible to netwerk.
b) toolkit/components/permission. the perm backend definitely should be optional.
c & d) browser/components/privacy. this is all UI stuff, and until it gets
rewritten or thought out better, we probably just want to lump it all into one
UI "dumping place".

the idl's in extensions/cookie would move to these places:

a) nsICookie{2}, nsICookieManager{2}, nsICookieService, nsICookiePermission
(wrapper iface for the permissionmanager), nsICookieConsent (p3p)
b) nsIPermissionManager, nsIImgManager
c & d) nsICookiePromptService, nsICookieAcceptDialog.

So, I think the above pretty much nails down our ideas for how we should carve
up cookies. Any comments?

Comment 12

[[e-mail]]

slight mistake above - nsIImgManager should live with the "privacy" components,
not with the core permissions backend. so it would go in with c & d), not with b).

so browser/components/privacy contains _all_ the cookie, image, and popup
manager UI, and the "middle-end" cpp's/idl's that implement them.

Comment 13

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

so that change is firebird-only?

Comment 14

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

The suggestion for the places to move the files to is firebird specific.
But the files can continue to live in extenstion/cookie for now, or move to
somewhere in xpfe. (Does firebird use other stuff from xpfe? In that case, only
one copy is needed)
The backend move to necko is unrelated to the location of the ui files ofcourse.

Comment 15

[Darin Fisher]

Comment on attachment 126417 [details] [diff] [review]
patch v1.6 (checked in)

>Index: netwerk/protocol/http/src/nsHttpChannel.cpp

>+nsresult
>+nsHttpChannel::SetCookie(const char *aCookieHeader)
>+{
>+    // empty header isn't an error
>+    NS_ENSURE_TRUE(aCookieHeader && *aCookieHeader, NS_OK);
>+
>+    nsICookieService *cookieService = gHttpHandler->GetCookieService();
>+    NS_ENSURE_TRUE(cookieService, NS_ERROR_INVALID_POINTER);
>+
>+    nsCOMPtr<nsIPrompt> prompt;
>+    // if we can get an nsIPrompt from our own nsIInterfaceRequestor,
>+    // just do that.
>+    if (mCallbacks)
>+        mCallbacks->GetInterface(NS_GET_IID(nsIPrompt), getter_AddRefs(prompt));
>+
>+    // otherwise, get the loadgroup and ask its channel...
>+    if (!prompt && mLoadGroup) {
>+        nsCOMPtr<nsIRequest> req;
>+        mLoadGroup->GetDefaultLoadRequest(getter_AddRefs(req));
>+        nsCOMPtr<nsIChannel> channel = do_QueryInterface(req);
>+
>+        if (channel) {
>+            nsCOMPtr<nsIInterfaceRequestor> interfaces;
>+            channel->GetNotificationCallbacks(getter_AddRefs(interfaces));
>+            if (interfaces)
>+                interfaces->GetInterface(NS_GET_IID(nsIPrompt), getter_AddRefs(prompt));
>+        }
>+    }

i don't think we need to ask the DefaultLoadRequest for its prompt.  it
should be enough to just query the nsILoadGroup, which means that all this
prompt getting code could just be changed to:

    GetCallback(NS_GET_IID(nsIPrompt), getter_AddRefs(prompt)); 

sr=darin with that change

Comment 16

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

Comment on attachment 126417 [details] [diff] [review]
patch v1.6 (checked in)

r=mvl

I don't see nsCookieService.* moving out of /extensions/cookie here. I guess
that will be the next step :)

Comment 17

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

Comment on attachment 126417 [details] [diff] [review]
patch v1.6 (checked in)

>+nsresult
>+nsHttpChannel::SetCookie(const char *aCookieHeader)
>+{
>+    // empty header isn't an error
>+    NS_ENSURE_TRUE(aCookieHeader && *aCookieHeader, NS_OK);

But this warns for every load that doesn't set a cookie. Please fix that.

Comment 18

[Darin Fisher]

since dwitte is out on vacation, i'm planning on checking this patch in today. 
then i will cut a patch to move the appropriate files into necko.  i'm only
concerned with moving the core cookie code into necko for now.  as for what to
do with permissions, i think it best to defer that for now.  dwitte seems to
have a good plan for that, but maybe we need to think about what will also work
for seamonkey since seamonkey seems to not be going anywhere just yet.

Comment 19

[Darin Fisher]

ok, the v1.6 patch with modifications is in the tree.

Comment 20

[Darin Fisher]

it turns out that we aren't yet able to just move nsCookieService and related
interfaces into necko.  a couple problems:

1- nsCookieService talks directly to nsIDocShell
2- nsCookieService talks directly to nsICookiePermission, which has nsIDOMWindow
as a method argument.

these are minor issues, but i would prefer to not have necko depend on these
other components.  the solution to #1 is probably to define a new interface and
have nsDocShell implement it.  the solution to #2 might be to just eliminate the
nsIDOMWindow parameter since anyways it doesn't seem to be used.  maybe the
nsIDOMWindow parameter will be used sometime in the future??

Comment 21

[Darin Fisher]

oh, and there's actually a direct dependency on nsIPermissionManager too :(

Comment 22

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

The idea with nsIPermissionManager was to add wrapper methods for it in
nsICookiePermission. That cookieperm must move somewhere into necko, and perm
manager can stay in extension/cookie or move to /tookit one day. The
implementations of these interfaces don't need to be in necko

nsIDOMWindow or something is needed for the parent window the "ask me" dialog
needs. But that is broken at the moment, that is why it looks unused. If we can
somehow get a parent window from the channel, we coudl drop that dependency.

for nsIDocShell, it seems to be for the "block cookies in mailnews" stuff. Maybe
we could move that check into nsCookiePermission. That can depend on docsheel,
as it don't need to live in necko. It is somewhat app-specific anyway. Why does
firebird need this check? :)

Comment 23

[Darin Fisher]

perhaps only nsICookiePermission needs to be in necko.  the implementation
should probably live with permission manager or possibly someplace else.  i like
the idea of passing a nsIChannel instead of a nsIDOMWindow.  there is hope that
we will be able to get at the nsIDOMWindow via the nsIChannel's notification
callbacks.  so, i will look at folding all of the permission related stuff back
behind nsICookiePermission.

Comment 24

[Darin Fisher]

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

Comment 25

[Darin Fisher]

Attachment: patch v2 : break dependency on nsIPermissionManager and nsIDocShell

ok, this patch is really just a first-cut at this point.  i'm still not
entirely happy with the changes to nsICookiePermisison, but it works.  this
patch also includes a fix to properly parent the "accept cookie" dialog (in
most cases).

i also just realized that nsCookieService depends on nsIWebProgressListener and
friends.  that's another barrier in the way preventing us from moving the
cookie service into necko.  we need to come up with an abstraction for that
(how about a general "do-idle-stuff-now" observer service notification driven
by the doc loader or something like that?).  alternatively, we could just drop
the code that shortens the delay before writing cookies when page load
finishes.  i'm not sure yet.

Comment 26

[[e-mail]]

Comment on attachment 129893 [details] [diff] [review]
patch v2 : break dependency on nsIPermissionManager and nsIDocShell

great stuff! comments to follow :)

>alternatively, we could just drop
>the code that shortens the delay before writing cookies when page load
>finishes.  i'm not sure yet.

that sounds fine... who cares about 1 second vs 10 seconds? we could just force
cookies to be flushed to disk 5 or 10 seconds after a set regardless, no?

>Index: nsCookiePermission.cpp
>===================================================================
>+// vim: ts=2 sw=2 expandtab

hmm ;)

>+nsresult
>+nsCookiePermission::Init()
>+{
...
>+  nsCOMPtr<nsIPrefBranch> prefBranch =
>+      do_GetService(NS_PREFSERVICE_CONTRACTID);
>+  if (prefBranch) {

wanna use the |, &rv)| form here?

>+      prefInt->AddObserver(kCookiesAskPermission, this, PR_FALSE);
>+      prefInt->AddObserver(kCookiesDisabledForMailNews, this, PR_FALSE);

you want PR_TRUE here for weak refs, no?

>+void
>+nsCookiePermission::PrefChanged(nsIPrefBranch *prefBranch,
>+                                const char    *pref)
...
>+#define PREF_CHANGED(_P) (!pref || strcmp(pref, _P) == 0)

nit: i prefer |!strcmp|, but whatever :)

>+  if (PREF_CHANGED(kCookiesAskPermission) &&
>+      NS_SUCCEEDED(prefBranch->GetBoolPref(kCookiesAskPermission, &val)))
>+    mCookiesAskPermission = val;
>+
>+  if (PREF_CHANGED(kCookiesDisabledForMailNews) &&
>+      NS_SUCCEEDED(prefBranch->GetBoolPref(kCookiesDisabledForMailNews, &val)))
>+    mCookiesDisabledForMailNews = val;

ho hum, this isn't healthy for phoenix. you init mCookiesDisabledForMailNews to
PR_TRUE (unconditionally) in the class ctor, so phoenix will be burning all
those cycles for the mailnews checks for each cookie. (i timed it and it's not
fast). i suppose we can forget about codesize for now, until we fork this file
for phoenix; but could we put in an #ifdef MOZ_PHOENIX in nsCookiePermission.h
to init this pref to false?

>+NS_IMETHODIMP
>+nsCookiePermission::SetAccess(nsIURI         *aURI,
>+                              nsCookieAccess  aAccess)
>+{
>+  //
>+  // NOTE: nsCookieAccess values conveniently match up with
>+  //       the permission codes used by nsIPermissionManager
>+  //

but this isn't necessary (except for backward compatibility, for existing
cookperm.txt's). can you mention that here, and also put a big highlighted
comment in the .idl to this effect?

>+  return mPermMgr->Add(aURI, "cookie", aAccess);

just a random thought; maybe we should declare "cookie" as a static const
char[] at the top, 'coz we use it a lot.

>+nsCookiePermission::CanSetCookie(nsIURI     *aURI,
>+                                 nsIChannel *aChannel,
>+                                 nsICookie  *aCookie,
>+                                 PRInt32     aNumCookiesFromHost,
>+                                 PRBool      aChangingCookie,
>+                                 PRBool     *aResult) 
...
>   nsresult rv;
>   if (!mPermMgr) {
>-    mPermMgr = do_GetService(NS_PERMISSIONMANAGER_CONTRACTID, &rv);
>-    if (NS_FAILED(rv)) return rv;
>   }

superfluous |if|

>Index: nsCookiePermission.h
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/cookie/nsCookiePermission.h,v
>retrieving revision 1.3
>diff -p -u -1 -0 -r1.3 nsCookiePermission.h
>--- nsCookiePermission.h	15 Aug 2003 21:04:52 -0000	1.3
>+++ nsCookiePermission.h	16 Aug 2003 00:52:14 -0000
>@@ -32,30 +32,44 @@
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #ifndef nsCookiePermission_h__
> #define nsCookiePermission_h__
> 
> #include "nsICookiePermission.h"
> #include "nsIPermissionManager.h"
>+#include "nsIObserver.h"
> #include "nsCOMPtr.h"
> 
>+class nsIPrefBranch;
>+
> class nsCookiePermission : public nsICookiePermission
>+                         , public nsIObserver
> {
> public:
>-  nsCookiePermission() {}
>-  virtual ~nsCookiePermission() {}
>-
>   NS_DECL_ISUPPORTS
>   NS_DECL_NSICOOKIEPERMISSION
>+  NS_DECL_NSIOBSERVER
>+
>+  nsCookiePermission() 
>+    : mCookiesAskPermission(PR_FALSE)
>+    , mCookiesDisabledForMailNews(PR_TRUE)

#ifdef MOZ_PHOENIX
    , mCookiesDisabledForMailNews(PR_FALSE)
#else
    , mCookiesDisabledForMailNews(PR_TRUE)
#endif

>Index: nsICookiePermission.idl
>===================================================================
>+   * nsCookieAccess values

see above about a meaty comment: something like "* these MUST match the ACTION
constants in nsIPermissionManager.idl to maintain backward compatibility"?

looking forward to seeing this in the tree :)

Comment 27

[[e-mail]]

btw, don't forget to remove the relevant function prototypes/members from
nsCookieService.h ;)

Comment 28

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

Comment on attachment 129893 [details] [diff] [review]
patch v2 : break dependency on nsIPermissionManager and nsIDocShell

>@@ -1528,26 +1490,25 @@ nsCookieService::SetCookieInternal(nsIUR

>+    mPermissionService->CanSetCookie(aHostURI,
>+                                     nsnull,
>+                                     NS_STATIC_CAST(nsICookie*, NS_STATIC_CAST(nsCookie*, cookie)),
>+                                     countFromHost, foundCookie,
>+                                     &permission);

Why are you passing nsnull for the channel? The only caller of
SetCookieInternal() has a channel, why not pass that?

Comment 29

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

>>Index: nsICookiePermission.idl
>>===================================================================
>>+   * nsCookieAccess values
>
>see above about a meaty comment: something like "* these MUST match the ACTION
>constants in nsIPermissionManager.idl to maintain backward compatibility"?

There is no need for them to be the same. The implementation here knows they
are, so uses that. But if these values would change, you need to change the
implementation to translate to nsIPermissionManager values.

Comment 30

[Darin Fisher]

thanks for all the great feedback guys!

Comment 31

[Darin Fisher]

Attachment: v2.1 patch

ok, this patch includes revisions based on comments from dwitte and mvl. 
couple points:

1- no need to check |rv| when trying to get access to preferences because
failure to access preferences should not prevent nsCookiePermission from being
instantiated.  hence, we only care if we actually got a reference to the pref
branch.

2- i inserted "#ifndef MOZ_PHOENIX" in a few places to compile out the code
that does the mailnews checks if we are compiling for Firebird.  eventually, we
should look into forking this file and/or allowing for multiple
nsICookiePermission instances (maybe by utilizing the category manager).  i did
not bother to compile out the mDisableCookiesForMailNews declaration since is a
packed bool and shouldn't contribute any to footprint.

3- i agree with MVL that the details of how nsCookiePermission is implemented,
namely that the ACCESS_XX constants map the the nsIPM::XXX_ACTION, should
remain an implementation detail.  so, i decided to only comment about it in the
.cpp file.  also, i think it is more than just an issue of backwards
compatibility since the cookie viewer UI talks to the permission manager
directly instead of going through nsICookiePermission.

4- went with dwitte's suggestion of killing the webprogresslistener fu, but i
needed to add a timer to delay the "cookieChanged" notification.  i thought
about moving this entirely into the cookieViewer UI, but since the permission
manager generates a similar notification event i decided to keep the code in
the cookie service.  plus, there isn't much code involved.  i would have used
the same timer as that used for writing except that i think the notification
timer should be shorter and should behave slightly differently (see comments in
the code).

that's that... i think with this patch we should be able to finally move the
cookie service into necko ;-)

Comment 32

[Darin Fisher]

Comment on attachment 130011 [details] [diff] [review]
v2.1 patch

(wish i could set a flag requesting review from both dwitte and mvl... mvl,
please jump in if you see anything else that needs to be tweaked... thanks!)

Comment 33

[Darin Fisher]

nevermind the printf in nsCookieService.cpp... i've removed it in my tree.

Comment 34

[Darin Fisher]

the v2.1 patch has been checked in on the minimo branch, and the cookie service
was successfully moved over to necko :)

... now to get final reviews and do the same thing on the trunk!

Comment 35

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

Comment on attachment 130011 [details] [diff] [review]
v2.1 patch

>Index: nsCookiePermission.cpp

>+
>+NS_IMETHODIMP
>+nsCookiePermission::CanAccess(nsIURI         *aURI,

>+      if (parent) {
>+        do {
>+            item = parent;
>+            nsCOMPtr<nsIDocShell> docshell = do_QueryInterface(item);
>+            if (docshell)
>+              docshell->GetAppType(&appType);
>+        } while (appType != nsIDocShell::APP_TYPE_MAIL &&
indented a bit much.


>Index: nsCookieService.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/cookie/nsCookieService.cpp,v
>retrieving revision 1.89

> NS_IMETHODIMP
> nsCookieService::Observe(nsISupports     *aSubject,
>-    if (mWriteTimer)
>+    if (mWriteTimer) {
>       mWriteTimer->Cancel();
>+      mWriteTimer = 0;

0? not nsnull?

>+    }
>+    if (mNotifyTimer) {
>+      mNotifyTimer->Cancel();
>+      mNotifyTimer = 0;
>+    }

same here. And in DoLazyWrite.


>Index: nsICookiePermission.idl

>+  /**
>+   * canSetCookie
>+   *
>+   * this method is called to test whether or not the given URI/channel may
>+   * set a specific cookie.  this method is always preceded by a call to
>+   * canAccessCookies.
>+   *
>+   * @param aURI
>+   *        the URI trying to set the cookie
>+   * @param aChannel
>+   *        the corresponding to aURI
Missing something here :)

Other then those nits, it looks fine to me. Leaving the flag for dwitte, since
he did most review work on the first patch :)

Comment 36

[Darin Fisher]

thanks mvl!  i fixed those things in my tree.

Comment 37

[[e-mail]]

Comment on attachment 130011 [details] [diff] [review]
v2.1 patch

>Index: nsCookiePermission.cpp
>===================================================================
>+// vim: ts=2 sw=2 expandtab

just a reminder in case you forgot to nuke this :)

>+nsresult
>+nsCookiePermission::Init()
...
>+    if (prefInt) {
>+      prefInt->AddObserver(kCookiesAskPermission, this, PR_FALSE);
>+      prefInt->AddObserver(kCookiesDisabledForMailNews, this, PR_FALSE);
>+    }

these are still strong refs, right? where do you remove these observers? weak
refs instead?

>+NS_IMETHODIMP
>+nsCookiePermission::SetAccess(nsIURI         *aURI,
>+                              nsCookieAccess  aAccess)
>+{
>+  //
>+  // NOTE: nsCookieAccess values conveniently match up with
>+  //       the permission codes used by nsIPermissionManager.
>+  //       this is nice because it avoids conversion code.

perfect :)

>+NS_IMETHODIMP 
>+nsCookiePermission::CanSetCookie(nsIURI     *aURI,
...
>     if (rememberDecision) {
>-      mPermMgr->Add(aURI, "cookie",
>-                   *aPermission ? (PRUint32) nsIPermissionManager::ALLOW_ACTION
>-                                : (PRUint32) nsIPermissionManager::DENY_ACTION);
>+      mPermMgr->Add(aURI, kPermissionType,
>+                   *aResult ? (PRUint32) nsIPermissionManager::ALLOW_ACTION
>+                            : (PRUint32) nsIPermissionManager::DENY_ACTION);
>     }

hmm, just an idle thought... should we be propagating the rv from ::Add?

>Index: nsCookieService.cpp
>===================================================================
> void
> nsCookieService::InitPrefObservers()
...
> #ifdef MOZ_PHOENIX
>-    mCookiesDisabledForMailNews = PR_FALSE; // for efficiency
> #else
>-    mCookiesDisabledForMailNews = PR_TRUE;
>     mCookiesP3PString = NS_LITERAL_CSTRING(kCookiesP3PString_Default);
> #endif

make that #ifndef?

with that, r=dwitte. oh, also... mvl reminded me of something on irc before...
when you do the move, can you kill that pesky nsCCookieManager.h/nsCCookie.h,
and merge them into nsNetCID.h or something? :)

we should give mscott a heads-up too, so he can add --disable-cookies into the
default tbird .mozconfig.

Comment 38

[[e-mail]]

-> darin, since he's doing all the hard work :)

Comment 39

[Darin Fisher]

sorry, i forgot to address these previous review comments:


>just a reminder in case you forgot to nuke this :)

no one ever complains about emacs modelines :(   ... but, i can easily remove them.


>these are still strong refs, right? where do you remove these observers? weak
>refs instead?

strong refs, yes.  but, it is ok because they will get dropped at shutdown time.
 during xpcom-shutdown or earlier.


as for the Add returning rv in that case, i think it's probably not needed.  if
that Add fails, it just means that we will "forget" the users decision.  that
doesn't mean that the function can't produce meaningful results.  the
remembering is just a side-effect that the caller of CanSetCookie shouldn't be
concerned with.

fixed the #ifdef in my local tree.

yes, i'll get rid of nsCCookieManager.h when i do the merge, and i'll be sure to
inform scott :)

thanks dwitte!

Comment 40

[[e-mail]]

>no one ever complains about emacs modelines :(   ... but, i can easily remove
>them.

oh i wasn't complaining... leave them in :) i was just pointing them out in case
you did mean to nuke them.

>strong refs, yes.  but, it is ok because they will get dropped at shutdown
>time. during xpcom-shutdown or earlier.

hmm, so that means calls to ->RemoveObserver() at shutdown are redundant? i
didn't know that... (does that mean we change the weak refs back to strong ones
in nsCookieService too?)

>the remembering is just a side-effect that the caller of CanSetCookie shouldn't
>be concerned with.

cool, sounds reasonable.

Comment 41

[Darin Fisher]

Attachment: v2.2 patch

cleaned up patch with all of dwitte's review comments addressed.  the plan is
to get this on the trunk, and then do the move separately.

Comment 42

[Darin Fisher]

so, when this latest patch goes in, we'll be ready to move files into necko.

ls -R netwerk/cookie/

  netwerk/cookie/public:
  CVS                 nsICookieConsent.idl   nsICookiePermission.idl
  Makefile.in         nsICookie.idl          nsICookieService.idl
  nsCCookieManager.h  nsICookieManager2.idl
  nsICookie2.idl      nsICookieManager.idl
 
  netwerk/cookie/src:
  CVS          nsCookie.cpp  nsCookieService.cpp
  Makefile.in  nsCookie.h    nsCookieService.h


disregard nsCCookieManager.h in the above... it will be removed in favor
of nsNetCID.h

Comment 43

[Darin Fisher]

ok, the latest patch is now on the trunk.

Comment 44

[Darin Fisher]

files have been moved, bug is fixed.

remaining side issues:

1) remove nsCCookieManager.h in favor of nsNetCID.h
2) possibly add a --disable-cookies configure option

Comment 45

[Darin Fisher]

see bug 221886 and bug 221885 for the two side issues i mentioned above.