Summary: | Use Function rather than std::function for thread safety in WebsiteDataStore | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||
Component: | WebKit2 | Assignee: | Brent Fulgham <bfulgham> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | bfulgham, cdumez, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | 173168 | ||||||
Bug Blocks: | |||||||
Attachments: |
|
Description
Brent Fulgham
2017-06-09 11:35:36 PDT
Created attachment 312467 [details]
Patch
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 && 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 &&. 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 Comment on attachment 312467 [details]
Patch
r=me with nit fixes I suggested.
Committed r218011: <http://trac.webkit.org/changeset/218011> |