Bug 193696 - Web Inspector: Network Waterfall column should redraw when adding/removing new columns
Summary: Web Inspector: Network Waterfall column should redraw when adding/removing ne...
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: 2019-01-22 16:06 PST by Joseph Pecoraro
Modified: 2019-01-25 18:21 PST (History)
5 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (5.50 KB, patch)
2019-01-22 16:09 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] For Landing (5.43 KB, patch)
2019-01-22 16:45 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2019-01-22 16:06:55 PST
Network Waterfall column should redraw when adding/removing new columns

Steps to Reproduce:
1. Inspect this page
2. Reload to have waterfall column
3. Hide "Domain" columns
  => Waterfall column graphs do not extend to edges as expected
4. Show another column
  => Waterfall column graphs do not extend to edges as expected

Note:
- Resizing the window causes graphs to update correctly, so should showing/hiding columns
Comment 1 Radar WebKit Bug Importer 2019-01-22 16:07:28 PST
<rdar://problem/47464149>
Comment 2 Joseph Pecoraro 2019-01-22 16:09:18 PST
Created attachment 359797 [details]
[PATCH] Proposed Fix

This has been a pet peeve of mine for a long time.
Comment 3 Devin Rousso 2019-01-22 16:24:24 PST
Comment on attachment 359797 [details]
[PATCH] Proposed Fix

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

r=me

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1073
> +            needsReloadOnResize: true,

And this is a perfect example of why I love optional objects in constructors 😍

> Source/WebInspectorUI/UserInterface/Views/Table.js:494
> +        // Now populate columns that may be sensitive to resizes.

Could you extract this logic into a separate function so that it can be called (instead of copied) by `hideColumn`?

> Source/WebInspectorUI/UserInterface/Views/Table.js:498
> +                    this.reloadVisibleColumnCells(visibleColumn);

Is it necessary to do all this work as soon as a new column is added, or can we leverage the layout system to wait until we need to actually display it (this also applies to the `this._delegate.tablePopulateCell` loop)?
Comment 4 Joseph Pecoraro 2019-01-22 16:41:40 PST
Comment on attachment 359797 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Views/Table.js:494
>> +        // Now populate columns that may be sensitive to resizes.
> 
> Could you extract this logic into a separate function so that it can be called (instead of copied) by `hideColumn`?

Turns out the logic can be slightly different given this excludes a single column.

>> Source/WebInspectorUI/UserInterface/Views/Table.js:498
>> +                    this.reloadVisibleColumnCells(visibleColumn);
> 
> Is it necessary to do all this work as soon as a new column is added, or can we leverage the layout system to wait until we need to actually display it (this also applies to the `this._delegate.tablePopulateCell` loop)?

I gave it a quick attempt and it didn't work. I'm not exactly sure why, but one benefit of this approach is that it does the minimum amount of work on show/hide. I'm going to keep as is to avoid going down a rabbit hole.

> Source/WebInspectorUI/UserInterface/Views/Table.js:557
> +            if (visibleColumn !== column) {

This loop doesn't need this check since `column` was just removed.
Comment 5 Joseph Pecoraro 2019-01-22 16:45:51 PST
Created attachment 359808 [details]
[PATCH] For Landing
Comment 6 WebKit Commit Bot 2019-01-23 10:47:02 PST
Comment on attachment 359808 [details]
[PATCH] For Landing

Clearing flags on attachment: 359808

Committed r240347: <https://trac.webkit.org/changeset/240347>