Address comments

Author: wu-hui

packages/firestore/src/platform/platform.ts

@@ -46,7 +46,7 @@ export interface Platform {
   /**
    * Generates `nBytes` of random bytes.
    *
-   * If `nBytes <= 0` , an error will be thrown.
+   * If `nBytes < 0` , an error will be thrown.
    */
   randomBytes(nBytes: number): Uint8Array;
 

packages/firestore/src/platform_browser/browser_platform.ts

@@ -76,21 +76,21 @@ export class BrowserPlatform implements Platform {
   }
 
   randomBytes(nBytes: number): Uint8Array {
-    debugAssert(nBytes >= 0, 'Number of random bytes must not be negative');
+    debugAssert(nBytes >= 0, `Expecting non-negative nBytes, got: ${nBytes}`);
 
-    // Polyfill for IE and WebWorker
+    // Polyfills for IE and WebWorker by using `self` and `msCrypto` when `crypto` is not available.
     const crypto =
       // eslint-disable-next-line @typescript-eslint/no-explicit-any
       typeof self !== 'undefined' && (self.crypto || (self as any)['msCrypto']);
-    const v = new Uint8Array(nBytes);
+    const bytes = new Uint8Array(nBytes);
     if (crypto) {
-      crypto.getRandomValues(v);
+      crypto.getRandomValues(bytes);
     } else {
       // Falls back to Math.random
       for (let i = 0; i < nBytes; i++) {
-        v[i] = Math.floor(Math.random() * 256);
+        bytes[i] = Math.floor(Math.random() * 256);
       }
     }
-    return v;
+    return bytes;
   }
 }

packages/firestore/src/platform_node/node_platform.ts

@@ -28,7 +28,7 @@ import { NoopConnectivityMonitor } from './../remote/connectivity_monitor_noop';
 
 import { GrpcConnection } from './grpc_connection';
 import { loadProtos } from './load_protos';
-import { validatePositiveNumber } from '../util/input_validation';
+import { debugAssert } from '../util/assert';
 
 export class NodePlatform implements Platform {
   readonly base64Available = true;
@@ -79,7 +79,7 @@ export class NodePlatform implements Platform {
   }
 
   randomBytes(nBytes: number): Uint8Array {
-    validatePositiveNumber('randomBytes', 1, nBytes);
+    debugAssert(nBytes >= 0, `Expecting non-negative nBytes, got: ${nBytes}`);
 
     return randomBytes(nBytes);
   }

packages/firestore/src/util/misc.ts

@@ -28,20 +28,27 @@ export class AutoId {
     // Alphanumeric characters
     const chars =
       'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789';
+    // The largest byte value that is a multiple of `char.length`.
+    const maxMultiple = Math.floor(256 / chars.length) * chars.length;
+    debugAssert(
+      0 < maxMultiple && maxMultiple < 256,
+      `Expect maxMultiple to be (0, 256), but got ${maxMultiple}`
+    );
+
     let autoId = '';
-    while (autoId.length < 20) {
+    const targetLength = 20;
+    while (autoId.length < targetLength) {
       const bytes = PlatformSupport.getPlatform().randomBytes(40);
       for (let i = 0; i < bytes.length; ++i) {
-        // Length of `chars` is 62. We only take bytes between 0 and 62*4-1
-        // (both inclusive). The value is then evenly mapped to indices of `char`
-        // via a modulo operation.
-        const maxValue = 62 * 4 - 1;
-        if (autoId.length < 20 && bytes[i] <= maxValue) {
-          autoId += chars.charAt(bytes[i] % 62);
+        // Only accept values that are [0, maxMultiple), this ensures they can
+        // be evenly mapped to indices of `chars` via a modulo operation.
+        if (autoId.length < targetLength && bytes[i] < maxMultiple) {
+          autoId += chars.charAt(bytes[i] % chars.length);
         }
       }
     }
-    debugAssert(autoId.length === 20, 'Invalid auto ID: ' + autoId);
+    debugAssert(autoId.length === targetLength, 'Invalid auto ID: ' + autoId);
+
     return autoId;
   }
 }