Bug 168029 - Web Inspector: Network tab content view is blank after reload
Summary: Web Inspector: Network tab content view is blank after reload
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P1 Normal
Assignee: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-02-08 17:10 PST by Matt Baker
Modified: 2017-03-29 11:58 PDT (History)
7 users (show)

See Also:


Attachments
Patch (1.83 KB, patch)
2017-02-22 13:18 PST, Sam Brodkin
no flags Details | Formatted Diff | Diff
Patch (1.76 KB, patch)
2017-02-22 14:08 PST, Sam Brodkin
no flags Details | Formatted Diff | Diff
Patch (2.12 KB, patch)
2017-02-24 13:35 PST, Sam Brodkin
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews113 for mac-elcapitan (1.71 MB, application/zip)
2017-02-24 18:14 PST, Build Bot
no flags Details
For Landing (1.82 KB, patch)
2017-03-29 10:51 PDT, BJ Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 2017-02-08 17:10:17 PST
Summary:
Network tab content view is blank after reload. Only occurs when navigation sidebar is collapsed.

Steps to Reproduce:
1. Inspect any page > Network tab
2. Collapse the left sidebar
3. Reload page
  => Content view is blank!
Comment 1 Nikita Vasilyev 2017-02-08 17:16:04 PST
I was able to reproduce it.
Comment 2 Radar WebKit Bug Importer 2017-02-08 17:18:22 PST
<rdar://problem/30433456>
Comment 3 Devin Rousso 2017-02-09 09:43:37 PST
I was unable to reproduce this on ToT.
Comment 4 Matt Baker 2017-02-09 12:08:31 PST
I still see this in ToT. I killed Safari local storage prior to testing just in case.

Make sure the left sidebar is collapsed before reloading the page.
Comment 5 Sam Brodkin 2017-02-22 13:18:55 PST
Created attachment 302429 [details]
Patch
Comment 6 Geoffrey Garen 2017-02-22 13:27:14 PST
Comment on attachment 302429 [details]
Patch

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

Usually we require each change in WebKit to include a regression test. You can see some examples in LayoutTests/inspector. Your patch should include a regression test or, in those rare cases, an explanation of why no test is possible in this case.

> Source/WebInspectorUI/ChangeLog:9
> +        (WebInspector.NetworkSidebarPanel.prototype._networkTimelineReset):

This is a good place to explain your understanding of the bug, and why your change makes things better.

> Source/WebInspectorUI/ChangeLog:19
> +2017-02-22  Sam Brodkin  <isam@apple.com>
> +
> +        Web Inspector: Network tab content view is blank after reload
> +        https://bugs.webkit.org/show_bug.cgi?id=168029
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * UserInterface/Views/NetworkSidebarPanel.js:
> +        (WebInspector.NetworkSidebarPanel.prototype._networkTimelineReset):

Looks like prepare-ChangeLog ran twice. Better to remove the second entry.
Comment 7 Matt Baker 2017-02-22 13:52:15 PST
Comment on attachment 302429 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/NetworkSidebarPanel.js:194
> +        this.showDefaultContentView();

In general we try to avoid doing work in tabs that aren't visible. A fairly common idiom used in the Inspector UI is:

1. Handle a message
2. is the view processing the message visible?
3. If so, update the view  (on the next animation frame, if coalescing multiple events in quick succession)
4. If not, mark the view as having pending changes and update when it is shown

For an example of this, see the treatment of "pending records" in Timeline content views.
Comment 8 Matt Baker 2017-02-22 13:57:10 PST
(In reply to comment #6)
> Comment on attachment 302429 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=302429&action=review
> 
> Usually we require each change in WebKit to include a regression test. You
> can see some examples in LayoutTests/inspector. Your patch should include a
> regression test or, in those rare cases, an explanation of why no test is
> possible in this case.

Changes to the Inspector view hierarchy are not readily testable, although there has been some exploratory work done in this area.

We typically require tests only for changes that impact the protocol, model hierarchy, or controllers/managers.
Comment 9 Sam Brodkin 2017-02-22 14:08:53 PST
Created attachment 302439 [details]
Patch
Comment 10 BJ Burg 2017-02-22 14:14:40 PST
Comment on attachment 302439 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:11
> +        showDefaultContentView() which shows networkGridView in the contentView was not being called when the NetworkSidebarPanel was not visible. Removing the if statement always shows the networkGridView which fixes the bug. This wouldn't be easy to regression test. 

Nit: We typically hard wrap changeling entries at about the column where 'called' is.
Comment 11 BJ Burg 2017-02-22 14:14:59 PST
(In reply to comment #10)
> Comment on attachment 302439 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=302439&action=review
> 
> > Source/WebInspectorUI/ChangeLog:11
> > +        showDefaultContentView() which shows networkGridView in the contentView was not being called when the NetworkSidebarPanel was not visible. Removing the if statement always shows the networkGridView which fixes the bug. This wouldn't be easy to regression test. 
> 
> Nit: We typically hard wrap changeling entries at about the column where
> 'called' is.

Changelog, lol.
Comment 12 Matt Baker 2017-02-22 14:19:18 PST
Comment on attachment 302439 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/NetworkSidebarPanel.js:194
> +        this.showDefaultContentView();

Have you tested this patch? I suspect that when the Network tab is in the background there will be layout assertions in NetworkGridContentView, related to laying out the graph column.
Comment 13 Matt Baker 2017-02-22 14:22:32 PST
In order to test this, enable inspection of the Inspector window, so that you can monitor assertions:

defaults write com.apple.Safari WebKitDeveloperExtrasEnabled -bool YES
defaults write com.apple.Safari __WebInspectorPageGroupLevel1__.WebKit2DeveloperExtrasEnabled -bool YES
Comment 14 Sam Brodkin 2017-02-22 14:43:17 PST
(In reply to comment #13)
> In order to test this, enable inspection of the Inspector window, so that
> you can monitor assertions:
> 
> defaults write com.apple.Safari WebKitDeveloperExtrasEnabled -bool YES
> defaults write com.apple.Safari
> __WebInspectorPageGroupLevel1__.WebKit2DeveloperExtrasEnabled -bool YES

That is so meta. I was wondering how to do this!
Comment 15 Sam Brodkin 2017-02-22 15:13:08 PST
(In reply to comment #13)

I inspected the Inspector window and saw no assertion failures (using an assertion failure breakpoint, looking in the console, and in the Activity Viewer) when reloading the network tab and changing to another tab while it was in flight.
Comment 16 Sam Brodkin 2017-02-24 13:35:06 PST
Created attachment 302682 [details]
Patch
Comment 17 Nikita Vasilyev 2017-02-24 14:45:11 PST
Comment on attachment 302682 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:23
> +        networkGridView which fixes the bug. This wouldn't be easy to regression test.

Looks like prepare-ChangeLog ran twice, again. You should remove the section that has no comments.
Comment 18 Sam Brodkin 2017-02-24 15:12:12 PST
(In reply to comment #17)

I'm trying but every time I run webkit-patch upload it creates a new entry that I can't figure out how to edit
Comment 19 Build Bot 2017-02-24 18:13:40 PST
Comment on attachment 302682 [details]
Patch

Attachment 302682 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3188262

New failing tests:
media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-drag.html
media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-drag-is-prevented-over-button.html
Comment 20 Build Bot 2017-02-24 18:14:12 PST
Created attachment 302721 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 21 BJ Burg 2017-03-29 10:50:34 PDT
Comment on attachment 302682 [details]
Patch

r=me, I'll fix the changelog.
Comment 22 BJ Burg 2017-03-29 10:51:28 PDT
Created attachment 305759 [details]
For Landing
Comment 23 WebKit Commit Bot 2017-03-29 11:57:55 PDT
Comment on attachment 305759 [details]
For Landing

Clearing flags on attachment: 305759

Committed r214551: <http://trac.webkit.org/changeset/214551>
Comment 24 WebKit Commit Bot 2017-03-29 11:58:00 PDT
All reviewed patches have been landed.  Closing bug.