Bug 182353 – Web Inspector: TabBar redesign: remove top-level search field and pin the Search tab

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

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

[Radar WebKit Bug Importer]

<rdar://problem/37088644>

[Matt Baker]

Created attachment 332831 [details]
Patch

[Devin Rousso]

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.

[Timothy Hatcher]

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.

[Timothy Hatcher]

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.

[Blaze Burg]

(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.

[Matt Baker]

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.

[Matt Baker]

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.

[Matt Baker]

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.

[Matt Baker]

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.

[Matt Baker]

Created attachment 332921 [details]
Patch

[Devin Rousso]

(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.

[Devin Rousso]

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;

[Matt Baker]

(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.

[Matt Baker]

Created attachment 333991 [details]
Patch

[Devin Rousso]

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).

[Matt Baker]

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.

[Matt Baker]

(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.

[Matt Baker]

Created attachment 334059 [details]
Patch for landing

[WebKit Commit Bot]

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

[WebKit Commit Bot]

Comment on attachment 334059 [details]
Patch for landing

Clearing flags on attachment: 334059

Committed r228581: <https://trac.webkit.org/changeset/228581>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.