Bug 182342 – Web Inspector: TabBar redesign: remove New Tab button and add experimental feature flag

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

[Matt Baker]

Summary:
Remove New Tab button and add experimental feature flag. Add LegacyTabBar to retain the existing behavior. For now the same CSS file can be used by both bars, but follow-up TabBar redesign patches will require that a LegacyTabBar.css be added.

[Radar WebKit Bug Importer]

<rdar://problem/37078662>

[Matt Baker]

Created attachment 332789 [details]
Patch

[Devin Rousso]

Comment on attachment 332789 [details]
Patch

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

Overall, it looks good.  I'd like to see the CSS/contextmenu changes I mentioned before I give r+, but I don't see any major issues.

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:172
> +        if (this.normalTabCount === 1)
> +            return null;

In addition to this check, you should add some CSS to make it so that if the last tab left is ephemeral, the close button isn't even visible.  All that's necessary is adding a few `:not(.single-tab)` to a few selectors in TabBar.css, although I'm not sure which.  The same is true of the contextmenu item "Close Tab", which is added in GeneralTabBarItem.js.

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:787
> +            }, !!openTabBarItem, disabled);

I think you meant to used `checked` instead of `!!openTabBarItem` here.

[Timothy Hatcher]

Comment on attachment 332789 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/TabBar.js:172
>> +            return null;
> 
> In addition to this check, you should add some CSS to make it so that if the last tab left is ephemeral, the close button isn't even visible.  All that's necessary is adding a few `:not(.single-tab)` to a few selectors in TabBar.css, although I'm not sure which.  The same is true of the contextmenu item "Close Tab", which is added in GeneralTabBarItem.js.

Yep, CSS changes would be needed for that case. In that case, it is likely best to fork the CSS file instead of adding additional CSS selectors and complicating the file. It will be easier to remove the legacy CSS file, than the selector complexities when this experiment is done.

[Matt Baker]

Comment on attachment 332789 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/TabBar.js:172
>>> +            return null;
>> 
>> In addition to this check, you should add some CSS to make it so that if the last tab left is ephemeral, the close button isn't even visible.  All that's necessary is adding a few `:not(.single-tab)` to a few selectors in TabBar.css, although I'm not sure which.  The same is true of the contextmenu item "Close Tab", which is added in GeneralTabBarItem.js.
> 
> Yep, CSS changes would be needed for that case. In that case, it is likely best to fork the CSS file instead of adding additional CSS selectors and complicating the file. It will be easier to remove the legacy CSS file, than the selector complexities when this experiment is done.

Nice catch Devin, I just tried:

1. Close all normal tabs except one (Elements, for example)
2. Do a search
3. Close Elements tab
4. Last open tab is ephemeral!

I think a better approach would be to prevent step 3 above.

[Matt Baker]

Created attachment 332905 [details]
Patch

[Devin Rousso]

Comment on attachment 332905 [details]
Patch

r=me.  Nice job!

[WebKit Commit Bot]

Comment on attachment 332905 [details]
Patch

Rejecting attachment 332905 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 332905, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
rdparty/autoinstalled/mechanize/_urllib2_fork.py", line 332, in _call_chain
    result = func(*args)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1170, in https_open
    return self.do_open(conn_factory, req)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1118, in do_open
    raise URLError(err)
urllib2.URLError: <urlopen error [Errno 60] Operation timed out>

Full output: http://webkit-queues.webkit.org/results/6328509

[WebKit Commit Bot]

Comment on attachment 332905 [details]
Patch

Clearing flags on attachment: 332905

Committed r228024: <https://trac.webkit.org/changeset/228024>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.