Bug 40227 – Web Inspector: Provide detailed network info in the resources panel.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Pavel Feldman]

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

[Pavel Feldman]

Created attachment 58018 [details]
[PATCH] Proposed change.

[Yury Semikhatsky]

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).

[Pavel Feldman]

Created attachment 58031 [details]
[PATCH] Same with comments addressed.

[Timothy Hatcher]

Bug about needing this info on Apple's WebKit port?

[Pavel Feldman]

(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?

[Joseph Pecoraro]

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?

[Pavel Feldman]

Created attachment 60879 [details]
[PATCH] Same rebaselined, now uses RefPtr for timing info.

[Pavel Feldman]

Created attachment 60882 [details]
[IMAGE] Looks with the patch applied.

[Yury Semikhatsky]

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

[Timothy Hatcher]

Can you make the time coulmn be right aligned, so the units line up making the nunbers easier to read?

[Pavel Feldman]

(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.

[Timothy Hatcher]

Another tab seems fine, but I like the popup since it is in context with the whole timeline.

[Pavel Feldman]

Created attachment 60931 [details]
[PATCH] Same with WebKit/chromium/*/WebURLLoadTiming

[Darin Fisher (:fishd, Google)]

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.

[Pavel Feldman]

(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.

[Pavel Feldman]

Created attachment 61010 [details]
[PATCH] Addressed review comments (added getters, removed asserts).

[Yury Semikhatsky]

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?

[Eric Seidel (no email)]

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.

[Pavel Feldman]

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