[webkit-reviews] review denied: [Bug 192527] Web Inspector: provide a way to make searches case sensitive or use a regular expression : [Attachment 358575] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 192527: Web Inspector: provide a way to make searches case sensitive or use
a regular expression
https://bugs.webkit.org/show_bug.cgi?id=192527

Attachment 358575: Patch

https://bugs.webkit.org/attachment.cgi?id=358575&action=review




--- Comment #12 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 358575
  --> https://bugs.webkit.org/attachment.cgi?id=358575
Patch

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

r-, but this patch feels great! I love the quick settings cog in the Search
tab.

Here are some issues. Address the Regex one and I think this is worth landing!

• Regex Search does not highlight the expected range. Search for "\d+" which
may match "foo0foo" and will highlight "foo|0fo|o" because the search term
length was 3 characters, but its a regex not a literal match!
• Case-sensitive search does not work in the DOM Search. A search for "test"
finds a text node "Tools / Tests" which it should not. That could  either be
addressed now or in a follow-up. Seems this would need to modify
`DOMAgent.performSearch`.

I wonder if we should include a color'd UI for regex searches to at least
distinguish it easier, and immediately know you're typing a regex and not a
literal string. If I saw the same dark orange as in the quick console when I
type a regex it would immediately be clear to me that I'm performing a regex
search.

> LayoutTests/inspector/debugger/search-scripts-expected.txt:50
> +QUERY: (search|OTHER)TEST {"caseSensitive":true,"isRegex":true}
> +SCRIPT: LayoutTests/inspector/debugger/search-scripts.html
> +RESULTS: 0

Looks like this should have matched the line with "OTHERTEST" in this file.

> LayoutTests/inspector/page/searchInResources.html:132
> +		   InspectorTest.expectThat(matches.length ===
result.matchesCount, "Should find previously mentioned number of matches.");

The "previously mentioned number of matches" makes the output hard to read. I'd
prefer to see:

    `Should find ${result.matchesCount} match(es).`

Then you'd immediately know from looking at the text file that 2 MATCH lines is
wrong if you expected 3 matches.