Jsrt: new string API #1830

obastemur · 2016-10-26T20:33:58Z

JsCreateString
JsCreateStringUtf8
JsCreateStringUtf16
JsCopyString
JsCopyStringUtf8
JsCopyStringUtf16
JsParse
JsRun
JsSerialize
JsParseSerialized
JsRunSerialized
JsCreatePropertyIdUtf8
JsCopyPropertyIdUtf8

New: ExternalArrayBuffer can be used as script source.
In case the ExternalArrayBuffer source is Utf8, CC uses it as is.
no additional copy/encode/decode..

p.s. [ Create/Copy String credits goes to @jianchun ]

jianchun · 2016-10-27T23:27:58Z

bin/ch/ChakraRtInterface.h

@@ -25,7 +25,6 @@ struct JsAPIHooks
    typedef JsErrorCode (WINAPI *JsrtConvertValueToBooleanPtr)(JsValueRef value, JsValueRef *booleanValue);
    typedef JsErrorCode (WINAPI *JsrtStringToPointerUtf8CopyPtr)(JsValueRef value, char **stringValue, size_t *length);


typedef JsErrorCode (WINAPI _JsrtStringToPointerUtf8CopyPtr)(JsValueRef value, char *_stringValue, size_t *length); [](start = 3, length = 116)

JsrtStringToPointerUtf8CopyPtr not needed?

jianchun · 2016-10-27T23:31:06Z

bin/ch/ChakraRtInterface.h

+    typedef JsErrorCode(WINAPI *JsrtWriteStringUtf8)(JsValueRef value, uint8_t* buffer, size_t bufferSize, size_t* written);
+    typedef JsErrorCode(WINAPI *JsrtCreatePropertyIdUtf8)(const char *name, size_t length, JsPropertyIdRef *propertyId);
+    typedef JsErrorCode(WINAPI *JsrtCreateStringUtf8)(const uint8_t *content, size_t length, JsValueRef *value);
+    typedef JsErrorCode(WINAPI *JsrtCreateExternalArrayBuffer)(void *data, unsigned int byteLength, JsFinalizeCallback finalizeCallback, void *callbackState, JsValueRef *result);


nit pick: please place CreateStringUtf8/WriteStringUtf8 next to each other.

jianchun · 2016-10-27T23:39:52Z

bin/ch/WScriptJsrt.cpp

@@ -414,7 +409,13 @@ JsValueRef WScriptJsrt::LoadScript(JsValueRef callee, LPCSTR fileName, LPCSTR fi

        IfJsrtErrorSetGo(ChakraRTInterface::JsSetCurrentContext(calleeContext));

-        errorCode = ChakraRTInterface::JsRunScriptUtf8(fileContent, GetNextSourceContext(), fullPathNarrow, &returnValue);
+        JsValueRef scriptSource;
+        ChakraRTInterface::JsCreateExternalArrayBuffer((void*)fileContent, (unsigned int)strlen(fileContent),


ChakraRTInterface::JsCreateExternalArrayBuffer [](start = 8, length = 46)

Some new code in this file like this may use handle error / IfJsrtErrorSetGo.

jianchun · 2016-10-27T23:43:30Z

bin/ch/WScriptJsrt.cpp

-    IfJsrtErrorFail(ChakraRTInterface::JsPointerToStringUtf8(CPU_ARCH_TEXT,strlen(CPU_ARCH_TEXT), &archValue), false);
-    IfJsrtErrorFail(ChakraRTInterface::JsSetProperty(platformObject, archProperty, archValue, true), false);
+    IfJsrtErrorFail(ChakraRTInterface::JsCreateStringUtf8(
+        (const uint8_t*)CPU_ARCH_TEXT,


We have "char*" version as well, should we use that or stay away from it?

Char version for ASCII input

jianchun · 2016-10-27T23:48:31Z

bin/ch/stdafx.h

        {
-            return;
+            size_t len = 0;
+            errorCode = ChakraRTInterface::JsWriteStringUtf8(strValue, nullptr, 0, &len);


This is fine, though we have JsWriteStringUtf16. In case the user instantiate this class only to GetWideString(), should we consider delay reading string content and also exercise JsWriteStringUtf16? May be a good chance to test out that API.

We have test/native-test for this purpose. Prefer a sample there

see new native-test test-char16.

/cc @liminzhu . We need to update Chakra-Samples repo parallel to this PR.

jianchun · 2016-10-27T23:51:27Z

lib/Jsrt/ChakraCommon.h

+        ///     ChakraCore assumes ExternalArrayBuffer is Utf8 by default.
+        ///     This one needs to be set for Utf16
+        /// </summary>
+        JsParseScriptAttributeUtf16Encoding = 0x2,


JsParseScriptAttributeUtf16Encoding [](start = 8, length = 35)

The comment is clear, but users may possibly be confused by the name and think this flag always apply? Should we explicitly have "ArrayBuffer" in the flag name? Or not?

Being explicit is better, though that will make this a long name but we should make it JsParseScriptAttributeArrayBufferIsUtf16Encoded

In reply to: 85450903 [](ancestors = 85450903)

Okay two vs one. Updating to JsParseScriptAttributeArrayBufferIsUtf16Encoded

jianchun · 2016-10-27T23:52:26Z

lib/Jsrt/ChakraCore.h

+/// </returns>
+typedef bool (CHAKRA_CALLBACK * JsSerializedLoadScriptCallback)
+    (JsSourceContext sourceContext, JsValueRef *value,
+    JsParseScriptAttributes *parseAttributes);


Missing SAL annotations

agarwal-sandeep · 2016-11-02T23:24:24Z

lib/Jsrt/ChakraCore.h

+        _In_ JsValueRef value,
+        _Out_opt_ uint8_t* buffer,
+        _In_ size_t bufferSize,
+        _Out_opt_ size_t* length);


length [](start = 26, length = 6)

Change to written, to be consistent with other APIs

jianchun · 2016-11-03T23:56:10Z

lib/Jsrt/ChakraCommon.h

+        JsParseScriptAttributeLibraryCode = 0x1,
+        /// <summary>
+        ///     ChakraCore assumes ExternalArrayBuffer is Utf8 by default.
+        ///     This one needs to be set for Utf16


nit: description may need to mention parse script context, otherwise reading by itself is confusing (why ExternalArrayBuffer has anything to do with utf8).

jianchun · 2016-11-03T23:57:31Z

lib/Jsrt/ChakraCore.h

+/// <summary>
+///     Called by the runtime to load the source code of the serialized script.
+///     The caller must keep the script buffer valid until the JsSerializedScriptUnloadCallback.
+///     This callback is only supported by the Win32 version of the API


Comment outdated (copied from old?)

jianchun · 2016-11-03T23:58:18Z

lib/Jsrt/ChakraCore.h

+///     The caller must keep the script buffer valid until the JsSerializedScriptUnloadCallback.
+///     This callback is only supported by the Win32 version of the API
+/// </summary>
+/// <param name="sourceContext">The context passed to Js[Parse|Run]SerializedScriptWithCallback</param>


Comment outdated (passed to "API names" changed)

jianchun · 2016-11-03T23:58:43Z

lib/Jsrt/ChakraCore.h

+///     </para>
+/// </remarks>
+/// <param name="content">Pointer to string memory.</param>
+/// <param name="length">Number of bytes within the string</param>


I assume "length" is number of characters, not number of bytes.

It is number of bytes not chars.

No, I think it is number of characters.

No, I think it is number of characters.

Appreciate if you can point me to source code showing that.

Appreciate if you can point me to source code showing that.

Track down the implementation. It calls JsPointerToString. And that length is "stringLength", cchLength, ...

On the email it shows JsCreateStringUtf8 on top... followed the line from here and what you mean here is Utf16... Yes for Utf16, it is the length of string..

jianchun · 2016-11-03T23:59:02Z

lib/Jsrt/ChakraCore.h

+///     </para>
+/// </remarks>
+/// <param name="content">Pointer to string memory.</param>
+/// <param name="length">Number of bytes within the string</param>


Consider "number of characters"?

This is also number of bytes and actually same thing for this one

jianchun · 2016-11-04T00:00:37Z

lib/Jsrt/ChakraCore.h

+        _Out_ JsValueRef *result);
+
+/// <summary>
+///     Gets the property ID associated with the name.


The 2 PropertyID APIs may have a different description than before, now that the API names don't contain "Get".

jianchun · 2016-11-04T00:04:07Z

LGTM. Left some comments on API descriptions. Maybe @liminzhu could go through "ChakraCore.h" and help with the API docs?

obastemur · 2016-11-04T00:10:51Z

Did some extra cleanup to old comments.

Maybe @liminzhu could go through "ChakraCore.h" and help with the API docs?

We may better decide on remaining types etc. part first (before updating anything again). Changing design again and again on the PR is a huge pain.

- JsCreateString - JsCreateStringUtf8 - JsCreateStringUtf16 - JsCopyString - JsCopyStringUtf8 - JsCopyStringUtf16 - JsParse - JsRun - JsSerialize - JsParseSerialized - JsRunSerialized - JsCreatePropertyIdUtf8 - JsCopyPropertyIdUtf8 With this update, `ExternalArrayBuffer` can be used as script source. When ExternalArrayBuffer source is Utf8, it is used as is and keeps script source from being copied.

obastemur · 2016-11-04T01:24:16Z

@jianchun @agarwal-sandeep @liminzhu Thanks for the review and discussion.

obastemur · 2016-11-04T01:24:29Z

@dotnet-bot test Windows x86_debug please

@jianchun

Merge pull request #1830 from obastemur:string_api - JsCreateString - JsCreateStringUtf8 - JsCreateStringUtf16 - JsCopyString - JsCopyStringUtf8 - JsCopyStringUtf16 - JsParse - JsRun - JsSerialize - JsParseSerialized - JsRunSerialized - JsCreatePropertyIdUtf8 - JsCopyPropertyIdUtf8 New: `ExternalArrayBuffer` can be used as script source. In case the ExternalArrayBuffer source is Utf8, CC uses it as is. no additional copy/encode/decode.. p.s. [ Create/Copy String credits goes to @jianchun ]

kphillisjr · 2016-11-15T08:43:04Z

This pull request is incomplete. The ChakraCore Samples needs updated. See this Issue: microsoft/Chakra-Samples#47

obastemur · 2016-11-15T09:22:53Z

@kphillisjr microsoft/Chakra-Samples#48

msftclas added the cla-already-signed label Oct 26, 2016

obastemur force-pushed the string_api branch 22 times, most recently from e7a9e71 to 45bfcd6 Compare October 27, 2016 15:51

jianchun reviewed Oct 27, 2016

View reviewed changes

obastemur force-pushed the string_api branch 2 times, most recently from d947b7d to d79da15 Compare November 2, 2016 23:12

agarwal-sandeep reviewed Nov 2, 2016

View reviewed changes

obastemur force-pushed the string_api branch 7 times, most recently from fd8e305 to 6c90306 Compare November 3, 2016 23:48

jianchun reviewed Nov 3, 2016

View reviewed changes

jianchun reviewed Nov 4, 2016

View reviewed changes

obastemur force-pushed the string_api branch from 6c90306 to 59d5d26 Compare November 4, 2016 00:08

obastemur force-pushed the string_api branch from 59d5d26 to f7f7132 Compare November 4, 2016 00:21

chakrabot merged commit f7f7132 into chakra-core:master Nov 4, 2016

liminzhu mentioned this pull request Nov 14, 2016

JsRunScriptUtf8 missing #2006

Closed

obastemur deleted the string_api branch November 15, 2016 13:20

agarwal-sandeep mentioned this pull request Dec 21, 2016

Update Jsrt Utf8 types and naming #2251

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jsrt: new string API #1830

Jsrt: new string API #1830

obastemur commented Oct 26, 2016 •

edited

Loading

jianchun Oct 27, 2016

jianchun Oct 27, 2016

jianchun Oct 27, 2016

jianchun Oct 27, 2016

obastemur Oct 29, 2016

jianchun Oct 27, 2016

obastemur Oct 29, 2016

obastemur Oct 30, 2016

jianchun Oct 27, 2016

agarwal-sandeep Oct 28, 2016

obastemur Oct 30, 2016

jianchun Oct 27, 2016

agarwal-sandeep Nov 2, 2016

jianchun Nov 3, 2016

jianchun Nov 3, 2016

jianchun Nov 3, 2016

jianchun Nov 3, 2016

obastemur Nov 4, 2016

jianchun Nov 4, 2016

obastemur Nov 4, 2016

jianchun Nov 4, 2016

obastemur Nov 4, 2016

jianchun Nov 3, 2016

obastemur Nov 4, 2016

jianchun Nov 4, 2016

jianchun commented Nov 4, 2016

obastemur commented Nov 4, 2016

obastemur commented Nov 4, 2016

obastemur commented Nov 4, 2016

kphillisjr commented Nov 15, 2016

obastemur commented Nov 15, 2016

		@@ -25,7 +25,6 @@ struct JsAPIHooks
		typedef JsErrorCode (WINAPI JsrtConvertValueToBooleanPtr)(JsValueRef value, JsValueRef booleanValue);
		typedef JsErrorCode (WINAPI JsrtStringToPointerUtf8CopyPtr)(JsValueRef value, char stringValue, size_t length);

Jsrt: new string API #1830

Jsrt: new string API #1830

Conversation

obastemur commented Oct 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianchun commented Nov 4, 2016

obastemur commented Nov 4, 2016

obastemur commented Nov 4, 2016

obastemur commented Nov 4, 2016

kphillisjr commented Nov 15, 2016

obastemur commented Nov 15, 2016

obastemur commented Oct 26, 2016 •

edited

Loading