WebKit2 needs a way to send a notification to the client when cookies change. <rdar://problem/9056027>
Created attachment 84166 [details] [PATCH] Fix Part 1
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.
(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.
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.
(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.
Created attachment 84337 [details] [PATCH] Fix Part 1 v2
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(¬ifyCookiesChangedOnMainThread, 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.
(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(¬ifyCookiesChangedOnMainThread, 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.
Landed part 1 in r80145.
http://trac.webkit.org/changeset/80145 might have broken WinCE Release (Build)
Created attachment 84483 [details] [PATCH] Fix Part 2
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
(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!
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.
(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.
(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.
Landed part 2 in r80263.
Created attachment 84637 [details] [PATCH] Cleanup
Landed cleanup in r80311.
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