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
<rdar://problem/37088644>
Created attachment 332831 [details] Patch
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 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 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.
(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 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 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.
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 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.
Created attachment 332921 [details] Patch
(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 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;
(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.
Created attachment 333991 [details] Patch
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 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.
(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.
Created attachment 334059 [details] Patch for landing
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 on attachment 334059 [details] Patch for landing Clearing flags on attachment: 334059 Committed r228581: <https://trac.webkit.org/changeset/228581>
All reviewed patches have been landed. Closing bug.