Bug 174033 - Move ResourceLoadStatisticsStore to WebKit2/UIProcess
Summary: Move ResourceLoadStatisticsStore to WebKit2/UIProcess
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-06-30 11:33 PDT by Chris Dumez
Modified: 2017-06-30 13:54 PDT (History)
2 users (show)

See Also:


Attachments
Patch (68.01 KB, patch)
2017-06-30 12:54 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-06-30 11:33:43 PDT
Move ResourceLoadStatisticsStore to WebKit2/UIProcess as it is only used in the WebKit2 UIProcess.
Comment 1 Chris Dumez 2017-06-30 12:54:35 PDT
Created attachment 314281 [details]
Patch
Comment 2 Build Bot 2017-06-30 12:55:41 PDT
Attachment 314281 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.h:91:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Brent Fulgham 2017-06-30 13:20:55 PDT
Comment on attachment 314281 [details]
Patch

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

I think this looks great! iOS is happy so far. r=me once the tests pass.

> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:55
> +    auto locker = holdLock(m_statisticsLock);

Now that this is only present in UIProcess, I suspect that this lock can go away, and we can just ASSERT that we are on the work queue.

This comment obviously applies to the rest of this file. But perhaps that should be a separate patch (in case I am wrong!) :-)

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsManager.h:28
> +#include <chrono>

It seems like we are using WallTime everywhere now. I wonder if we can get rid of <chrono> entirely?
Comment 4 Chris Dumez 2017-06-30 13:34:07 PDT
(In reply to Brent Fulgham from comment #3)
> Comment on attachment 314281 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=314281&action=review
> 
> I think this looks great! iOS is happy so far. r=me once the tests pass.

Thanks.

> 
> > Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:55
> > +    auto locker = holdLock(m_statisticsLock);
> 
> Now that this is only present in UIProcess, I suspect that this lock can go
> away, and we can just ASSERT that we are on the work queue.
> 
> This comment obviously applies to the rest of this file. But perhaps that
> should be a separate patch (in case I am wrong!) :-)

Separate patch since non-trivial. The WK C API which is used by TestRunner is synchronous and relies on those locks to dose database operations from the main thread still. I plan to fix this in a follow-up.

> 
> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsManager.h:28
> > +#include <chrono>
> 
> It seems like we are using WallTime everywhere now. I wonder if we can get
> rid of <chrono> entirely?

We still use std::chrono in one API, which I have not corrected yet.
Comment 5 Chris Dumez 2017-06-30 13:54:31 PDT
Committed r219010: <http://trac.webkit.org/changeset/219010>