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.
Created attachment 230839 [details] Patch
Created attachment 230943 [details] Patch
Created attachment 230948 [details] Patch
Created attachment 230955 [details] Patch
Created attachment 230957 [details] Patch
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()
Created attachment 231004 [details] Patch
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!
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.
Created attachment 231078 [details] Patch
Created attachment 231079 [details] Patch
Created attachment 231084 [details] Patch
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).
Created attachment 231161 [details] Patch
Created attachment 231164 [details] Patch
Created attachment 231167 [details] Patch
Created attachment 231170 [details] Patch
Created attachment 231171 [details] Patch
Created attachment 231181 [details] Patch
Created attachment 231182 [details] Patch
Created attachment 231183 [details] Patch
Created attachment 231195 [details] Patch
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).
Created attachment 231298 [details] Patch
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?
(r=me is conditional on fixing WebKitLegacy)
(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.
Created attachment 231322 [details] Patch
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 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.
I'm going to put this in and fix the duplicate code and reference problems in another patch. http://trac.webkit.org/changeset/168647