Bug 103777

Created attachment 177029 [details] Patch

Nate, this is the Resource Timing patch I'd discussed with you earlier. I record the timing information in a map on CachedResourceLoader. Let me know what you think.

Comment on attachment 177029 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177029&action=review > Source/WebCore/loader/cache/CachedResourceLoader.cpp:520 > +#if ENABLE(RESOURCE_TIMING) > + InitiatorInfo info = { request.initiatorName(), monotonicallyIncreasingTime() }; > + m_initiatorMap.add(resource.get(), info); > +#endif Is it intentional that this won't populate m_initiatorMap for revalidations? > Source/WebCore/loader/cache/CachedResourceLoader.cpp:709 > + if (initiatorIt != m_initiatorMap.end()) { What are the cases where we won't find the resource in the map? > Source/WebCore/loader/cache/CachedResourceLoader.cpp:712 > + if (document()) > + document()->domWindow()->performance()->addResourceTiming(info.name, document(), resource->resourceRequest(), resource->response(), info.startTime, resource->loadFinishTime()); Probably want to ASSERT(document()) here?

Created attachment 177038 [details] Patch

(In reply to comment #3) > (From update of attachment 177029 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=177029&action=review > > > Source/WebCore/loader/cache/CachedResourceLoader.cpp:520 > > +#if ENABLE(RESOURCE_TIMING) > > + InitiatorInfo info = { request.initiatorName(), monotonicallyIncreasingTime() }; > > + m_initiatorMap.add(resource.get(), info); > > +#endif > > Is it intentional that this won't populate m_initiatorMap for revalidations? Nope. Good catch. > > > Source/WebCore/loader/cache/CachedResourceLoader.cpp:709 > > + if (initiatorIt != m_initiatorMap.end()) { > > What are the cases where we won't find the resource in the map? loadDone() is called twice. See the traces below. > > > Source/WebCore/loader/cache/CachedResourceLoader.cpp:712 > > + if (document()) > > + document()->domWindow()->performance()->addResourceTiming(info.name, document(), resource->resourceRequest(), resource->response(), info.startTime, resource->loadFinishTime()); > > Probably want to ASSERT(document()) here? Done. Traces: (gdb) bt #0 WebCore::CachedResourceLoader::loadDone (this=0x7fffe367a280, resource= 0x7fffe2456500) at ../../third_party/WebKit/Source/WebCore/loader/cache/CachedResourceLoader #1 0x00007ffff308502c in WebCore::SubresourceLoader::releaseResources (this= 0x7fffe2487000) at ../../third_party/WebKit/Source/WebCore/loader/SubresourceLoader.cpp:318 #2 0x00007ffff307f8cc in WebCore::ResourceLoader::didFinishLoading (this= 0x7fffe2487000, finishTime=0) at ../../third_party/WebKit/Source/WebCore/loader/ResourceLoader.cpp:314 #3 0x00007ffff3084bdc in WebCore::SubresourceLoader::didFinishLoading (this= 0x7fffe2487000, finishTime=0) at ../../third_party/WebKit/Source/WebCore/loader/SubresourceLoader.cpp:277 #4 0x00007ffff3080045 in WebCore::ResourceLoader::didFinishLoading (this= 0x7fffe2487000, finishTime=0) at ../../third_party/WebKit/Source/WebCore/loader/ResourceLoader.cpp:453 #5 0x00007ffff2a61ee2 in WebCore::ResourceHandleInternal::didFinishLoading ( this=0x7fffe3678000, finishTime=0) at ../../third_party/WebKit/Source/WebCore/platform/network/chromium/Resourc - and - STDERR: base::debug::StackTrace::StackTrace() [0x7fc909c8cae4] STDERR: base::debug::(anonymous namespace)::StackDumpSignalHandler() [0x7fc909c8c7e5] STDERR: <unknown> [0x7fc90280d4a0] STDERR: WebCore::CachedResourceLoader::loadDone() [0x7fc90722c2a0] STDERR: WebCore::SubresourceLoader::releaseResources() [0x7fc90720202c] STDERR: WebCore::ResourceLoader::cancel() [0x7fc9071fcd47] STDERR: WebCore::ResourceLoader::cancel() [0x7fc9071fcaf7] STDERR: WebCore::cancelAll() [0x7fc9071ad2c4] STDERR: WebCore::DocumentLoader::stopLoadingSubresources() [0x7fc9071b02f6] STDERR: WebCore::DocumentLoader::stopLoading() [0x7fc9071ae156] STDERR: WebCore::FrameLoader::stopAllLoaders() [0x7fc9071d0c3d] STDERR: WebCore::FrameLoader::continueLoadAfterNavigationPolicy() [0x7fc9071d6194] STDERR: WebCore::FrameLoader::callContinueLoadAfterNavigationPolicy() [0x7fc9071d5a61] STDERR: WebCore::PolicyCallback::call() [0x7fc9071eeed2] STDERR: WebCore::PolicyChecker::continueAfterNavigationPolicy() [0x7fc9071efd4b] STDERR: WebKit::FrameLoaderClientImpl::dispatchDecidePolicyForNavigationAction() [0x7fc905f1b068] STDERR: WebCore::PolicyChecker::checkNavigationPolicy() [0x7fc9071ef756] STDERR: WebCore::FrameLoader::loadWithDocumentLoader() [0x7fc9071cff1c] STDERR: WebCore::FrameLoader::loadWithNavigationAction() [0x7fc9071cf7b5] STDERR: WebCore::FrameLoader::loadURL() [0x7fc9071cee0f] STDERR: WebCore::FrameLoader::loadFrameRequest() [0x7fc9071ce6ed] STDERR: WebCore::FrameLoader::urlSelected() [0x7fc9071cacca] STDERR: WebCore::FrameLoader::urlSelected() [0x7fc9071caa18] STDERR: WebCore::HTMLAnchorElement::handleClick() [0x7fc9073ccb7f] STDERR: WebCore::HTMLAnchorElement::defaultEventHandler() [0x7fc9073caeca] STDERR: WebCore::EventDispatcher::dispatchEventPostProcess() [0x7fc9066179d6] STDERR: WebCore::EventDispatcher::dispatchEvent() [0x7fc906616b70] STDERR: WebCore::EventDispatchMediator::dispatchEvent() [0x7fc906614e6c] STDERR: WebCore::EventDispatcher::dispatchEvent() [0x7fc906615af0] STDERR: WebCore::Node::dispatchEvent() [0x7fc90664ca56] STDERR: WebCore::EventTarget::dispatchEvent() [0x7fc906620cbb] STDERR: WebCore::NodeV8Internal::dispatchEventCallback() [0x7fc9078ae7ef] STDERR: v8::internal::HandleApiCallHelper<>() [0x7fc90b90890a] STDERR: v8::internal::Builtin_Impl_HandleApiCall() [0x7fc90b902fa3] STDERR: v8::internal::Builtin_HandleApiCall() [0x7fc90b902f74] STDERR: <unknown> [0x3e08aaf0654e]

Comment on attachment 177038 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177038&action=review LGTM, just some formatting pedantry: > Source/WebCore/loader/cache/CachedResourceLoader.cpp:712 > +#if ENABLE(RESOURCE_TIMING) newlines before and after this #if/#else/#endif block, please. > Source/WebCore/loader/cache/CachedResourceLoader.cpp:724 > +#endif This is long enough that an // ENABLE(RESOURCE_TIMING) would make me happy.

Created attachment 177315 [details] Patch for landing

(In reply to comment #6) > (From update of attachment 177038 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=177038&action=review > > LGTM, just some formatting pedantry: Thanks. > > > Source/WebCore/loader/cache/CachedResourceLoader.cpp:712 > > +#if ENABLE(RESOURCE_TIMING) > > newlines before and after this #if/#else/#endif block, please. Done. > > > Source/WebCore/loader/cache/CachedResourceLoader.cpp:724 > > +#endif > > This is long enough that an // ENABLE(RESOURCE_TIMING) would make me happy. Done.

Comment on attachment 177315 [details] Patch for landing Rejecting attachment 177315 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: ripts/update-webkit line 152. Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2 Updating OpenSource From http://git.chromium.org/external/Webkit e9cfa82..99a0591 HEAD -> origin/HEAD error: Ref refs/remotes/origin/master is at 99a0591e2f7986bdde68fdbbfa7ba90e4741de91 but expected e9cfa82bccbe6dbf99a5b026e145a5ad84051691 ! e9cfa82..99a0591 master -> origin/master (unable to update local ref) Died at Tools/Scripts/update-webkit line 152. Full output: http://queues.webkit.org/results/15100694

Created attachment 177328 [details] Patch for landing

Comment on attachment 177328 [details] Patch for landing Rejecting attachment 177328 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/15103668

Committed r136439: <http://trac.webkit.org/changeset/136439>