Bug 180996 - Web Inspector: Network Table - Redesign the waterfall popover showing timing data
Summary: Web Inspector: Network Table - Redesign the waterfall popover showing timing ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-19 13:41 PST by Joseph Pecoraro
Modified: 2017-12-19 16:57 PST (History)
6 users (show)

See Also:


Attachments
[IMAGE] Waterfall Popover - All Fields (551.71 KB, image/png)
2017-12-19 13:53 PST, Joseph Pecoraro
no flags Details
[IMAGE] Waterfall Popover - Some Fields (551.21 KB, image/png)
2017-12-19 13:54 PST, Joseph Pecoraro
no flags Details
[IMAGE] Timing Detail View - Most Fields (386.83 KB, image/png)
2017-12-19 13:54 PST, Joseph Pecoraro
no flags Details
[PATCH] Proposed Fix (33.68 KB, patch)
2017-12-19 13:54 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (31.64 KB, patch)
2017-12-19 14:01 PST, Joseph Pecoraro
mattbaker: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (3.07 MB, application/zip)
2017-12-19 15:52 PST, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2017-12-19 13:41:17 PST
Network Table - Redesign the waterfall popover showing timing data

The current popover just shows a "big graph". Redesign the popover to be more about explanation.
Comment 1 Joseph Pecoraro 2017-12-19 13:41:44 PST
<rdar://problem/34072542>
Comment 2 Joseph Pecoraro 2017-12-19 13:53:59 PST
Created attachment 329810 [details]
[IMAGE] Waterfall Popover - All Fields
Comment 3 Joseph Pecoraro 2017-12-19 13:54:10 PST
Created attachment 329811 [details]
[IMAGE] Waterfall Popover - Some Fields
Comment 4 Joseph Pecoraro 2017-12-19 13:54:32 PST
Created attachment 329812 [details]
[IMAGE] Timing Detail View - Most Fields
Comment 5 Joseph Pecoraro 2017-12-19 13:54:48 PST
Created attachment 329813 [details]
[PATCH] Proposed Fix
Comment 6 Joseph Pecoraro 2017-12-19 13:56:01 PST
Comment on attachment 329813 [details]
[PATCH] Proposed Fix

Oops, need to rebase.
Comment 7 Joseph Pecoraro 2017-12-19 14:01:40 PST
Created attachment 329817 [details]
[PATCH] Proposed Fix
Comment 8 EWS Watchlist 2017-12-19 15:52:17 PST Comment hidden (obsolete)
Comment 9 EWS Watchlist 2017-12-19 15:52:19 PST Comment hidden (obsolete)
Comment 10 Joseph Pecoraro 2017-12-19 16:32:42 PST
Comment on attachment 329845 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

Failure appears to be unrelated to the patch.
Comment 11 Matt Baker 2017-12-19 16:46:03 PST
Comment on attachment 329817 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=329817&action=review

r=me, with a couple nits/comments.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.css:32
> +    color: gray;

Style: every use of `gray` could be replaced with var(--text-color-gray-medium), but I'll leave it up to you. We should probably just update the variable's value to be gray instead of hsl(0, 0%, 52%).

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.css:41
> +    padding-left: 5px;

Use -webkit-padding-start for this to be RTL ready.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:33
> +        console.assert(!fixedWidth || fixedWidth >= 100, "fixedWidth must be at least wide enough for strings");

Nit: assert description should end in a period.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:56
> +        if (additionalClassName)

I was about to suggest row.classList.add("header", additionalClassName), but learned that `null` and `undefined` get converted to "null" and "undefined" and added as classes!

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingContentView.js:97
> +        breakdownView.updateLayout();

The breakdownView isn't being added to the view hierarchy. This is okay, so long as it never needs to layout again (which seems to be the case, as it implements View.initialLayout but not View.layout). The call to updateLayout does the trick just fine here (a scheduled layout would never run), but thought it was worth mentioning.

Earlier ResourceTimingBreakdownView is used in as popover, which isn't a part of the View hierarchy currently, so updateLayout is required. That said, eventually we should make Popover a view that is a child of the root (document.body).
Comment 12 Joseph Pecoraro 2017-12-19 16:57:14 PST
<https://trac.webkit.org/r226158>