[webkit-reviews] review granted: [Bug 170099] Web Inspector: Add regular expression support to XHR breakpoints : [Attachment 307502] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Matt Baker
<mattbaker at apple.com>'s request for review:
Bug 170099: Web Inspector: Add regular expression support to XHR breakpoints
https://bugs.webkit.org/show_bug.cgi?id=170099

Attachment 307502: Patch

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




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

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

r=me

> Source/WebCore/inspector/InspectorDOMDebuggerAgent.cpp:412
> +	       int matchesCount =
ContentSearchUtilities::countRegularExpressionMatches(regex, url);
> +	       if (matchesCount) {

We can just check for a single match and not run the regex multiple times:

    bool hasMatch = regex.match(url) != -1;
    if (hasMatch) {

> Source/WebCore/inspector/InspectorDOMDebuggerAgent.h:109
> +    enum XHRBreakpointType {
> +	   Text = 0,
> +	   RegularExpression,
> +    };
> +
> +    HashMap<String, XHRBreakpointType> m_xhrBreakpoints;

Nit: enum class (I'm pretty sure that works here, you might need to give it a
type tho) and you can just make it one line.

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorRegexMode.css:39
> +    color: var(--text-color-gray-medium);

Maybe create a new syntax-highlight-regex-group-color?

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorRegexMode.js:36
> +	   // An unescaped closing bracket is allowed if it immediately follows
the
> +	   // opening bracket, or the negating caret.

I mis-interpreted "an unescaped closing bracket is allowed" to mean this would
be the equivalent of a closing bracket. I'd suggest rewriting this to something
like:

    // Check for early closing bracket to close the character set.
    // We are guaranteed not to have an escape character this early.

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorRegexMode.js:83
> +	   if (stream.next())
> +	       return "regex-literal";

We should return an error if there is no character left. So after this I'd
expect a return "error".

    js> new RegExp("\\")
    Exception: SyntaxError: Invalid regular expression: \ at end of pattern

> Source/WebInspectorUI/UserInterface/Views/XHRBreakpointTreeElement.js:41
> +		   subtitle = "/" + breakpoint.url + "/";

Hmm, if we show it with /s we might want to consider escaping /s in the regex,
which are likely to be common in regexs. But I suspect this won't be a problem
so I think keeping it as is is fine for now.