[webkit-reviews] review denied: [Bug 185261] Adopt new async _savecookies SPI for keeping networking process active during flushing cookies : [Attachment 339454] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 185261: Adopt new async _savecookies SPI for keeping networking process
active during flushing cookies
https://bugs.webkit.org/show_bug.cgi?id=185261

Attachment 339454: Patch

https://bugs.webkit.org/attachment.cgi?id=339454&action=review




--- Comment #3 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 339454
  --> https://bugs.webkit.org/attachment.cgi?id=339454
Patch

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

To fix the build, I think you need something like this:

#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400) ||
(PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 120000)
// new stuff
#else
// old stuff
#endif

> Source/WebCore/platform/network/NetworkStorageSession.cpp:65
> +    return globalSessionMap().size() + 1;

It would be nice to add a small comment explaining that the + 1 is for the
default session, like so: // + 1 for the default session

> Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:232
> +    m_syncingCookiesResultsToCollect =
WebCore::NetworkStorageSession::sessionCount();

In order to handle multiple calls to syncAllCookies, where call #2 might happen
before call #1 completes its async activity, this needs to be += instead of =.

> Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:235
> +    auto *resultsToCollectPtr = &m_syncingCookiesResultsToCollect;
> +    WebCore::NetworkStorageSession::forEach([&] (const
WebCore::NetworkStorageSession& networkStorageSession) {

We don't need to use a pointer in order to do this capture. What we're trying
to capture is this->m_syncingCookiesResultsToCollect. I would do this by
capture "this" in the C++ capture statement, and then using the implicitly
in-scope value "m_syncingCookiesResultsToCollect" inside the Objective-C block.

> Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:239
> +#if PLATFORM(IOS)

I don't think we need to go out of our way to make iOS and macOS different in
this regard. Might as well make everything cross-platform if we can.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:551
> +void NetworkProcessProxy::setIsSyncingCookies(bool isSyncingCookies)

You can early return here with "if (isSyncingCookies ==
!!m_tokenForSyncingCookies) return;".

> Source/WebKit/UIProcess/WebProcessPool.cpp:1600
> +#if PLATFORM(IOS)
> +    if (m_tokenForSyncingCookies)
> +	   return;
> +    m_networkprocess->setIsSyncingCookies(true);
> +#endif

You mentioned that it's a bug that a new call to this function while an
existing call is in flight will not sync cookies again. I think the best way to
fix this is to make setIsSyncingCookies() return early if you set the same
value, and then call it unconditionally, and remove the early return.