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.
Created attachment 314285 [details] WIP Patch
Created attachment 314290 [details] Patch
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.
/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'?
(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 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.
(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.
Created attachment 314295 [details] Patch
Comment on attachment 314295 [details] Patch r=me
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 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 :)
Created attachment 314307 [details] Patch
Comment on attachment 314307 [details] Patch Clearing flags on attachment: 314307 Committed r219025: <http://trac.webkit.org/changeset/219025>
All reviewed patches have been landed. Closing bug.
(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