Bug 38807 - Web Inspector: Searching in multiple scripts in the scripts tab
Summary: Web Inspector: Searching in multiple scripts in the scripts tab
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Vsevolod Vlasov
URL:
Keywords:
Depends on: 70005
Blocks:
  Show dependency treegraph
 
Reported: 2010-05-08 15:37 PDT by Andrew Moedinger
Modified: 2011-10-13 00:00 PDT (History)
11 users (show)

See Also:


Attachments
Patch (42.17 KB, patch)
2011-10-10 12:45 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (rebaselined) (42.08 KB, patch)
2011-10-10 12:53 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Search example (screenshot) (186.65 KB, image/png)
2011-10-12 08:37 PDT, Vsevolod Vlasov
no flags Details
Patch (37.67 KB, patch)
2011-10-12 08:58 PDT, Vsevolod Vlasov
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Moedinger 2010-05-08 15:37:31 PDT
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.
Comment 1 Pavel Feldman 2010-05-09 01:46:17 PDT
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.
Comment 2 Tak 2010-08-10 13:31:27 PDT
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.
Comment 3 Pavel Feldman 2011-08-22 22:16:41 PDT
The proposed solution is a search results pane (console-alike drawer) that we are planning on implementing.
Comment 4 Vsevolod Vlasov 2011-10-10 12:45:37 PDT
Created attachment 110382 [details]
Patch
Comment 5 Vsevolod Vlasov 2011-10-10 12:53:29 PDT
Created attachment 110385 [details]
Patch (rebaselined)
Comment 6 Pavel Feldman 2011-10-11 03:06:46 PDT
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.
Comment 7 Vsevolod Vlasov 2011-10-12 08:37:33 PDT
Created attachment 110688 [details]
Search example (screenshot)
Comment 8 Vsevolod Vlasov 2011-10-12 08:48:07 PDT
(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.
Comment 9 Vsevolod Vlasov 2011-10-12 08:58:18 PDT
Created attachment 110689 [details]
Patch
Comment 10 Pavel Feldman 2011-10-12 09:04:02 PDT
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.
Comment 11 Vsevolod Vlasov 2011-10-12 09:23:23 PDT
Committed r97267: <http://trac.webkit.org/changeset/97267>