Bug 172470 - Web Inspector: use initialLayout for NetworkSidebarPanel
Summary: Web Inspector: use initialLayout for NetworkSidebarPanel
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: Devin Rousso
URL:
Keywords:
Depends on:
Blocks: 172303
  Show dependency treegraph
 
Reported: 2017-05-22 13:42 PDT by Devin Rousso
Modified: 2017-05-23 13:34 PDT (History)
3 users (show)

See Also:


Attachments
Patch (5.45 KB, patch)
2017-05-22 13:44 PDT, Devin Rousso
joepeck: review+
Details | Formatted Diff | Diff
[Image] Timeline Before (494.19 KB, image/png)
2017-05-22 13:44 PDT, Devin Rousso
no flags Details
[Image] Timeline After (483.99 KB, image/png)
2017-05-22 13:45 PDT, Devin Rousso
no flags Details
Patch (5.56 KB, patch)
2017-05-23 13:09 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2017-05-22 13:42:14 PDT
Also move the creation of the NetworkGridContentView to right before it is shown.
Comment 1 Devin Rousso 2017-05-22 13:44:20 PDT
Created attachment 310910 [details]
Patch
Comment 2 Devin Rousso 2017-05-22 13:44:58 PDT
Created attachment 310912 [details]
[Image] Timeline Before
Comment 3 Devin Rousso 2017-05-22 13:45:12 PDT
Created attachment 310913 [details]
[Image] Timeline After
Comment 4 Joseph Pecoraro 2017-05-22 21:40:42 PDT
Comment on attachment 310910 [details]
Patch

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

r=me, but with concerns

> Source/WebInspectorUI/UserInterface/Views/NetworkSidebarPanel.js:41
>          this.contentBrowser.addEventListener(WebInspector.ContentBrowser.Event.CurrentContentViewDidChange, this._contentBrowserCurrentContentViewDidChange, this);

As was mentioned previously, it doesn't make sense to me why this is not in initialLayout. It seems potentially problematic keeping it separate. It is quite small right now but I do worry about future changes adding code in there that is unsafe unless performed after initial layout.

> Source/WebInspectorUI/UserInterface/Views/NetworkSidebarPanel.js:82
> +        var scopeItemPrefix = "network-sidebar-";

Style: Lets upgrade these to `let` now.
Comment 5 Devin Rousso 2017-05-23 13:09:24 PDT
Created attachment 311042 [details]
Patch
Comment 6 WebKit Commit Bot 2017-05-23 13:34:55 PDT
Comment on attachment 311042 [details]
Patch

Clearing flags on attachment: 311042

Committed r217295: <http://trac.webkit.org/changeset/217295>
Comment 7 WebKit Commit Bot 2017-05-23 13:34:57 PDT
All reviewed patches have been landed.  Closing bug.