Bug 152285 – Implement Web Timing when using NETWORK_SESSION

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Implement Web Timing when using NETWORK_SESSION

[Alex Christensen]

Created attachment 267331 [details]
Patch

[Alex Christensen]

Created attachment 267333 [details]
Patch

[Darin Adler]

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?

[Alex Christensen]

Created attachment 267398 [details]
Patch

[Alex Christensen]

Created attachment 267401 [details]
Patch

[Alex Christensen]

Created attachment 267402 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

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.

[Alex Christensen]

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

[Alex Christensen]

Build fix in r196503