| Summary: | Web Inspector: render network timing as gant chart in popover. | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Pavel Feldman <pfeldman> | ||||||||
| Component: | Web Inspector (Deprecated) | Assignee: | Pavel Feldman <pfeldman> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | bweinstein, joepeck, keishi, pfeldman, pmuellr, rik, timothy, yurys | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Pavel Feldman
2010-07-27 08:14:17 PDT
Created attachment 62692 [details]
[IMAGE] Proposed looks
Created attachment 62693 [details]
[PATCH] Proposed change.
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 because its clear, but if you want to keep the zero width space please add a comment. (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 because its clear, but > if you want to keep the zero width space please add a comment. Done! Created attachment 62709 [details]
[PATCH] Proposed change.
Comment on attachment 62709 [details]
[PATCH] Proposed change.
r=me. Thanks for the fixes!
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 |