Bug 55427 – WebKit2: Need a way to send notifications to client when cookies change

• Component: WebKit2
• Status: RESOLVED FIXED

[Brian Weinstein]

WebKit2 needs a way to send a notification to the client when cookies change.

<rdar://problem/9056027>

[Brian Weinstein]

Created attachment 84166 [details]
[PATCH] Fix Part 1

[Jessie Berlin]

Comment on attachment 84166 [details]
[PATCH] Fix Part 1

View in context: https://bugs.webkit.org/attachment.cgi?id=84166&action=review

Overall looks good to me. I don't have review privileges

> Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:2123
> +				3354F5B5131C55B800ED70FF /* mac */,

Why is this here?

> Source/WebKit2/WebProcess/Cookies/WebCookieManager.cpp:82
> +#if !PLATFORM(MAC) && !(PLATFORM(WIN) && USE(CFNETWORK))

Who is not Mac or Win but uses CFNetwork?

> Source/WebKit2/WebProcess/Cookies/win/WebCookieManagerWin.cpp:38
> +const int maxCookiesWaitMsec = 5000;

Where did this value come from? It might be nice to have a comment here explaining it.

[Brian Weinstein]

(In reply to comment #2)
> (From update of attachment 84166 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84166&action=review
> 
> Overall looks good to me. I don't have review privileges
> 
> > Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:2123
> > +				3354F5B5131C55B800ED70FF /* mac */,
> 
> Why is this here?

This is just something for the project structure.

Cookies
    > mac
        > WebCookieManagerMac.mm
    > WebCookieManager.cpp
    > WebCookieManager.h
    > WebCookieManager.messages.in

> 
> > Source/WebKit2/WebProcess/Cookies/WebCookieManager.cpp:82
> > +#if !PLATFORM(MAC) && !(PLATFORM(WIN) && USE(CFNETWORK))
> 
> Who is not Mac or Win but uses CFNetwork?

There's WinCE, who's Win but doesn't use CFNetwork.

> 
> > Source/WebKit2/WebProcess/Cookies/win/WebCookieManagerWin.cpp:38
> > +const int maxCookiesWaitMsec = 5000;
> 
> Where did this value come from? It might be nice to have a comment here explaining it.

This was ported from some other code I was reading. I will add a comment.

[Brian Weinstein]

Alexey thought this code should be in WebCore, which makes sense, but a worry I have is that it does make the callback to WebCookieManager::dispatchDidModifyCookies awkward, because WebCore shouldn't know about that WebKit function.

[Darin Adler]

(In reply to comment #4)
> Alexey thought this code should be in WebCore, which makes sense, but a worry I have is that it does make the callback to WebCookieManager::dispatchDidModifyCookies awkward, because WebCore shouldn't know about that WebKit function.

Right, you wouldn’t call back directly. You have to use a client or a strategy to call back.

[Brian Weinstein]

Created attachment 84337 [details]
[PATCH] Fix Part 1 v2

[Adam Roben (:aroben)]

Comment on attachment 84337 [details]
[PATCH] Fix Part 1 v2

View in context: https://bugs.webkit.org/attachment.cgi?id=84337&action=review

Looks nice!

> Source/WebCore/platform/CookiesStrategy.h:40
> +    virtual ~CookiesStrategy()
> +    {
> +    }

You can put this all on one line: virtual ~CookiesStrategy() { }

> Source/WebCore/platform/network/CookieStorage.h:33
> +void beginObservingCookieChanges();
> +void finishObservingCookieChanges();

I think start/stop is a little clearer than begin/finish.

> Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:90
> +    callOnMainThread(&notifyCookiesChangedOnMainThread, 0);

No need for the & in C++ code.

> Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:97
> +    CFRunLoopRef runLoop = loaderRunLoop();

I think it's worth making it clear that we don't specifically need the loader run loop; we just need any run loop. One way to do this would be to add a new helper function:

static inline CFRunLoopRef cookieStorageObserverRunLoop()
{
    return loaderRunLoop();
}

Then you could put a comment in there explaining why we're using the load run loop, and maybe a FIXME about renaming loaderRunLoop to something more generic.

> Source/WebCore/platform/network/mac/CookieStorageMac.mm:44
> +-(void)registerForCookieChangeNotifications;
> +-(void)unregisterForCookieChangeNotifications;

I think you could use the same start/stop terminology here as in the CookieStorage functions.

> Source/WebCore/platform/network/mac/CookieStorageMac.mm:60
> +-(void)cookiesChangedNotificationHandler:(NSNotification *)notification
> +{
> +    UNUSED_PARAM(notification);
> +
> +    [self performSelectorOnMainThread:@selector(notifyCookiesChangedOnMainThread) withObject:nil waitUntilDone:FALSE];

I guess you determined that this notification is in fact sent on a non-main thread?

> Source/WebCore/platform/network/mac/CookieStorageMac.mm:87
> +        cookieStorageAdapter = [CookieStorageObjCAdapter alloc];

You're missing a call to -init here.

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:117
> +    WebCookieManager::shared().dispatchDidModifyCookies();

Not directly related to this patch, but: I think WebCookiesManager (with an "s") is a little better.

[Brian Weinstein]

(In reply to comment #7)
> (From update of attachment 84337 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84337&action=review
> 
> Looks nice!
> 
> > Source/WebCore/platform/CookiesStrategy.h:40
> > +    virtual ~CookiesStrategy()
> > +    {
> > +    }
> 
> You can put this all on one line: virtual ~CookiesStrategy() { }

Fixed.

> 
> > Source/WebCore/platform/network/CookieStorage.h:33
> > +void beginObservingCookieChanges();
> > +void finishObservingCookieChanges();
> 
> I think start/stop is a little clearer than begin/finish.

Fixed.

> 
> > Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:90
> > +    callOnMainThread(&notifyCookiesChangedOnMainThread, 0);
> 
> No need for the & in C++ code.

Removed.

> 
> > Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:97
> > +    CFRunLoopRef runLoop = loaderRunLoop();
> 
> I think it's worth making it clear that we don't specifically need the loader run loop; we just need any run loop. One way to do this would be to add a new helper function:
> 
> static inline CFRunLoopRef cookieStorageObserverRunLoop()
> {
>     return loaderRunLoop();
> }
> 
> Then you could put a comment in there explaining why we're using the load run loop, and maybe a FIXME about renaming loaderRunLoop to something more generic.


Done.

> 
> > Source/WebCore/platform/network/mac/CookieStorageMac.mm:44
> > +-(void)registerForCookieChangeNotifications;
> > +-(void)unregisterForCookieChangeNotifications;
> 
> I think you could use the same start/stop terminology here as in the CookieStorage functions.

Fixed.

Renamed to startListeningForCookieChangeNotifications and stopListeningForCookieChangeNotifications.

> 
> > Source/WebCore/platform/network/mac/CookieStorageMac.mm:60
> > +-(void)cookiesChangedNotificationHandler:(NSNotification *)notification
> > +{
> > +    UNUSED_PARAM(notification);
> > +
> > +    [self performSelectorOnMainThread:@selector(notifyCookiesChangedOnMainThread) withObject:nil waitUntilDone:FALSE];
> 
> I guess you determined that this notification is in fact sent on a non-main thread?

Yeah, the notification is sent on a non-main thread.

> 
> > Source/WebCore/platform/network/mac/CookieStorageMac.mm:87
> > +        cookieStorageAdapter = [CookieStorageObjCAdapter alloc];
> 
> You're missing a call to -init here.

Fixed.

> 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:117
> > +    WebCookieManager::shared().dispatchDidModifyCookies();
> 
> Not directly related to this patch, but: I think WebCookiesManager (with an "s") is a little better.

This can be renamed, but will probably be done later.

[Brian Weinstein]

Landed part 1 in r80145.

[WebKit Review Bot]

http://trac.webkit.org/changeset/80145 might have broken WinCE Release (Build)

[Brian Weinstein]

Created attachment 84483 [details]
[PATCH] Fix Part 2

[Jessie Berlin]

Comment on attachment 84483 [details]
[PATCH] Fix Part 2

View in context: https://bugs.webkit.org/attachment.cgi?id=84483&action=review

Overall LGTM. However, I don't have reviewer privileges.

> Source/WebKit2/ChangeLog:10
> +        to listen for cookies changing, and adds a mechanism for the WebProcess to notify the UIProcess

I think you meant "listening" for cookies changing, not "to listen".

> Source/WebKit2/UIProcess/WebCookieManagerProxyClient.cpp:2
> + * Copyright (C) 2010 Apple Inc. All rights reserved.

Copyright should be 2011

[Brian Weinstein]

(In reply to comment #12)
> (From update of attachment 84483 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84483&action=review
> 
> Overall LGTM. However, I don't have reviewer privileges.
> 
> > Source/WebKit2/ChangeLog:10
> > +        to listen for cookies changing, and adds a mechanism for the WebProcess to notify the UIProcess
> 
> I think you meant "listening" for cookies changing, not "to listen".

Fixed.

> 
> > Source/WebKit2/UIProcess/WebCookieManagerProxyClient.cpp:2
> > + * Copyright (C) 2010 Apple Inc. All rights reserved.
> 
> Copyright should be 2011

And fixed.

Thanks!

[Adam Roben (:aroben)]

Comment on attachment 84483 [details]
[PATCH] Fix Part 2

View in context: https://bugs.webkit.org/attachment.cgi?id=84483&action=review

> Source/WebKit2/UIProcess/WebCookieManagerProxyClient.h:30
> +#include "WKCookieManager.h"

I don't think this #include is needed.

> Source/WebKit2/UIProcess/API/C/WKCookieManager.h:41
> +    WKCookieManagerDidModifyCookiesCallback                             didModifyCookies;

CookiesDidChange seems more accurate than DidModifyCookies. The latter makes it sound like the manager itself is modifying cookies.

> Source/WebKit2/WebProcess/Cookies/WebCookieManager.cpp:89
> +void WebCookieManager::startObservingCookieChanges()
> +{
> +#if USE(CFNETWORK) || PLATFORM(MAC)
> +    WebCore::startObservingCookieChanges();
> +#endif
> +}
> +
> +void WebCookieManager::stopObservingCookieChanges()
> +{
> +#if USE(CFNETWORK) || PLATFORM(MAC)
> +    WebCore::stopObservingCookieChanges();
> +#endif
> +}

It would be nice if WebKit didn't have to know which ports have implemented this feature. If all ports implemented this function (but some ports just had stub implementations), WebKit wouldn't have to care.

[Brian Weinstein]

(In reply to comment #14)
> (From update of attachment 84483 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84483&action=review
> 
> > Source/WebKit2/UIProcess/WebCookieManagerProxyClient.h:30
> > +#include "WKCookieManager.h"
> 
> I don't think this #include is needed.

I am seeing compile errors without it - WebCookieManagerProxyClient doesn't know what a WKCookieManagerClient is.

> 
> > Source/WebKit2/UIProcess/API/C/WKCookieManager.h:41
> > +    WKCookieManagerDidModifyCookiesCallback                             didModifyCookies;
> 
> CookiesDidChange seems more accurate than DidModifyCookies. The latter makes it sound like the manager itself is modifying cookies.

I agree - fixed.

> 
> > Source/WebKit2/WebProcess/Cookies/WebCookieManager.cpp:89
> > +void WebCookieManager::startObservingCookieChanges()
> > +{
> > +#if USE(CFNETWORK) || PLATFORM(MAC)
> > +    WebCore::startObservingCookieChanges();
> > +#endif
> > +}
> > +
> > +void WebCookieManager::stopObservingCookieChanges()
> > +{
> > +#if USE(CFNETWORK) || PLATFORM(MAC)
> > +    WebCore::stopObservingCookieChanges();
> > +#endif
> > +}
> 
> It would be nice if WebKit didn't have to know which ports have implemented this feature. If all ports implemented this function (but some ports just had stub implementations), WebKit wouldn't have to care.

I'd like to take care of this in a follow-up patch - because the other ports don't have a CookieStorage - and I feel like adding CookieStorage headers and implementations to other ports would make this patch more confusing.

[Brian Weinstein]

(In reply to comment #15)
> (In reply to comment #14)
> > It would be nice if WebKit didn't have to know which ports have implemented this feature. If all ports implemented this function (but some ports just had stub implementations), WebKit wouldn't have to care.
> 
> I'd like to take care of this in a follow-up patch - because the other ports don't have a CookieStorage - and I feel like adding CookieStorage headers and implementations to other ports would make this patch more confusing.

It looks like I can use TemporaryLinkStubs for the other platforms. I will implement the stubs in a follow-up and attach it to this bug.

[Brian Weinstein]

Landed part 2 in r80263.

[Brian Weinstein]

Created attachment 84637 [details]
[PATCH] Cleanup

[Brian Weinstein]

Landed cleanup in r80311.

[WebKit Review Bot]

http://trac.webkit.org/changeset/80331 might have broken Windows 7 Release (Tests)
The following tests are not passing:
http/tests/xmlhttprequest/supported-xml-content-types.html