Bug 182353 - Web Inspector: TabBar redesign: remove top-level search field and pin the Search tab
Summary: Web Inspector: TabBar redesign: remove top-level search field and pin the Sea...
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: 182342
Blocks: 181611
  Show dependency treegraph
 
Reported: 2018-01-31 14:55 PST by Matt Baker
Modified: 2018-02-16 14:06 PST (History)
8 users (show)

See Also:


Attachments
[Image] Pinned Search tab (314.58 KB, image/png)
2018-01-31 14:55 PST, Matt Baker
no flags Details
Patch (20.16 KB, patch)
2018-01-31 18:35 PST, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (20.49 KB, patch)
2018-02-01 16:03 PST, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (18.96 KB, patch)
2018-02-15 19:49 PST, Matt Baker
no flags Details | Formatted Diff | Diff
Patch for landing (18.95 KB, patch)
2018-02-16 12:37 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 14:55:52 PST
Created attachment 332804 [details]
[Image] Pinned Search tab

Summary:
Remove top-level search field and pin the Search tab.

Other improvements:
- Add keyboard shortcut to Search and Settings tab tooltips
- Update magnifying glass icon to more closely match Safari (shorten then handle slightly)
- SearchTabContentView should not be saved/restored
Comment 1 Radar WebKit Bug Importer 2018-01-31 14:56:10 PST
<rdar://problem/37088644>
Comment 2 Matt Baker 2018-01-31 18:35:08 PST
Created attachment 332831 [details]
Patch
Comment 3 Devin Rousso 2018-01-31 22:37:21 PST
Comment on attachment 332831 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/SearchTabContentView.js:46
> +            tabBarItem.representedObject = this;

You might also need to implement `static shouldSaveTab()` on WI.SearchTabContentView since it is always created in Main.js.

I ran into an error with this when switching the experimental setting on and then back off and trying to open WebInspector.
Comment 4 Timothy Hatcher 2018-02-01 09:19:51 PST
Comment on attachment 332831 [details]
Patch

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

Overall the patch looks fine. One design comment about the helper function you added.

> Source/WebInspectorUI/UserInterface/Views/PinnedTabBarItem.js:37
> +    static fromTabContentViewConstructor(constructor)

This is not good form. It muddies this model class with external view class knowledge (the tabInfo API).

The better form would be to add a `fromInfo({image, title}) { ... }` or `fromOptions(…)` (not sure what the best name would be). Then it separates the view from the model, and you can still just pass `tabInfo()` in.

Really, more of our model constructors could move to an options/info object destructuring model for the main constructor. That would be another option here, and it avoids the naming question of adding a helper constructor function. You would just have a few more places to changes in the patch.
Comment 5 Timothy Hatcher 2018-02-01 09:21:58 PST
Comment on attachment 332831 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/PinnedTabBarItem.js:37
>> +    static fromTabContentViewConstructor(constructor)
> 
> This is not good form. It muddies this model class with external view class knowledge (the tabInfo API).
> 
> The better form would be to add a `fromInfo({image, title}) { ... }` or `fromOptions(…)` (not sure what the best name would be). Then it separates the view from the model, and you can still just pass `tabInfo()` in.
> 
> Really, more of our model constructors could move to an options/info object destructuring model for the main constructor. That would be another option here, and it avoids the naming question of adding a helper constructor function. You would just have a few more places to changes in the patch.

And I forgot TabBarItem is in the Views directory. Maybe it shouldn't be, it feels more like a model since it doesn't use the DOM, it just holds values (title, image, etc.) for the TabBar view to render.
Comment 6 BJ Burg 2018-02-01 10:48:15 PST
(In reply to Timothy Hatcher from comment #5)
> Comment on attachment 332831 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332831&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Views/PinnedTabBarItem.js:37
> >> +    static fromTabContentViewConstructor(constructor)
> > 
> > This is not good form. It muddies this model class with external view class knowledge (the tabInfo API).
> > 
> > The better form would be to add a `fromInfo({image, title}) { ... }` or `fromOptions(…)` (not sure what the best name would be). Then it separates the view from the model, and you can still just pass `tabInfo()` in.
> > 
> > Really, more of our model constructors could move to an options/info object destructuring model for the main constructor. That would be another option here, and it avoids the naming question of adding a helper constructor function. You would just have a few more places to changes in the patch.
> 
> And I forgot TabBarItem is in the Views directory. Maybe it shouldn't be, it
> feels more like a model since it doesn't use the DOM, it just holds values
> (title, image, etc.) for the TabBar view to render.

I agree. It will be possible to unit test outside of Views/ as well.
Comment 7 Matt Baker 2018-02-01 11:00:49 PST
Comment on attachment 332831 [details]
Patch

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

>>>> Source/WebInspectorUI/UserInterface/Views/PinnedTabBarItem.js:37
>>>> +    static fromTabContentViewConstructor(constructor)
>>> 
>>> This is not good form. It muddies this model class with external view class knowledge (the tabInfo API).
>>> 
>>> The better form would be to add a `fromInfo({image, title}) { ... }` or `fromOptions(…)` (not sure what the best name would be). Then it separates the view from the model, and you can still just pass `tabInfo()` in.
>>> 
>>> Really, more of our model constructors could move to an options/info object destructuring model for the main constructor. That would be another option here, and it avoids the naming question of adding a helper constructor function. You would just have a few more places to changes in the patch.
>> 
>> And I forgot TabBarItem is in the Views directory. Maybe it shouldn't be, it feels more like a model since it doesn't use the DOM, it just holds values (title, image, etc.) for the TabBar view to render.
> 
> I agree. It will be possible to unit test outside of Views/ as well.

TabBarItem creates DOM elements. It's no more a model than a TreeElement.
Comment 8 Matt Baker 2018-02-01 11:21:18 PST
Comment on attachment 332831 [details]
Patch

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

>>>>> Source/WebInspectorUI/UserInterface/Views/PinnedTabBarItem.js:37
>>>>> +    static fromTabContentViewConstructor(constructor)
>>>> 
>>>> This is not good form. It muddies this model class with external view class knowledge (the tabInfo API).
>>>> 
>>>> The better form would be to add a `fromInfo({image, title}) { ... }` or `fromOptions(…)` (not sure what the best name would be). Then it separates the view from the model, and you can still just pass `tabInfo()` in.
>>>> 
>>>> Really, more of our model constructors could move to an options/info object destructuring model for the main constructor. That would be another option here, and it avoids the naming question of adding a helper constructor function. You would just have a few more places to changes in the patch.
>>> 
>>> And I forgot TabBarItem is in the Views directory. Maybe it shouldn't be, it feels more like a model since it doesn't use the DOM, it just holds values (title, image, etc.) for the TabBar view to render.
>> 
>> I agree. It will be possible to unit test outside of Views/ as well.
> 
> TabBarItem creates DOM elements. It's no more a model than a TreeElement.

`isEphemeral` will need to be specified as part of the tab info object, instead of a static TabContentView method.
Comment 9 Matt Baker 2018-02-01 13:06:09 PST
I'll fix the layering violation in https://webkit.org/b/182342, since it introduces `GeneralTabBarItem.fromTabContentViewConstructor`.

As a side observation, it seems that having TabContentView's constructor take a `tabBarItem`, which it just stores and exposes via a property is a poor design. This forces derived classes to create the item before calling `super()`, which can be awkward, and couples the view to its tab item.

An external class should associate TabContentViews with TabBarItems, using the `tabInfo` object provided by the view.
Comment 10 Matt Baker 2018-02-01 16:01:15 PST
Comment on attachment 332831 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SearchTabContentView.js:46
>> +            tabBarItem.representedObject = this;
> 
> You might also need to implement `static shouldSaveTab()` on WI.SearchTabContentView since it is always created in Main.js.
> 
> I ran into an error with this when switching the experimental setting on and then back off and trying to open WebInspector.

I'm not sure what you mean. The base class implementation of `shouldSaveTab()` returns true, which is what we want. Previously the Inspector restored the Search tab if it was selected, so we should continue to do so.

The error you ran into has been resolved. The TabBar was trying to restore the tab selection, and the saved index matched the tab picker (>>). I added a check for this, since legacy settings data could easily collide with the new button.
Comment 11 Matt Baker 2018-02-01 16:03:51 PST
Created attachment 332921 [details]
Patch
Comment 12 Devin Rousso 2018-02-01 22:52:08 PST
(In reply to Matt Baker from comment #9)
> As a side observation, it seems that having TabContentView's constructor
> take a `tabBarItem`, which it just stores and exposes via a property is a
> poor design. This forces derived classes to create the item before calling
> `super()`, which can be awkward, and couples the view to its tab item.
> 
> An external class should associate TabContentViews with TabBarItems, using
> the `tabInfo` object provided by the view.
I agree with this.  It seems very trivial to move the creation of the TabBarItem from subclasses of TabContentView to TabBrowser or TabBar.
Comment 13 Devin Rousso 2018-02-02 14:41:03 PST
Comment on attachment 332921 [details]
Patch

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

r-.  I'm not sure where this happens, but with the new setting enabled, I am unable to select any search results.  Whenever I try, it always jumps to Elements/Resources, as if I had double-clicked.  It works as expected with the experimental setting turned off.  My guess is that making the Search tab pinned means that it gets removed from the list of open tabs (`_recentTabContentViews` in TabBrowser.js), and therefore when something tries to get shown via WI.showRepresentedObject, it doesn't even consider showing it in the Search tab.

Other than this issue, it looks great =D

> Source/WebInspectorUI/UserInterface/Views/SearchTabContentView.js:30
> +        let tabBarItem;

NIT: I think our preferred style is to always assign some value to variables, even if it's just `null`.

    let tabBarItem = null;
Comment 14 Matt Baker 2018-02-02 15:00:13 PST
(In reply to Devin Rousso from comment #13)
> Comment on attachment 332921 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332921&action=review
> 
> r-.  I'm not sure where this happens, but with the new setting enabled, I am
> unable to select any search results.  Whenever I try, it always jumps to
> Elements/Resources, as if I had double-clicked.  It works as expected with
> the experimental setting turned off.  My guess is that making the Search tab
> pinned means that it gets removed from the list of open tabs
> (`_recentTabContentViews` in TabBrowser.js), and therefore when something
> tries to get shown via WI.showRepresentedObject, it doesn't even consider
> showing it in the Search tab.

I think you are correct about the root cause. Will fix.
Comment 15 Matt Baker 2018-02-15 19:49:53 PST
Created attachment 333991 [details]
Patch
Comment 16 Devin Rousso 2018-02-16 11:24:28 PST
Comment on attachment 333991 [details]
Patch

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

r=me.  This looks great!  The only other piece of feedback I can think of is that we might want to shift the Inspect button a bit now that there is a lot of empty space, unless you have other ideas :)

> Source/WebInspectorUI/UserInterface/Views/OpenResourceDialog.css:75
> +    --open-resource-dialog-search-icon-offset-start: 10px;

Since this value matches `top`, you could use the variable there as well (unless there is some other styling that I am not seeing).
Comment 17 Matt Baker 2018-02-16 12:29:14 PST
Comment on attachment 333991 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/OpenResourceDialog.css:75
>> +    --open-resource-dialog-search-icon-offset-start: 10px;
> 
> Since this value matches `top`, you could use the variable there as well (unless there is some other styling that I am not seeing).

They share the same value but are different things, so I'd rather not overload the variable.
Comment 18 Matt Baker 2018-02-16 12:30:55 PST
(In reply to Devin Rousso from comment #16)
> Comment on attachment 333991 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333991&action=review
> 
> r=me.  This looks great!  The only other piece of feedback I can think of is
> that we might want to shift the Inspect button a bit now that there is a lot
> of empty space, unless you have other ideas :)

I'm not too concerned with keeping the dashboard/toolbar area pretty during development, since this is all behind an experimental setting.
Comment 19 Matt Baker 2018-02-16 12:37:00 PST
Created attachment 334059 [details]
Patch for landing
Comment 20 WebKit Commit Bot 2018-02-16 13:24:06 PST
Comment on attachment 334059 [details]
Patch for landing

Rejecting attachment 334059 [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', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 334059, '--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/6541386
Comment 21 WebKit Commit Bot 2018-02-16 14:06:06 PST
Comment on attachment 334059 [details]
Patch for landing

Clearing flags on attachment: 334059

Committed r228581: <https://trac.webkit.org/changeset/228581>
Comment 22 WebKit Commit Bot 2018-02-16 14:06:08 PST
All reviewed patches have been landed.  Closing bug.