[webkit-reviews] review granted: [Bug 202566] Implement Promise.any and AggregateError : [Attachment 396680] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 202566: Implement Promise.any and AggregateError
https://bugs.webkit.org/show_bug.cgi?id=202566

Attachment 396680: Patch

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




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

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

r=me with some comments.

> Source/JavaScriptCore/runtime/AggregateError.cpp:38
> +AggregateError::AggregateError(VM& vm, Structure* structure,
MarkedArgumentBuffer&& errors)

I recommend using `const MarkedArgumentBuffer&` here since I think this should
be non-movable too.
Can you annotate MarkedArgumentBuffer class with `WTF_MAKE_NONMOVABLE(...)`?

> Source/JavaScriptCore/runtime/AggregateError.cpp:55
> +AggregateError* AggregateError::create(VM& vm, JSGlobalObject* globalObject,
Structure* structure, JSValue errors, JSValue message, SourceAppender appender,
RuntimeType type, bool useCurrentFrame)

If the function fails with error, typically, we put JSGlobalObject* as a first
argument.

> Source/JavaScriptCore/runtime/AggregateError.cpp:73
> +    return create(vm, globalObject, structure, WTFMove(errorsList),
messageString, appender, type, useCurrentFrame);

Let's release scope by using,

RELEASE_AND_RETURN(scope, create(vm, globalObject, structure,
WTFMove(errorsList), messageString, appender, type, useCurrentFrame));

> Source/JavaScriptCore/runtime/AggregateError.h:43
> +class AggregateError : public ErrorInstance {

Let's annotate it with `final`. Using `final` is encouraged for JSC JSObject
classes because (1) it makes inheritance explicit and (2) jsDynamicCast has an
optimization if the class is annotated with `final`.

> Source/JavaScriptCore/runtime/AggregateError.h:69
> +    static AggregateError* create(VM& vm, JSGlobalObject* globalObject,
Structure* structure, MarkedArgumentBuffer&& errors, const String& message,
SourceAppender appender = nullptr, RuntimeType type = TypeNothing, bool
useCurrentFrame = true)

Ditto, let's use `const MarkedArgumentBuffer&` instead.

> Source/JavaScriptCore/runtime/AggregateErrorPrototype.h:55
> +	   return Structure::create(vm, globalObject, prototype,
TypeInfo(ErrorInstanceType, StructureFlags), info());

I guess this is not correct even in ErrorPrototypeBase side &
NativeErrorPrototype side (looks like harmless but still wrong). Can you change
ErrorInstanceType to ObjectType?
ErrorInstanceType should be only used for the derived classes of ErrorInstance.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:880
> +	   });

You need to add `thisObject->m_aggregateErrorStructure.visit(this)` in
visitChildren of JSGlobalObject as the same to m_URIErrorStructure etc.