[webkit-reviews] review denied: [Bug 90659] Web Inspector: Option to filter search based on source type in Advanced-Search : [Attachment 157125] Patch

[bugzilla-daemon at webkit.org]

Vsevolod Vlasov <vsevik at chromium.org> has denied sam <dsam2912 at gmail.com>'s
request for review:
Bug 90659: Web Inspector: Option to filter search based on source type in
Advanced-Search
https://bugs.webkit.org/show_bug.cgi?id=90659

Attachment 157125: Patch
https://bugs.webkit.org/attachment.cgi?id=157125&action=review

------- Additional Comments from Vsevolod Vlasov <vsevik at chromium.org>
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|,.;'\[\]\(\){}!@<>#$%:^&*]+)/g
i);	

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.