We should clear network website data when a user clears history/website data
<rdar://problem/75288338>
Created attachment 424851 [details] Patch
Comment on attachment 424851 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424851&action=review r=me, but please double-check with David for the SOFT_LINK stuff. > Source/WTF/wtf/cocoa/SoftLinking.h:120 > +#define SOFT_LINK_PRIVATE_FRAMEWORK_IN_UMBRELLA(umbrella, framework) \ I'd suggest getting David's r+ on this. I can never remember when you need the _SOURCE version of these macros. Looks good to me though.
Comment on attachment 424851 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424851&action=review r=me >> Source/WTF/wtf/cocoa/SoftLinking.h:120 >> +#define SOFT_LINK_PRIVATE_FRAMEWORK_IN_UMBRELLA(umbrella, framework) \ > > I'd suggest getting David's r+ on this. I can never remember when you need the _SOURCE version of these macros. Looks good to me though. This is correct. The _SOURCE and _HEADER variants are only needed if you're creating a single source/header for all the soft-linked items (which we aren't doing here because it's the first time we're soft-linking these frameworks).
Created attachment 424963 [details] Patch
(In reply to katherine_cheney from comment #5) > Created attachment 424963 [details] > Patch Updated this based on an offline conversation to handle the soft-link case where the framework isn't available. Dave, could you take another look?
Comment on attachment 424963 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424963&action=review > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1617 > +#if PLATFORM(COCOA) > + if (auto* networkSession = this->networkSession(sessionID)) > + networkSession->removeNetworkWebsiteData(modifiedSince, [clearTasksHandler] { }); > +#endif Since networkSession is already specialized for ports, I would suggest removing the #ifdefs here, and just making the default implementation of NetworkSession::removeNetworkWebsiteData() do nothing, allowing ports to implement as they see fit.
Please do try not add #if PLATFORM(COCOA) in shared code if you can avoid it at all. And if you really do need to add an #ifdef in shared code, please use a more specific HAVE(), USE() or ENABLE() macro to make it clear what you are conditionalizing on.
Comment on attachment 424963 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424963&action=review > Source/WebKit/ChangeLog:10 > + Clear network-related website data for domains when clearing history/ > + website data. Please add a test for this new functionality or explain why no test is feasible in this case.
(In reply to Sam Weinig from comment #7) > Comment on attachment 424963 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=424963&action=review > > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1617 > > +#if PLATFORM(COCOA) > > + if (auto* networkSession = this->networkSession(sessionID)) > > + networkSession->removeNetworkWebsiteData(modifiedSince, [clearTasksHandler] { }); > > +#endif > > Since networkSession is already specialized for ports, I would suggest > removing the #ifdefs here, and just making the default implementation of > NetworkSession::removeNetworkWebsiteData() do nothing, allowing ports to > implement as they see fit. okay.
(In reply to Sam Weinig from comment #9) > Comment on attachment 424963 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=424963&action=review > > > Source/WebKit/ChangeLog:10 > > + Clear network-related website data for domains when clearing history/ > > + website data. > > Please add a test for this new functionality or explain why no test is > feasible in this case. Will fix.
Created attachment 424968 [details] Patch
Comment on attachment 424968 [details] Patch r=me
Committed r275401: <https://commits.webkit.org/r275401> All reviewed patches have been landed. Closing bug and clearing flags on attachment 424968 [details].
(In reply to katherine_cheney from comment #11) > (In reply to Sam Weinig from comment #9) > > Comment on attachment 424963 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=424963&action=review > > > > > Source/WebKit/ChangeLog:10 > > > + Clear network-related website data for domains when clearing history/ > > > + website data. > > > > Please add a test for this new functionality or explain why no test is > > feasible in this case. > > Will fix. Thanks.