Bug 168029 – Web Inspector: Network tab content view is blank after reload

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

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!

[Nikita Vasilyev]

I was able to reproduce it.

[Radar WebKit Bug Importer]

<rdar://problem/30433456>

[Devin Rousso]

I was unable to reproduce this on ToT.

[Matt Baker]

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.

[Sam Brodkin]

Created attachment 302429 [details]
Patch

[Geoffrey Garen]

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
> [phone]  Sam Brodkin  <[e-mail]>
> +
> +        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.

[Matt Baker]

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.

[Matt Baker]

(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.

[Sam Brodkin]

Created attachment 302439 [details]
Patch

[Blaze Burg]

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.

[Blaze Burg]

(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.

[Matt Baker]

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.

[Matt Baker]

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

[Sam Brodkin]

(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!

[Sam Brodkin]

(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.

[Sam Brodkin]

Created attachment 302682 [details]
Patch

[Nikita Vasilyev]

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.

[Sam Brodkin]

(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

[Build Bot]

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

[Build Bot]

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

[Blaze Burg]

Comment on attachment 302682 [details]
Patch

r=me, I'll fix the changelog.

[Blaze Burg]

Created attachment 305759 [details]
For Landing

[WebKit Commit Bot]

Comment on attachment 305759 [details]
For Landing

Clearing flags on attachment: 305759

Committed r214551: <http://trac.webkit.org/changeset/214551>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.