Resource Load Statistics: More efficient network process messaging + Fix bug in user interaction reporting

• Component: WebKit2
• Status: RESOLVED FIXED

[John Wilander]

We should send batches of domains to remove and add in one message type to the network process.

Also, there is a bug in how user interaction gets reported to the network process. It has to go through the UI process which will be addressed by the same change.

We should make sure we behave gracefully if we have nothing to report to the network process.

[John Wilander]

Created attachment 304123 [details]
Patch

[Alex Christensen]

Comment on attachment 304123 [details]
Patch

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

> Source/WebCore/loader/ResourceLoadStatistics.cpp:297
> +    // In-memory only

I'm not sure this comment helps anything

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:185
> -    m_resourceLoadStatisticsStore->setShouldPartitionCookiesCallback([this, shouldPartitionCookiesForDomainsHandler = WTFMove(shouldPartitionCookiesForDomainsHandler)] (const Vector<String>& primaryDomains, bool value) {
> -        shouldPartitionCookiesForDomainsHandler(primaryDomains, value);
> +    m_resourceLoadStatisticsStore->setShouldPartitionCookiesCallback([this, shouldPartitionCookiesForDomainsHandler] (const Vector<String>& domainsToRemove, const Vector<String>& domainsToAdd) {

The WTFMove will still be necessary when we change the std::function to WTF::Function and make sure it's never copied.  Removing the WTFMove just makes a copy, which doesn't cause a problem in this case.

[John Wilander]

Created attachment 304295 [details]
Patch

[John Wilander]

(In reply to comment #2)
> Comment on attachment 304123 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=304123&action=review
> 
> > Source/WebCore/loader/ResourceLoadStatistics.cpp:297
> > +    // In-memory only
> 
> I'm not sure this comment helps anything

I commented on it to minimize confusion as to why this value is not persisted to or decoded from disk and that it has to be generated at runtime. This is because the state in the network process only exists at runtime.

> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:185
> > -    m_resourceLoadStatisticsStore->setShouldPartitionCookiesCallback([this, shouldPartitionCookiesForDomainsHandler = WTFMove(shouldPartitionCookiesForDomainsHandler)] (const Vector<String>& primaryDomains, bool value) {
> > -        shouldPartitionCookiesForDomainsHandler(primaryDomains, value);
> > +    m_resourceLoadStatisticsStore->setShouldPartitionCookiesCallback([this, shouldPartitionCookiesForDomainsHandler] (const Vector<String>& domainsToRemove, const Vector<String>& domainsToAdd) {
> 
> The WTFMove will still be necessary when we change the std::function to
> WTF::Function and make sure it's never copied.  Removing the WTFMove just
> makes a copy, which doesn't cause a problem in this case.

Good catch! That change was unintentional. I was changing things as part of my debugging. The move is now back in.

Thanks for the review, Alex!

[Radar WebKit Bug Importer]

<rdar://problem/31019494>

[WebKit Commit Bot]

Comment on attachment 304295 [details]
Patch

Clearing flags on attachment: 304295

Committed r213871: <http://trac.webkit.org/changeset/213871>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.