Bug 43051 - Web Inspector: render network timing as gant chart in popover.
Summary: Web Inspector: render network timing as gant chart in popover.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Pavel Feldman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-27 08:14 PDT by Pavel Feldman
Modified: 2010-07-27 11:32 PDT (History)
8 users (show)

See Also:


Attachments
[IMAGE] Proposed looks (92.44 KB, image/png)
2010-07-27 08:16 PDT, Pavel Feldman
no flags Details
[PATCH] Proposed change. (9.63 KB, patch)
2010-07-27 08:21 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
[PATCH] Proposed change. (9.53 KB, patch)
2010-07-27 10:25 PDT, Pavel Feldman
joepeck: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2010-07-27 08:14:17 PDT
Image to follow.
Comment 1 Pavel Feldman 2010-07-27 08:16:29 PDT
Created attachment 62692 [details]
[IMAGE] Proposed looks
Comment 2 Pavel Feldman 2010-07-27 08:21:42 PDT
Created attachment 62693 [details]
[PATCH] Proposed change.
Comment 3 Joseph Pecoraro 2010-07-27 09:54:49 PDT
Comment on attachment 62693 [details]
[PATCH] Proposed change.

This looks okay. I haven't actually had a chance to use the extra
timing information yet, so I can't really comment. But it looks
useful and the visual comparison is nice. Some small points on the
patch. r? pending the questions below or a review from someone
with more experience with the timing data.

> +++ b/WebCore/inspector/front-end/ResourcesPanel.js
> +        function addRow(title, start, end, color)
> +        {
> +            var row = {};
> +            row.title = title;
> +            row.start = start - resource.timing.proxyStart;
> +            row.end = end - resource.timing.proxyStart;
> +            rows.push(row);

This uses resource.timing.proxyStart, which is -1 checked below.
What are the chances proxyStart will actually be -1, and how
does this display then?


> +        // TODO: Waiting includes SSL, subtract it here.
> +        addRow(WebInspector.UIString("Waiting"), resource.timing.sendEnd, resource.timing.receiveHeadersEnd);

These should be "// FIXME:". And you have the sslStart and sslEnd. Do you
need other data to complete this task?


> +        var total = (resource.endTime - resource.timing.requestTime) * 1000 - resource.timing.proxyStart;
> +        var scale = 200 / total;

200, because of the width. Might be nice to put that into a
const variable, and use it below when setting the td.width.


> +        for (var i = 0; i < rows.length; ++i) {

I'm a proponent of caching row[i] into a local variable.
Too minor of an optimization?


> +            bar.textContent = "\u200B";

Is this actually needed? I normally use &nbsp; because its clear, but
if you want to keep the zero width space please add a comment.
Comment 4 Pavel Feldman 2010-07-27 10:25:12 PDT
(In reply to comment #3)

> This uses resource.timing.proxyStart, which is -1 checked below.
> What are the chances proxyStart will actually be -1, and how
> does this display then?

You just caught a bug, thank you!

> These should be "// FIXME:". And you have the sslStart and sslEnd. Do you
> need other data to complete this task?

Removed the whole thing.

> 200, because of the width. Might be nice to put that into a
> const variable, and use it below when setting the td.width.
> 

Done.

> 
> I'm a proponent of caching row[i] into a local variable.
> Too minor of an optimization?
> 

Too minor! This should be a handful of instructions!

> 
> > +            bar.textContent = "\u200B";
> 
> Is this actually needed? I normally use &nbsp; because its clear, but
> if you want to keep the zero width space please add a comment.

Done!
Comment 5 Pavel Feldman 2010-07-27 10:25:45 PDT
Created attachment 62709 [details]
[PATCH] Proposed change.
Comment 6 Joseph Pecoraro 2010-07-27 10:52:26 PDT
Comment on attachment 62709 [details]
[PATCH] Proposed change.

r=me. Thanks for the fixes!
Comment 7 Pavel Feldman 2010-07-27 11:32:58 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/inspector/InspectorResource.cpp
	M	WebCore/inspector/front-end/ResourcesPanel.js
	M	WebCore/inspector/front-end/inspector.css
	M	WebCore/inspector/front-end/utilities.js
Committed r64144