Bug 162515 - Web Inspector: Pretty print / type info / code coverage buttons disappear after switching tabs
Summary: Web Inspector: Pretty print / type info / code coverage buttons disappear aft...
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: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-09-23 14:54 PDT by Matt Baker
Modified: 2017-06-14 18:44 PDT (History)
4 users (show)

See Also:


Attachments
Patch (7.89 KB, patch)
2017-05-26 20:49 PDT, Nikita Vasilyev
nvasilyev: review-
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
Patch (8.67 KB, patch)
2017-05-30 17:44 PDT, Nikita Vasilyev
joepeck: review-
Details | Formatted Diff | Diff
Patch (6.62 KB, patch)
2017-06-02 13:28 PDT, Nikita Vasilyev
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
Patch (5.21 KB, patch)
2017-06-09 18:36 PDT, Nikita Vasilyev
joepeck: review+
Details | Formatted Diff | Diff
Patch (5.21 KB, patch)
2017-06-14 18:06 PDT, Nikita Vasilyev
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 2016-09-23 14:54:23 PDT
Summary:
Pretty print / type info / code coverage buttons disappear after switching tabs.

Steps to Reproduce:
1. Inspect data:text/html,<script>debugger;</script>
2. Reload page
  => Pause in Debugger tab
3. Switch to Resources tab
  => Buttons disappear

The problem affects the Debugger tab as well, with buttons disappearing after some combination of reloading the page and switching tabs. However I couldn't reliably reproduce this.
Comment 1 Radar WebKit Bug Importer 2016-09-23 14:54:51 PDT
<rdar://problem/28455322>
Comment 2 Nikita Vasilyev 2016-11-09 13:23:59 PST
I was able to reproduce it.

This isn't a recent regression. I reproduced it in Safari 10 (macOS Sierra).
Comment 3 Nikita Vasilyev 2017-05-25 18:58:35 PDT
    removeNavigationItem (NavigationBar.js:114)
    insertNavigationItem (NavigationBar.js:70)
    (anonymous function) (ContentBrowser.js:407)
    forEach
    _updateContentViewNavigationItems (ContentBrowser.js:399)
    _currentContentViewDidChange (ContentBrowser.js:483)
    dispatch (Object.js:170)
    dispatchEventToListeners (Object.js:177)
    showBackForwardEntryForIndex (ContentViewContainer.js:166)
    showContentView (ContentViewContainer.js:138)
    showDefaultContentView (ResourceSidebarPanel.js:104)
    shown (ContentBrowserTabContentView.js:103)
    prepareToShow (BackForwardEntry.js:86)
    _showEntry (ContentViewContainer.js:445)
    showBackForwardEntryForIndex (ContentViewContainer.js:162)
    showContentView (ContentViewContainer.js:138)
    _tabBarItemSelected (TabBrowser.js:239)
    dispatch (Object.js:170)
    dispatchEventToListeners (Object.js:177)
    selectedTabBarItem (TabBar.js:376)
    _handleMouseDown (TabBar.js:569)
    _handleMouseDown

Looks like when a navigation item is inserted, it gets removed from the previous navigationBar.

    insertNavigationItem(navigationItem, index, parentElement)
    {
        console.assert(navigationItem instanceof WebInspector.NavigationItem);
        if (!(navigationItem instanceof WebInspector.NavigationItem))
            return null;

        if (navigationItem.parentNavigationBar)
            navigationItem.parentNavigationBar.removeNavigationItem(navigationItem);
Comment 4 Nikita Vasilyev 2017-05-26 19:37:14 PDT
The bug reproduces when switching from Debugger to Resources (or vice versa) and when (and only when) the both tabs have the same selected script file.

_updateContentViewNavigationItems doesn't necessarily get called when switching tabs. Always calling it when switching tabs would be one way of solving this.

Another way would be to clone navigation items before attaching them to the new content view.
Comment 5 Nikita Vasilyev 2017-05-26 20:49:57 PDT
Created attachment 311405 [details]
Patch

(In reply to Nikita Vasilyev from comment #4)
> The bug reproduces when switching from Debugger to Resources (or vice versa)
> and when (and only when) the both tabs have the same selected script file.
> 
> _updateContentViewNavigationItems doesn't necessarily get called when
> switching tabs. Always calling it when switching tabs would be one way of
> solving this.

I tried this approach and it worked.

I made updateContentViewNavigationItems public. I'll try to find a more elegant way of fixing it that doesn't require making _updateContentViewNavigationItems public.
Comment 6 Nikita Vasilyev 2017-05-27 20:31:17 PDT
My patch introduced a failing assert:

    [Error] Assertion Failed: Cannot remove item with unexpected parent bar. – ActivateButtonNavigationItem
    removeNavigationItem (NavigationBar.js:107)
    _removeAllNavigationItems (ContentBrowser.js:435)
    updateContentViewNavigationItems (ContentBrowser.js:326)
    (anonymous function) (ContentBrowser.js:101)
    dispatch (Object.js:170)
    dispatchEventToListeners (Object.js:185)
    _tabBarItemSelected (TabBrowser.js:262)
    dispatch (Object.js:170)
    dispatchEventToListeners (Object.js:177)
    selectedTabBarItem (TabBar.js:376)
    _handleMouseDown (TabBar.js:569)
    _handleMouseDown
Comment 7 Nikita Vasilyev 2017-05-27 20:40:56 PDT
I don't understand why each tab has its own NavigationBar instance, but NavigationItem instances are shared (get removed from one NavigationBar and attached to another). Is it an inheritance from pre-tabs UI? If so, does it make sense to stop moving NavigationItem instances from one NavigationBar to another now?
Comment 8 Nikita Vasilyev 2017-05-30 17:44:11 PDT
Created attachment 311562 [details]
Patch

This patch is using the same approach as the previous one, but I fixed the assert.
Comment 9 Joseph Pecoraro 2017-05-31 14:27:24 PDT
Comment on attachment 311562 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:10
> +        Pretty print, type info, and code coverage buttons disappear because NavigationBar#insertNavigationItem
> +        removes them from NavigationBar of the previous tab. Switching tabs does not update ContentBrowser's

Lets provide a clearer description of the problem.

    Each ContentView owns a list of NavigationItems. When the ContentView moves across
    Tabs (ContentBrowsers) it removes its NavigationItems from the old Tab's NavigationBar
    and adds them to the new Tab's NavigationBar. When switching back to the original tab
    the ContentView is restored, but its NavigationItems are not carried back.

Just writing this really makes me think that Tombstone restoration should solve this problem by dispatching a WebInspector.ContentView.Event.NavigationItemsDidChange event in the right place. The changes below make it the ContentBrowser's responsibility that when it is shown/reshown that it updates its ContentView. This is potentially a waste of work if the ContentView was not a Tombstone. I really think we should take another look at the Tombstone restoration path.

> Source/WebInspectorUI/UserInterface/Views/ContentBrowser.js:292
> +        var currentContentView = this.currentContentView;

Nit: Lets upgrade this to `let`

> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:254
> +        const forceUpdate = true;
> +        tabContentView.contentBrowser.updateContentViewNavigationItems(forceUpdate);

This has a couple minor issues:

• Code above and below assume tabContentView can be null, so this would throw an exception in that case.
• This assumes that the tabContentView has a contentBrowser, which is not always the case (NetTabContentView, SettingsTabContentView).

---

If we can't solve this with tombstone restoration lets take this opportunity to generalize this (selecting a tab) as an action on TabContentView. We already call tabContentView.updateLayout below, lets create something new.

How about:

    tabContentView.updateCurrentContentViewComponents()

The default TabContentView can have an empty implementation. ContentBrowserTabContentView can then forward that on to the ContentBrowser:

    contentBrowser.updateCurrentContentViewComponents()

Which can just call into the existing private function (_updateContentViewNavigationItems). We don't seem to have to update path components right now, but in the future if we needed to we could do that in here as well.

I think this is clearer for a few reasons:

    1. TabBrowser isn't reaching into the Tab assuming any knowledge
    2. This creates a public API documenting "Sometimes we need to update the UI coming back to a Tab / ContentBrowser for its current ContentView, and these are the operations...)

So in the case if something else runs into this problem this may be a suitable API to solve the problem.
Comment 10 Nikita Vasilyev 2017-06-02 13:28:34 PDT
Created attachment 311853 [details]
Patch

(In reply to Joseph Pecoraro from comment #9)
> ---
> 
> If we can't solve this with tombstone restoration lets take this opportunity
> to generalize this (selecting a tab) as an action on TabContentView. We
> already call tabContentView.updateLayout below, lets create something new.
> 
> How about:
> 
>     tabContentView.updateCurrentContentViewComponents()
> 
> The default TabContentView can have an empty implementation.
> ContentBrowserTabContentView can then forward that on to the ContentBrowser:
> 
>     contentBrowser.updateCurrentContentViewComponents()
> 
> Which can just call into the existing private function
> (_updateContentViewNavigationItems). We don't seem to have to update path
> components right now, but in the future if we needed to we could do that in
> here as well.
> 
> I think this is clearer for a few reasons:
> 
>     1. TabBrowser isn't reaching into the Tab assuming any knowledge
>     2. This creates a public API documenting "Sometimes we need to update
> the UI coming back to a Tab / ContentBrowser for its current ContentView,
> and these are the operations...)
> 
> So in the case if something else runs into this problem this may be a
> suitable API to solve the problem.

I did it in this patch and it worked.

I'm looking at the tombstone restoration. If it doesn't work, I'll set this patch to "r?".
Comment 11 Nikita Vasilyev 2017-06-09 18:36:02 PDT
Created attachment 312527 [details]
Patch
Comment 12 Joseph Pecoraro 2017-06-14 17:15:23 PDT
Comment on attachment 312527 [details]
Patch

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

r=me, I think this is a more correct approach!

> Source/WebInspectorUI/ChangeLog:16
> +        Add forceUpdate property that is needed when navigationItems are unchanged but navigationBar is

Nit: "Add forceUpdate property" => "Add a forceUpdate parameter"
Comment 13 Nikita Vasilyev 2017-06-14 18:06:41 PDT
Created attachment 312940 [details]
Patch
Comment 14 WebKit Commit Bot 2017-06-14 18:44:44 PDT
Comment on attachment 312940 [details]
Patch

Clearing flags on attachment: 312940

Committed r218305: <http://trac.webkit.org/changeset/218305>
Comment 15 WebKit Commit Bot 2017-06-14 18:44:46 PDT
All reviewed patches have been landed.  Closing bug.