Bug 173499 – Resource Load Statistics: Add telemetry

• Component: WebKit2
• Status: RESOLVED FIXED

[John Wilander]

Add telemetry. Only numbers on top prevalent resources, i.e. no domain names or other sensitive data such as timestamps.

[Radar WebKit Bug Importer]

<rdar://problem/32826094>

[John Wilander]

Created attachment 313161 [details]
Patch

[Chris Dumez]

Comment on attachment 313161 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313161&action=review

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:244
> +        m_fireTelementryHandler();

typo: m_fireTelementryHandler -> m_fireTelemetryHandler

> Source/WebKit2/UIProcess/WebProcessProxy.cpp:344
> +        page.value->postMessageToInjectedBundle("ResourceLoadStatisticsTelemetryFinished", messageBody);

Why doesn't this send an IPC to the WebProcess and have it iterate over the pages? This would reduce the amount of IPC.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:65
> +    m_telemetryTimer.start(5_s, WTF::Seconds::fromHours(24));

24_h ?

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:202
> +        m_telemetryTimer.startOneShot(WTF::Seconds::fromMilliseconds(100));

100_ms ?

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.h:113
> +    WebCore::Timer m_telemetryTimer;

We should not be using WebCore::Timer in the UIProcess. RunLoop::Timer is the one you want I believe.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:42
> +static unsigned minimumPrevalentResourcesForTelemetry = 3;

const unsigned minimumPrevalentResourcesForTelemetry = 3;

Should be const and does not need the static.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:46
> +    unsigned numberOfTimesDataRecordsRemoved;

I believe these members should have inline initializers.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:53
> +static inline Vector<PrevalentResourceTelemetry> sortPrevalentResources(Vector<WebCore::ResourceLoadStatistics>& resourceLoadStatistics)

Seems big to inline. The name is a bit weird because it does more than sorting. Maybe toSortedPrevalentResources()?

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:55
> +    Vector<PrevalentResourceTelemetry> sorted;

You can reverseInitialCapacity(resourceLoadStatistics.size());

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:57
> +        sorted.append(PrevalentResourceTelemetry {

You can uncheckedAppend().

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:67
> +        bool operator()(PrevalentResourceTelemetry a, PrevalentResourceTelemetry b) const

Shouldn't these be const PrevalentResourceTelemetry& ?

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:79
> +static inline unsigned withUserInteraction(Vector<PrevalentResourceTelemetry> v, unsigned begin, unsigned end)

Why are all these functions inline?

I doubt we should be passing the Vector by value here. Should likely be a const Vector&.

v is a bad name.

The name withUserInteraction() is really unclear. What does this method do?

Shouldn't we babe using size_t for those begin / end (same comment below).

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:85
> +    for (unsigned i = begin; i < end; i++) {

++i.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:86
> +        if (v.at(i).hasHadUserInteraction)

We prefer [I] when the vector is not a pointer.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:87
> +            result++;

++result;

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:93
> +static inline unsigned median(Vector<unsigned> v)

Again passing Vector by value.. and inline

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:98
> +        return v.at(0);

[0]

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:104
> +    return (v.at(middle - 1) + v.at(middle)) / 2;

[middle -1]

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:107
> +static inline unsigned median(Vector<PrevalentResourceTelemetry> v, unsigned begin, unsigned end, std::function<unsigned(const PrevalentResourceTelemetry telemetry)>&& statisticGetter)

Again passing Vector by value.. and inline.

Please use WTF::Function instead of std::function.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:113
> +    for (unsigned i = begin; i <= end; i++)

++i

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:114
> +        part.append(statisticGetter(v.at(i)));

[I]

[Chris Dumez]

Comment on attachment 313161 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313161&action=review

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:123
> +    Vector<WebCore::ResourceLoadStatistics> prevalentResources = resourceLoadStatisticsStore.prevalentResources();

auto

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:128
> +    Vector<PrevalentResourceTelemetry> sortedPrevalentResources = sortPrevalentResources(prevalentResources);

auto

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:131
> +    for (PrevalentResourceTelemetry prevalentResource : sortedPrevalentResources) {

auto&

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:133
> +            totalNumberOfPrevalentResourcesWithUserInteraction++;

++totalNumberOfPrevalentResourcesWithUserInteraction;

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:216
> +    if (processPool) {

This logic should probably do into a separate function.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:224
> +                goto foundNonEphemeralWebPageProxy;

Please no goto :(

[Chris Dumez]

Comment on attachment 313161 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313161&action=review

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:231
> +    nonEphemeralWebPageProxy->logDiagnosticMessageWithValue("totalNumberOfPrevalentResources", "Total number of prevalent resources.", prevalentResources.size(), 0, WebCore::ShouldSample::No);

These should really use ASCIILiteral()

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:232
> +    nonEphemeralWebPageProxy->logDiagnosticMessageWithValue("totalNumberOfPrevalentResourcesWithUserInteraction", "Total number of prevalent resources with user interaction.", totalNumberOfPrevalentResourcesWithUserInteraction, 0, WebCore::ShouldSample::No);

Does this really work when there are spaces in the keys?

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:240
> +        nonEphemeralWebPageProxy->logDiagnosticMessageWithValue("top3PrevalentResourcesWithUserInteraction", "Top  3: Number of prevalent resources with user interaction.", top3PrevalentResourcesWithUserInteraction, 0, WebCore::ShouldSample::No);

Double space between top and 3.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:272
> +        WKRetainPtr<WKMutableDictionaryRef> messageBody(AdoptWK, WKMutableDictionaryCreate());

Since what we really want is an API::Dictionary, I think we should construct an API::Dictionary and work at this level instead of using the WK C API.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:286
> +        RunLoop::main().dispatch([messageBody] () {

unnecessary ()

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.h:33
> +class WebResourceLoadStatisticsTelemetry {

Feels like this should be a namespace? This object has no state and I could be wrong but it looks like both methods should be static.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.h:35
> +    void calculateAndSubmit(WebCore::ResourceLoadStatisticsStore&);

can this be static too?

[Chris Dumez]

Comment on attachment 313161 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313161&action=review

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:138
> +    bool shouldDoTop3 = sortedPrevalentResourcesWithoutUserInteraction.size() >= 3;

Feel like it may be possible to refactor this code to use a loop and avoid a lot of code duplication. This seems unnecessarily huge.

[Chris Dumez]

Comment on attachment 313161 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313161&action=review

> LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:6
> +    <script src="../../resources/js-test-pre.js"></script>

<script src="/js-test-resources/js-test.js"></script>

> LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:20
> +        if (!enable) {

These curly brackets are not needed.

> LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:28
> +        setEnableFeature(false);

We should not rely on tests to reset state. The TestRunner should do this between tests. See TestController::resetStateToConsistentValues().

> LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:29
> +        testRunner.notifyDone();

finishJSTest();

> LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:52
> +        // Prevalent resource 1

Missing period at the end.

> LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:138
> +    testRunner.waitUntilDone();

jsTestIsAsync = true;

[John Wilander]

Created attachment 313448 [details]
Patch

[John Wilander]

Thanks, Chris, for your detailed review!

The only thing I left as-is is WebProcessProxy::notifyPageStatisticsTelemetryFinished(). It is only used for testing, clearly named for this purpose, and there is only one page when the TestRunner executes.

[Brent Fulgham]

Comment on attachment 313448 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313448&action=review

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:55
> +static Vector<PrevalentResourceTelemetry> toSortedPrevalentResources(Vector<WebCore::ResourceLoadStatistics>& resourceLoadStatistics)

resourceLoadStatistics should be a const&, since we do not (and should not!) mutate resourceLoadStatistics in this function.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:96
> +static unsigned median(Vector<unsigned>& v)

v should be a const&

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:115
> +    Vector<unsigned> part;

We should do a part.reserveInitialCapacity since these vectors might be large.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:125
> +    if (processPool) {

I would do an early return here: "if (!processPool) return nullptr"

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:141
> +    WTF::Function<unsigned(const PrevalentResourceTelemetry telemetry)> subframeUnderTopFrameOriginsGetter = [] (const PrevalentResourceTelemetry& t) {

These are all making const copies of telemetry. Shouldn't they be declared as "const PrevalentResourceTelemetry& telemetry)"?

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:158
> +    unsigned topNumberOfTimesDataRecordsRemoved = 0;

Delete the above 5 lines, and just declare and assign them in the 5 lines below.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:250
> +        return;

It would be nice if we could avoid making the copy of 'prevalentResources' if there aren't enough statistics to do the telemetry. I guess since that implies a small vector the cost is low, but it still seems wasteful.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:252
> +    auto sortedPrevalentResources = toSortedPrevalentResources(prevalentResources);

We always get the resources, then sort them. Seems like we should just return a sorted vector rather than make two copies of the same data. I guess this is necessary because the Telemetry concept is only in WK2, and we change from ResourceLoadStatistics to Telemetry objects. But if the Telemetry class was a WebCore concept, you could build them as you sorted the list, and avoid a copy of the vector contents.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:253
> +    Vector<PrevalentResourceTelemetry> sortedPrevalentResourcesWithoutUserInteraction;

We should do a sortedPrevalentResourcesWithoutUserInteraction.reserveInitialCapacity since these vectors might be large.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:254
> +    unsigned totalNumberOfPrevalentResourcesWithUserInteraction = 0;

You could also compute this value while making your vector of Telemetry objects! :-)

[Brent Fulgham]

Comment on attachment 313448 [details]
Patch

I think this is looking good, but it's not quite ready. Can you please address some of my suggestions? We should also get a second look from Chris.

[Chris Dumez]

Comment on attachment 313448 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313448&action=review

> Source/WebKit2/UIProcess/WebProcessProxy.cpp:344
> +        page.value->postMessageToInjectedBundle("ResourceLoadStatisticsTelemetryFinished", messageBody);

Can we instead send *one* IPC to the WebContent process and do the looping over the pages on the WebContent process side (i.e. calling WebPage::postInjectedBundleMessage() from a new method in WebProcess.cpp)

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:202
> +        // This cancels the one shot timer and is only intended for testing purposes.

Wasn't this supposed to cancel the *repeating* timer? At least that's what the previous patch iteration says.

It does not make much sense to call stop() before calling startOneShot() on the same timer I believe. startOneShot() will re-schedule the timer.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.h:113
> +    RunLoop::Timer<WebResourceLoadStatisticsStore> m_telemetryRepeatedTimer;

Do we really need 2 timers? Do they need to run at the same time?

>> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:55
>> +static Vector<PrevalentResourceTelemetry> toSortedPrevalentResources(Vector<WebCore::ResourceLoadStatistics>& resourceLoadStatistics)
> 
> resourceLoadStatistics should be a const&, since we do not (and should not!) mutate resourceLoadStatistics in this function.

+1

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:77
> +    std::sort(sorted.begin(), sorted.end(), comparator);

Let's just pass in a lambda function rather than constructing a struct with an operator() for this.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:88
> +    for (unsigned i = begin; i < end; ++i) {

size_t

>> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:96
>> +static unsigned median(Vector<unsigned>& v)
> 
> v should be a const&

+1

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:110
> +static unsigned median(const Vector<PrevalentResourceTelemetry>& v, unsigned begin, unsigned end, WTF::Function<unsigned(const PrevalentResourceTelemetry telemetry)>& statisticGetter)

statisticGetter should be const.
telemetry parameter to the WTF::Function should be const too.

>> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:115
>> +    Vector<unsigned> part;
> 
> We should do a part.reserveInitialCapacity since these vectors might be large.

+1

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:116
> +    for (unsigned i = begin; i <= end; ++i)

size_t

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:124
> +    auto processPool = WebProcessPool::allProcessPools().at(0);

[0]

Also, why no bounds checking before accessing? at(0) will crash if out of bounds rather than returning null. Which makes the null check below useless I believe.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:127
> +            if (!webProcess || !webProcess->pageCount())

This check looks useless to me. I don't think webProcess can be null. If webProcess->pageCount() is 0 then the loop below will be a no-op.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:130
> +                if (!page || page->sessionID().isEphemeral())

I don't believe the page can be null.

>> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:141
>> +    WTF::Function<unsigned(const PrevalentResourceTelemetry telemetry)> subframeUnderTopFrameOriginsGetter = [] (const PrevalentResourceTelemetry& t) {
> 
> These are all making const copies of telemetry. Shouldn't they be declared as "const PrevalentResourceTelemetry& telemetry)"?

+1

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:165
> +    WTF::StringBuilder preambleBuilder;

WTF:: seems redundant.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:166
> +    preambleBuilder.append("top");

appendLiteral()

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:169
> +    WTF::StringBuilder descriptionPreambleBuilder;

WTF:: seems redundant.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:170
> +    descriptionPreambleBuilder.append("Top ");

appendLiteral()

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:174
> +    if (!webPageProxy)

Isn't this too late to check if webPageProxy is null? What was the point of doing all this computation if we are no going to be able to log it?

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:178
> +        descriptionPreamble + ": Number of prevalent resources with user interaction.",

Is it really OK to have spaces in the description here? We've never done this before.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:194
> +static void submitTopLists(const Vector<PrevalentResourceTelemetry>& sortedPrevalentResources, const Vector<PrevalentResourceTelemetry>& sortedPrevalentResourcesWithoutUserInteraction, WebPageProxy* webPageProxy)

Should probably take a WebPageProxy by reference.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:218
> +    WKRetainPtr<WKMutableDictionaryRef> messageBody(AdoptWK, WKMutableDictionaryCreate());

Why are we using the WK API instead of constructing an API::Dictionary directly?

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:238
> +    RunLoop::main().dispatch([messageBody] {

Looks like you can WTFMove() messageBody here.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:243
> +void WebResourceLoadStatisticsTelemetry::calculateAndSubmit(WebCore::ResourceLoadStatisticsStore& resourceLoadStatisticsStore)

Can the parameter be const?

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:266
> +    webPageProxy->logDiagnosticMessageWithValue("totalNumberOfPrevalentResources", "Total number of prevalent resources.", prevalentResources.size(), 0, WebCore::ShouldSample::No);

Use ASCIILiteral() for those literals.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:269
> +    submitTopLists(sortedPrevalentResources, sortedPrevalentResourcesWithoutUserInteraction, webPageProxy);

*webPageProxy

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.h:28
> +#include <WebCore/ResourceLoadStatisticsStore.h>

Looks like this could be a forward declaration.

> Tools/WebKitTestRunner/TestController.cpp:841
> +    statisticsResetToConsistentState();

This seems reversed. Maybe resetLoadStatistics(); ?

> LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:2
> +<html lang="en">

Why do we need the lang?

> LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:10
> +    const topFrame1 = "http://127.0.0.1:8000/temp";

Would be nice to have a description("...."); for this test.

> LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:11
> +    const topFrame2 = "http://127.0.0.2:8000/temp";

Should those variable names have "URL" in them given their values?

> LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:14
> +    const prevalentResource1 = "http://127.0.1.1:8000/temp";

ditto.

> LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:25
> +        setEnableFeature(false);

Is this really needed? Can't we just remove this function and call finishJSTest() directly?

> LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:30
> +        if (!testRunner.isStatisticsPrevalentResource(prevalentResource1)) {

shouldBeTrue("testRunner.isStatisticsPrevalentResource(prevalentResource1)"); ?
ditto below.

> LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:48
> +    function setUpStatisticsAndContinue() {

Could we use a loop in here to reduce the code size?

> LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:124
> +        if (result.totalPrevalentResources === 4)

testResult = result;
shouldBe("testResult.totalPrevalentResources", "4");
Ditto below.

> LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:133
> +    setEnableFeature(true);

Not sure we need a function for this if this is one only once and with true as parameter.

> LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:135
> +    testRunner.installStatisticsDidRunTelemetryCallback(checkTelemetry);

if (window.testRunner) {
  ...

[John Wilander]

Thanks, Brent and Chris! I'll fix everything Brent brought up unless commented otherwise below. Also replied to Chris's questions.

(In reply to Chris Dumez from comment #12)
> Comment on attachment 313448 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313448&action=review
> 
> > Source/WebKit2/UIProcess/WebProcessProxy.cpp:344
> > +        page.value->postMessageToInjectedBundle("ResourceLoadStatisticsTelemetryFinished", messageBody);
> 
> Can we instead send *one* IPC to the WebContent process and do the looping
> over the pages on the WebContent process side (i.e. calling
> WebPage::postInjectedBundleMessage() from a new method in WebProcess.cpp)

I made a comment about this above. The function is only used in testing and there is only one page at that time.

> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:202
> > +        // This cancels the one shot timer and is only intended for testing purposes.
> 
> Wasn't this supposed to cancel the *repeating* timer? At least that's what
> the previous patch iteration says.

With WebCore::Timer you get both the initial 5_s shot and the repeated 24_h one. That does not seem to be possible with RunLoop::Timer. Therefore, I use two timers and only have to cancel the one-shot one.

> It does not make much sense to call stop() before calling startOneShot() on
> the same timer I believe. startOneShot() will re-schedule the timer.

Got it, will fix.

> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.h:113
> > +    RunLoop::Timer<WebResourceLoadStatisticsStore> m_telemetryRepeatedTimer;
> 
> Do we really need 2 timers? Do they need to run at the same time?

We need the 5_s timer for the case where the browser is launched and killed/quit often.
We need the 24_h timer for the case where the browser is long-lived.

I need two since RunLoop::Timer doesn't offer a way to start a repeated timer with an initial one-shot.

> >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:55
> >> +static Vector<PrevalentResourceTelemetry> toSortedPrevalentResources(Vector<WebCore::ResourceLoadStatistics>& resourceLoadStatistics)
> > 
> > resourceLoadStatistics should be a const&, since we do not (and should not!) mutate resourceLoadStatistics in this function.
> 
> +1
> 
> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:77
> > +    std::sort(sorted.begin(), sorted.end(), comparator);
> 
> Let's just pass in a lambda function rather than constructing a struct with
> an operator() for this.

OK.

> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:88
> > +    for (unsigned i = begin; i < end; ++i) {
> 
> size_t

Will fix.

> >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:96
> >> +static unsigned median(Vector<unsigned>& v)
> > 
> > v should be a const&
> 
> +1
> 
> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:110
> > +static unsigned median(const Vector<PrevalentResourceTelemetry>& v, unsigned begin, unsigned end, WTF::Function<unsigned(const PrevalentResourceTelemetry telemetry)>& statisticGetter)
> 
> statisticGetter should be const.
> telemetry parameter to the WTF::Function should be const too.

Will fix.

> >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:115
> >> +    Vector<unsigned> part;
> > 
> > We should do a part.reserveInitialCapacity since these vectors might be large.
> 
> +1
> 
> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:116
> > +    for (unsigned i = begin; i <= end; ++i)
> 
> size_t

Will fix.

> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:124
> > +    auto processPool = WebProcessPool::allProcessPools().at(0);
> 
> [0]

Will fix.

> Also, why no bounds checking before accessing? at(0) will crash if out of
> bounds rather than returning null. Which makes the null check below useless
> I believe.

Will fix.

> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:127
> > +            if (!webProcess || !webProcess->pageCount())
> 
> This check looks useless to me. I don't think webProcess can be null. If
> webProcess->pageCount() is 0 then the loop below will be a no-op.

I don't know if webProcess can be null so I have the habit of null checking. I can remove it if we don't think it's necessary but this is not a hot code path.

> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:130
> > +                if (!page || page->sessionID().isEphemeral())
> 
> I don't believe the page can be null.

See comment on webProcess above.

> >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:141
> >> +    WTF::Function<unsigned(const PrevalentResourceTelemetry telemetry)> subframeUnderTopFrameOriginsGetter = [] (const PrevalentResourceTelemetry& t) {
> > 
> > These are all making const copies of telemetry. Shouldn't they be declared as "const PrevalentResourceTelemetry& telemetry)"?
> 
> +1
> 
> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:165
> > +    WTF::StringBuilder preambleBuilder;
> 
> WTF:: seems redundant.

Will fix. Might have been Xcode auto-complete. :P

> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:166
> > +    preambleBuilder.append("top");
> 
> appendLiteral()

Will fix.

> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:169
> > +    WTF::StringBuilder descriptionPreambleBuilder;
> 
> WTF:: seems redundant.

Will fix.

> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:170
> > +    descriptionPreambleBuilder.append("Top ");
> 
> appendLiteral()

Will fix.

> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:174
> > +    if (!webPageProxy)
> 
> Isn't this too late to check if webPageProxy is null? What was the point of
> doing all this computation if we are no going to be able to log it?

I'm doing the check as close to the use as possible if it has been invalidated since I got the pointer. The reason I get the pointer so much earlier is to be able to return early if we only have ephemeral pages.

> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:178
> > +        descriptionPreamble + ": Number of prevalent resources with user interaction.",
> 
> Is it really OK to have spaces in the description here? We've never done
> this before.

I'll check documentation.

> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:194
> > +static void submitTopLists(const Vector<PrevalentResourceTelemetry>& sortedPrevalentResources, const Vector<PrevalentResourceTelemetry>& sortedPrevalentResourcesWithoutUserInteraction, WebPageProxy* webPageProxy)
> 
> Should probably take a WebPageProxy by reference.

The reason for this is to check it later. Are you saying I should rely on webPageProxy staying valid?

> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:218
> > +    WKRetainPtr<WKMutableDictionaryRef> messageBody(AdoptWK, WKMutableDictionaryCreate());
> 
> Why are we using the WK API instead of constructing an API::Dictionary
> directly?

Sorry, missed this in your previous comments. This is code only used in the test case but I can change it.

> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:238
> > +    RunLoop::main().dispatch([messageBody] {
> 
> Looks like you can WTFMove() messageBody here.

Will fix.

> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:243
> > +void WebResourceLoadStatisticsTelemetry::calculateAndSubmit(WebCore::ResourceLoadStatisticsStore& resourceLoadStatisticsStore)
> 
> Can the parameter be const?

Will fix.

> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:266
> > +    webPageProxy->logDiagnosticMessageWithValue("totalNumberOfPrevalentResources", "Total number of prevalent resources.", prevalentResources.size(), 0, WebCore::ShouldSample::No);
> 
> Use ASCIILiteral() for those literals.

Will fix.

> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:269
> > +    submitTopLists(sortedPrevalentResources, sortedPrevalentResourcesWithoutUserInteraction, webPageProxy);
> 
> *webPageProxy

Will fix.

> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.h:28
> > +#include <WebCore/ResourceLoadStatisticsStore.h>
> 
> Looks like this could be a forward declaration.

Will fix.

> > Tools/WebKitTestRunner/TestController.cpp:841
> > +    statisticsResetToConsistentState();
> 
> This seems reversed. Maybe resetLoadStatistics(); ?

I have preampled all these functions with statistics to make them consistent and scoped in name. We have like 20-30 test functions for this feature.

> > LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:2
> > +<html lang="en">
> 
> Why do we need the lang?

That's just my IDE.

> > LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:10
> > +    const topFrame1 = "http://127.0.0.1:8000/temp";
> 
> Would be nice to have a description("...."); for this test.

Will fix.

> > LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:11
> > +    const topFrame2 = "http://127.0.0.2:8000/temp";
> 
> Should those variable names have "URL" in them given their values?

I could add that.

> > LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:14
> > +    const prevalentResource1 = "http://127.0.1.1:8000/temp";
> 
> ditto.
> 
> > LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:25
> > +        setEnableFeature(false);
> 
> Is this really needed? Can't we just remove this function and call
> finishJSTest() directly?

Maybe nowadays. Many of the tests were written when the feature was off by default and we didn't want to leave it on after the test was run.

> > LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:30
> > +        if (!testRunner.isStatisticsPrevalentResource(prevalentResource1)) {
> 
> shouldBeTrue("testRunner.isStatisticsPrevalentResource(prevalentResource1)");
> ?
> ditto below.

Will fix.

> > LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:48
> > +    function setUpStatisticsAndContinue() {
> 
> Could we use a loop in here to reduce the code size?

Will fix.

> > LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:124
> > +        if (result.totalPrevalentResources === 4)
> 
> testResult = result;
> shouldBe("testResult.totalPrevalentResources", "4");
> Ditto below.

Will fix.

> > LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:133
> > +    setEnableFeature(true);
> 
> Not sure we need a function for this if this is one only once and with true
> as parameter.

Yeah, if we remove the call with 'false' then I agree.

> > LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:135
> > +    testRunner.installStatisticsDidRunTelemetryCallback(checkTelemetry);
> 
> if (window.testRunner) {
>   ...

Will fix.

[Chris Dumez]

(In reply to John Wilander from comment #13)
> Thanks, Brent and Chris! I'll fix everything Brent brought up unless
> commented otherwise below. Also replied to Chris's questions.
> 
> (In reply to Chris Dumez from comment #12)
> > Comment on attachment 313448 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=313448&action=review
> > 
> > > Source/WebKit2/UIProcess/WebProcessProxy.cpp:344
> > > +        page.value->postMessageToInjectedBundle("ResourceLoadStatisticsTelemetryFinished", messageBody);
> > 
> > Can we instead send *one* IPC to the WebContent process and do the looping
> > over the pages on the WebContent process side (i.e. calling
> > WebPage::postInjectedBundleMessage() from a new method in WebProcess.cpp)
> 
> I made a comment about this above. The function is only used in testing and
> there is only one page at that time.

Still not more code AFAICT and better practice. I'd rather this got fixed.

> 
> > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:202
> > > +        // This cancels the one shot timer and is only intended for testing purposes.
> > 
> > Wasn't this supposed to cancel the *repeating* timer? At least that's what
> > the previous patch iteration says.
> 
> With WebCore::Timer you get both the initial 5_s shot and the repeated 24_h
> one. That does not seem to be possible with RunLoop::Timer. Therefore, I use
> two timers and only have to cancel the one-shot one.
> 
> > It does not make much sense to call stop() before calling startOneShot() on
> > the same timer I believe. startOneShot() will re-schedule the timer.
> 
> Got it, will fix.
> 
> > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.h:113
> > > +    RunLoop::Timer<WebResourceLoadStatisticsStore> m_telemetryRepeatedTimer;
> > 
> > Do we really need 2 timers? Do they need to run at the same time?
> 
> We need the 5_s timer for the case where the browser is launched and
> killed/quit often.
> We need the 24_h timer for the case where the browser is long-lived.
> 
> I need two since RunLoop::Timer doesn't offer a way to start a repeated
> timer with an initial one-shot.
> 
> > >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:55
> > >> +static Vector<PrevalentResourceTelemetry> toSortedPrevalentResources(Vector<WebCore::ResourceLoadStatistics>& resourceLoadStatistics)
> > > 
> > > resourceLoadStatistics should be a const&, since we do not (and should not!) mutate resourceLoadStatistics in this function.
> > 
> > +1
> > 
> > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:77
> > > +    std::sort(sorted.begin(), sorted.end(), comparator);
> > 
> > Let's just pass in a lambda function rather than constructing a struct with
> > an operator() for this.
> 
> OK.
> 
> > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:88
> > > +    for (unsigned i = begin; i < end; ++i) {
> > 
> > size_t
> 
> Will fix.
> 
> > >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:96
> > >> +static unsigned median(Vector<unsigned>& v)
> > > 
> > > v should be a const&
> > 
> > +1
> > 
> > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:110
> > > +static unsigned median(const Vector<PrevalentResourceTelemetry>& v, unsigned begin, unsigned end, WTF::Function<unsigned(const PrevalentResourceTelemetry telemetry)>& statisticGetter)
> > 
> > statisticGetter should be const.
> > telemetry parameter to the WTF::Function should be const too.
> 
> Will fix.
> 
> > >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:115
> > >> +    Vector<unsigned> part;
> > > 
> > > We should do a part.reserveInitialCapacity since these vectors might be large.
> > 
> > +1
> > 
> > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:116
> > > +    for (unsigned i = begin; i <= end; ++i)
> > 
> > size_t
> 
> Will fix.
> 
> > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:124
> > > +    auto processPool = WebProcessPool::allProcessPools().at(0);
> > 
> > [0]
> 
> Will fix.
> 
> > Also, why no bounds checking before accessing? at(0) will crash if out of
> > bounds rather than returning null. Which makes the null check below useless
> > I believe.
> 
> Will fix.
> 
> > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:127
> > > +            if (!webProcess || !webProcess->pageCount())
> > 
> > This check looks useless to me. I don't think webProcess can be null. If
> > webProcess->pageCount() is 0 then the loop below will be a no-op.
> 
> I don't know if webProcess can be null so I have the habit of null checking.
> I can remove it if we don't think it's necessary but this is not a hot code
> path.

This should not be null. I think we should remove unnecessary null checks, even if it is not hot code. This makes the code larger unnecessarily.

> 
> > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:130
> > > +                if (!page || page->sessionID().isEphemeral())
> > 
> > I don't believe the page can be null.
> 
> See comment on webProcess above.
> 
> > >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:141
> > >> +    WTF::Function<unsigned(const PrevalentResourceTelemetry telemetry)> subframeUnderTopFrameOriginsGetter = [] (const PrevalentResourceTelemetry& t) {
> > > 
> > > These are all making const copies of telemetry. Shouldn't they be declared as "const PrevalentResourceTelemetry& telemetry)"?
> > 
> > +1
> > 
> > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:165
> > > +    WTF::StringBuilder preambleBuilder;
> > 
> > WTF:: seems redundant.
> 
> Will fix. Might have been Xcode auto-complete. :P
> 
> > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:166
> > > +    preambleBuilder.append("top");
> > 
> > appendLiteral()
> 
> Will fix.
> 
> > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:169
> > > +    WTF::StringBuilder descriptionPreambleBuilder;
> > 
> > WTF:: seems redundant.
> 
> Will fix.
> 
> > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:170
> > > +    descriptionPreambleBuilder.append("Top ");
> > 
> > appendLiteral()
> 
> Will fix.
> 
> > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:174
> > > +    if (!webPageProxy)
> > 
> > Isn't this too late to check if webPageProxy is null? What was the point of
> > doing all this computation if we are no going to be able to log it?
> 
> I'm doing the check as close to the use as possible if it has been
> invalidated since I got the pointer. The reason I get the pointer so much
> earlier is to be able to return early if we only have ephemeral pages.

You already have a null check before calling those functions. After doing the null check, I think we should start passing the page by reference and not do any null check downstream. Not sure what you mean by this pointer getting invalidated, nothing here seems like it could reset it to null.

> 
> > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:178
> > > +        descriptionPreamble + ": Number of prevalent resources with user interaction.",
> > 
> > Is it really OK to have spaces in the description here? We've never done
> > this before.
> 
> I'll check documentation.
> 
> > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:194
> > > +static void submitTopLists(const Vector<PrevalentResourceTelemetry>& sortedPrevalentResources, const Vector<PrevalentResourceTelemetry>& sortedPrevalentResourcesWithoutUserInteraction, WebPageProxy* webPageProxy)
> > 
> > Should probably take a WebPageProxy by reference.
> 
> The reason for this is to check it later. Are you saying I should rely on
> webPageProxy staying valid?
> 
> > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:218
> > > +    WKRetainPtr<WKMutableDictionaryRef> messageBody(AdoptWK, WKMutableDictionaryCreate());
> > 
> > Why are we using the WK API instead of constructing an API::Dictionary
> > directly?
> 
> Sorry, missed this in your previous comments. This is code only used in the
> test case but I can change it.
> 
> > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:238
> > > +    RunLoop::main().dispatch([messageBody] {
> > 
> > Looks like you can WTFMove() messageBody here.
> 
> Will fix.
> 
> > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:243
> > > +void WebResourceLoadStatisticsTelemetry::calculateAndSubmit(WebCore::ResourceLoadStatisticsStore& resourceLoadStatisticsStore)
> > 
> > Can the parameter be const?
> 
> Will fix.
> 
> > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:266
> > > +    webPageProxy->logDiagnosticMessageWithValue("totalNumberOfPrevalentResources", "Total number of prevalent resources.", prevalentResources.size(), 0, WebCore::ShouldSample::No);
> > 
> > Use ASCIILiteral() for those literals.
> 
> Will fix.
> 
> > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:269
> > > +    submitTopLists(sortedPrevalentResources, sortedPrevalentResourcesWithoutUserInteraction, webPageProxy);
> > 
> > *webPageProxy
> 
> Will fix.
> 
> > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.h:28
> > > +#include <WebCore/ResourceLoadStatisticsStore.h>
> > 
> > Looks like this could be a forward declaration.
> 
> Will fix.
> 
> > > Tools/WebKitTestRunner/TestController.cpp:841
> > > +    statisticsResetToConsistentState();
> > 
> > This seems reversed. Maybe resetLoadStatistics(); ?
> 
> I have preampled all these functions with statistics to make them consistent
> and scoped in name. We have like 20-30 test functions for this feature.
> 
> > > LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:2
> > > +<html lang="en">
> > 
> > Why do we need the lang?
> 
> That's just my IDE.
> 
> > > LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:10
> > > +    const topFrame1 = "http://127.0.0.1:8000/temp";
> > 
> > Would be nice to have a description("...."); for this test.
> 
> Will fix.
> 
> > > LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:11
> > > +    const topFrame2 = "http://127.0.0.2:8000/temp";
> > 
> > Should those variable names have "URL" in them given their values?
> 
> I could add that.
> 
> > > LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:14
> > > +    const prevalentResource1 = "http://127.0.1.1:8000/temp";
> > 
> > ditto.
> > 
> > > LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:25
> > > +        setEnableFeature(false);
> > 
> > Is this really needed? Can't we just remove this function and call
> > finishJSTest() directly?
> 
> Maybe nowadays. Many of the tests were written when the feature was off by
> default and we didn't want to leave it on after the test was run.

Now that this patch supposedly reset state between tests in WKTR, we should not need this. Otherwise your resetState is wrong (e.g. it fails to turn off feature after the test). We should NEVER rely on tests to reset state as they may fail.

> 
> > > LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:30
> > > +        if (!testRunner.isStatisticsPrevalentResource(prevalentResource1)) {
> > 
> > shouldBeTrue("testRunner.isStatisticsPrevalentResource(prevalentResource1)");
> > ?
> > ditto below.
> 
> Will fix.
> 
> > > LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:48
> > > +    function setUpStatisticsAndContinue() {
> > 
> > Could we use a loop in here to reduce the code size?
> 
> Will fix.
> 
> > > LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:124
> > > +        if (result.totalPrevalentResources === 4)
> > 
> > testResult = result;
> > shouldBe("testResult.totalPrevalentResources", "4");
> > Ditto below.
> 
> Will fix.
> 
> > > LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:133
> > > +    setEnableFeature(true);
> > 
> > Not sure we need a function for this if this is one only once and with true
> > as parameter.
> 
> Yeah, if we remove the call with 'false' then I agree.
> 
> > > LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:135
> > > +    testRunner.installStatisticsDidRunTelemetryCallback(checkTelemetry);
> > 
> > if (window.testRunner) {
> >   ...
> 
> Will fix.

[John Wilander]

Created attachment 313735 [details]
Patch

[John Wilander]

Created attachment 313742 [details]
Patch

[John Wilander]

Hopefully fixed Windows build error.

[John Wilander]

Created attachment 313746 [details]
Patch

[John Wilander]

Resolved conflict from https://trac.webkit.org/changeset/218758/webkit.

[John Wilander]

Created attachment 313759 [details]
Patch

[John Wilander]

Windows build still not happy. New try.

[John Wilander]

Created attachment 313857 [details]
Patch

[John Wilander]

Fixed the merge and added testing for insufficient statistics.

[Brent Fulgham]

Comment on attachment 313857 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313857&action=review

> LayoutTests/platform/wk2/TestExpectations:708
> +http/tests/loading/resourceLoadStatistics/telemetry-generation.html [ Pass ]

We should only need this line if it is skipped or marked as "Fail" at the top level of LayoutTests. Is that the case here? It doesn't seem to be in this patch.

I suspect you can get rid of this line.

[John Wilander]

Thanks for all the review comments, Chris and Brent! I appreciate it.

Brent, the whole http/tests/loading/resourceLoadStatistics/ directory is skipped by default. That's why these are enabled here. Some tests are also dependent on CFNetwork.

[Brent Fulgham]

(In reply to John Wilander from comment #25)
> Thanks for all the review comments, Chris and Brent! I appreciate it.
> 
> Brent, the whole http/tests/loading/resourceLoadStatistics/ directory is
> skipped by default. That's why these are enabled here. Some tests are also
> dependent on CFNetwork.

Ah! Okay -- then your patch is perfect as-is.

[Chris Dumez]

Comment on attachment 313857 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313857&action=review

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:384
> +        if (statistic.isPrevalentResource) {

Is it common for resources to be prevalent? I am wondering because of the sorted.reserveInitialCapacity() call.

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:399
> +        sorted.clear();

You could just return { }.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:203
> +        m_telemetryOneShotTimer.stop();

As commented previously, why do we need to stop the timer before calling startOneShot()?

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:81
> +        part.append(statisticGetter(v[i]));

uncheckedAppend() ?

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:132
> +    webPageProxy.logDiagnosticMessageWithValue(DiagnosticLoggingKeys::resourceLoadStatisticsTelemetryKey(), descriptionPreamble + ASCIILiteral("PrevalentResourcesWithUserInteraction"),

I don't think we should/need use ASCIILiteral() in a + operation.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:168
> +    API::Dictionary::MapType messageBody;

Why cannot we do this *in* the RunLoop::main() dispatch?

[Chris Dumez]

Comment on attachment 313857 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313857&action=review

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:187
> +    API::Dictionary::MapType messageBody;

Looks like we could construct this in the dispatch() and merely capture the few integers we need?

Also, it seems that notifyPagesThatNothingWasDone() could be renamed (and take some parameters) to be reused here and avoid code duplication.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:208
> +    sortedPrevalentResourcesWithoutUserInteraction.reserveInitialCapacity(sortedPrevalentResources.size());

We normally use these reserveInitialCapacity() with close to accurate sizes. In this case, I am not sure the optimization is worth it given it will go in either of those 2 vectors..

> LayoutTests/http/tests/loading/resourceLoadStatistics/telemetry-generation.html:112
> +        jsTestIsAsync = true;

We usually put this right after the description().

[John Wilander]

Created attachment 313864 [details]
Patch

[John Wilander]

Trying to fix the Windows build.

[Chris Dumez]

(In reply to John Wilander from comment #30)
> Trying to fix the Windows build.

Please see review comments too.

[John Wilander]

Created attachment 313890 [details]
Patch

[John Wilander]

Another stab at the Windows build issue. Also fixed Chris's last requests.

[John Wilander]

Created attachment 313892 [details]
Patch

[John Wilander]

Comment on attachment 313892 [details]
Patch

Style checker bug filed here: https://bugs.webkit.org/show_bug.cgi?id=173859

[Build Bot]

Attachment 313892 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:397:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

Comment on attachment 313892 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313892&action=review

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:392
> +                statistic.subresourceUniqueRedirectsTo.size()

You might just need to make a constructor to keep Visual Studio happy here.

[John Wilander]

Created attachment 313923 [details]
Patch

[John Wilander]

Added constructors to the struct to see if I can get the Windows build working.

Also found the style checker issue. It was Xcode that had indented 8 chars which confused the checker.

[Brent Fulgham]

Comment on attachment 313923 [details]
Patch

Finally green! r=me.

[John Wilander]

Comment on attachment 313923 [details]
Patch

Thanks!

[WebKit Commit Bot]

Comment on attachment 313923 [details]
Patch

Clearing flags on attachment: 313923

Committed r218841: <http://trac.webkit.org/changeset/218841>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Lewis]

The test http/tests/loading/resourceLoadStatistics/telemetry-generation.html started failing on all Release builds.
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Floading%2FresourceLoadStatistics%2Ftelemetry-generation.html

https://build.webkit.org/results/Apple%20iOS%2010%20Simulator%20Release%20WK2%20(Tests)/r218841%20(2554)/results.html
https://build.webkit.org/builders/Apple%20iOS%2010%20Simulator%20Release%20WK2%20(Tests)/builds/2554

--- /Volumes/Data/slave/ios-simulator-10-release-tests-wk2/build/layout-test-results/http/tests/loading/resourceLoadStatistics/telemetry-generation-expected.txt
+++ /Volumes/Data/slave/ios-simulator-10-release-tests-wk2/build/layout-test-results/http/tests/loading/resourceLoadStatistics/telemetry-generation-actual.txt
@@ -14,10 +14,11 @@
 PASS testResult.totalPrevalentResourcesWithUserInteraction is 0
 PASS testResult.top3SubframeUnderTopFrameOrigins is 0
 PASS Hosts classified as prevalent resources.
-PASS testResult.totalPrevalentResources is 4
-PASS testResult.totalPrevalentResourcesWithUserInteraction is 1
-PASS testResult.top3SubframeUnderTopFrameOrigins is 4
+FAIL testResult.totalPrevalentResources should be 4. Was 0.
+FAIL testResult.totalPrevalentResourcesWithUserInteraction should be 1. Was 0.
+FAIL testResult.top3SubframeUnderTopFrameOrigins should be 4. Was 0.
 PASS successfullyParsed is true
+Some tests failed.
 
 TEST COMPLETE

[Matt Lewis]

Marked test as flaky after speaking with John.
https://trac.webkit.org/changeset/218854/webkit

[Matt Lewis]

(In reply to Matt Lewis from comment #45)
> Marked test as flaky after speaking with John.
> https://trac.webkit.org/changeset/218854/webkit

Reopened bug due to failing test.

[Matt Lewis]

(In reply to Matt Lewis from comment #46)
> (In reply to Matt Lewis from comment #45)
> > Marked test as flaky after speaking with John.
> > https://trac.webkit.org/changeset/218854/webkit
> 
> Reopened bug due to failing test.

Adjusted expectations as test is failing on all testers.
https://trac.webkit.org/changeset/218882/webkit

[John Wilander]

I have found the issue. It's a pure testing thing. The non-testing part of this fires a telemetry timer 5 seconds after launch. I cancel it when setting my test callback. But sometimes that happens too late and so the regular timer fires, there are no statistics set up by the test code, and we get a test failure.

I'm working on a follow-up patch that allows the TestController to turn off telemetry for the regular timers. That should be good for not submitting telemetry during test runs anyway.

[John Wilander]

The fix for the failing test is handled in https://bugs.webkit.org/show_bug.cgi?id=173940. Resolving this one.