Move store logic from WebResourceLoadStatisticsManager to WebResourceLoadStatisticsStore

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 314285 [details]
WIP Patch

[Chris Dumez]

Created attachment 314290 [details]
Patch

[Brent Fulgham]

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.

[Brent Fulgham]

/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'?

[Chris Dumez]

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

[Brent Fulgham]

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.

[Chris Dumez]

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

[Chris Dumez]

Created attachment 314295 [details]
Patch

[Brent Fulgham]

Comment on attachment 314295 [details]
Patch

r=me

[Chris Dumez]

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.

[Chris Dumez]

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 :)

[Chris Dumez]

Created attachment 314307 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 314307 [details]
Patch

Clearing flags on attachment: 314307

Committed r219025: <http://trac.webkit.org/changeset/219025>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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