Bug 174038

Summary: Move store logic from WebResourceLoadStatisticsManager to WebResourceLoadStatisticsStore
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, commit-queue, ggaren, ossy, wilander
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2017-06-30 14:02:10 PDT
Move store logic from WebResourceLoadStatisticsManager to WebResourceLoadStatisticsStore. WebResourceLoadStatisticsManager will essentially become a WK C API proxy for the WebResourceLoadStatisticsStore.

In a follow-up, the idea is to make WebResourceLoadStatisticsManager not be a singleton and instead be an APIObject for the WebResourceLoadStatisticsStore.
This will require replacing the C API currently used by WKTR with a Cocoa API that works on a WebResourceLoadStatisticsStore, rather than require a singleton.
Comment 1 Chris Dumez 2017-06-30 14:08:22 PDT
Created attachment 314285 [details]
WIP Patch
Comment 2 Chris Dumez 2017-06-30 14:28:39 PDT
Created attachment 314290 [details]
Patch
Comment 3 Brent Fulgham 2017-06-30 14:32:04 PDT
Comment on attachment 314290 [details]
Patch

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

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:548
> +    // FIXME: Why is this potentially creating a store entry just for querying?

This looks wrong. I can't find any place that relies on this query having the side-effect of creating a new entry. I suspect this was just a matter of copy/paste as these test routines were added.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:569
> +    // FIXME: Why is this potentially creating a store entry just for querying?

Ditto. We should just return 'false' if the entry doesn't exist. These methods should probably be 'const', too.
Comment 4 Brent Fulgham 2017-06-30 14:32:41 PDT
/home/ews/ltilve-gtk-wk2-ews/WebKit/Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:1087:35: error: 'class WebKit::WebResourceLoadStatisticsStore' has no member named 'clearInMemoryAndPersistentStore'; did you mean 'clearInMemoryAndPersistent'?
Comment 5 Chris Dumez 2017-06-30 14:33:28 PDT
(In reply to Brent Fulgham from comment #3)
> Comment on attachment 314290 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=314290&action=review
> 
> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:548
> > +    // FIXME: Why is this potentially creating a store entry just for querying?
> 
> This looks wrong. I can't find any place that relies on this query having
> the side-effect of creating a new entry. I suspect this was just a matter of
> copy/paste as these test routines were added.
> 
> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:569
> > +    // FIXME: Why is this potentially creating a store entry just for querying?
> 
> Ditto. We should just return 'false' if the entry doesn't exist. These
> methods should probably be 'const', too.

I suspect this is wrong indeed. However, I am trying hard not to change behavior in big refactoring patches :) I can take care of this in a follow-up.
Comment 6 Brent Fulgham 2017-06-30 14:33:48 PDT
Comment on attachment 314290 [details]
Patch

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

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:818
> +        m_resourceLoadStatistics->clearInMemoryAndPersistentStore(modifiedSince);

Should this be 'clearInMemoryAndPersistent'?

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:1087
> +        m_resourceLoadStatistics->clearInMemoryAndPersistentStore();

Ditto.
Comment 7 Chris Dumez 2017-06-30 14:34:04 PDT
(In reply to Brent Fulgham from comment #4)
> /home/ews/ltilve-gtk-wk2-ews/WebKit/Source/WebKit2/UIProcess/WebsiteData/
> WebsiteDataStore.cpp:1087:35: error: 'class
> WebKit::WebResourceLoadStatisticsStore' has no member named
> 'clearInMemoryAndPersistentStore'; did you mean 'clearInMemoryAndPersistent'?

Yea yea, I am still building locally :P which is why there is no r? flag.
Comment 8 Chris Dumez 2017-06-30 14:46:07 PDT
Created attachment 314295 [details]
Patch
Comment 9 Brent Fulgham 2017-06-30 14:49:04 PDT
Comment on attachment 314295 [details]
Patch

r=me
Comment 10 Chris Dumez 2017-06-30 15:25:38 PDT
Darn, it looks like I broke something:
Regressions: Unexpected timeouts (2)
  http/tests/loading/resourceLoadStatistics/non-prevalent-resource-without-user-interaction.html [ Timeout ]
  http/tests/loading/resourceLoadStatistics/prevalent-resource-with-user-interaction.html [ Timeout ]

Must have made a copy/paste error somewhere.
Comment 11 Chris Dumez 2017-06-30 15:36:35 PDT
Comment on attachment 314295 [details]
Patch

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

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.h:374
> +    void setStatisticsMinimumTimeBetweenDataRecordsRemoval(double);

Ah... the tests use setStatisticsMinimumTimeBetweeenDataRecordsRemoval with 3 'e's :)
Comment 12 Chris Dumez 2017-06-30 15:41:16 PDT
Created attachment 314307 [details]
Patch
Comment 13 WebKit Commit Bot 2017-06-30 16:23:02 PDT
Comment on attachment 314307 [details]
Patch

Clearing flags on attachment: 314307

Committed r219025: <http://trac.webkit.org/changeset/219025>
Comment 14 WebKit Commit Bot 2017-06-30 16:23:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Csaba Osztrogon√°c 2017-06-30 22:47:58 PDT
(In reply to WebKit Commit Bot from comment #13)
> Comment on attachment 314307 [details]
> Patch
> 
> Clearing flags on attachment: 314307
> 
> Committed r219025: <http://trac.webkit.org/changeset/219025>

It broke the Apple Mac cmake build
https://build.webkit.org/builders/Apple%20El%20Capitan%20CMake%20Debug%20%28Build%29/builds/3035