Bug 174013 - ResourceLoadObserver does not need a ResourceLoadStatisticsStore
Summary: ResourceLoadObserver does not need a ResourceLoadStatisticsStore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-06-29 21:18 PDT by Chris Dumez
Modified: 2017-06-30 11:18 PDT (History)
4 users (show)

See Also:


Attachments
WIP Patch (31.23 KB, patch)
2017-06-29 21:20 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (33.33 KB, patch)
2017-06-29 21:31 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (33.46 KB, patch)
2017-06-29 22:03 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (38.23 KB, patch)
2017-06-30 09:11 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (38.51 KB, patch)
2017-06-30 09:15 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-06-29 21:18:33 PDT
ResourceLoadObserver does not need a ResourceLoadStatisticsStore. ResourceLoadStatisticsStore is too complicated for its needs. ResourceLoadStatisticsStore can then be moved to WebKit2/UIProcess.
Comment 1 Chris Dumez 2017-06-29 21:20:24 PDT
Created attachment 314226 [details]
WIP Patch
Comment 2 Chris Dumez 2017-06-29 21:31:56 PDT
Created attachment 314227 [details]
WIP Patch
Comment 3 Chris Dumez 2017-06-29 22:03:40 PDT
Created attachment 314233 [details]
WIP Patch
Comment 4 Brent Fulgham 2017-06-29 23:22:08 PDT
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 5 Chris Dumez 2017-06-30 09:09:04 PDT
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.
Comment 6 Chris Dumez 2017-06-30 09:11:55 PDT
Created attachment 314269 [details]
Patch
Comment 7 Chris Dumez 2017-06-30 09:15:08 PDT
Created attachment 314271 [details]
Patch
Comment 8 Brent Fulgham 2017-06-30 11:15:39 PDT
Comment on attachment 314271 [details]
Patch

Looks great. r=me.
Comment 9 Chris Dumez 2017-06-30 11:18:27 PDT
Comment on attachment 314271 [details]
Patch

Clearing flags on attachment: 314271

Committed r219005: <http://trac.webkit.org/changeset/219005>
Comment 10 Chris Dumez 2017-06-30 11:18:29 PDT
All reviewed patches have been landed.  Closing bug.