Bug 181822 – Resource Load Statistics: Implement callback support for removal of WebsiteDataType::ResourceLoadStatistics

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[John Wilander]

The WebsiteDataStore needs to wait for removal of WebsiteDataType::ResourceLoadStatistics before returning.

[John Wilander]

<rdar://problem/33491222>

[John Wilander]

Created attachment 331676 [details]
Patch

[John Wilander]

Style errors are all of the 'Extra space before ( in function call' type.

[John Wilander]

Created attachment 331677 [details]
Patch

[EWS Watchlist]

Attachment 331677 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:308:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:596:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:615:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:122:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:123:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:140:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1441:  Extra space before ( in function call  [whitespace/parens] [4]
WARNING: This machine could support 4 simulators, but is only configured for 3.
WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>.
ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:110:  Extra space before ( in function call  [whitespace/parens] [4]
WARNING: This machine could support 4 simulators, but is only configured for 3.
WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>.
WARNING: This machine could support 4 simulators, but is only configured for 3.
WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>.
Total errors found: 8 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[John Wilander]

Created attachment 331683 [details]
Patch

[EWS Watchlist]

Attachment 331683 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:308:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:596:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:615:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:122:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:123:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:140:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1441:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:110:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 8 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 331683 [details]
Patch

Attachment 331683 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6128813

New failing tests:
http/tests/xmlhttprequest/redirect-cross-origin-tripmine.html

[EWS Watchlist]

Created attachment 331691 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[John Wilander]

(In reply to Build Bot from comment #8)
> Comment on attachment 331683 [details]
> Patch
> 
> Attachment 331683 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.webkit.org/results/6128813
> 
> New failing tests:
> http/tests/xmlhttprequest/redirect-cross-origin-tripmine.html

The failed test is unrelated. I’ve seen it fail on other patches.

[Alex Christensen]

Comment on attachment 331683 [details]
Patch

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

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:308
> +void WebResourceLoadStatisticsStore::grandfatherExistingWebsiteData(WTF::CompletionHandler<void ()>&& callback)

WTF should be unnecessary.
Space before () is against style guidelines.
Everywhere in this patch.

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:563
> +    auto completionHandlerCopy = makeBlockPtr(completionHandler);

This could just be done inside the lambda capture.  Then you won't need a local variable with "Copy" in its name.  Otherwise this should be WTFMoved into the lambda.

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:582
> +    auto completionHandlerCopy = makeBlockPtr(completionHandler);

ditto

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:82
>  - (void)_resourceLoadStatisticsClearInMemoryAndPersistentStore WK_API_AVAILABLE(macosx(10.13), ios(11.0));

These could use a WK_API_DEPRECATED_WITH_REPLACEMENT

[John Wilander]

(In reply to Alex Christensen from comment #11)
> Comment on attachment 331683 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331683&action=review
> 
> > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:308
> > +void WebResourceLoadStatisticsStore::grandfatherExistingWebsiteData(WTF::CompletionHandler<void ()>&& callback)
> 
> WTF should be unnecessary.

Fixed.

> Space before () is against style guidelines.
> Everywhere in this patch.

Oh, I thought that was a bug in the style checker. I've always declared these with a space. :/ Fixed.

> > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:563
> > +    auto completionHandlerCopy = makeBlockPtr(completionHandler);
> 
> This could just be done inside the lambda capture.  Then you won't need a
> local variable with "Copy" in its name.  Otherwise this should be WTFMoved
> into the lambda.

Got it. Fixed.

> > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:582
> > +    auto completionHandlerCopy = makeBlockPtr(completionHandler);
> 
> ditto

Fixed.

> > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:82
> >  - (void)_resourceLoadStatisticsClearInMemoryAndPersistentStore WK_API_AVAILABLE(macosx(10.13), ios(11.0));
> 
> These could use a WK_API_DEPRECATED_WITH_REPLACEMENT

Fixed.

Thanks, Alex!

[John Wilander]

Created attachment 331751 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 331751 [details]
Patch for landing

Clearing flags on attachment: 331751

Committed r227223: <https://trac.webkit.org/changeset/227223>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[John Wilander]

A supplemental build fix was fixed in https://trac.webkit.org/changeset/227227.

[John Wilander]

A supplemental API test fix landed in https://trac.webkit.org/changeset/227248.