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.
<rdar://problem/35551541>
<rdar://problem/35551542>
(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.
Another consequence of this regression is that `contentBrowser` isn't passed to any of the navigation sidebar panels during construction.
(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.
Created attachment 326970 [details] Patch
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.
Created attachment 327024 [details] Patch
(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.
Comment on attachment 327024 [details] Patch r=me
Comment on attachment 327024 [details] Patch Clearing flags on attachment: 327024 Committed r224905: <https://trac.webkit.org/changeset/224905>
All reviewed patches have been landed. Closing bug.