refactor: move defaults to Realm (#2066)

Moves the defaults to the `Realm` class rather then the `ScriptProcessor`, so we can use them from anywhere. Also some small changes that I will mark as comments.
GoogleChromeLabs · Mar 22, 2024 · 405b995 · 405b995
1 parent c44e1e0
commit 405b995
Show file tree

Hide file tree

Showing 9 changed files with 55 additions and 104 deletions.
diff --git a/src/bidiMapper/domains/context/BrowsingContextImpl.ts b/src/bidiMapper/domains/context/BrowsingContextImpl.ts
@@ -789,15 +789,7 @@ export class BrowsingContextImpl {
       }
     }
     const realm = await this.getOrCreateSandbox(undefined);
-    const originResult = await realm.callFunction(
-      script,
-      {type: 'undefined'},
-      [],
-      false,
-      Script.ResultOwnership.None,
-      {},
-      false
-    );
+    const originResult = await realm.callFunction(script, false);
     assert(originResult.type === 'success');
     const origin = deserializeDOMRect(originResult.result);
     assert(origin);
@@ -929,11 +921,9 @@ export class BrowsingContextImpl {
           String((element: unknown) => {
             return element instanceof Element;
           }),
-          {type: 'undefined'},
-          [clip.element],
           false,
-          Script.ResultOwnership.None,
-          {}
+          {type: 'undefined'},
+          [clip.element]
         );
         if (result.type === 'exception') {
           throw new NoSuchElementException(
@@ -957,11 +947,9 @@ export class BrowsingContextImpl {
                 width: rect.width,
               };
             }),
-            {type: 'undefined'},
-            [clip.element],
             false,
-            Script.ResultOwnership.None,
-            {}
+            {type: 'undefined'},
+            [clip.element]
           );
           assert(result.type === 'success');
           const rect = deserializeDOMRect(result.result);
@@ -1235,12 +1223,11 @@ export class BrowsingContextImpl {
 
     const locatorResult = await realm.callFunction(
       locatorDelegate.functionDeclaration,
+      false,
       {type: 'undefined'},
       locatorDelegate.argumentsLocalValues,
-      false,
       Script.ResultOwnership.None,
-      serializationOptions,
-      false
+      serializationOptions
     );
 
     if (locatorResult.type !== 'success') {

diff --git a/src/bidiMapper/domains/context/BrowsingContextStorage.ts b/src/bidiMapper/domains/context/BrowsingContextStorage.ts
@@ -103,7 +103,9 @@ export class BrowsingContextStorage {
     return result;
   }
 
-  verifyContextsList(contexts: BrowsingContext.BrowsingContext[] | undefined) {
+  verifyTopLevelContextsList(
+    contexts: BrowsingContext.BrowsingContext[] | undefined
+  ) {
     const foundContexts = new Set<BrowsingContextImpl>();
     if (!contexts) {
       return foundContexts;

diff --git a/src/bidiMapper/domains/input/ActionDispatcher.ts b/src/bidiMapper/domains/input/ActionDispatcher.ts
@@ -20,7 +20,7 @@ import {
   InvalidArgumentException,
   MoveTargetOutOfBoundsException,
   NoSuchElementException,
-  Script,
+  type Script,
 } from '../../../protocol/protocol.js';
 import {assert} from '../../../utils/assert.js';
 import type {BrowsingContextImpl} from '../context/BrowsingContextImpl.js';
@@ -56,11 +56,9 @@ async function getElementCenter(
   const sandbox = await context.getOrCreateSandbox(undefined);
   const result = await sandbox.callFunction(
     CALCULATE_IN_VIEW_CENTER_PT_DECL,
-    {type: 'undefined'},
-    [element],
     false,
-    Script.ResultOwnership.None,
-    {}
+    {type: 'undefined'},
+    [element]
   );
   if (result.type === 'exception') {
     throw new NoSuchElementException(
@@ -82,14 +80,7 @@ export class ActionDispatcher {
   static isMacOS = async (context: BrowsingContextImpl) => {
     const result = await (
       await context.getOrCreateSandbox(undefined)
-    ).callFunction(
-      IS_MAC_DECL,
-      {type: 'undefined'},
-      [],
-      false,
-      Script.ResultOwnership.None,
-      {}
-    );
+    ).callFunction(IS_MAC_DECL, false);
     assert(result.type !== 'exception');
     assert(result.result.type === 'boolean');
     return result.result.value;

diff --git a/src/bidiMapper/domains/input/InputProcessor.ts b/src/bidiMapper/domains/input/InputProcessor.ts
@@ -105,12 +105,9 @@ export class InputProcessor {
           }
           return;
         }),
-        params.element,
-        [{type: 'number', value: params.files.length}],
         false,
-        Script.ResultOwnership.None,
-        {},
-        false
+        params.element,
+        [{type: 'number', value: params.files.length}]
       );
     } catch {
       throw new NoSuchElementException(
@@ -171,12 +168,8 @@ export class InputProcessor {
           );
           this.dispatchEvent(new Event('change', {bubbles: true}));
         }),
-        params.element,
-        [],
         false,
-        Script.ResultOwnership.None,
-        {},
-        false
+        params.element
       );
       return {};
     }
@@ -189,12 +182,10 @@ export class InputProcessor {
         String(function getFiles(this: HTMLInputElement, index: number) {
           return this.files?.item(index);
         }),
+        false,
         params.element,
         [{type: 'number', value: 0}],
-        false,
-        Script.ResultOwnership.Root,
-        {},
-        false
+        Script.ResultOwnership.Root
       );
       assert(result.type === 'success');
       if (result.result.type !== 'object') {
@@ -238,12 +229,8 @@ export class InputProcessor {
             })
           );
         }),
-        params.element,
-        [],
         false,
-        Script.ResultOwnership.None,
-        {},
-        false
+        params.element
       );
     }
     return {};

diff --git a/src/bidiMapper/domains/network/NetworkProcessor.ts b/src/bidiMapper/domains/network/NetworkProcessor.ts
@@ -22,7 +22,6 @@ import {
   NoSuchRequestException,
   InvalidArgumentException,
 } from '../../../protocol/protocol.js';
-import {assert} from '../../../utils/assert.js';
 import type {BrowsingContextStorage} from '../context/BrowsingContextStorage.js';
 
 import type {NetworkRequest} from './NetworkRequest.js';
@@ -48,7 +47,7 @@ export class NetworkProcessor {
   async addIntercept(
     params: Network.AddInterceptParameters
   ): Promise<Network.AddInterceptResult> {
-    this.#browsingContextStorage.verifyContextsList(params.contexts);
+    this.#browsingContextStorage.verifyTopLevelContextsList(params.contexts);
 
     const urlPatterns: Network.UrlPattern[] = params.urlPatterns ?? [];
     const parsedUrlPatterns: Network.UrlPattern[] =
@@ -75,7 +74,7 @@ export class NetworkProcessor {
   async continueRequest(
     params: Network.ContinueRequestParameters
   ): Promise<EmptyResult> {
-    const networkId = params.request;
+    const {url, method, headers, request: networkId} = params;
 
     if (params.url !== undefined) {
       NetworkProcessor.parseUrlString(params.url);
@@ -85,14 +84,12 @@ export class NetworkProcessor {
       Network.InterceptPhase.BeforeRequestSent,
     ]);
 
-    const {url, method, headers} = params;
-    // TODO: Set / expand.
-    // ; Step 9. cookies
-    // ; Step 10. body
-
     const requestHeaders: Protocol.Fetch.HeaderEntry[] | undefined =
       cdpFetchHeadersFromBidiNetworkHeaders(headers);
 
+    // TODO: Set / expand.
+    // ; Step 9. cookies
+    // ; Step 10. body
     await request.continueRequest(url, method, requestHeaders);
 
     return {};
@@ -160,12 +157,6 @@ export class NetworkProcessor {
 
       username = credentials.username;
       password = credentials.password;
-      // TODO: This should be invalid argument exception.
-      // Spec may need to be updated.
-      assert(
-        credentials.type === 'password',
-        `Credentials type ${credentials.type} must be 'password'`
-      );
     }
 
     const response = cdpAuthChallengeResponseFromBidiAuthContinueWithAuthAction(

diff --git a/src/bidiMapper/domains/script/Realm.ts b/src/bidiMapper/domains/script/Realm.ts
@@ -189,8 +189,8 @@ export abstract class Realm {
   async evaluate(
     expression: string,
     awaitPromise: boolean,
-    resultOwnership: Script.ResultOwnership,
-    serializationOptions: Script.SerializationOptions,
+    resultOwnership: Script.ResultOwnership = Script.ResultOwnership.None,
+    serializationOptions: Script.SerializationOptions = {},
     userActivation = false
   ): Promise<Script.EvaluateResult> {
     const cdpEvaluateResult = await this.cdpClient.sendCommand(
@@ -362,11 +362,13 @@ export abstract class Realm {
 
   async callFunction(
     functionDeclaration: string,
-    thisLocalValue: Script.LocalValue,
-    argumentsLocalValues: Script.LocalValue[],
     awaitPromise: boolean,
-    resultOwnership: Script.ResultOwnership,
-    serializationOptions: Script.SerializationOptions,
+    thisLocalValue: Script.LocalValue = {
+      type: 'undefined',
+    },
+    argumentsLocalValues: Script.LocalValue[] = [],
+    resultOwnership: Script.ResultOwnership = Script.ResultOwnership.None,
+    serializationOptions: Script.SerializationOptions = {},
     userActivation = false
   ): Promise<Script.EvaluateResult> {
     const callFunctionAndSerializeScript = `(...args) => {

diff --git a/src/bidiMapper/domains/script/ScriptProcessor.ts b/src/bidiMapper/domains/script/ScriptProcessor.ts
@@ -17,7 +17,7 @@
 
 import {
   type EmptyResult,
-  Script,
+  type Script,
   NoSuchScriptException,
 } from '../../../protocol/protocol';
 import type {LoggerFn} from '../../../utils/log';
@@ -50,7 +50,7 @@ export class ScriptProcessor {
   async addPreloadScript(
     params: Script.AddPreloadScriptParameters
   ): Promise<Script.AddPreloadScriptResult> {
-    const contexts = this.#browsingContextStorage.verifyContextsList(
+    const contexts = this.#browsingContextStorage.verifyTopLevelContextsList(
       params.contexts
     );
 
@@ -78,23 +78,17 @@ export class ScriptProcessor {
   async removePreloadScript(
     params: Script.RemovePreloadScriptParameters
   ): Promise<EmptyResult> {
-    const bidiId = params.script;
+    const {script: id} = params;
 
-    const scripts = this.#preloadScriptStorage.find({
-      id: bidiId,
-    });
+    const scripts = this.#preloadScriptStorage.find({id});
 
     if (scripts.length === 0) {
-      throw new NoSuchScriptException(
-        `No preload script with BiDi ID '${bidiId}'`
-      );
+      throw new NoSuchScriptException(`No preload script with id '${id}'`);
     }
 
     await Promise.all(scripts.map((script) => script.remove()));
 
-    this.#preloadScriptStorage.remove({
-      id: bidiId,
-    });
+    this.#preloadScriptStorage.remove({id});
 
     return {};
   }
@@ -105,14 +99,12 @@ export class ScriptProcessor {
     const realm = await this.#getRealm(params.target);
     return await realm.callFunction(
       params.functionDeclaration,
-      params.this ?? {
-        type: 'undefined',
-      }, // `this` is `undefined` by default.
-      params.arguments ?? [], // `arguments` is `[]` by default.
       params.awaitPromise,
-      params.resultOwnership ?? Script.ResultOwnership.None,
-      params.serializationOptions ?? {},
-      params.userActivation ?? false
+      params.this,
+      params.arguments,
+      params.resultOwnership,
+      params.serializationOptions,
+      params.userActivation
     );
   }
 
@@ -123,9 +115,9 @@ export class ScriptProcessor {
     return await realm.evaluate(
       params.expression,
       params.awaitPromise,
-      params.resultOwnership ?? Script.ResultOwnership.None,
-      params.serializationOptions ?? {},
-      params.userActivation ?? false
+      params.resultOwnership,
+      params.serializationOptions,
+      params.userActivation
     );
   }
 

diff --git a/src/bidiMapper/domains/script/WindowRealm.ts b/src/bidiMapper/domains/script/WindowRealm.ts
@@ -215,9 +215,9 @@ export class WindowRealm extends Realm {
 
   override async callFunction(
     functionDeclaration: string,
+    awaitPromise: boolean,
     thisLocalValue: Script.LocalValue,
     argumentsLocalValues: Script.LocalValue[],
-    awaitPromise: boolean,
     resultOwnership: Script.ResultOwnership,
     serializationOptions: Script.SerializationOptions,
     userActivation?: boolean
@@ -228,9 +228,9 @@ export class WindowRealm extends Realm {
 
     return await super.callFunction(
       functionDeclaration,
+      awaitPromise,
       thisLocalValue,
       argumentsLocalValues,
-      awaitPromise,
       resultOwnership,
       serializationOptions,
       userActivation

diff --git a/tests/script/test_remove_preload_script.py b/tests/script/test_remove_preload_script.py
@@ -22,7 +22,7 @@ async def test_preloadScript_remove_nonExistingScript_fails(websocket):
     with pytest.raises(Exception,
                        match=str({
                            'error': 'no such script',
-                           'message': "No preload script with BiDi ID '42'"
+                           'message': "No preload script with id '42'"
                        })):
         await execute_command(websocket, {
             "method": "script.removePreloadScript",
@@ -70,12 +70,11 @@ async def test_preloadScript_remove_addAndRemoveIsNoop_secondRemoval_fails(
     assert result["result"] == {"type": "undefined"}
 
     # Ensure script was removed
-    with pytest.raises(
-            Exception,
-            match=str({
-                'error': 'no such script',
-                'message': f"No preload script with BiDi ID '{bidi_id}'"
-            })):
+    with pytest.raises(Exception,
+                       match=str({
+                           'error': 'no such script',
+                           'message': f"No preload script with id '{bidi_id}'"
+                       })):
         await execute_command(
             websocket, {
                 "method": "script.removePreloadScript",