Bug 143442 – Web Inspector: Add a WebInspector.TabBar class

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

[Timothy Hatcher]

For a new UX we need a tab bar. Lets add one.

[Timothy Hatcher]

Created attachment 250203 [details]
Patch

[Tobias Reiss]

Comment on attachment 250203 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:87
> +            return null;

For the sake of being consistent: return;

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:136
> +            setTimeout(removeStyles.bind(this), WebInspector.TabBar.AnimationDurtation);

What about `this._element.addEventListener('webkitTransitionEnd', removeStyles.bind(this))`?
 - setTimeout is not synchronized with rAF, which means `removeStyles` is either called too late or too early
 - get rid of `WebInspector.TabBar.AnimationDurtation`. You don't need to maintain the duration in both, CSS and JS file.

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:251
> +            setTimeout(removeStyles.bind(this), WebInspector.TabBar.AnimationDurtation);

Ditto

[Timothy Hatcher]

Comment on attachment 250203 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/TabBar.js:87
>> +            return null;
> 
> For the sake of being consistent: return;

Actually the rest should be return null, since the last return is "return tabBarItem;".

>> Source/WebInspectorUI/UserInterface/Views/TabBar.js:136
>> +            setTimeout(removeStyles.bind(this), WebInspector.TabBar.AnimationDurtation);
> 
> What about `this._element.addEventListener('webkitTransitionEnd', removeStyles.bind(this))`?
>  - setTimeout is not synchronized with rAF, which means `removeStyles` is either called too late or too early
>  - get rid of `WebInspector.TabBar.AnimationDurtation`. You don't need to maintain the duration in both, CSS and JS file.

I did this. Thanks!

>> Source/WebInspectorUI/UserInterface/Views/TabBar.js:251
>> +            setTimeout(removeStyles.bind(this), WebInspector.TabBar.AnimationDurtation);
> 
> Ditto

Same. And in other other place.

[Joseph Pecoraro]

Comment on attachment 250203 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Protocol/InspectorFrontendHostStub.js:38
> +if (!window.Symbol) {
> +    window.Symbol = function(string)
> +    {
> +        return string;
> +    }
> +}

I think this should return an object, for uniqueness. Maybe this will work better:

    window.Symbol = function(name)
    {
        name = name || "";
        return {name};
    }

> Source/WebInspectorUI/UserInterface/Views/TabBar.css:2
> + * Copyright (C) 2013 Apple Inc. All rights reserved.

Nit: 2015

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:38
> +        var topBorderElement = document.createElement("div");
> +        topBorderElement.classList.add("top-border");
> +        this._element.appendChild(topBorderElement);

Style: I've been combining the appendChild and createElement lines if there isn't anything special going on:

    var topBorderElement = this._element.appendChild(document.createElement("div"));
    topBorderElement.classList.add("top-border");

Worth continuing that style?

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:127
> +        tabBarItem.element.style.left = null;
> +        tabBarItem.element.style.width = null;

Whoa, I didn't know this worked!

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:195
> +            if (!nextTabBarItem || nextTabBarItem.pinned) {
> +                console.error("One normal tab is always required in a WebInspector.TabBar.")
> +                return;
> +            }

Style: Missing semicolon on line 193.

Should the constructor guarantee this? Is this counting the "newTabItem" which doesn't seem to be required?

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:289
> +            delete this._updateLayoutTimeout;

Lets avoid delete and just assign undefined here.

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:447
> +            this._element.classList.remove("static-layout");

"static-layout" is showing up in lots of places. Maybe it should be a constant?

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:486
> +        // Only handle left mouse clicks.
> +        if (event.button !== 0)
> +            return;

Likewise event.ctrlKey. In fact we may want to consider a Context Menu on tabs.

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:635
> +        // FIXME: Is this a WebKit bug or correct behavior?

I have had a lot of trouble with "mouseleave" because of this. Maybe "mouseout" would be better.

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:657
> +WebInspector.TabBar.AnimationDurtation = 250;

Typo: "Durtation" => "Duration"

> Source/WebInspectorUI/UserInterface/Views/TabBarItem.js:145
> +    updateLayout(expandOnly)
> +    {
> +        // Implemented by subclasses.
> +    }

Not called yet?

[Timothy Hatcher]

r183322 – r183342

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