Bug 132574 - clean up ResourceLoadTiming
Summary: clean up ResourceLoadTiming
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.9
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-05-05 11:08 PDT by Alex Christensen
Modified: 2014-05-12 14:39 PDT (History)
18 users (show)

See Also:


Attachments
Patch (28.95 KB, patch)
2014-05-05 11:31 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (41.21 KB, patch)
2014-05-06 15:47 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (53.46 KB, patch)
2014-05-06 16:43 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (53.61 KB, patch)
2014-05-06 17:17 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (53.60 KB, patch)
2014-05-06 17:24 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (54.13 KB, patch)
2014-05-07 10:02 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (54.45 KB, patch)
2014-05-08 10:35 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (54.45 KB, patch)
2014-05-08 10:47 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (54.55 KB, patch)
2014-05-08 11:47 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (45.08 KB, patch)
2014-05-09 10:23 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (45.22 KB, patch)
2014-05-09 10:40 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (45.75 KB, patch)
2014-05-09 11:08 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (45.21 KB, patch)
2014-05-09 11:24 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (45.22 KB, patch)
2014-05-09 11:41 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (45.24 KB, patch)
2014-05-09 13:10 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (46.78 KB, patch)
2014-05-09 13:16 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (46.78 KB, patch)
2014-05-09 13:20 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (45.58 KB, patch)
2014-05-09 16:37 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (45.60 KB, patch)
2014-05-12 08:50 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (48.87 KB, patch)
2014-05-12 13:57 PDT, Alex Christensen
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2014-05-05 11:08:11 PDT
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.
Comment 1 Alex Christensen 2014-05-05 11:31:10 PDT
Created attachment 230839 [details]
Patch
Comment 2 Alex Christensen 2014-05-06 15:47:39 PDT
Created attachment 230943 [details]
Patch
Comment 3 Alex Christensen 2014-05-06 16:43:24 PDT
Created attachment 230948 [details]
Patch
Comment 4 Alex Christensen 2014-05-06 17:17:17 PDT
Created attachment 230955 [details]
Patch
Comment 5 Alex Christensen 2014-05-06 17:24:14 PDT
Created attachment 230957 [details]
Patch
Comment 6 Sergio Villar Senin 2014-05-07 03:08:47 PDT
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()
Comment 7 Alex Christensen 2014-05-07 10:02:50 PDT
Created attachment 231004 [details]
Patch
Comment 8 Alexey Proskuryakov 2014-05-07 14:11:18 PDT
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!
Comment 9 Alex Christensen 2014-05-07 18:01:04 PDT
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.
Comment 10 Alex Christensen 2014-05-08 10:35:57 PDT
Created attachment 231078 [details]
Patch
Comment 11 Alex Christensen 2014-05-08 10:47:36 PDT
Created attachment 231079 [details]
Patch
Comment 12 Alex Christensen 2014-05-08 11:47:17 PDT
Created attachment 231084 [details]
Patch
Comment 13 Alexey Proskuryakov 2014-05-08 14:11:09 PDT
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).
Comment 14 Alex Christensen 2014-05-09 10:23:06 PDT
Created attachment 231161 [details]
Patch
Comment 15 Alex Christensen 2014-05-09 10:40:31 PDT
Created attachment 231164 [details]
Patch
Comment 16 Alex Christensen 2014-05-09 11:08:20 PDT
Created attachment 231167 [details]
Patch
Comment 17 Alex Christensen 2014-05-09 11:24:53 PDT
Created attachment 231170 [details]
Patch
Comment 18 Alex Christensen 2014-05-09 11:41:05 PDT
Created attachment 231171 [details]
Patch
Comment 19 Alex Christensen 2014-05-09 13:10:57 PDT
Created attachment 231181 [details]
Patch
Comment 20 Alex Christensen 2014-05-09 13:16:17 PDT
Created attachment 231182 [details]
Patch
Comment 21 Alex Christensen 2014-05-09 13:20:46 PDT
Created attachment 231183 [details]
Patch
Comment 22 Alex Christensen 2014-05-09 16:37:42 PDT
Created attachment 231195 [details]
Patch
Comment 23 Sam Weinig 2014-05-10 13:46:09 PDT
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).
Comment 24 Alex Christensen 2014-05-12 08:50:02 PDT
Created attachment 231298 [details]
Patch
Comment 25 Alexey Proskuryakov 2014-05-12 11:11:30 PDT
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?
Comment 26 Alexey Proskuryakov 2014-05-12 11:12:03 PDT
(r=me is conditional on fixing WebKitLegacy)
Comment 27 Alex Christensen 2014-05-12 11:24:41 PDT
(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.
Comment 28 Alex Christensen 2014-05-12 13:57:06 PDT
Created attachment 231322 [details]
Patch
Comment 29 Alexey Proskuryakov 2014-05-12 14:21:04 PDT
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()
Comment 30 Alexey Proskuryakov 2014-05-12 14:25:44 PDT
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.
Comment 31 Alex Christensen 2014-05-12 14:39:00 PDT
I'm going to put this in and fix the duplicate code and reference problems in another patch.

http://trac.webkit.org/changeset/168647
Comment 32 Alex Christensen 2014-05-12 14:39:54 PDT
I'm going to put this in and fix the duplicate code and reference problems in another patch.

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