[webkit-reviews] review granted: [Bug 169904] Add support for Error.stackTraceLimit. : [Attachment 305064] proposed patch: rebased.

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 169904: Add support for Error.stackTraceLimit.
https://bugs.webkit.org/show_bug.cgi?id=169904

Attachment 305064: proposed patch: rebased.

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




--- Comment #13 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 305064
  --> https://bugs.webkit.org/attachment.cgi?id=305064
proposed patch: rebased.

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

Thanks for you response.
r=me with a comment.

>>> Source/JavaScriptCore/runtime/ErrorConstructor.cpp:50
>>> +	 unsigned defaultStackTraceLimit =
Options::defaultErrorStackTraceLimit();
>>> +	 globalObject()->setErrorStackTraceLimit(defaultStackTraceLimit);
>>> +	 putDirectWithoutTransition(vm, vm.propertyNames->stackTraceLimit,
jsNumber(defaultStackTraceLimit), None);
>> 
>> Why not just make this a getter/setter? What does V8 do? If we were to
actually propose this as spec, I think a getter/setter is cleaner.
> 
> Because then Object.getOwnPropertyDescriptor(Error, "stackTraceLimit") will
return { value; undefined, ..., get: function ...., set: function ... }. 
Chrome's impl returns { value: 10, ... } with no get and set.

Style nit:
It's weird that this code lives here. What I'd do is this:
1. Make global object set its own value via the option during JSGlobalObject()
constructor.
2. Make this grab the value from the global object, and use that in putDirect,
so something like: putDirect(..., globalObject->errorLimit() ?
globalObject.errorLimit.get() : Options::defaultErrorStackTraceLimit(), ...)