Bug 132574 – clean up ResourceLoadTiming

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

[Alex Christensen]

ResourceLoadTiming has a bunch of values that seem to be in the Chromium WebKit api from https://bugs.webkit.org/show_bug.cgi?id=42091 but are not in the w3c spec at https://dvcs.w3.org/hg/webperf/raw-file/tip/specs/NavigationTiming/Overview.html and it uses int offsets from a double instead of storing all the times as doubles.  I store only what is needed in the spec.

[Alex Christensen]

Created attachment 230839 [details]
Patch

[Alex Christensen]

Created attachment 230943 [details]
Patch

[Alex Christensen]

Created attachment 230948 [details]
Patch

[Alex Christensen]

Created attachment 230955 [details]
Patch

[Alex Christensen]

Created attachment 230957 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 230957 [details]
Patch

Nice!. The Soup part looks good to me. Somebody else with more expertise in the area should review the changes in the core specially the changes related to the removal of resourceTimeToDocumentMilliseconds()

[Alex Christensen]

Created attachment 231004 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 231004 [details]
Patch

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

Yes, the situation with monotonic times doesn't seem right. Discussing in person.

> Source/WebCore/platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.mm:220
> +#if __has_include(<CFNetwork/CFNSURLConnection.h>)
> +        ResourceResponse resourceResponse(r);

Open source builds should not magically have the feature broken. What we should do is re-declare _timingData as a category, and have the code under if ENABLE(RESOURCE_TIMING).

> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:806
> +    

Spurious whitespace!

[Alex Christensen]

Also, it's not encoding the right ResourceLoadTiming.  I'm not sure where it's coming from or where it's supposed to come from.

[Alex Christensen]

Created attachment 231078 [details]
Patch

[Alex Christensen]

Created attachment 231079 [details]
Patch

[Alex Christensen]

Created attachment 231084 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 231084 [details]
Patch

This still doesn't ensure that all times are monotonic and relative to root navigation start. Discussed in person how this can be achieved (essentially keep more of the existing code).

[Alex Christensen]

Created attachment 231161 [details]
Patch

[Alex Christensen]

Created attachment 231164 [details]
Patch

[Alex Christensen]

Created attachment 231167 [details]
Patch

[Alex Christensen]

Created attachment 231170 [details]
Patch

[Alex Christensen]

Created attachment 231171 [details]
Patch

[Alex Christensen]

Created attachment 231181 [details]
Patch

[Alex Christensen]

Created attachment 231182 [details]
Patch

[Alex Christensen]

Created attachment 231183 [details]
Patch

[Alex Christensen]

Created attachment 231195 [details]
Patch

[Sam Weinig]

Comment on attachment 231195 [details]
Patch

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

> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:702
> +    ResourceLoadTiming* timing = resourceResponse.resourceLoadTiming();
> +    bool hasResourceLoadTiming = timing;
> +    encoder << hasResourceLoadTiming;
> +    if (hasResourceLoadTiming) {
> +        encoder << timing->domainLookupStart;
> +        encoder << timing->domainLookupEnd;
> +        encoder << timing->connectStart;
> +        encoder << timing->connectEnd;
> +        encoder << timing->requestStart;
> +        encoder << timing->responseStart;
> +        encoder << timing->secureConnectionStart;
> +    }

This should probably be wrapped in an #if ENABLE(WEB_TIMING).

> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:785
> +    bool hasResourceLoadTiming;
> +    if (!decoder.decode(hasResourceLoadTiming))
> +        return false;
> +    if (hasResourceLoadTiming) {
> +        response.setResourceLoadTiming(ResourceLoadTiming::create());
> +        ResourceLoadTiming* timing = response.resourceLoadTiming();
> +        if (!decoder.decode(timing->domainLookupStart)
> +            || !decoder.decode(timing->domainLookupEnd)
> +            || !decoder.decode(timing->connectStart)
> +            || !decoder.decode(timing->connectEnd)
> +            || !decoder.decode(timing->requestStart)
> +            || !decoder.decode(timing->responseStart)
> +            || !decoder.decode(timing->secureConnectionStart))
> +            return false;
> +    }

This should probably be wrapped in an #if ENABLE(WEB_TIMING).

[Alex Christensen]

Created attachment 231298 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 231298 [details]
Patch

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

> Source/WebCore/platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.mm:224
> +#if ENABLE(WEB_TIMING)

This is all needed in WebCoreResourceHandleAsDelegate.mm, too. There is no reason to have it work incorrectly in WebKitLegacy.

> Source/WebCore/platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.mm:226
> +        NSDictionary* timingData = [connection _timingData];
> +        if (timingData) {

Style nit: star goes to the other side for Objective-C, and you could combine the definition with if:

    if (NSDictionary *timingData = [connection _timingData])
        ...

> Source/WebCore/platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.mm:228
> +            ResourceLoadTiming* timing = resourceResponse.resourceLoadTiming();

It seems strange to see a pointer variable used without a null check, or any obvious reason why it can't be null. Can the function return a reference?

[Alexey Proskuryakov]

(r=me is conditional on fixing WebKitLegacy)

[Alex Christensen]

(In reply to comment #25)
> It seems strange to see a pointer variable used without a null check, or any obvious reason why it can't be null. Can the function return a reference?
In this case it was just created in the previous line.  I'll add an if(timing) just to be extra safe in case the creation failed for some reason or someone changes the behavior in the future.  It needs to be a pointer because not all ResourceResponse objects have a resourceLoadTiming.

[Alex Christensen]

Created attachment 231322 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 231322 [details]
Patch

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

> Source/WebCore/platform/network/mac/WebCoreResourceHandleAsDelegate.mm:199
> +        if (timing) {

You actually just create ResourceLoadTiming above, so the null check is misleading. The right way to do this is to use this idiom:

ResourceLoadTiming* resourceLoadTimingIfExists()
ResourceLoadTiming& resourceLoadTiming()

[Alexey Proskuryakov]

Comment on attachment 231322 [details]
Patch

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

> Source/WebCore/platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.mm:225
> +        if (NSDictionary *timingData = [connection _timingData]) {

All this code could be shared if it was in ResourceHandleMac.mm. You could call a function in ResourceHandle, which would then collect the data, and call the client.

[Alex Christensen]

I'm going to put this in and fix the duplicate code and reference problems in another patch.

http://trac.webkit.org/changeset/168647

[Alex Christensen]

I'm going to put this in and fix the duplicate code and reference problems in another patch.

http://trac.webkit.org/changeset/168647