Bug 206450

Created attachment 388099 [details] Patch

Created attachment 388104 [details] Patch

Comment on attachment 388104 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388104&action=review > Source/WebKit/WebProcess/Network/NetworkProcessConnection.cpp:234 > +bool NetworkProcessConnection::cookiesEnabled() > +{ > + if (m_cachedCookiedEnabled) > + return *m_cachedCookiedEnabled; > + > + bool result = false; > + connection().sendSync(Messages::NetworkConnectionToWebProcess::CookiesEnabled(), Messages::NetworkConnectionToWebProcess::CookiesEnabled::Reply(result), 0); > + m_cachedCookiedEnabled = result; > + return result; > +} What is the value of keeping this sync vs. having the network process proactively update the WebProcesses whenever the policy changes? e.g. cookieAcceptPolicyMayHaveChanged becomes cookieAcceptPolicyChanged(newPolicy).

That would mean waking up all WebContent processes just for bookkeeping. Which may be frozen.

(In reply to Alexey Proskuryakov from comment #4) > That would mean waking up all WebContent processes just for bookkeeping. > Which may be frozen. It seems like for cache invalidation via cookieAcceptPolicyMayHaveChanged you also have to wake up all WebContent processes just for book keeping, no? It that's something to optimize for, seems like having a stardarized delayed updating mechanism might be useful.

Comment on attachment 388104 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388104&action=review >> Source/WebKit/WebProcess/Network/NetworkProcessConnection.cpp:234 >> +} > > What is the value of keeping this sync vs. having the network process proactively update the WebProcesses whenever the policy changes? e.g. cookieAcceptPolicyMayHaveChanged becomes cookieAcceptPolicyChanged(newPolicy). I was going to ask the same thing.

Comment on attachment 388104 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388104&action=review >>> Source/WebKit/WebProcess/Network/NetworkProcessConnection.cpp:234 >>> +} >> >> What is the value of keeping this sync vs. having the network process proactively update the WebProcesses whenever the policy changes? e.g. cookieAcceptPolicyMayHaveChanged becomes cookieAcceptPolicyChanged(newPolicy). > > I was going to ask the same thing. We would need to both send the initial value to the WebContent process (maybe when it gets the IPC connection to the network process) and then keep the value in sync, whenever it changes. It is a bit of extra code complexity. My patch gets most of the perf benefit with a bit less complexity. That's not to say that we cannot do better but I believe my patch was a step in the right direction.

(In reply to Alexey Proskuryakov from comment #4) > That would mean waking up all WebContent processes just for bookkeeping. > Which may be frozen. Sending an IPC to a suspended process does not wake it up, unless you actually explicitly take a process assertion to do so when sending the IPC.

(In reply to Chris Dumez from comment #7) > Comment on attachment 388104 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=388104&action=review > > >>> Source/WebKit/WebProcess/Network/NetworkProcessConnection.cpp:234 > >>> +} > >> > >> What is the value of keeping this sync vs. having the network process proactively update the WebProcesses whenever the policy changes? e.g. cookieAcceptPolicyMayHaveChanged becomes cookieAcceptPolicyChanged(newPolicy). > > > > I was going to ask the same thing. > > We would need to both send the initial value to the WebContent process > (maybe when it gets the IPC connection to the network process) and then keep > the value in sync, whenever it changes. It is a bit of extra code > complexity. My patch gets most of the perf benefit with a bit less > complexity. That's not to say that we cannot do better but I believe my > patch was a step in the right direction. I generally think getting rid of the sync IPC rather than adding caching, where possible, should be done, even at the cost of a little more complexity (though in this case I don't really think it would be any more complex). The sync IPC tends to come back and bite you at an inopportune time. Given you already have to do cache invalidation via CookieAcceptPolicyMayHaveChanged, I would expect setting an initial value when getting the network process connection would be all that remains (famous last words on my part there). Probably could add the bit to NetworkProcessConnectionInfo.

(In reply to Sam Weinig from comment #9) > (In reply to Chris Dumez from comment #7) > > Comment on attachment 388104 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=388104&action=review > > > > >>> Source/WebKit/WebProcess/Network/NetworkProcessConnection.cpp:234 > > >>> +} > > >> > > >> What is the value of keeping this sync vs. having the network process proactively update the WebProcesses whenever the policy changes? e.g. cookieAcceptPolicyMayHaveChanged becomes cookieAcceptPolicyChanged(newPolicy). > > > > > > I was going to ask the same thing. > > > > We would need to both send the initial value to the WebContent process > > (maybe when it gets the IPC connection to the network process) and then keep > > the value in sync, whenever it changes. It is a bit of extra code > > complexity. My patch gets most of the perf benefit with a bit less > > complexity. That's not to say that we cannot do better but I believe my > > patch was a step in the right direction. > > I generally think getting rid of the sync IPC rather than adding caching, > where possible, should be done, even at the cost of a little more complexity > (though in this case I don't really think it would be any more complex). The > sync IPC tends to come back and bite you at an inopportune time. > > Given you already have to do cache invalidation via > CookieAcceptPolicyMayHaveChanged, I would expect setting an initial value > when getting the network process connection would be all that remains > (famous last words on my part there). Probably could add the bit to > NetworkProcessConnectionInfo. Ok, I will look into this.

Created attachment 388343 [details] Patch

Created attachment 388345 [details] Patch

Created attachment 388346 [details] Patch

Created attachment 388347 [details] Patch

Created attachment 388353 [details] Patch

Created attachment 388354 [details] Patch

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

Created attachment 388358 [details] Patch

Created attachment 388359 [details] Patch

Comment on attachment 388359 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388359&action=review > Source/WebCore/platform/network/cf/NetworkStorageSessionCFNetWin.cpp:272 > + if (policy == CFHTTPCookieStorageAcceptPolicyAlways) > + return HTTPCookieAcceptPolicy::AlwaysAccept; > + if (policy == CFHTTPCookieStorageAcceptPolicyOnlyFromMainDocumentDomain) > + return HTTPCookieAcceptPolicy::OnlyFromMainDocumentDomain; > + if (policy == CFHTTPCookieStorageAcceptPolicyExclusivelyFromMainDocumentDomain) > + return HTTPCookieAcceptPolicy::ExclusivelyFromMainDocumentDomain; > + > + return HTTPCookieAcceptPolicy::Never; Why not use a switch? I guess because it’s not an enum we don’t get that extra compiler checking, but I’d still be tempted to write it that way. > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:464 > + if (cookieAcceptPolicy == NSHTTPCookieAcceptPolicyAlways) > + return HTTPCookieAcceptPolicy::AlwaysAccept; > + if (cookieAcceptPolicy == NSHTTPCookieAcceptPolicyOnlyFromMainDocumentDomain) > + return HTTPCookieAcceptPolicy::OnlyFromMainDocumentDomain; > + if (cookieAcceptPolicy == NSHTTPCookieAcceptPolicyExclusivelyFromMainDocumentDomain) > + return HTTPCookieAcceptPolicy::ExclusivelyFromMainDocumentDomain; > END_BLOCK_OBJC_EXCEPTIONS; > - return false; > + > + return HTTPCookieAcceptPolicy::Never; Ditto. > Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:252 > + if (policy == SOUP_COOKIE_JAR_ACCEPT_ALWAYS) > + return HTTPCookieAcceptPolicy::AlwaysAccept; > + if (policy == SOUP_COOKIE_JAR_ACCEPT_NO_THIRD_PARTY) > + return HTTPCookieAcceptPolicy::OnlyFromMainDocumentDomain; > + return HTTPCookieAcceptPolicy::Never; Ditto. > Source/WebKit/NetworkProcess/Cookies/WebCookieManager.cpp:158 > +#if PLATFORM(COCOA) > + completionHandler(WebCookieManager::platformGetHTTPCookieAcceptPolicy()); > +#else > + completionHandler(m_process.defaultStorageSession().cookieAcceptPolicy()); > +#endif I don’t understand why Cocoa is different here. Maybe it should be obvious but somehow it’s not. > Source/WebKit/NetworkProcess/NetworkProcess.cpp:390 > + HTTPCookieAcceptPolicy cookieAcceptPolicy = HTTPCookieAcceptPolicy::Never; > + if (auto* storage = storageSession(sessionID)) > + cookieAcceptPolicy = storage->cookieAcceptPolicy(); > + > + completionHandler(WTFMove(ipcConnection->second), cookieAcceptPolicy); Consider: auto* storage = storageSession(sessionID); completionHandler(WTFMove(ipcConnection->second), storage ? storage->cookieAcceptPolicy() : HTTPCookieAcceptPolicy::Never); Maybe you still like the if better? > Source/WebKit/UIProcess/API/Cocoa/WKProcessPool.mm:203 > +static WebCore::HTTPCookieAcceptPolicy toHTTPCookieAcceptPolicy(NSHTTPCookieAcceptPolicy policy) Seems to duplicate code in NetworkStorageSession::cookieAcceptPolicy. Might be nice to find a way to share these conversion functions. > Source/WebKit/WebProcess/Network/NetworkProcessConnection.h:84 > + bool cookiesEnabled(); Maybe const.

Comment on attachment 388359 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388359&action=review >> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNetWin.cpp:272 >> + return HTTPCookieAcceptPolicy::Never; > > Why not use a switch? I guess because it’s not an enum we don’t get that extra compiler checking, but I’d still be tempted to write it that way. Because it was not an enum I did not use a switch, but I guess you're right, it does not hurt to use an enum, we simply don't get the extra validation. >> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:464 >> + return HTTPCookieAcceptPolicy::Never; > > Ditto. Ok. >> Source/WebKit/NetworkProcess/Cookies/WebCookieManager.cpp:158 >> +#endif > > I don’t understand why Cocoa is different here. Maybe it should be obvious but somehow it’s not. I will double check. I don't remember exactly but I think it was me trying to be conservative because the Cocoa implementation was slightly different (used Cocoa directly instead of m_process.defaultStorageSession()). I was worried about constructing the DefaultStorageSession unnecessarily, which may not be an issue. >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:390 >> + completionHandler(WTFMove(ipcConnection->second), cookieAcceptPolicy); > > Consider: > > auto* storage = storageSession(sessionID); > completionHandler(WTFMove(ipcConnection->second), > storage ? storage->cookieAcceptPolicy() : HTTPCookieAcceptPolicy::Never); > > Maybe you still like the if better? I like your proposal, will update. >> Source/WebKit/UIProcess/API/Cocoa/WKProcessPool.mm:203 >> +static WebCore::HTTPCookieAcceptPolicy toHTTPCookieAcceptPolicy(NSHTTPCookieAcceptPolicy policy) > > Seems to duplicate code in NetworkStorageSession::cookieAcceptPolicy. Might be nice to find a way to share these conversion functions. Oh, indeed. >> Source/WebKit/WebProcess/Network/NetworkProcessConnection.h:84 >> + bool cookiesEnabled(); > > Maybe const. Ok.

Created attachment 388426 [details] Patch

Comment on attachment 388426 [details] Patch Clearing flags on attachment: 388426 Committed r254931: <https://trac.webkit.org/changeset/254931>

All reviewed patches have been landed. Closing bug.

<rdar://problem/58814695>