Bug 174013 – ResourceLoadObserver does not need a ResourceLoadStatisticsStore

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

ResourceLoadObserver does not need a ResourceLoadStatisticsStore. ResourceLoadStatisticsStore is too complicated for its needs. ResourceLoadStatisticsStore can then be moved to WebKit2/UIProcess.

[Chris Dumez]

Created attachment 314226 [details]
WIP Patch

[Chris Dumez]

Created attachment 314227 [details]
WIP Patch

[Chris Dumez]

Created attachment 314233 [details]
WIP Patch

[Brent Fulgham]

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() ?

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 314269 [details]
Patch

[Chris Dumez]

Created attachment 314271 [details]
Patch

[Brent Fulgham]

Comment on attachment 314271 [details]
Patch

Looks great. r=me.

[Chris Dumez]

Comment on attachment 314271 [details]
Patch

Clearing flags on attachment: 314271

Committed r219005: <http://trac.webkit.org/changeset/219005>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.