[webkit-reviews] review granted: [Bug 226291] Implement Object.hasOwn : [Attachment 436076] Patch

[bugzilla-daemon at webkit.org]

Alexey Shvayka <shvaikalesh at gmail.com> has granted Aditi Singh
<asingh at igalia.com>'s request for review:
Bug 226291: Implement Object.hasOwn
https://bugs.webkit.org/show_bug.cgi?id=226291

Attachment 436076: Patch

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




--- Comment #17 from Alexey Shvayka <shvaikalesh at gmail.com> ---
Comment on attachment 436076
  --> https://bugs.webkit.org/attachment.cgi?id=436076
Patch

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

Great job, Aditi! r=me with few nits, rebase, and ChangeLog with proposal link.
Please reupload with cq? flag then.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1024
>      JSString* string = asString(property);

for/in was recently reengineered, this op is now called
"slow_path_enumerator_has_own_property".

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1027
> +    JSValue thisValue = base.toThis(globalObject, ECMAMode::strict());

�� for preserving the previous behavior; on such scale, even a smallest change
may break apps / sites.

nit: I am pretty sure toThis() call here is unnecessary (same goes for
DFGOperations.cpp): we don't emit HasOwnPropertyFunctionCallDotNode for bases
that need it. Not sure if we should add it, but please note toThis(...,
ECMAMode::strict()) will be soon removed in
https://bugs.webkit.org/show_bug.cgi?id=225397.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:1024
> +    RETURN_IF_EXCEPTION(scope, encodedJSValue());

nit: RETURN_IF_EXCEPTION(scope, { });

> Source/JavaScriptCore/runtime/ObjectPrototype.cpp:96
> +    Structure* structure = object->structure(vm);

nit: WebKit project has a nice rule of "not making unnecessary changes" to keep
diffs / history cleaner. I appreciate it during every `git blame`. If I was
making this change, I would have just keep "thisObject" name.