Bug 182342 - Web Inspector: TabBar redesign: remove New Tab button and add experimental feature flag
Summary: Web Inspector: TabBar redesign: remove New Tab button and add experimental fe...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Matt Baker
URL:
Keywords: InRadar
Depends on:
Blocks: 181611 182413 182353
  Show dependency treegraph
 
Reported: 2018-01-31 11:21 PST by Matt Baker
Modified: 2018-02-02 13:04 PST (History)
5 users (show)

See Also:


Attachments
Patch (50.98 KB, patch)
2018-01-31 11:50 PST, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (68.42 KB, patch)
2018-02-01 13:31 PST, Matt Baker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 2018-01-31 11:21:57 PST
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.
Comment 1 Radar WebKit Bug Importer 2018-01-31 11:22:30 PST
<rdar://problem/37078662>
Comment 2 Matt Baker 2018-01-31 11:50:21 PST
Created attachment 332789 [details]
Patch
Comment 3 Devin Rousso 2018-01-31 22:23:44 PST
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.
Comment 4 Timothy Hatcher 2018-02-01 09:25:33 PST
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.
Comment 5 Matt Baker 2018-02-01 11:07:30 PST
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.
Comment 6 Matt Baker 2018-02-01 13:31:46 PST
Created attachment 332905 [details]
Patch
Comment 7 Devin Rousso 2018-02-01 22:59:36 PST
Comment on attachment 332905 [details]
Patch

r=me.  Nice job!
Comment 8 WebKit Commit Bot 2018-02-02 01:02:37 PST
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
Comment 9 WebKit Commit Bot 2018-02-02 13:04:57 PST
Comment on attachment 332905 [details]
Patch

Clearing flags on attachment: 332905

Committed r228024: <https://trac.webkit.org/changeset/228024>
Comment 10 WebKit Commit Bot 2018-02-02 13:04:59 PST
All reviewed patches have been landed.  Closing bug.