Summary: | Implement Web Timing when using NETWORK_SESSION | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||||||
Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | ||||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Alex Christensen
2015-12-14 17:33:24 PST
Created attachment 267331 [details]
Patch
Created attachment 267333 [details]
Patch
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? Created attachment 267398 [details]
Patch
Created attachment 267401 [details]
Patch
Created attachment 267402 [details]
Patch
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 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 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. Build fix in r196503 |