Bug 144185 – Web Inspector: New Toolbar UI for tabs

• Component: Web Inspector
• Status: RESOLVED FIXED

[Timothy Hatcher]

.

[Timothy Hatcher]

Created attachment 251611 [details]
Patch

[Timothy Hatcher]

Created attachment 251612 [details]
Screenshot

[Joseph Pecoraro]

Comment on attachment 251611 [details]
Patch

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

Looks good! r=me

> Source/WebInspectorUI/UserInterface/Base/Main.js:251
> +    this._closeToolbarButton = new WebInspector.ControlToolbarItem("dock-close", WebInspector.UIString("Close"), platformImagePath("Close.svg"), 16, 14);

Screenshot of an attached case?

> Source/WebInspectorUI/UserInterface/Base/Main.js:276
> +    this.notifications.addEventListener(WebInspector.Notification.PageArchiveStarted, this._pageArchiveStarted, this);
> +    this.notifications.addEventListener(WebInspector.Notification.PageArchiveEnded, this._pageArchiveEnded, this);

Technically since WebInspector is the one dispatching it and since it is the only listener, we could drop the events and just inline the handlers at the dispatch sites.

> Source/WebInspectorUI/UserInterface/Base/Main.js:1322
> +    WebInspector.archiveMainFrame();

Normally we use "this" inside WebInspector static functions. So this.archiveMainFrame().

> Source/WebInspectorUI/UserInterface/Base/Main.js:1333
> +    PageAgent.reload(window.event ? window.event.shiftKey : false);

Heh, this shouldn't need to use window.event if it has an event parameter passed in. This should probably drop the comment too:

    var ignoreCache = event ? event.shiftKey : false;
    PageAgent.reload(ignoreCache);

> Source/WebInspectorUI/UserInterface/Images/Network.svg:1
> +<?xml version="1.0" encoding="utf-8"?>

Unused at the moment? Fine to add, or should it be added when it gets used?

> Source/WebInspectorUI/UserInterface/Views/ActivateButtonToolbarItem.js:52
> +        if (!newLabel || !this._labelElement)

Should "get label()" also if check this._labelElement then? Maybe it would be simpler to just always create the element, even if it isn't shown at the moment.

> Source/WebInspectorUI/UserInterface/Views/Toolbar.js:26
> +WebInspector.Toolbar = function(element, navigationItems, dontAllowModeChanges) {

This boolean may read slightly better as "disallowModeChanges". Or, since this is the only toolbar and thus only one call site flips it to be "allowModeChanges".

> Source/WebInspectorUI/UserInterface/Views/Toolbar.js:41
> +    this._centerLeftSectionElement.className = WebInspector.Toolbar.ItemSectionStyleClassName + " " + WebInspector.Toolbar.CenterLeftItemSectionStyleClassName;

Probably cleaner with classList:

    this._centerLeftSectionElement.classList.add(WebInspector.Toolbar.ItemSectionStyleClassName, WebInspector.Toolbar.CenterLeftItemSectionStyleClassName);

> Source/WebInspectorUI/UserInterface/Views/Toolbar.js:49
> +    this._centerRightSectionElement.className = WebInspector.Toolbar.ItemSectionStyleClassName + " " + WebInspector.Toolbar.CenterRightItemSectionStyleClassName;

Ditto

[Timothy Hatcher]

r183322 – r183342

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