Respond to review feedback

boingoing · boingoing · commit ac00296d5964 · 2018-01-11T14:12:19.000-08:00
diff --git a/bin/NativeTests/JsRTApiTest.cpp b/bin/NativeTests/JsRTApiTest.cpp
@@ -660,7 +660,7 @@ namespace JsRTApiTest
         JsRTApiTest::RunWithAttributes(JsRTApiTest::ExternalFunctionTest);
     }
 
-    JsValueRef CALLBACK ExternaEnhancedFunctionTestCallback(JsValueRef callee, JsValueRef *arguments, unsigned short argumentCount, JsNativeFunctionInfo *info, void *callbackData)
+    JsValueRef CALLBACK ExternalEnhancedFunctionTestCallback(JsValueRef callee, JsValueRef *arguments, unsigned short argumentCount, JsNativeFunctionInfo *info, void *callbackData)
     {
         REQUIRE(callbackData != nullptr);
         REQUIRE(*static_cast<int*>(callbackData) == 123);
@@ -671,8 +671,7 @@ namespace JsRTApiTest
         REQUIRE(JsGetTrueValue(&_true) == JsNoError);
         JsValueRef _false;
         REQUIRE(JsGetFalseValue(&_false) == JsNoError);
-        JsValueRef _null;
-        REQUIRE(JsGetNullValue(&_null) == JsNoError);
+        
 
         REQUIRE(JsStrictEquals(_true, arguments[0], &success) == JsNoError);
         REQUIRE(success);
@@ -682,13 +681,17 @@ namespace JsRTApiTest
         REQUIRE(!info->isConstructCall);
         REQUIRE(info->thisArg == arguments[0]);
 
-        REQUIRE(JsStrictEquals(_null, info->newTargetArg, &success) == JsNoError);
+        JsValueRef undefined;
+        REQUIRE(JsGetUndefinedValue(&undefined) == JsNoError);
+        REQUIRE(JsStrictEquals(undefined, info->newTargetArg, &success) == JsNoError);
         REQUIRE(success);
 
+        JsValueRef _null;
+        REQUIRE(JsGetNullValue(&_null) == JsNoError);
         return _null;
     }
 
-    JsValueRef CALLBACK ExternaEnhancedConstructorFunctionTestCallback(JsValueRef callee, JsValueRef *arguments, unsigned short argumentCount, JsNativeFunctionInfo *info, void *callbackData)
+    JsValueRef CALLBACK ExternalEnhancedConstructorFunctionTestCallback(JsValueRef callee, JsValueRef *arguments, unsigned short argumentCount, JsNativeFunctionInfo *info, void *callbackData)
     {
         REQUIRE(callbackData != nullptr);
         REQUIRE(*static_cast<int*>(callbackData) == 456);
@@ -719,11 +722,11 @@ namespace JsRTApiTest
         return info->thisArg;
     }
 
-    void ExternaEnhancedFunctionTest(JsRuntimeAttributes attributes, JsRuntimeHandle runtime)
+    void ExternalEnhancedFunctionTest(JsRuntimeAttributes attributes, JsRuntimeHandle runtime)
     {
         int sentinel = 123;
         JsValueRef function = JS_INVALID_REFERENCE;
-        REQUIRE(JsCreateEnhancedFunction(ExternaEnhancedFunctionTestCallback, nullptr, &sentinel, &function) == JsNoError);
+        REQUIRE(JsCreateEnhancedFunction(ExternalEnhancedFunctionTestCallback, nullptr, &sentinel, &function) == JsNoError);
         JsValueRef _true;
         REQUIRE(JsGetTrueValue(&_true) == JsNoError);
         JsValueRef _false;
@@ -739,30 +742,30 @@ namespace JsRTApiTest
 
         sentinel = 456;
         function = JS_INVALID_REFERENCE;
-        REQUIRE(JsCreateEnhancedFunction(ExternaEnhancedConstructorFunctionTestCallback, nullptr, &sentinel, &function) == JsNoError);
+        REQUIRE(JsCreateEnhancedFunction(ExternalEnhancedConstructorFunctionTestCallback, nullptr, &sentinel, &function) == JsNoError);
         JsValueRef ctorArgs[3] = { _null, _true, _false };
         REQUIRE(JsConstructObject(function, ctorArgs, 3, &result) == JsNoError);
         JsValueType t;
         REQUIRE(JsGetValueType(result, &t) == JsNoError);
         REQUIRE(t == JsObject);
     }
 
-    TEST_CASE("ApiTest_ExternaEnhancedFunctionTest", "[ApiTest]")
+    TEST_CASE("ApiTest_ExternalEnhancedFunctionTest", "[ApiTest]")
     {
-        JsRTApiTest::RunWithAttributes(JsRTApiTest::ExternaEnhancedFunctionTest);
+        JsRTApiTest::RunWithAttributes(JsRTApiTest::ExternalEnhancedFunctionTest);
     }
 
-    struct ExternaEnhancedBaseClassFunctionTestInfo
+    struct ExternalEnhancedBaseClassFunctionTestInfo
     {
         JsValueRef derived;
         JsValueRef base;
     };
 
-    JsValueRef CALLBACK ExternaEnhancedBaseClassFunctionTestCallback(JsValueRef callee, JsValueRef *arguments, unsigned short argumentCount, JsNativeFunctionInfo *info, void *callbackData)
+    JsValueRef CALLBACK ExternalEnhancedBaseClassFunctionTestCallback(JsValueRef callee, JsValueRef *arguments, unsigned short argumentCount, JsNativeFunctionInfo *info, void *callbackData)
     {
         REQUIRE(callbackData != nullptr);
 
-        ExternaEnhancedBaseClassFunctionTestInfo* testinfo = (ExternaEnhancedBaseClassFunctionTestInfo*)callbackData;
+        ExternalEnhancedBaseClassFunctionTestInfo* testinfo = (ExternalEnhancedBaseClassFunctionTestInfo*)callbackData;
         JsValueType t;
         REQUIRE(JsGetValueType(testinfo->derived, &t) == JsNoError);
         REQUIRE(t == JsFunction);
@@ -812,12 +815,11 @@ namespace JsRTApiTest
 
     void ExternalEnhancedBaseClassFunctionTest(JsRuntimeAttributes attributes, JsRuntimeHandle runtime)
     {
-        ExternaEnhancedBaseClassFunctionTestInfo info;
-
-        JsValueRef name;
+        ExternalEnhancedBaseClassFunctionTestInfo info = { nullptr, nullptr };
+        JsValueRef name = JS_INVALID_REFERENCE;
         REQUIRE(JsCreateString("BaseClass", 10, &name) == JsNoError);
         JsValueRef base = JS_INVALID_REFERENCE;
-        REQUIRE(JsCreateEnhancedFunction(ExternaEnhancedBaseClassFunctionTestCallback, name, &info, &base) == JsNoError);
+        REQUIRE(JsCreateEnhancedFunction(ExternalEnhancedBaseClassFunctionTestCallback, name, &info, &base) == JsNoError);
         info.base = base;
 
         JsValueRef global = JS_INVALID_REFERENCE;
diff --git a/lib/Jsrt/ChakraCore.h b/lib/Jsrt/ChakraCore.h
@@ -107,12 +107,12 @@ typedef JsErrorCode(CHAKRA_CALLBACK * NotifyModuleReadyCallback)(_In_opt_ JsModu
 /// <summary>
 ///     A structure containing information about a native function callback.
 /// </summary>
-struct JsNativeFunctionInfo
+typedef struct JsNativeFunctionInfo
 {
     JsValueRef thisArg;
     JsValueRef newTargetArg;
     bool isConstructCall;
-};
+}JsNativeFunctionInfo;
 
 /// <summary>
 ///     A function callback.
@@ -136,7 +136,7 @@ typedef _Ret_maybenull_ JsValueRef(CHAKRA_CALLBACK * JsEnhancedNativeFunction)(_
 ///     Requires an active script context.
 /// </remarks>
 /// <param name="nativeFunction">The method to call when the function is invoked.</param>
-/// <param name="metadata">If this is a string, it is used as the name of the function.</param>
+/// <param name="metadata">If this is not <c>JS_INVALID_REFERENCE</c>, it is converted to a string and used as the name of the function.</param>
 /// <param name="callbackState">
 ///     User provided state that will be passed back to the callback.
 /// </param>
diff --git a/lib/Jsrt/Jsrt.cpp b/lib/Jsrt/Jsrt.cpp
@@ -2792,21 +2792,21 @@ CHAKRA_API JsConstructObject(_In_ JsValueRef function, _In_reads_(cargs) JsValue
 }
 
 #ifndef _CHAKRACOREBUILD
-struct JsNativeFunctionInfo
+typedef struct JsNativeFunctionInfo
 {
     JsValueRef thisArg;
     JsValueRef newTargetArg;
     bool isConstructCall;
-};
+}JsNativeFunctionInfo;
 
 typedef _Ret_maybenull_ JsValueRef(CHAKRA_CALLBACK * JsEnhancedNativeFunction)(_In_ JsValueRef callee, _In_ JsValueRef *arguments, _In_ unsigned short argumentCount, _In_ JsNativeFunctionInfo *info, _In_opt_ void *callbackState);
 #endif
 
-struct JsNativeFunctionWrapperHolder
+typedef struct JsNativeFunctionWrapperHolder
 {
-    void *callbackState;
-    JsNativeFunction nativeFunction;
-};
+    FieldNoBarrier(void *) callbackState;
+    FieldNoBarrier(JsNativeFunction) nativeFunction;
+}JsNativeFunctionWrapperHolder;
 
 JsValueRef CALLBACK JsNativeFunctionWrapper(JsValueRef callee, JsValueRef *arguments, unsigned short argumentCount, JsNativeFunctionInfo *info, void *wrapperData)
 {
@@ -2819,7 +2819,7 @@ template <bool wrapNativeFunction, class T>
 JsErrorCode JsCreateEnhancedFunctionHelper(_In_ T nativeFunction, _In_opt_ JsValueRef metadata, _In_opt_ void *callbackState, _Out_ JsValueRef *function)
 {
     return ContextAPIWrapper<JSRT_MAYBE_TRUE>([&](Js::ScriptContext *scriptContext, TTDRecorder& _actionEntryPopper) -> JsErrorCode {
-        PERFORM_JSRT_TTD_RECORD_ACTION(scriptContext, RecordJsRTAllocateFunction, false, nullptr);
+        PERFORM_JSRT_TTD_RECORD_ACTION(scriptContext, RecordJsRTAllocateFunction, true, metadata);
         PARAM_NOT_NULL(nativeFunction);
         PARAM_NOT_NULL(function);
         *function = nullptr;
@@ -2865,7 +2865,7 @@ CHAKRA_API JsCreateEnhancedFunction(_In_ JsEnhancedNativeFunction nativeFunction
 
 CHAKRA_API JsCreateFunction(_In_ JsNativeFunction nativeFunction, _In_opt_ void *callbackState, _Out_ JsValueRef *function)
 {
-    return JsCreateNamedFunction(JS_INVALID_REFERENCE, nativeFunction, callbackState, function);
+    return JsCreateEnhancedFunctionHelper<true>(nativeFunction, JS_INVALID_REFERENCE, callbackState, function);
 }
 
 CHAKRA_API JsCreateNamedFunction(_In_ JsValueRef name, _In_ JsNativeFunction nativeFunction, _In_opt_ void *callbackState, _Out_ JsValueRef *function)
diff --git a/lib/Runtime/Library/JavascriptExternalFunction.cpp b/lib/Runtime/Library/JavascriptExternalFunction.cpp
@@ -274,10 +274,11 @@ namespace Js
         ScriptContext * scriptContext = externalFunction->type->GetScriptContext();
         AnalysisAssert(scriptContext);
         Var result = nullptr;
+        Assert(callInfo.Count > 0);
 
         StdCallJavascriptMethodInfo info = {
-            callInfo.Count > 0 ? args[0] : scriptContext->GetLibrary()->GetNull(),
-            args.HasNewTarget() ? args.GetNewTarget() : args.IsNewCall() ? function : scriptContext->GetLibrary()->GetNull(),
+            args[0],
+            args.HasNewTarget() ? args.GetNewTarget() : args.IsNewCall() ? function : scriptContext->GetLibrary()->GetUndefined(),
             args.IsNewCall()
         };
 
@@ -431,8 +432,8 @@ namespace Js
             TTD::NSLogEvents::EventLogEntry* callEvent = elog->RecordExternalCallEvent(externalFunction, scriptContext->GetThreadContext()->TTDRootNestingCount, args.Info.Count, args.Values, true);
 
             StdCallJavascriptMethodInfo info = {
-                callInfo.Count > 0 ? args[0] : scriptContext->GetLibrary()->GetNull(),
-                args.HasNewTarget() ? args.GetNewTarget() : args.IsNewCall() ? function : scriptContext->GetLibrary()->GetNull(),
+                args[0],
+                args.HasNewTarget() ? args.GetNewTarget() : args.IsNewCall() ? function : scriptContext->GetLibrary()->GetUndefined(),
                 args.IsNewCall()
             };
 

Original file line number	Diff line number	Diff line change
`@@ -2792,21 +2792,21 @@ CHAKRA_API JsConstructObject(_In_ JsValueRef function, _In_reads_(cargs) JsValue`
`2792`	`2792`	`}`
`2793`	`2793`
`2794`	`2794`	`#ifndef _CHAKRACOREBUILD`
`2795`		`-struct JsNativeFunctionInfo`
	`2795`	`+typedef struct JsNativeFunctionInfo`
`2796`	`2796`	`{`
`2797`	`2797`	`JsValueRef thisArg;`
`2798`	`2798`	`JsValueRef newTargetArg;`
`2799`	`2799`	`bool isConstructCall;`
`2800`		`-};`
	`2800`	`+}JsNativeFunctionInfo;`
`2801`	`2801`
`2802`	`2802`	`typedef _Ret_maybenull_ JsValueRef(CHAKRA_CALLBACK * JsEnhancedNativeFunction)(_In_ JsValueRef callee, _In_ JsValueRef arguments, _In_ unsigned short argumentCount, _In_ JsNativeFunctionInfo info, _In_opt_ void *callbackState);`
`2803`	`2803`	`#endif`
`2804`	`2804`
`2805`		`-struct JsNativeFunctionWrapperHolder`
	`2805`	`+typedef struct JsNativeFunctionWrapperHolder`
`2806`	`2806`	`{`
`2807`		`- void *callbackState;`
`2808`		`- JsNativeFunction nativeFunction;`
`2809`		`-};`
	`2807`	`+ FieldNoBarrier(void *) callbackState;`
	`2808`	`+ FieldNoBarrier(JsNativeFunction) nativeFunction;`
	`2809`	`+}JsNativeFunctionWrapperHolder;`
`2810`	`2810`
`2811`	`2811`	`JsValueRef CALLBACK JsNativeFunctionWrapper(JsValueRef callee, JsValueRef arguments, unsigned short argumentCount, JsNativeFunctionInfo info, void *wrapperData)`
`2812`	`2812`	`{`
`@@ -2819,7 +2819,7 @@ template <bool wrapNativeFunction, class T>`
`2819`	`2819`	`JsErrorCode JsCreateEnhancedFunctionHelper(_In_ T nativeFunction, _In_opt_ JsValueRef metadata, _In_opt_ void callbackState, _Out_ JsValueRef function)`
`2820`	`2820`	`{`
`2821`	`2821`	`return ContextAPIWrapper<JSRT_MAYBE_TRUE>([&](Js::ScriptContext *scriptContext, TTDRecorder& _actionEntryPopper) -> JsErrorCode {`
`2822`		`- PERFORM_JSRT_TTD_RECORD_ACTION(scriptContext, RecordJsRTAllocateFunction, false, nullptr);`
	`2822`	`+ PERFORM_JSRT_TTD_RECORD_ACTION(scriptContext, RecordJsRTAllocateFunction, true, metadata);`
`2823`	`2823`	`PARAM_NOT_NULL(nativeFunction);`
`2824`	`2824`	`PARAM_NOT_NULL(function);`
`2825`	`2825`	`*function = nullptr;`
`@@ -2865,7 +2865,7 @@ CHAKRA_API JsCreateEnhancedFunction(_In_ JsEnhancedNativeFunction nativeFunction`
`2865`	`2865`
`2866`	`2866`	`CHAKRA_API JsCreateFunction(_In_ JsNativeFunction nativeFunction, _In_opt_ void callbackState, _Out_ JsValueRef function)`
`2867`	`2867`	`{`
`2868`		`- return JsCreateNamedFunction(JS_INVALID_REFERENCE, nativeFunction, callbackState, function);`
	`2868`	`+ return JsCreateEnhancedFunctionHelper<true>(nativeFunction, JS_INVALID_REFERENCE, callbackState, function);`
`2869`	`2869`	`}`
`2870`	`2870`
`2871`	`2871`	`CHAKRA_API JsCreateNamedFunction(_In_ JsValueRef name, _In_ JsNativeFunction nativeFunction, _In_opt_ void callbackState, _Out_ JsValueRef function)`