Bug 162515 – Web Inspector: Pretty print / type info / code coverage buttons disappear after switching tabs

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

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.

[Radar WebKit Bug Importer]

<rdar://problem/28455322>

[Nikita Vasilyev]

I was able to reproduce it.

This isn't a recent regression. I reproduced it in Safari 10 (macOS Sierra).

[Nikita Vasilyev]

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

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

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

[Nikita Vasilyev]

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?

[Nikita Vasilyev]

Created attachment 311562 [details]
Patch

This patch is using the same approach as the previous one, but I fixed the assert.

[Joseph Pecoraro]

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.

[Nikita Vasilyev]

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?".

[Nikita Vasilyev]

Created attachment 312527 [details]
Patch

[Joseph Pecoraro]

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"

[Nikita Vasilyev]

Created attachment 312940 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 312940 [details]
Patch

Clearing flags on attachment: 312940

Committed r218305: <http://trac.webkit.org/changeset/218305>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.