[webkit-reviews] review denied: [Bug 189407] Test262 failure with Named Capture Groups - using a reference before the group is defined : [Attachment 349212] Patch with suggested updates

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 189407: Test262 failure with Named Capture Groups - using a reference
before the group is defined
https://bugs.webkit.org/show_bug.cgi?id=189407

Attachment 349212: Patch with suggested updates

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




--- Comment #7 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 349212
  --> https://bugs.webkit.org/attachment.cgi?id=349212
Patch with suggested updates

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

> Source/JavaScriptCore/yarr/YarrPattern.cpp:687
> +    bool isValidNamedForwardReference(const String& subpatternName)
> +    {
> +	   return !m_unmatchedNamedForwardReferences.contains(subpatternName);

All the other YARR callbacks are informative.  If you moved this check to
inside your implementation of atomNamedForwardReference, then YARR would remain
a parser with a nice interface.
Said another way, I don't think YARR should ask if it should inform the
delegate of something.	It should just inform the delegate and let the delegate
decide what to do with it.

> Source/WebCore/contentextensions/URLFilterParser.cpp:144
> +	   fail(URLFilterParser::ForwardReference);

Could you please add a test to the ContentExtensionTest.ParsingFailures test to
verify this can be hit?  I'm assuming backwards compatibility isn't an issue