Expose JsHasOwnProperty - #3316

kfarnung · 2017-07-11T01:33:44Z

Enable support for usage in node's n-api. Added a new unit test for
the functionality.

kfarnung · 2017-07-11T01:36:04Z

@obastemur This may need to be updated based on the outcome of #3207. I just followed the existing pattern for JsHasProperty before I saw that PR.

kfarnung · 2017-07-11T02:00:07Z

/cc @liminzhu

kunalspathak · 2017-07-11T05:17:50Z

+        VALIDATE_INCOMING_OBJECT(object, scriptContext);
+        VALIDATE_INCOMING_PROPERTYID(propertyId);
+        PARAM_NOT_NULL(hasOwnProperty);
+        *hasOwnProperty = nullptr;


*hasOwnProperty = nullptr; [](start = 7, length = 27)

shouldn't be initialized to false? I know at other places these variables are initialized to nullptr, but it should be false in those cases as well.

Ah, yeah, that came up in @obastemur PR as well, I'll take a quick scan and fix them up.

kunalspathak

We should add this to the wiki as well.

obastemur · 2017-07-11T11:13:48Z

LGTM

kfarnung · 2017-07-11T15:24:49Z


-            Count
+            Count,
+            HasOwnPropertyActionTag


That was dumb.

Enable support for usage in node's n-api. Added a new unit test for the functionality.

kfarnung · 2017-07-11T15:31:06Z

@mrkmarron Can you quickly sanity check my TTD changes? I'm going to need to write a new test to validate them.

mrkmarron · 2017-07-11T15:52:21Z

From a quick look I would say the changes look good. Let me know if you want me to look at anything specific. Mark Get Outlook for iOS<https://aka.ms/o0ukef>

…

_____________________________ From: Kyle Farnung <notifications@github.com<mailto:notifications@github.com>> Sent: Tuesday, July 11, 2017 8:31 AM Subject: Re: [Microsoft/ChakraCore] Expose JsHasOwnProperty (#3316) To: Microsoft/ChakraCore <chakracore@noreply.github.com<mailto:chakracore@noreply.github.com>> Cc: Mark Marron <marron@microsoft.com<mailto:marron@microsoft.com>>, Mention <mention@noreply.github.com<mailto:mention@noreply.github.com>> @mrkmarron<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmrkmarron&data=02%7C01%7Cmarron%40microsoft.com%7C819864157c724446134808d4c871e8e9%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636353838940797292&sdata=RhIkYIAg%2B1%2BtY9h5AiEziKBZjU9fcNqpm7L5%2Ft4XGSw%3D&reserved=0> Can you quickly sanity check my TTD changes? I'm going to need to write a new test to validate them. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMicrosoft%2FChakraCore%2Fpull%2F3316%23issuecomment-314482146&data=02%7C01%7Cmarron%40microsoft.com%7C819864157c724446134808d4c871e8e9%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636353838940797292&sdata=v5C5%2B19V%2BIT%2FFIiwRcMcpvZkLd8Dnn96yTZzMqlHpUI%3D&reserved=0>, or mute the thread<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAOLOqtEVibNliw8OSaEPQIM3VQ5DDe1Nks5sM5VCgaJpZM4OTnd-&data=02%7C01%7Cmarron%40microsoft.com%7C819864157c724446134808d4c871e8e9%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636353838940797292&sdata=i0%2BlZlBnuWxqfS9N7geGqO%2FV5EjI3LMVZ1RBs0TZU2M%3D&reserved=0>.

liminzhu · 2017-07-11T17:19:34Z

LGTM

Merge pull request #3316 from kfarnung:hasownprop Enable support for usage in node's n-api. Added a new unit test for the functionality.

kfarnung · 2017-07-11T20:56:04Z

Added a page to the wiki and updated the reference list:

https://github.com/Microsoft/ChakraCore/wiki/JsHasOwnProperty

/cc @kunalspathak @liminzhu

liminzhu · 2017-07-12T00:37:05Z

@kfarnung awesome! I added an experimental note to the wiki temporarily. Seems like a great addition to Chakra APIs as well and this is just in case Windows team has some feedback along the process.

msftclas added the cla-not-required label Jul 11, 2017

kfarnung requested review from digitalinfinity and kunalspathak July 11, 2017 01:35

kfarnung requested review from digitalinfinity, kunalspathak and mrkmarron and removed request for digitalinfinity and kunalspathak July 11, 2017 01:37

kunalspathak reviewed Jul 11, 2017

View reviewed changes

kunalspathak approved these changes Jul 11, 2017

View reviewed changes

kfarnung commented Jul 11, 2017

View reviewed changes

Comment thread lib/Runtime/Debug/TTEvents.h Outdated

Count

Count,

HasOwnPropertyActionTag

kfarnung Jul 11, 2017

Copy link
Copy Markdown

Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was dumb.

Expose JsHasOwnProperty

4ba4c1a

Enable support for usage in node's n-api. Added a new unit test for the functionality.

kfarnung force-pushed the hasownprop branch from 7f3565b to 4ba4c1a Compare July 11, 2017 15:27

chakrabot merged commit 4ba4c1a into chakra-core:release/1.7 Jul 11, 2017

chakrabot pushed a commit that referenced this pull request Jul 11, 2017

[MERGE #3316 @kfarnung] Expose JsHasOwnProperty

6450f4f

Merge pull request #3316 from kfarnung:hasownprop Enable support for usage in node's n-api. Added a new unit test for the functionality.

chakrabot pushed a commit that referenced this pull request Jul 11, 2017

[1.7>master] [MERGE #3316 @kfarnung] Expose JsHasOwnProperty

c339739

Merge pull request #3316 from kfarnung:hasownprop Enable support for usage in node's n-api. Added a new unit test for the functionality.

kfarnung deleted the hasownprop branch July 11, 2017 20:20

Navigation Menu

Uh oh!

Expose JsHasOwnProperty - #3316