Bug 90659 – Web Inspector: Option to filter search based on source type in Advanced-Search

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

[sam]

Created attachment 151020 [details]
Illustration for problem statement.

It would be good to have an option to filter search based on source file types. This may help in reducing search-result-clutter.

Screen-shot, for illustration, is also attached.

Please let me know your opinions. Will like to build a patch for this if its ok.

[Pavel Feldman]

(In reply to comment #0)
> Created an attachment (id=151020) [details]
> Illustration for problem statement.
> 
> It would be good to have an option to filter search based on source file types. This may help in reducing search-result-clutter.
> 
> Screen-shot, for illustration, is also attached.
> 
> Please let me know your opinions. Will like to build a patch for this if its ok.

I'd rather do a unified search such as http://cs.chromium.org. You can type:

"file:css Foo"

and it'll search only in files containing "css".

[sam]

(In reply to comment #1)
> (In reply to comment #0)
> > Created an attachment (id=151020) [details] [details]
> > Illustration for problem statement.
> > 
> > It would be good to have an option to filter search based on source file types. This may help in reducing search-result-clutter.
> > 
> > Screen-shot, for illustration, is also attached.
> > 
> > Please let me know your opinions. Will like to build a patch for this if its ok.
> 
> I'd rather do a unified search such as http://cs.chromium.org. You can type:
> 
> "file:css Foo"
> 
> and it'll search only in files containing "css".

Hi Pavel,
tried to catch you on irc for this :).

i was just giving a thought that whether allowing a file filter using sort of wildcards will be a good use case.

for ex: 
file:*.css foo           - would search "foo" in all css files.
file:Sidebar*.js slice   - would search "slice" in all js files starting with Sidebar.
file:Sidebar*.(css|js)   - would search in all cs and js starting with Sidebar

i was trying with a mock patch for this and its seems a bit useful. 

Plz let me know your thoughts.

[Pavel Feldman]

> Hi Pavel,
> tried to catch you on irc for this :).
> 

I am there!

> i was just giving a thought that whether allowing a file filter using sort of wildcards will be a good use case.
> 
> for ex: 
> file:*.css foo           - would search "foo" in all css files.
> file:Sidebar*.js slice   - would search "slice" in all js files starting with Sidebar.
> file:Sidebar*.(css|js)   - would search in all cs and js starting with Sidebar
> 

That's exactly what cs.chromium.org would do. So I am definitely supportive here!

> i was trying with a mock patch for this and its seems a bit useful. 
> 
> Plz let me know your thoughts.

[sam]

Created attachment 152215 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 152215 [details]
Patch

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

> Source/WebCore/inspector/front-end/AdvancedSearchController.js:150
> +            var searchScopeToken = searchConfig.query.split(" ")[0].split(":");

What about
" file:*.js query"
" query file:*.js"? r- for this.

And even more tricky
" query file:*.js file:name query2"?
Codesearch supports them all and if are making this search smarter we should probably support them too.

[sam]

(In reply to comment #5)
> (From update of attachment 152215 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152215&action=review
> 
> > Source/WebCore/inspector/front-end/AdvancedSearchController.js:150
> > +            var searchScopeToken = searchConfig.query.split(" ")[0].split(":");
> 
> What about
> " file:*.js query"
> " query file:*.js"? r- for this.
> 
oh ok. I was just exploring codesearch. but got a bad query for "file:*.js query", i guess it is treating selector part as regex. "file:.*.js query" fetched  me the right results. -  (ignoring trailing spaces).

I will extend the patch for "query file:xxx" also. Thanks you.

> And even more tricky
> " query file:*.js file:name query2"?
> Codesearch supports them all and if are making this search smarter we should probably support them too.
yes, true. I will extend the patch to have these. 

Thanks a lot for review and thoughts Vsevolod.

[sam]

Created attachment 152946 [details]
snapshot showing mock ui

Hi Pavel & Vsevolod,
I have attached mock ui with in-progress patch showing results output.
Is it look ok? please let me know.

[sam]

Created attachment 156012 [details]
Patch

[sam]

(In reply to comment #8)
> Created an attachment (id=156012) [details]
> Patch

Sorry for the delay, I was away on vacations.

Its a WIP patch, please let me know your comments/suggestions. I am uploading screenshot for the same.

[sam]

Created attachment 156013 [details]
Screenshot for attached WIP patch

[Vsevolod Vlasov]

I don't like the idea of showing results for several different searches at once. 
This is confusing for user. 
Probably I didn't explain the way I'd like this feature to work good enough:
we should parse user's query and show (one) search result for it.
If search string has several file:foo entries we should show only files matching all of them. 
So for a search string "search file:inspector style file:js" I'd like to see all files having both "inspector" and "js" in their name and having both "style" and "search" in their content.

[sam]

(In reply to comment #11)
> I don't like the idea of showing results for several different searches at once. 
> This is confusing for user. 
> Probably I didn't explain the way I'd like this feature to work good enough:
> we should parse user's query and show (one) search result for it.
> If search string has several file:foo entries we should show only files matching all of them. 
> So for a search string "search file:inspector style file:js" I'd like to see all files having both "inspector" and "js" in their name and having both "style" and "search" in their content.

Oh ok. I shall upload the modified patch soon. Thank you for your feedback Vsevolod.

[sam]

Created attachment 157125 [details]
Patch

[sam]

Created attachment 157126 [details]
screenshot with output

[Vsevolod Vlasov]

Comment on attachment 157125 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

Please remove this line.

> Source/WebCore/inspector/front-end/AdvancedSearchController.js:137
> +    _processQueryForSearchScope: function(searchConfig)

I'd make query parsing logic in SearchConfig and make queries and files getters.

> Source/WebCore/inspector/front-end/AdvancedSearchController.js:145
> +            if (token)

return token && token.match(/^file:/gi);

or even 

var matches = /^file:(.*)/gi.exec("FiLe:foo.bar");
return matches && matches[1];

> Source/WebCore/inspector/front-end/AdvancedSearchController.js:163
> +            var preTokens = queryString.match(/([^\s]*\'[^\']+\'[^\s]*)|([\w|,.;'\[\]\(\){}!@<>#$%:^&*]+)/gi);     

It is not clear at all what is [\w|,.;'\[\]\(\){}!@<>#$%:^&*] ? Isn't it just [^\s] ?
Let's agree on what type of tokens we want to have first.

I'd say
 - quoted string (codesearch uses double quotes so let's keep it that way) where we allow any character except double quote and backslash OR anything escaped with backslash.
("foo", "foo bar", "foo \"bar\" foo", "foo\\bar")
 - simple string where we allow any character except whitespace and double quote and backslash OR anything escaped with a backslash.
(foo, foo'bar, foo\"bar\"foo, foo\ bar\ foo, foo\\bar\sfoo)

Also let's make it more readable, e.g.
var quotedStringPattern = ...;
var simpleStringPattern = ...;

var tokenRegex = new RegExp(quotedStringPattern + "|" + simpleStringPattern, "gi");

> Source/WebCore/inspector/front-end/AdvancedSearchController.js:169
> +                    queryConfig.selectors.push({isSelector:true, selectorRegex : new RegExp(preTokens[i].split(":")[1],"gi")});

isSelector is not needed.

> Source/WebCore/inspector/front-end/AdvancedSearchController.js:607
> +                query = query + "|";

This is not going to work properly. We have isRegex flag and createSearchRegex() in utilities.js will work differently depending on its value.
In particular "|" will be escaped when isRegex is false.

> Source/WebCore/inspector/front-end/ScriptsSearchScope.js:69
> +            var fileUrl = uiSourceCode.parsedURL.url;

uiSourceCode.contentURL();

> Source/WebCore/inspector/front-end/ScriptsSearchScope.js:95
> +                    if (isViableSearchSource (uiSourceCode, queryIndex) ) {

This looks too complex. Why not just filter out uiSourceCodes with non-matching url before starting the search like it is done for content scripts above?

> Source/WebCore/inspector/front-end/ScriptsSearchScope.js:98
> +                    }

} else {

> Source/WebCore/inspector/front-end/ScriptsSearchScope.js:137
> +        

Please revert this change.

> Source/WebCore/inspector/front-end/ScriptsSearchScope.js:139
> +    

Please revert this change.

[sam]

Created attachment 163532 [details]
Patch

[WebKit Review Bot]

Comment on attachment 163532 [details]
Patch

Attachment 163532 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13845364

New failing tests:
inspector/debugger/debugger-reload-on-pause.html
inspector/debugger/debugger-pause-on-failed-assertion.html
fast/loader/unload-form-post-about-blank.html
inspector/debugger/debugger-eval-while-paused.html
http/tests/xmlhttprequest/web-apps/001.html
inspector/debugger/debugger-eval-on-call-frame.html
inspector/debugger/debugger-scripts.html
http/tests/inspector/search/search-in-concatenated-script.html
inspector/debugger/debugger-pause-in-internal.html
inspector/tabbed-editors-history.html
inspector/console/console-api-on-call-frame.html
inspector/debugger/debugger-compile-and-run.html
inspector/debugger/debugger-pause-in-eval-script.html
inspector/debugger/debugger-proto-property.html
inspector/debugger/debugger-activation-crash2.html
http/tests/inspector/search/search-in-script.html
inspector/debugger/debugger-completions-on-call-frame.html
inspector/debugger/debugger-pause-on-exception.html
inspector/debugger/debugger-no-nested-pause.html
inspector/debugger/debugger-autocontinue-on-syntax-error.html
http/tests/inspector/compiler-source-mapping-debug.html
inspector/debugger/callstack-placards-discarded.html
inspector/debugger/copy-stack-trace.html
fast/frames/frame-limit.html
inspector/filtered-item-selection-dialog-filtering.html
inspector/debugger/debugger-pause-on-debugger-statement.html
inspector/debugger/debugger-expand-scope.html
inspector/debugger/debugger-breakpoints-not-activated-on-reload.html
fast/loader/local-CSS-from-local.html
inspector/debugger/debugger-activation-crash.html

[Vsevolod Vlasov]

Comment on attachment 163532 [details]
Patch

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

> Source/WebCore/inspector/front-end/AdvancedSearchController.js:428
> +        var preTokens = queryString.match(/([^\s]*\"([^\\"]|\\\\|\\")*\"[^\s]*)|([^\s]+)/gi);      

I don't think this regular expression is correctly parsing the examples set I suggested before.

[Vsevolod Vlasov]

After giving it another thought I would actually like to see an (extendable) test for the query parsing part of this patch. This way we can easily assess its quality.