Bug 152285 - Implement Web Timing when using NETWORK_SESSION
Summary: Implement Web Timing when using NETWORK_SESSION
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-12-14 17:33 PST by Alex Christensen
Modified: 2016-02-12 13:52 PST (History)
0 users

See Also:


Attachments
Patch (17.36 KB, patch)
2015-12-14 17:37 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (15.15 KB, patch)
2015-12-14 17:54 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (21.82 KB, patch)
2015-12-15 15:22 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (21.86 KB, patch)
2015-12-15 15:32 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (21.89 KB, patch)
2015-12-15 15:40 PST, Alex Christensen
darin: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-yosemite (898.19 KB, application/zip)
2015-12-15 18:59 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2015-12-14 17:33:24 PST
Implement Web Timing when using NETWORK_SESSION
Comment 1 Alex Christensen 2015-12-14 17:37:32 PST
Created attachment 267331 [details]
Patch
Comment 2 Alex Christensen 2015-12-14 17:54:31 PST
Created attachment 267333 [details]
Patch
Comment 3 Darin Adler 2015-12-14 22:07:44 PST
Comment on attachment 267333 [details]
Patch

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

> Source/WebCore/platform/network/ResourceLoadTiming.h:30
> +OBJC_CLASS NSDictionary;

This is a Foundation-only concept, should not be in platform-independent code without a guard.

> Source/WebCore/platform/network/ResourceLoadTiming.h:99
> +WEBCORE_EXPORT void copyTimingData(NSDictionary *timingData, ResourceLoadTiming&);

NSDictionary is a Foundation-only concept, should not be in platform-independent code without a guard.

> Source/WebCore/platform/network/cocoa/ResourceLoadTiming.mm:38
> +    double referenceStart = [[timingData valueForKey:@"_kCFNTimingDataFetchStart"] doubleValue];

Some mistakes in the code we are moving:

- This will yield an unpredictable values if there is nothing in the dictionary since calling doubleValue on a nil gives unpredictable results. Do we have a guarantee that there are values for all these keys?
- This should be using objectForKey:, not valueForKey:, right?

> Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:220
> +    const int64_t TimingDataOptionsEnableW3CNavigationTiming = (1 << 0);

Why is it safe to define this constant here?
Comment 4 Alex Christensen 2015-12-15 15:22:18 PST
Created attachment 267398 [details]
Patch
Comment 5 Alex Christensen 2015-12-15 15:32:46 PST
Created attachment 267401 [details]
Patch
Comment 6 Alex Christensen 2015-12-15 15:40:46 PST
Created attachment 267402 [details]
Patch
Comment 7 Build Bot 2015-12-15 18:59:42 PST
Comment on attachment 267402 [details]
Patch

Attachment 267402 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/564812

New failing tests:
imported/w3c/web-platform-tests/streams-api/readable-streams/garbage-collection.html
Comment 8 Build Bot 2015-12-15 18:59:44 PST
Created attachment 267420 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 9 Darin Adler 2015-12-16 09:07:18 PST
Comment on attachment 267402 [details]
Patch

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

> Source/WebCore/platform/network/ResourceLoadTiming.h:106
> +#if PLATFORM(COCOA)
> +WEBCORE_EXPORT void copyTimingData(NSDictionary *timingData, ResourceLoadTiming&);
> +#if !HAVE(TIMINGDATAOPTIONS)
> +WEBCORE_EXPORT void setCollectsTimingData();
> +#endif
> +#endif

I suggest doing these in separate paragraphs because I think the nesting is confusing.

    #if PLATFORM(COCOA)
    WEBCORE_EXPORT void copyTimingData(NSDictionary *timingData, ResourceLoadTiming&);
    #endif

    #if PLATFORM(COCOA) && !HAVE(TIMINGDATAOPTIONS)
    WEBCORE_EXPORT void setCollectsTimingData();
    #endif

I’ve had to untangle other headers by doing this before and it always seems to improve readability.

> Source/WebCore/platform/network/cocoa/ResourceLoadTiming.mm:36
> +    if (!timingData)
> +        return 0.0;

Don’t need this check. If timingData is nil then objectForKey: will return nil. I suggest omitting this code.

> Source/WebCore/platform/network/cocoa/ResourceLoadTiming.mm:74
> +        [NSURLConnection _collectTimingDataWithOptions: TimingDataCollectionNStatsOff | TimingDataCollectionConnectionDataOff];

Space after ":" here is not the normal formatting.

> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:735
> +    copyTimingData((__bridge NSDictionary*)(adoptCF(_CFURLConnectionCopyTimingData(connection)).get()), timing);

Extra parentheses here that aren’t needed.

> Source/WebCore/platform/spi/cocoa/NSURLConnectionSPI.h:42
> +#if !HAVE(TIMINGDATAOPTIONS)

Using one of the WebKit configuration macros doesn’t make logical sense in an SPI.h header, other than USE(APPLE_INTERNAL_SDK). These should either be unconditional, or the condition should be based on the OS version or something like that.
Comment 10 Alex Christensen 2015-12-16 10:25:40 PST
http://trac.webkit.org/changeset/194156
Comment 11 Alex Christensen 2016-02-12 13:52:26 PST
Build fix in r196503