I looked at what information other tools provide: 1) Firebug Network info: - DNS Lookup: DNS resolution time - Connecting: Elapsed time required to create a TCP connection. - Blocking: Elapsed time spent in a browser queue waiting for a network connection. Displayed only in the case where this situation happens. - Sending: Time needed to send request data to the server. - Waiting: Waiting for the response (till the first byte is received from the server). - Receiving: Time necessary to download response body. FireBug Cache info: - Last Modified - last time the cache entry was modified - Last Fetched - last time the cache entry was opened - Expires - expiration time of the cache entry - Data Size - cache entry data size - Fetch Count - number of times the cache entry has been opened - Device - id for the device that stores this cache entry 2) Page Speed - DNS - The browser is performing a DNS lookup for this resource - Wait - The browser is waiting to establish a network (TCP) connection with the web server. - Connect - The browser is establishing a network (TCP) connection with the web server. This event only appears for new connections; not for connections that are reused. - Send - The browser has sent the HTTP request. Only GET requests are shown. - Connected - The browser is waiting for data over the network. The event ends when the browser ends the TCP connection. Resources that show lengthy Connected events may benefit from optimization to reduce payload size, such as compression 3) dynaTrace seems to present subset: - DNS - Connect - Server - Transfer
Created attachment 58018 [details] [PATCH] Proposed change.
Comment on attachment 58018 [details] [PATCH] Proposed change. WebCore/platform/network/ResourceResponseBase.h:173 + mutable unsigned m_connectionID; These fields should be behind #if ENABLE(INSPECTOR).
Created attachment 58031 [details] [PATCH] Same with comments addressed.
Bug about needing this info on Apple's WebKit port?
(In reply to comment #4) > Bug about needing this info on Apple's WebKit port? Yes, it does need a bug for your port! Leaving it to you as we discussed offline. Who would be the best person to look at the WebCore/platform/network/ResourceResponseBase.h and allow 6 new fields there?
Comment on attachment 58031 [details] [PATCH] Same with comments addressed. > + var data = [WebInspector.UIString("DNS Lookup"), Number.secondsToString(resource.dnsResolveTime), > + WebInspector.UIString("Blocking"), Number.secondsToString(resource.blockingTime), > + WebInspector.UIString("Connecting"), Number.secondsToString(resource.connectingTime), > + WebInspector.UIString("Sending"), Number.secondsToString(resource.sendingTime), > + WebInspector.UIString("Waiting"), Number.secondsToString(resource.waitingTime), > + WebInspector.UIString("Receiving"), Number.secondsToString(resource.endTime - resource.responseReceivedTime)]; These would need WebInpector.UIString in the Number.secondsToString call. I wonder if we should just make that automatically the default. > .cpp > +unsigned ResourceResponseBase::connectionID() const > +{ > + return m_connectionID; > +} > + > +void ResourceResponseBase::setConnectionID(unsigned connectionID) > +{ > + m_connectionID = connectionID; > +} > .h > + unsigned connectionID() const; > + void setConnectionID(unsigned connectionID); Can these implementations just move to the .h? That seems to be common for single lines. It also makes it easier to read (for me). That way we know its the obvious thing =) > +#if ENABLE(INSPECTOR) > + mutable unsigned m_connectionID; > + mutable double m_dnsResolveTime; > + mutable double m_connectingTime; > + mutable double m_blockingTime; > + mutable double m_sendingTime; > + mutable double m_waitingTime; > +#endif > }; Did these need to be mutable? I don't see them being set when they are in any const functions like m_age and m_date. Or does this have to do with something else? > +double WebURLResponse::waitingTime() const > +{ > + return m_private->m_resourceResponse->waitingTime(); > +} > + > +void WebURLResponse::setWaitingTime(double waitingTime) > +{ > + m_private->m_resourceResponse->setWaitingTime(waitingTime); > +} Ditto. This is chromium code, but I think its much easier scanning through lots of code when simple functions like this are onelined in the .h. I don't know the details on the mac side, so I'll ask around. I'm leaving r? because I don't really know how much this affects memory usage. However, this is INSPECTOR only, so my guess is its fine. I'd like someone with more experience to take a look. Otherwise I give it a thumbs up. Got a screenshot?
Created attachment 60879 [details] [PATCH] Same rebaselined, now uses RefPtr for timing info.
Created attachment 60882 [details] [IMAGE] Looks with the patch applied.
Comment on attachment 60879 [details] [PATCH] Same rebaselined, now uses RefPtr for timing info. WebKit/chromium/src/WebURLResponse.cpp:35 + #include "ResourceLoadTiming.h" mind includes order
Can you make the time coulmn be right aligned, so the units line up making the nunbers easier to read?
(In reply to comment #10) > Can you make the time coulmn be right aligned, so the units line up making the nunbers easier to read? Sure. I'm actually thinking of putting this data into a separate tab (next to Headers and Content). Something line "Timing". Not sure how much data we are able to put there though.
Another tab seems fine, but I like the popup since it is in context with the whole timeline.
Created attachment 60931 [details] [PATCH] Same with WebKit/chromium/*/WebURLLoadTiming
Comment on attachment 60931 [details] [PATCH] Same with WebKit/chromium/*/WebURLLoadTiming WebKit/chromium/public/WebURLResponse.h:76 + WEBKIT_API void setConnectionID(unsigned); nit: please make these attributes readable as well. WebKit/chromium/src/WebURLResponse.cpp:103 + m_private->m_resourceResponse->setResourceLoadTiming(ResourceLoadTiming::create()); why should setting a connection ID set an implicit ResourceLoadTiming object? WebKit/chromium/src/WebURLResponse.cpp:109 + ASSERT(loadTiming.get()); why is it invalid to set a null loadTiming object? WebKit/chromium/public/WebURLLoadTiming.h:60 + WEBKIT_API void setDomainLookupTimes(double start, double end); i know it means more typing for you, but individual setters and getters for these properties is really the way to go. that way, this API is more flexible and potentially useful for future applications without requiring an API change. these functions will get optimized away by the compiler :) I did not review any of the inspector changes.
(In reply to comment #14) > (From update of attachment 60931 [details]) > WebKit/chromium/public/WebURLResponse.h:76 > + WEBKIT_API void setConnectionID(unsigned); > nit: please make these attributes readable as well. > Will do. > WebKit/chromium/src/WebURLResponse.cpp:103 > + m_private->m_resourceResponse->setResourceLoadTiming(ResourceLoadTiming::create()); > why should setting a connection ID set an implicit ResourceLoadTiming object? > > WebKit/chromium/src/WebURLResponse.cpp:109 > + ASSERT(loadTiming.get()); > why is it invalid to set a null loadTiming object? Will fix. Both have been there before I migrated to the new struct and introduced the flag that controls the timing. It is just that every time I am looking for review overseas I find myself in a hurry and in the midnight :) > WebKit/chromium/public/WebURLLoadTiming.h:60 > + WEBKIT_API void setDomainLookupTimes(double start, double end); > i know it means more typing for you, but individual setters and getters > for these properties is really the way to go. that way, this API is > more flexible and potentially useful for future applications without > requiring an API change. these functions will get optimized away by > the compiler :) > Heh. I am happy to add compiler some more optimization work. > I did not review any of the inspector changes. Thanks! I had it reviewed earlier.
Created attachment 61010 [details] [PATCH] Addressed review comments (added getters, removed asserts).
Comment on attachment 61010 [details] [PATCH] Addressed review comments (added getters, removed asserts). WebKit/chromium/public/WebURLLoadTiming.h:37 + namespace WebCore { class ResourceLoadTiming; } move this to its own line?
Comment on attachment 60879 [details] [PATCH] Same rebaselined, now uses RefPtr for timing info. Cleared Yury Semikhatsky's review+ from obsolete attachment 60879 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/English.lproj/localizedStrings.js M WebCore/inspector/InspectorController.cpp M WebCore/inspector/InspectorController.h M WebCore/inspector/InspectorResource.cpp M WebCore/inspector/InspectorResource.h M WebCore/inspector/front-end/ResourcesPanel.js M WebCore/inspector/front-end/inspector.js M WebCore/loader/appcache/ApplicationCacheGroup.cpp M WebCore/platform/network/ResourceLoadTiming.h M WebCore/platform/network/ResourceRequestBase.h M WebCore/platform/network/ResourceResponseBase.cpp M WebCore/platform/network/ResourceResponseBase.h M WebKit/chromium/ChangeLog M WebKit/chromium/WebKit.gyp M WebKit/chromium/public/WebDevToolsAgent.h A WebKit/chromium/public/WebURLLoadTiming.h M WebKit/chromium/public/WebURLRequest.h M WebKit/chromium/public/WebURLResponse.h M WebKit/chromium/src/WebDevToolsAgentImpl.cpp M WebKit/chromium/src/WebDevToolsAgentImpl.h A WebKit/chromium/src/WebURLLoadTiming.cpp M WebKit/chromium/src/WebURLRequest.cpp M WebKit/chromium/src/WebURLResponse.cpp Committed r62927