Bug 38807 – Web Inspector: Searching in multiple scripts in the scripts tab

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Andrew Moedinger]

A few script tab searching requests:
- Searching in the scripts tab of the dev tools doesn't work across script inputs. One must select each script and then press enter in the search box to search one at a time.

- It would be nice if there were a way to search backwards and forwards. Either with buttons or shift+enter/ctrl+enter.

Let me know if I should separate these into different bugs or file this under a different component.

[Pavel Feldman]

There are two different search capabilities in general. Search current view (cmd f) and search all sources (specific command). While we implement first only, second requires a search results pane we don't currently have. I think your request should be converted into one requesting such pane.

A workaround for you would be to search from the resources panel. We have a way of showing search results for multiple files there.

[Tak]

I'd also like to chime in for the Script Panel search to search across all scripts that are loading instead of just the one in the currently selected view.

I'd argue that it doesnt need a general search results panel.  

As an example of the experience -- I'm quite happy with Firebug's search experience:  It searches the currently selected view, then moves on to the next. I havent noticed a way to see how many matches there are, but this small inconvenience is more than made up for by the utility of not having to select each of the (dozens) of script views that were loaded for our particular app as I try and find the segment of code I'm looking for.

I'd be happy to Cmd-G / Enter my way across the search results, having the currently selected script view change as I advance to the next result.

[Pavel Feldman]

The proposed solution is a search results pane (console-alike drawer) that we are planning on implementing.

[Vsevolod Vlasov]

Created attachment 110382 [details]
Patch

[Vsevolod Vlasov]

Created attachment 110385 [details]
Patch (rebaselined)

[Pavel Feldman]

Comment on attachment 110385 [details]
Patch (rebaselined)

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

> Source/WebCore/ChangeLog:10
> +        with Ctrl+Shift+F (Cmd+Shift+F) shortcut which is currently disabled.

Could you attach a screenshot with the search results pane even though it is currently disabled?

> Source/WebCore/inspector/front-end/AdvancedSearchController.js:42
> +    return WebInspector.KeyboardShortcut.makeDescriptor("f", WebInspector.KeyboardShortcut.Modifiers.Ctrl | WebInspector.KeyboardShortcut.Modifiers.Shift);

Drive-by: we usually do CtrlOrMeta variable. Should we make it a part of the keyboardShortcut?

> Source/WebCore/inspector/front-end/AdvancedSearchController.js:53
> +     * @param {!Event} event

Here and below: drop ! we are non-nullable by default.

> Source/WebCore/inspector/front-end/AdvancedSearchController.js:74
> +            this._searchView.wasShown();

You should never call wasShown explicitly.

> Source/WebCore/inspector/front-end/AdvancedSearchController.js:90
> +        this._searchView.resultsView = this._searchResultsView; 

You don't really need different views, do you? I'd say you only need one that is placed in the drawer.

> Source/WebCore/inspector/front-end/Drawer.js:57
> +WebInspector.Drawer.AnimationType = {

This can land as a separate patch.

> Source/WebCore/inspector/front-end/ScriptsPanel.js:1201
> +    canPerformAdvancedSearch: function()

I don't think that advanced search should be panel specific. You probably need to put this code into a separate class that depends on the debugger presentation model.

> Source/WebCore/inspector/front-end/ScriptsPanel.js:1356
> +WebInspector.ScriptsPanelSearchResultsView = function(panel)

Also could be defined outside the scripts panel.

> Source/WebCore/inspector/front-end/Settings.js:92
> +    this.advancedSearchOptions = this.createSetting("advancedSearchOptions", {queries: [], ignoreCase: true, isRegex: false});

Could you define explicit type for the second parameter (SearchConfig)?

> Source/WebCore/inspector/front-end/inspector.css:4139
> +.search-view .search-panel label {

These are very expensive selectors. They start matching by label, input, input of particular type, etc. Given that we don't use search that much, I'd suggest implementing search results view in the iframe with its own css.

[Vsevolod Vlasov]

Created attachment 110688 [details]
Search example (screenshot)

[Vsevolod Vlasov]

(In reply to comment #6)
> (From update of attachment 110385 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110385&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        with Ctrl+Shift+F (Cmd+Shift+F) shortcut which is currently disabled.
> 
> Could you attach a screenshot with the search results pane even though it is currently disabled?
I attached a chromium linux screenshot. We did not tune design yet, so I assumed it gives a good enough overview on the way search was implemented.
> 
> > Source/WebCore/inspector/front-end/AdvancedSearchController.js:42
> > +    return WebInspector.KeyboardShortcut.makeDescriptor("f", WebInspector.KeyboardShortcut.Modifiers.Ctrl | WebInspector.KeyboardShortcut.Modifiers.Shift);
> 
> Drive-by: we usually do CtrlOrMeta variable. Should we make it a part of the keyboardShortcut?
It's already there, I am using it now.

> > Source/WebCore/inspector/front-end/AdvancedSearchController.js:53
> > +     * @param {!Event} event
> 
> Here and below: drop ! we are non-nullable by default.
Done.
 
> > Source/WebCore/inspector/front-end/AdvancedSearchController.js:74
> > +            this._searchView.wasShown();
> 
> You should never call wasShown explicitly.
Done.

> > Source/WebCore/inspector/front-end/AdvancedSearchController.js:90
> > +        this._searchView.resultsView = this._searchResultsView; 
> 
> You don't really need different views, do you? I'd say you only need one that is placed in the drawer.
Done, SearchResultsPane contains an element with results now.


> > Source/WebCore/inspector/front-end/Drawer.js:57
> > +WebInspector.Drawer.AnimationType = {
> 
> This can land as a separate patch.
This was landed. https://bugs.webkit.org/show_bug.cgi?id=69831

> > Source/WebCore/inspector/front-end/ScriptsPanel.js:1201
> > +    canPerformAdvancedSearch: function()
> 
> I don't think that advanced search should be panel specific. You probably need to put this code into a separate class that depends on the debugger presentation model.
Moved this logic to a new SearchScope inteface.

> > Source/WebCore/inspector/front-end/ScriptsPanel.js:1356
> > +WebInspector.ScriptsPanelSearchResultsView = function(panel)
> 
> Also could be defined outside the scripts panel.
This is now part of ScriptsSearchScope.

> > Source/WebCore/inspector/front-end/Settings.js:92
> > +    this.advancedSearchOptions = this.createSetting("advancedSearchOptions", {queries: [], ignoreCase: true, isRegex: false});
> 
> Could you define explicit type for the second parameter (SearchConfig)?
Done.

> > Source/WebCore/inspector/front-end/inspector.css:4139
> > +.search-view .search-panel label {
> 
> These are very expensive selectors. They start matching by label, input, input of particular type, etc. Given that we don't use search that much, I'd suggest implementing search results view in the iframe with its own css.
Removed these selectors for now as ignoreCase and isRegex checkboxes are not part of this change anymore.

[Vsevolod Vlasov]

Created attachment 110689 [details]
Patch

[Pavel Feldman]

Comment on attachment 110689 [details]
Patch

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

> Source/WebCore/inspector/front-end/inspector.css:4182
> +#search-results-pane-file-based .search-match:not(:hover) .webkit-line-number {

this will match against all line-number elements in the textview. consider optimizing.

[Vsevolod Vlasov]

Committed r97267: <http://trac.webkit.org/changeset/97267>