Bug 224030

Summary: Clear network website data when a user clears history/website data
Product: WebKit Reporter: katherine_cheney
Component: WebKit Misc.Assignee: katherine_cheney
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, benjamin, bfulgham, cdumez, cmarcelo, ddkilzer, ews-watchlist, sam
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description katherine_cheney 2021-03-31 17:36:29 PDT
We should clear network website data when a user clears history/website data
Comment 1 katherine_cheney 2021-03-31 17:36:45 PDT
<rdar://problem/75288338>
Comment 2 katherine_cheney 2021-03-31 18:01:19 PDT
Created attachment 424851 [details]
Patch
Comment 3 Brent Fulgham 2021-03-31 19:24:15 PDT
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 4 David Kilzer (:ddkilzer) 2021-04-01 12:11:01 PDT
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).
Comment 5 katherine_cheney 2021-04-01 17:04:49 PDT
Created attachment 424963 [details]
Patch
Comment 6 katherine_cheney 2021-04-01 17:06:09 PDT
(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 7 Sam Weinig 2021-04-01 17:08:18 PDT
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.
Comment 8 Sam Weinig 2021-04-01 17:10:08 PDT
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 9 Sam Weinig 2021-04-01 17:10:50 PDT
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.
Comment 10 katherine_cheney 2021-04-01 17:34:37 PDT
(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.
Comment 11 katherine_cheney 2021-04-01 17:34:45 PDT
(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.
Comment 12 katherine_cheney 2021-04-01 17:37:07 PDT
Created attachment 424968 [details]
Patch
Comment 13 David Kilzer (:ddkilzer) 2021-04-01 18:43:25 PDT
Comment on attachment 424968 [details]
Patch

r=me
Comment 14 EWS 2021-04-01 20:19:45 PDT
Committed r275401: <https://commits.webkit.org/r275401>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424968 [details].
Comment 15 Sam Weinig 2021-04-02 14:35:46 PDT
(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.