Bug 143886 – Web Inspector: Add TabBrowser and TabContentView

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

[Timothy Hatcher]

Add the new classes for tab based UI.

[Timothy Hatcher]

Created attachment 251034 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 251034 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/ContentBrowserTabContentView.js:29
> +    if (typeof styleClassNames === "string")
> +        styleClassNames = [styleClassNames];

Heh, I hate this pattern.

> Source/WebInspectorUI/UserInterface/Views/ContentBrowserTabContentView.js:49
> +        this._showNavigationSidebarItem.addEventListener(WebInspector.ButtonNavigationItem.Event.Clicked, WebInspector.toggleNavigationSidebar, WebInspector);

WebInspector.toggleNavigationSidebar is not in this patch. It is a little weird that this needs to jump out to WebInspector though for toggling the navigation sidebar inside this Tab.

> Source/WebInspectorUI/UserInterface/Views/ContentBrowserTabContentView.js:65
> +        this._showDetailsSidebarItem.addEventListener(WebInspector.ButtonNavigationItem.Event.Clicked, WebInspector.toggleDetailsSidebar, WebInspector);

Same for "toggleDetailsSidebar".

> Source/WebInspectorUI/UserInterface/Views/ContentBrowserTabContentView.js:109
> +        WebInspector.navigationSidebar.removeEventListener(WebInspector.Sidebar.Event.CollapsedStateDidChange, this._navigationSidebarCollapsedStateDidChange, this);
> +        WebInspector.detailsSidebar.addEventListener(WebInspector.Sidebar.Event.CollapsedStateDidChange, this._detailsSidebarCollapsedStateDidChange, this);

I think this second one should also be a remove event listener. Also the WebInspector.Sidebar.Event.SidebarPanelSelected event.

> Source/WebInspectorUI/UserInterface/Views/ContentBrowserTabContentView.js:194
> +                var currentSidebarPanelIndex = currentSidebarPanels.indexOf(sidebarPanel);
> +                if (currentSidebarPanelIndex !== -1) {

Nit: You can use includes here. The index is not important.

    if (currentSidebarPanels.includes(sidebarPanel))

> Source/WebInspectorUI/UserInterface/Views/ContentBrowserTabContentView.js:215
> +        this._ignoreDetailsSidebarPanelCollapsedEvent = true;

Would be nice to have this next to the other ignore.

> Source/WebInspectorUI/UserInterface/Views/ElementsTabContentView.js:54
> +        var mainFrame = WebInspector.frameResourceManager.mainFrame;
> +        if (mainFrame)
> +            this.contentBrowser.showContentViewForRepresentedObject(mainFrame.domTree);

Do you happen to know when we if check this? When will we not have a mainFrame? JSContext inspection?

> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:233
> +        if (!tabContentView)
> +            return;
> +
> +        var navigationSidebarPanel = tabContentView.navigationSidebarPanel;
> +        if (!navigationSidebarPanel) {
> +            this._navigationSidebar.collapsed = true;
> +            return;
> +        }

These early returns will leave "this._ignoreSidebarEvents" in a bad state.

> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:267
> +        if (!tabContentView)
> +            return;
> +
> +        if (tabContentView.managesDetailsSidebarPanels) {
> +            tabContentView.showDetailsSidebarPanels();
> +            return;
> +        }
> +
> +        var detailsSidebarPanels = tabContentView.detailsSidebarPanels;
> +        if (!detailsSidebarPanels) {
> +            this._detailsSidebar.collapsed = true;
> +            return;
> +        }

Same here.

> Source/WebInspectorUI/UserInterface/Views/TabContentView.js:29
> +    console.assert(typeof styleClassNames === "string" || styleClassNames.reduce(function(previousValue, className) { return previousValue && typeof className === "string"; }, true));

Instead of the "previousValue" trick to carry a true/false all the way through, you can use Array.prototype.every:

    styleClassNames.every(function(className) { return typeof className === "string"; })

> Source/WebInspectorUI/UserInterface/Views/TabContentView.js:32
> +    console.assert(!detailsSidebarPanels || detailsSidebarPanels.reduce(function(previousValue, detailsSidebarPanel) { return previousValue && detailsSidebarPanel instanceof WebInspector.DetailsSidebarPanel; }, true));

Same here. Note these will be excellent candidates for arrow functions when/if they land.

[Timothy Hatcher]

r183322 – r183342

http://trac.webkit.org/log/?revs=183322-183342