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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jun 13 11:16:01 PDT 2019

Yusuke Suzuki <ysuzuki at apple.com> has denied Alexey Shvayka
<shvaikalesh at gmail.com>'s request for review:
Bug 186694: [ESNExt] String.prototype.matchAll

Attachment 372055: Patch


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

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.

More information about the webkit-reviews mailing list