Jsrt: new string API by obastemur · Pull Request #1830 · chakra-core/ChakraCore

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

@@ -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

+    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


-        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

-    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

        {
-            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

+        ///     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

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


Missing SAL annotations

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

+        _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

+        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

+/// <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

+///     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

+///     </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

+///     </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

+        _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

		@@ -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);

Conversation

obastemur commented Oct 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jianchun commented Nov 4, 2016

Uh oh!

obastemur commented Nov 4, 2016

Uh oh!

obastemur commented Nov 4, 2016

Uh oh!

obastemur commented Nov 4, 2016

Uh oh!

kphillisjr commented Nov 15, 2016

Uh oh!

obastemur commented Nov 15, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

obastemur commented Oct 26, 2016 •

edited

Loading