ResourceLoadObserver does not need a ResourceLoadStatisticsStore. ResourceLoadStatisticsStore is too complicated for its needs. ResourceLoadStatisticsStore can then be moved to WebKit2/UIProcess.
Created attachment 314226 [details] WIP Patch
Created attachment 314227 [details] WIP Patch
Created attachment 314233 [details] WIP Patch
Comment on attachment 314233 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314233&action=review This is a GREAT cleanup! I’m so excited to see these simplifications. Thank you! > Source/WebCore/loader/ResourceLoadObserver.cpp:112 > + shouldCallNotificationCallback = targetStatistics.dataRecordsRemoved > 0; Why don’t we just declare and initialize right here? > Source/WebCore/loader/ResourceLoadObserver.cpp:194 > + shouldCallNotificationCallback = targetStatistics.dataRecordsRemoved > 0; Bool shoukdCallNotificationCallback ... ? > Source/WebCore/loader/ResourceLoadObserver.cpp:204 > + auto& updatedTargetStatistics = ensureResourceStatisticsForPrimaryDomain(targetPrimaryDomain); Could we do your ‘take’ approach here like you did in an earlier patch and avoid getting multiple times? > Source/WebCore/loader/ResourceLoadObserver.cpp:261 > + shouldCallNotificationCallback = targetStatistics.dataRecordsRemoved > 0; Bool ... > Source/WebCore/loader/ResourceLoadObserver.cpp:294 > + WallTime newTime = reduceTimeResolution(WallTime::now()); Auto? > Source/WebCore/loader/ResourceLoadObserver.cpp:335 > + return "Statistics for " + origin + ":\n" + iter->value.toString(); Should this be done with a StringBuilder? I’m never sure when it is the best time to do so. > Source/WebCore/loader/ResourceLoadObserver.cpp:338 > +Vector<ResourceLoadStatistics> ResourceLoadObserver::takeStatistics() Should this be returning a move value? Or does return-value-optimization do the right thing here? > Source/WebCore/loader/ResourceLoadObserver.h:-59 > - WEBCORE_EXPORT void setStatisticsQueue(Ref<WTF::WorkQueue>&&); Hooray! > Source/WebKit/mac/WebView/WebView.mm:-1180 > - Hooray ++ ! > Source/WebKit2/WebProcess/WebProcess.cpp:202 > + ResourceLoadObserver::sharedObserver().setNotificationCallback([this] { ResourceLoadObserver::shared() ?
Comment on attachment 314233 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314233&action=review >> Source/WebCore/loader/ResourceLoadObserver.cpp:335 >> + return "Statistics for " + origin + ":\n" + iter->value.toString(); > > Should this be done with a StringBuilder? I’m never sure when it is the best time to do so. I don't know either but this is used only for testing so this is probably good enough. It has the benefit of being concise. >> Source/WebCore/loader/ResourceLoadObserver.cpp:338 >> +Vector<ResourceLoadStatistics> ResourceLoadObserver::takeStatistics() > > Should this be returning a move value? Or does return-value-optimization do the right thing here? return-value-optimization does the right thing.
Created attachment 314269 [details] Patch
Created attachment 314271 [details] Patch
Comment on attachment 314271 [details] Patch Looks great. r=me.
Comment on attachment 314271 [details] Patch Clearing flags on attachment: 314271 Committed r219005: <http://trac.webkit.org/changeset/219005>
All reviewed patches have been landed. Closing bug.