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

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

Summary: Resource Load Statistics: Implement callback support for removal of WebsiteDa...

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	WebKit Misc. (show other bugs)
Version:	WebKit Nightly Build
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	John Wilander

URL:
Keywords:	InRadar

Depends on:
Blocks:

Reported:	2018-01-18 17:21 PST by John Wilander
Modified:	2018-01-19 16:12 PST (History)
CC List:	7 users (show)

See Also:	181870 181890

Attachments
Patch (32.60 KB, patch) 2018-01-18 17:26 PST, John Wilander	no flags	Details \| Formatted Diff \| Diff
Patch (32.99 KB, patch) 2018-01-18 17:29 PST, John Wilander	no flags	Details \| Formatted Diff \| Diff
Patch (33.20 KB, patch) 2018-01-18 18:05 PST, John Wilander	no flags	Details \| Formatted Diff \| Diff
Archive of layout-test-results from ews102 for mac-sierra (2.22 MB, application/zip) 2018-01-18 19:07 PST, EWS Watchlist	no flags	Details
Patch for landing (33.67 KB, patch) 2018-01-19 10:34 PST, John Wilander	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description John Wilander 2018-01-18 17:21:55 PST

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

Comment 1 John Wilander 2018-01-18 17:22:01 PST

<rdar://problem/33491222>

Comment 2 John Wilander 2018-01-18 17:26:08 PST

Created attachment 331676 [details]
Patch

Comment 3 John Wilander 2018-01-18 17:27:04 PST

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

Comment 4 John Wilander 2018-01-18 17:29:51 PST

Created attachment 331677 [details]
Patch

Comment 5 EWS Watchlist 2018-01-18 17:31:20 PST

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.

Comment 6 John Wilander 2018-01-18 18:05:49 PST

Created attachment 331683 [details]
Patch

Comment 7 EWS Watchlist 2018-01-18 18:08:10 PST

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.

Comment 8 EWS Watchlist 2018-01-18 19:07:15 PST

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

Comment 9 EWS Watchlist 2018-01-18 19:07:16 PST

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

Comment 10 John Wilander 2018-01-18 19:17:19 PST

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

Comment 11 Alex Christensen 2018-01-19 09:44:07 PST

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

Comment 12 John Wilander 2018-01-19 09:51:50 PST

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

Comment 13 John Wilander 2018-01-19 10:34:41 PST

Created attachment 331751 [details]
Patch for landing

Comment 14 WebKit Commit Bot 2018-01-19 11:26:19 PST

Comment on attachment 331751 [details]
Patch for landing

Clearing flags on attachment: 331751

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

Comment 15 WebKit Commit Bot 2018-01-19 11:26:21 PST

All reviewed patches have been landed.  Closing bug.

Comment 16 John Wilander 2018-01-19 12:15:48 PST

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

Comment 17 John Wilander 2018-01-19 16:12:38 PST

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