[webkit-reviews] review denied: [Bug 186694] [ESNExt] String.prototype.matchAll : [Attachment 372055] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has denied Alexey Shvayka
<shvaikalesh at gmail.com>'s request for review:
Bug 186694: [ESNExt] String.prototype.matchAll
https://bugs.webkit.org/show_bug.cgi?id=186694

Attachment 372055: Patch

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




--- Comment #27 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 372055
  --> https://bugs.webkit.org/attachment.cgi?id=372055
Patch

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

The logic looks nice.

> Source/JavaScriptCore/CMakeLists.txt:934
> +    runtime/RegExpStringIteratorPrototype.h

Let’s add these files (added h, cpp, and generated code from js file etc. maybe
js file too.) to Xcode project file too?
At that time, be careful that Xcode should not build the added cpp files
directly. We are using unified builds.
So we need to set “target” configuration in Xcode property.
See the similar files and align these files to them.

> Source/JavaScriptCore/builtins/RegExpPrototype.js:28
> +function RegExpStringIteratorConstructor(R, S, global, fullUnicode)

Rename this to createRegExpStringIterator.

> Source/JavaScriptCore/builtins/RegExpPrototype.js:163
> +    let R = this;

I understand that these names come from the spec. But we use descriptive name
in our source code.
Can you replace R to regexp, S to string etc. in your patch?

> Source/JavaScriptCore/runtime/RegExpStringIteratorPrototype.h:34
> +    typedef JSNonFinalObject Base;

Use “using” in newly added code.

> Source/JavaScriptCore/runtime/RegExpStringIteratorPrototype.h:39
> +	   auto structure = Structure::create(vm, globalObject,
iteratorPrototype, TypeInfo(ObjectType, StructureFlags), info());

Create createStructure function, call it in JSGlobalObject side and pass the
structure to this function. I think this is the convention in JSC.