Bug 224030 – Clear network website data when a user clears history/website data

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Kate Cheney]

We should clear network website data when a user clears history/website data

[Kate Cheney]

<rdar://problem/75288338>

[Kate Cheney]

Created attachment 424851 [details]
Patch

[Brent Fulgham]

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.

[David Kilzer (:ddkilzer)]

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).

[Kate Cheney]

Created attachment 424963 [details]
Patch

[Kate Cheney]

(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?

[Sam Weinig]

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.

[Sam Weinig]

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.

[Sam Weinig]

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.

[Kate Cheney]

(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.

[Kate Cheney]

(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.

[Kate Cheney]

Created attachment 424968 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 424968 [details]
Patch

r=me

[EWS]

Committed r275401: <https://commits.webkit.org/r275401>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424968 [details].

[Sam Weinig]

(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.