[webkit-reviews] review denied: [Bug 162515] Web Inspector: Pretty print / type info / code coverage buttons disappear after switching tabs : [Attachment 311562] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 162515: Web Inspector: Pretty print / type info / code coverage buttons
disappear after switching tabs
https://bugs.webkit.org/show_bug.cgi?id=162515

Attachment 311562: Patch

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




--- Comment #9 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 311562
  --> https://bugs.webkit.org/attachment.cgi?id=311562
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.