Bug 207593

<rdar://problem/56027027>

Created attachment 390450 [details]
WIP Patch

Created attachment 390462 [details]
WIP Patch

Created attachment 390533 [details]
WIP Patch

Created attachment 390536 [details]
WIP Patch

Created attachment 390539 [details]
WIP Patch

Created attachment 390549 [details]
WIP Patch

Now passing layout tests locally.

Created attachment 390561 [details]
WIP Patch

Created attachment 390567 [details]
WIP Patch

With more layout tests.

Created attachment 390575 [details]
Patch

Comment on attachment 390575 [details]
Patch

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

> Source/WTF/wtf/PlatformEnable.h:802
> +#if !defined(ENABLE_COOKIE_CACHE) && PLATFORM(COCOA) && HAVE(CFNETWORK_COOKIE_CHANGE_LISTENER_API)
> +#define ENABLE_COOKIE_CACHE 1
> +#endif

Couldn't this just be a runtime no-op without CFNETWORK_COOKIE_CHANGE_LISTENER_API and avoid most #ifs?

> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:617
> +    ASSERT(observers.add(&observer).isNewEntry);

Why is observer only added in debug build?

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:131
> +    if (!m_hostsWithCookieListeners.isEmpty()) {
> +        auto hostsWithCookieListeners = copyToVector(m_hostsWithCookieListeners);
> +        unsubscribeFromCookieChangeNotifications(hostsWithCookieListeners);
> +    }

I think it would be nicer to have a separate function or just loop over m_hostsWithCookieListeners here (avoiding the vector copy). It is just call to stopListeningForCookieChangeNotifications for each.

> Source/WebKit/WebProcess/WebPage/WebCookieCache.h:44
> +    bool isFunctional();

isEnabled/isSupported?

Comment on attachment 390575 [details]
Patch

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

>> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:617
>> +    ASSERT(observers.add(&observer).isNewEntry);
> 
> Why is observer only added in debug build?

ouch, last minute clean up I made :/ Thanks for catching.

> ouch, last minute clean up I made :/ Thanks for catching.

Observers don't seem to have much test coverage.

(In reply to Antti Koivisto from comment #13)
> > ouch, last minute clean up I made :/ Thanks for catching.
> 
> Observers don't seem to have much test coverage.

EWS does not have the CFNetwork SPI needed and thus does not cover my change. Actual layout test bots will run the tests with the cookie cache enabled (at least some of the bots will).

Created attachment 390687 [details]
Patch

Created attachment 390688 [details]
Patch

Created attachment 390691 [details]
Patch

Created attachment 390695 [details]
Patch

Created attachment 390925 [details]
Patch

Created attachment 390926 [details]
Patch

Created attachment 390988 [details]
Patch

Created attachment 391004 [details]
Patch

Comment on attachment 391004 [details]
Patch

Clearing flags on attachment: 391004

Committed r256820: <https://trac.webkit.org/changeset/256820>

All reviewed patches have been landed.  Closing bug.

The new test http/tests/cookies/document-cookie-after-showModalDialog.html

is a flaky failure. tracking in https://bugs.webkit.org/show_bug.cgi?id=208165