chakrashim: fixing ObjectTemplate implementation #439
Conversation
|
@kfarnung this was broken by design? What we gain by implementing? ie perf/memory effect? How to test it? |
|
I don't think this was broken by design, it looks like this is just an edge case implementation detail that wasn't implemented previously. This behavior is required in the latest http2 changes from upstream. I tried to minimize the impact and have a local test suite that I'm using to validate the behavior against V8. |
| } | ||
| } | ||
| } | ||
|
|
||
| if (!prototype.IsEmpty()) { |
There was a problem hiding this comment.
Is there a reason you chose not to make this an else?
I'd also like to see a comment here explaining the scenario this is supporting; it's something that I could easily see being broken if it is forgotten.
There was a problem hiding this comment.
The reason is that the previous conditional is just an alternate way to get the prototype. It could have already been provided directly.
There was a problem hiding this comment.
Ah, I'd missed that it was setting prototype
It's possible to create an object template with a constructor in V8, but we didn't support it. It's a little weird (even in V8) since objects constructed this way will never trigger the CallHandler of the FunctionTemplate object, it just inherits the prototype from it. PR-URL: nodejs#439 Reviewed-By: Jimmy Thomson <jithomso@microsoft.com>
486732f to
ca720a3
Compare
It's possible to create an object template with a constructor in V8, but we didn't support it. It's a little weird (even in V8) since objects constructed this way will never trigger the CallHandler of the FunctionTemplate object, it just inherits the prototype from it. PR-URL: nodejs#439 Reviewed-By: Jimmy Thomson <jithomso@microsoft.com>
It's possible to create an object template with a constructor in V8,
but we didn't support it. It's a little weird (even in V8) since
objects constructed this way will never trigger the CallHandler of the
FunctionTemplate object, it just inherits the prototype from it.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
chakrashim