Bug 173172 – Use Function rather than std::function for thread safety in WebsiteDataStore

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brent Fulgham]

Audit the WebsiteDataStore code for use of std::function, and switch to our safer WTF::Function version.

Also attempt to reduce copies and ref churn by using WTFMove in more places.

[Radar WebKit Bug Importer]

<rdar://problem/32679311>

[Brent Fulgham]

Created attachment 312467 [details]
Patch

[Chris Dumez]

Comment on attachment 312467 [details]
Patch

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

Will wait for debug bot to be happy before r+ given all the new assertions :)

> Source/WebKit2/UIProcess/WebProcessProxy.cpp:267
> +            callbackAggregator->addDomainsWithDeletedWebsiteData(domainsWithDeletedWebsiteData);

Wouldn't hurt to WTFMove(domainsWithDeletedWebsiteData) here.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:533
> +            String&& domain = dataRecord.topPrivatelyControlledDomain();

This is weird. I think this should stay a String.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:583
> +        explicit CallbackAggregator(Function<void()> completionHandler)

Missing &&.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:820
> +        explicit CallbackAggregator(Function<void()> completionHandler)

Missing &&

[Chris Dumez]

Comment on attachment 312467 [details]
Patch

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

> Source/WebKit2/UIProcess/WebProcessProxy.cpp:214
> +    // We expect this to be called on the main thread so we get the default website data store

Missing period at the end.

> Source/WebKit2/UIProcess/WebProcessProxy.cpp:275
> +    // We expect this to be called on the main thread so we get the default website data store

Missing period at the end.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:188
> +        explicit CallbackAggregator(OptionSet<WebsiteDataFetchOption> fetchOptions, Function<void(Vector<WebsiteDataRecord>)> completionHandler)

Missing &&.

[Brent Fulgham]

Comment on attachment 312467 [details]
Patch

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

>> Source/WebKit2/UIProcess/WebProcessProxy.cpp:267
>> +            callbackAggregator->addDomainsWithDeletedWebsiteData(domainsWithDeletedWebsiteData);
> 
> Wouldn't hurt to WTFMove(domainsWithDeletedWebsiteData) here.

OK!

>> Source/WebKit2/UIProcess/WebProcessProxy.cpp:275
>> +    // We expect this to be called on the main thread so we get the default website data store
> 
> Missing period at the end.

Fixed (and the other one).

>> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:188
>> +        explicit CallbackAggregator(OptionSet<WebsiteDataFetchOption> fetchOptions, Function<void(Vector<WebsiteDataRecord>)> completionHandler)
> 
> Missing &&.

Whoops!

>> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:533
>> +            String&& domain = dataRecord.topPrivatelyControlledDomain();
> 
> This is weird. I think this should stay a String.

OK!

>> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:583
>> +        explicit CallbackAggregator(Function<void()> completionHandler)
> 
> Missing &&.

Good catch!

>> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:820
>> +        explicit CallbackAggregator(Function<void()> completionHandler)
> 
> Missing &&

+1

[Chris Dumez]

Comment on attachment 312467 [details]
Patch

r=me with nit fixes I suggested.

[Brent Fulgham]

Committed r218011: <http://trac.webkit.org/changeset/218011>