Bug 204954

Created attachment 385048 [details] Patch

Comment on attachment 385048 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385048&action=review > Source/WebCore/ChangeLog:10 > + If `isRegex` is false, add `^` and `$` to the beginning and end of the search string to > + ensure that the search string is exactly matched, not just contained within the potentially > + intercepted URL. Isn't this incorrect if there are any special characters? For example if my URL was: "http://example.com/(test" "http://example.com/$/test" I'd expect each of these to match potential URLs not treated like regexes. However it seems (1) would be an invalid regex and (2) would behave different if treated like a regex.

Comment on attachment 385048 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385048&action=review >> Source/WebCore/ChangeLog:10 >> + intercepted URL. > > Isn't this incorrect if there are any special characters? > > For example if my URL was: > > "http://example.com/(test" > "http://example.com/$/test" > > I'd expect each of these to match potential URLs not treated like regexes. However it seems (1) would be an invalid regex and (2) would behave different if treated like a regex. Both `Inspector::ContentSearchUtilities::createSearchRegex` and `Inspector::ContentSearchUtilities::createMatchRegex` escape the query string if the `isRegex` parameter is false (they add "\\" before any of "[](){}+-*.,?\\^$|"). In the case that it is true, we don't add the `^` and `$` to the query string, and let it wholly be treated as a regex. See how `createSearchRegexSource` is used in 'Source/JavaScriptCore/inspector/ContentSearchUtilities.cpp'.

Comment on attachment 385048 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385048&action=review > Source/JavaScriptCore/inspector/ContentSearchUtilities.cpp:139 > + return RegularExpression { isRegex ? query : makeString("^"_s, createSearchRegexSource(query), "$"_s), caseSensitive ? TextCaseSensitive : TextCaseInsensitive }; A `makeString` shouldn't require WTFStrings for single characters. It likely could just be: makeString('^', ..., '$') > Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:858 > for (auto& intercept : m_intercepts) { > - auto regex = ContentSearchUtilities::createSearchRegex(intercept.url, intercept.caseSensitive, intercept.isRegex); > + auto regex = ContentSearchUtilities::createMatchRegex(intercept.url, intercept.caseSensitive, intercept.isRegex); > if (regex.match(urlString) != -1) > return true; > } I'd expect to see: if (intercept.isRegex) { ... regex ... } else { ... string compare ... }

(In reply to Devin Rousso from comment #3) > Comment on attachment 385048 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=385048&action=review > > >> Source/WebCore/ChangeLog:10 > >> + intercepted URL. > > > > Isn't this incorrect if there are any special characters? > > > > For example if my URL was: > > > > "http://example.com/(test" > > "http://example.com/$/test" > > > > I'd expect each of these to match potential URLs not treated like regexes. However it seems (1) would be an invalid regex and (2) would behave different if treated like a regex. > > Both `Inspector::ContentSearchUtilities::createSearchRegex` and > `Inspector::ContentSearchUtilities::createMatchRegex` escape the query > string if the `isRegex` parameter is false (they add "\\" before any of > "[](){}+-*.,?\\^$|"). In the case that it is true, we don't add the `^` and > `$` to the query string, and let it wholly be treated as a regex. > > See how `createSearchRegexSource` is used in > 'Source/JavaScriptCore/inspector/ContentSearchUtilities.cpp'. Ugh, gross, and very non-obvious.

Comment on attachment 385048 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385048&action=review > Source/JavaScriptCore/inspector/ContentSearchUtilities.cpp:137 > +RegularExpression createMatchRegex(const String& query, bool caseSensitive, bool isRegex) Maybe name this `createExactMatchRegex`? >> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:858 >> } > > I'd expect to see: > > if (intercept.isRegex) { > ... regex ... > } else { > ... string compare ... > } I see you answer this because `ContentSearchUtilities` is clever.

Comment on attachment 385048 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385048&action=review >> Source/JavaScriptCore/inspector/ContentSearchUtilities.cpp:137 >> +RegularExpression createMatchRegex(const String& query, bool caseSensitive, bool isRegex) > > Maybe name this `createExactMatchRegex`? In my mind, "exact match" implies that the resulting regex would match against an _entire_ string, which isn't really accurate for when `isRegex` is true (e.g. `https\:\/\/webkit\.org\/` would match every response to any URL that contained "https://webkit.org/", even if the URL was something like "https://example.org/?redirect=https://webkit.org/?params=foobarbaz"). >> Source/JavaScriptCore/inspector/ContentSearchUtilities.cpp:139 >> + return RegularExpression { isRegex ? query : makeString("^"_s, createSearchRegexSource(query), "$"_s), caseSensitive ? TextCaseSensitive : TextCaseInsensitive }; > > A `makeString` shouldn't require WTFStrings for single characters. It likely could just be: > > makeString('^', ..., '$') What is the usual style for this. I'd though to use `ASCIILiteral` for all ASCII strings, regardless of the number of characters, but perhaps that is unnecessary. I'm fine either way. I'll change this.

Created attachment 385050 [details] Patch

(In reply to Devin Rousso from comment #7) > Comment on attachment 385048 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=385048&action=review > > >> Source/JavaScriptCore/inspector/ContentSearchUtilities.cpp:139 > >> + return RegularExpression { isRegex ? query : makeString("^"_s, createSearchRegexSource(query), "$"_s), caseSensitive ? TextCaseSensitive : TextCaseInsensitive }; > > > > A `makeString` shouldn't require WTFStrings for single characters. It likely could just be: > > > > makeString('^', ..., '$') > > What is the usual style for this. I'd though to use `ASCIILiteral` for all > ASCII strings, regardless of the number of characters, but perhaps that is > unnecessary. I'm fine either way. I'll change this. `makeString` is constructing a single WTFString by concatenating components. See StringConcatenate.h. Therefore the simplest components that you can concatenate together should cause the most efficient construction. Lets compare: 1. makeString(char, WTFString, char) 2. makeString(ASCIILiteral, WTFString, ASCIILiteral) (1) is likely to be simpler then (2) because its components are simpler. Note that in StringConcatenate.h that an ASCIILiteral has a special case. This means an ASCIILiteral like "foo"_s should be treated exactly the same as char* "foo". As it should be, because why convert to a WTFString and back to characters to concatenate! But a char literal ('f') is simpler than a char* literal ("f"), it avoids a potential strlen(...) and StringImpl::copyCharacters call, both of which are unnecessary complexity for a single character. In general, operations (searching, comparing, concatenating) are almost always simpler / more efficient to use a character instead of a char*/ASCIILiteral/WTFString.

Comment on attachment 385050 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385050&action=review > Source/JavaScriptCore/inspector/ContentSearchUtilities.cpp:139 > + return RegularExpression { isRegex ? query : makeString('^', createSearchRegexSource(query), '$'), caseSensitive ? TextCaseSensitive : TextCaseInsensitive }; A better name for `createSearchRegexSource` seems like `escapeRegex` or something. I find these functions confusing. "createFooRegex" which takes an `isRegex` flag seems counter-intuitive. Perhaps this could be better: `createRegularExpressionForSearchString(const String& searchString, bool caseSensitive, SearchStringType type)` where `enum SearchStringType { Regex, ExactString, ContainsString }`. Maybe I'm over thinking it but it seems very error prone to read this function.

Comment on attachment 385050 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385050&action=review >> Source/JavaScriptCore/inspector/ContentSearchUtilities.cpp:139 >> + return RegularExpression { isRegex ? query : makeString('^', createSearchRegexSource(query), '$'), caseSensitive ? TextCaseSensitive : TextCaseInsensitive }; > > A better name for `createSearchRegexSource` seems like `escapeRegex` or something. > > I find these functions confusing. "createFooRegex" which takes an `isRegex` flag seems counter-intuitive. > > Perhaps this could be better: `createRegularExpressionForSearchString(const String& searchString, bool caseSensitive, SearchStringType type)` where `enum SearchStringType { Regex, ExactString, ContainsString }`. > > Maybe I'm over thinking it but it seems very error prone to read this function. I like this! I'll take a stab at it :)

Created attachment 385080 [details] Patch

The commit-queue encountered the following flaky tests while processing attachment 385080 [details]: webgl/1.0.3/conformance/ogles/GL/mat/mat_025_to_032.html bug 204981 (author: justin_fan@apple.com) The commit-queue is continuing to process your patch.

Comment on attachment 385080 [details] Patch Clearing flags on attachment: 385080 Committed r253246: <https://trac.webkit.org/changeset/253246>

All reviewed patches have been landed. Closing bug.

<rdar://problem/57725755>