Bug 179717 – Web Inspector: REGRESSION (r217750): Navigation sidebar broken after closing and re-opening tab

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

Summary:
Navigation sidebar broken after closing and re-opening tab.

Steps to Reproduce:
1. Goto Debugger tab
2. Toggle "disable all breakpoints" in the navigation bar
3. Close and then re-open the Debugger tab
4. Toggle "disable all breakpoints"
  => Nothing happens

The regression was caused by http://trac.webkit.org/changeset/220114:
<https://webkit.org/b/174484> Web Inspector: create Recording tab for displaying recordings

Navigation sidebar creation was changed to use WI.instanceForClass, which keeps the sidebar around instead of recreating it each time. Classes implementing closed() expect to be destroyed and recreated. When the Debugger tab is closed the sidebar panel removes its event listeners. When the Debugger tab is reopened, the sidebar returns as a zombie.

[Radar WebKit Bug Importer]

<rdar://problem/35551541>

[Radar WebKit Bug Importer]

<rdar://problem/35551542>

[Matt Baker]

(In reply to Matt Baker from comment #0)
> Summary:
> Navigation sidebar broken after closing and re-opening tab.
> 
> Steps to Reproduce:
> 1. Goto Debugger tab
> 2. Toggle "disable all breakpoints" in the navigation bar
> 3. Close and then re-open the Debugger tab
> 4. Toggle "disable all breakpoints"
>   => Nothing happens
> 
> The regression was caused by http://trac.webkit.org/changeset/220114:
> <https://webkit.org/b/174484> Web Inspector: create Recording tab for
> displaying recordings

Wrong bug!

It should be http://trac.webkit.org/changeset/217750:
<https://webkit.org/b/172621> Web Inspector: Don't create NavigationSidebarPanel classes until they are needed by a Tab


> Navigation sidebar creation was changed to use WI.instanceForClass, which
> keeps the sidebar around instead of recreating it each time. Classes
> implementing closed() expect to be destroyed and recreated. When the
> Debugger tab is closed the sidebar panel removes its event listeners. When
> the Debugger tab is reopened, the sidebar returns as a zombie.

[Matt Baker]

Another consequence of this regression is that `contentBrowser` isn't passed to any of the navigation sidebar panels during construction.

[Matt Baker]

(In reply to Matt Baker from comment #4)
> Another consequence of this regression is that `contentBrowser` isn't passed
> to any of the navigation sidebar panels during construction.

Okay, while it's true it caused the constructor arg to be undefined, ContentBrowserTabContentView.prototype.show included a workaround to set the content browser.

[Matt Baker]

Created attachment 326970 [details]
Patch

[Devin Rousso]

Comment on attachment 326970 [details]
Patch

r-.  The original intent of this change was twofold:
    1) ensure that each sidebar panel is only created once (really only an issue for DetailsSidebarPanel)
    2) allow lazy instantiation of sidebar panels so that when each tab is created, we don't need to do extra work until the tab is shown

This patch will reintroduce (2), and I don't see this as a positive change.  I think that we should either:
    - refactor the existing NavigationSidebarPanels to use shown/hidden (moving any addEventListener to shown) instead of closed (Debugger, Resources, Search, and Storage)
    - allow multiple instances of the sidebars, but still instantiate them lazily (basically have two member variables, one for the constructors and one for the constructed SidebarPanels)

I think the first option is the more future-forward approach, as I don't think we should ever really be doing work in a UI class that isn't visible (I like what we do with FolderizedTreeElement), but the second option is definitely the "faster" solution.

[Matt Baker]

Created attachment 327024 [details]
Patch

[Matt Baker]

(In reply to Devin Rousso from comment #7)
> Comment on attachment 326970 [details]
> Patch
> 
> r-.  The original intent of this change was twofold:
>     1) ensure that each sidebar panel is only created once (really only an
> issue for DetailsSidebarPanel)
>     2) allow lazy instantiation of sidebar panels so that when each tab is
> created, we don't need to do extra work until the tab is shown
> 
> This patch will reintroduce (2), and I don't see this as a positive change. 
> I think that we should either:
>     - refactor the existing NavigationSidebarPanels to use shown/hidden
> (moving any addEventListener to shown) instead of closed (Debugger,
> Resources, Search, and Storage)
>     - allow multiple instances of the sidebars, but still instantiate them
> lazily (basically have two member variables, one for the constructors and
> one for the constructed SidebarPanels)

I went with this option.

[Devin Rousso]

Comment on attachment 327024 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 327024 [details]
Patch

Clearing flags on attachment: 327024

Committed r224905: <https://trac.webkit.org/changeset/224905>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.