[webkit-reviews] review granted: [Bug 202566] Implement Promise.any and AggregateError : [Attachment 396680] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Apr 16 12:11:32 PDT 2020
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.
More information about the webkit-reviews
mailing list