[webkit-reviews] review granted: [Bug 55427] WebKit2: Need a way to send notifications to client when cookies change : [Attachment 84337] [PATCH] Fix Part 1 v2

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has granted Brian Weinstein
<bweinstein at apple.com>'s request for review:
Bug 55427: WebKit2: Need a way to send notifications to client when cookies
change
https://bugs.webkit.org/show_bug.cgi?id=55427

Attachment 84337: [PATCH] Fix Part 1 v2
https://bugs.webkit.org/attachment.cgi?id=84337&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
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.