Web Inspector: Network Waterfall column should redraw when adding/removing new columns

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

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

[Radar WebKit Bug Importer]

<rdar://problem/47464149>

[Joseph Pecoraro]

Created attachment 359797 [details]
[PATCH] Proposed Fix

This has been a pet peeve of mine for a long time.

[Devin Rousso]

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

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 359808 [details]
[PATCH] For Landing

[WebKit Commit Bot]

Comment on attachment 359808 [details]
[PATCH] For Landing

Clearing flags on attachment: 359808

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