mike-jumper commented on code in PR #1066:
URL: https://github.com/apache/guacamole-client/pull/1066#discussion_r1984191611


##########
guacamole-common-js/src/main/webapp/modules/SessionRecording.js:
##########
@@ -352,11 +381,12 @@ Guacamole.SessionRecording = function 
SessionRecording(source, refreshInterval)
      */
     var getElementSize = function getElementSize(value) {

Review Comment:
   The `@returns` documentation for this function needs to be updated for the 
change in behavior (number of bytes instead of number of Unicode characters).



##########
guacamole-common-js/src/main/webapp/modules/SessionRecording.js:
##########
@@ -352,11 +381,12 @@ Guacamole.SessionRecording = function 
SessionRecording(source, refreshInterval)
      */
     var getElementSize = function getElementSize(value) {
 
-        var valueLength = value.length;
-
         // Calculate base size, assuming at least one digit, the "."
         // separator, and the "," or ";" terminator
-        var protocolSize = valueLength + 3;
+        var protocolSize = getUtf8StringByteSize(value) + 3;
+
+        // We need this to calculate the size of the length substring.
+        var valueLength = value.length;

Review Comment:
   Won't this potentially not be correct for elements containing Unicode 
characters that involve surrogate pairs in their JavaScript string 
representations?
   
   If so, I think you need to use `Guacamole.Parser.codePointCount()` instead.



##########
guacamole-common-js/src/main/webapp/modules/SessionRecording.js:
##########
@@ -334,9 +334,38 @@ Guacamole.SessionRecording = function 
SessionRecording(source, refreshInterval)
 
     };
 
+    /**
+     * Calculates the size in bytes of the given UTF8 string.
+     *
+     * @private
+     * @param {!string} str
+     *     The string to calculate the size in bytes.
+     *
+     * @returns {!number}
+     *     The size in bytes of the given string.
+     */
+    var getUtf8StringByteSize = function(str) {
+
+        var byteSize = str.length;
+        for (var i = str.length - 1; i >= 0; i--) {
+            var code = str.charCodeAt(i);
+            if (code > 0x7F && code <= 0x7FF)
+                byteSize++;
+            else if (code > 0x7FF && code <= 0xFFFF)
+                byteSize += 2;            
+            if (code >= 0xDC00 && code <= 0xDFFF)
+                i--; // trail surrogate

Review Comment:
   It is unclear what "trail surrogate" means here.



##########
guacamole-common-js/src/main/webapp/modules/SessionRecording.js:
##########
@@ -352,11 +381,12 @@ Guacamole.SessionRecording = function 
SessionRecording(source, refreshInterval)
      */
     var getElementSize = function getElementSize(value) {
 
-        var valueLength = value.length;
-
         // Calculate base size, assuming at least one digit, the "."
         // separator, and the "," or ";" terminator
-        var protocolSize = valueLength + 3;
+        var protocolSize = getUtf8StringByteSize(value) + 3;

Review Comment:
   The original name `protocolSize` refers to the size of the element as 
represented by the protocol (in characters). Since this value is in bytes here, 
the variable should be renamed accordingly.



##########
guacamole-common-js/src/main/webapp/modules/SessionRecording.js:
##########
@@ -334,9 +334,38 @@ Guacamole.SessionRecording = function 
SessionRecording(source, refreshInterval)
 
     };
 
+    /**
+     * Calculates the size in bytes of the given UTF8 string.
+     *
+     * @private
+     * @param {!string} str
+     *     The string to calculate the size in bytes.
+     *
+     * @returns {!number}
+     *     The size in bytes of the given string.
+     */
+    var getUtf8StringByteSize = function(str) {
+
+        var byteSize = str.length;
+        for (var i = str.length - 1; i >= 0; i--) {
+            var code = str.charCodeAt(i);
+            if (code > 0x7F && code <= 0x7FF)
+                byteSize++;
+            else if (code > 0x7FF && code <= 0xFFFF)
+                byteSize += 2;            
+            if (code >= 0xDC00 && code <= 0xDFFF)

Review Comment:
   There are a good few magic numbers here. Comments describing the high-level 
intent of each section are needed to de-magic the logic.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@guacamole.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to