From 8eaa5e94fbbd68fe45b1920b633174bdb963505c Mon Sep 17 00:00:00 2001
From: Alex Kaminskii <alex@openreplay.com>
Date: Wed, 5 Apr 2023 13:20:28 +0200
Subject: [PATCH] style(player): few renamings; comments improved

---
 frontend/app/player/common/ListWalker.ts      | 21 ++++++----
 .../app/player/web/managers/DOM/DOMManager.ts | 41 +++++++++++--------
 .../app/player/web/managers/DOM/VirtualDOM.ts | 33 +++++++++++----
 3 files changed, 61 insertions(+), 34 deletions(-)

diff --git a/frontend/app/player/common/ListWalker.ts b/frontend/app/player/common/ListWalker.ts
index ca3d409ac..61f93f8df 100644
--- a/frontend/app/player/common/ListWalker.ts
+++ b/frontend/app/player/common/ListWalker.ts
@@ -105,11 +105,11 @@ export default class ListWalker<T extends Timed> {
 			: null
 	}
 
-	/*
-		Returns last message with the time <= t.
-		Assumed that the current message is already handled so
-		if pointer doesn't cahnge <null> is returned.
-	*/
+	/**
+	 * @returns last message with the time <= t.
+	 * Assumed that the current message is already handled so
+	 * if pointer doesn't cahnge <null> is returned.
+	 */
 	moveGetLast(t: number, index?: number): T | null {
 		let key: string = "time"; //TODO
 		let val = t;
@@ -130,7 +130,13 @@ export default class ListWalker<T extends Timed> {
 		return changed ? this.list[ this.p - 1 ] : null;
 	}
 
-	async moveWait(t: number, callback: (msg: T) => Promise<any> | undefined): Promise<void> {
+	/**
+	 * Moves over the messages starting from the current+1 to the last one with the time <= t
+	 * applying callback on each of them
+	 * @param t - max message time to move to; will move & apply callback while msg.time <= t
+	 * @param callback - a callback to apply on each message passing by while moving
+	 */
+	moveApply(t: number, callback: (msg: T) => void): void {
 		// Applying only in increment order for now
 		if (t < this.timeNow) {
 			this.reset();
@@ -138,8 +144,7 @@ export default class ListWalker<T extends Timed> {
 
 		const list = this.list
 		while (list[this.p] && list[this.p].time <= t) {
-			const maybePromise = callback(this.list[ this.p++ ]);
-			if (maybePromise) { await maybePromise }
+			callback(this.list[ this.p++ ])
 		}
 	}
 
diff --git a/frontend/app/player/web/managers/DOM/DOMManager.ts b/frontend/app/player/web/managers/DOM/DOMManager.ts
index 542e22b0a..c646aef5f 100644
--- a/frontend/app/player/web/managers/DOM/DOMManager.ts
+++ b/frontend/app/player/web/managers/DOM/DOMManager.ts
@@ -22,8 +22,8 @@ import { deleteRule, insertRule } from './safeCSSRules';
 
 type HTMLElementWithValue = HTMLInputElement | HTMLTextAreaElement | HTMLSelectElement;
 
-const IGNORED_ATTRS = [ "autocomplete" ];
-const ATTR_NAME_REGEXP = /([^\t\n\f \/>"'=]+)/; // regexp costs ~
+const IGNORED_ATTRS = [ "autocomplete" ]
+const ATTR_NAME_REGEXP = /([^\t\n\f \/>"'=]+)/
 
 export default class DOMManager extends ListWalker<Message> {
   private readonly vTexts: Map<number, VText> = new Map() // map vs object here?
@@ -31,7 +31,7 @@ export default class DOMManager extends ListWalker<Message> {
   private readonly olVRoots: Map<number, OnloadVRoot> = new Map()
   /** Constructed StyleSheets https://developer.mozilla.org/en-US/docs/Web/API/Document/adoptedStyleSheets
    * as well as <style> tag owned StyleSheets
-   * */
+   */
   private olStyleSheets: Map<number, OnloadStyleSheet> = new Map()
   /** @depreacted since tracker 4.0.2 Mapping by nodeID */
   private olStyleSheetsDeprecated: Map<number, OnloadStyleSheet> = new Map()
@@ -139,7 +139,7 @@ export default class DOMManager extends ListWalker<Message> {
     if (!vn) { logger.error("SetNodeAttribute: Node not found", msg); return }
 
     if (vn.tagName === "INPUT" && name === "name") {
-      // Otherwise binds local autocomplete values (maybe should ignore on the tracker level)
+      // Otherwise binds local autocomplete values (maybe should ignore on the tracker level?)
       return
     }
     if (name === "href" && vn.tagName === "LINK") {
@@ -148,15 +148,17 @@ export default class DOMManager extends ListWalker<Message> {
       //   value = value.replace("?", "%3F");
       // }
       if (!value.startsWith("http")) {
+        /* blob:... value can happen here for some reason.
+         * which will result in that link being unable to load and having 4sec timeout in the below function.
+         */
         return
       }
-      // blob:... value can happen here for some reason.
-      // which will result in that link being unable to load and having 4sec timeout in the below function.
 
       // TODO: check if node actually exists on the page, not just in memory
       this.stylesManager.setStyleHandlers(vn.node as HTMLLinkElement, value);
     }
     if (vn.isSVG && value.startsWith("url(")) {
+      /* SVG shape ID-s for masks etc. Sometimes referred with the full-page url, which we don't have in replay */
       value = "url(#" + (value.split("#")[1] ||")")
     }
     vn.setAttribute(name, value)
@@ -185,7 +187,7 @@ export default class DOMManager extends ListWalker<Message> {
         this.olVRoots.clear()
         this.olVRoots.set(0, vDoc) // watchout: id==0 for both Document and documentElement
         // this is done for the AdoptedCSS logic
-        // todo: start from 0-node (sync logic with tracker)
+        // Maybetodo: start Document as 0-node in tracker
         this.vTexts.clear()
         this.stylesManager.reset()
         this.stringDict = {}
@@ -339,7 +341,7 @@ export default class DOMManager extends ListWalker<Message> {
           logger.warn("No stylesheet was created for ", msg)
           return
         }
-        // @ts-ignore
+        // @ts-ignore (configure ts with recent WebaAPI)
         styleSheet.replaceSync(msg.text)
         return
       }
@@ -359,7 +361,7 @@ export default class DOMManager extends ListWalker<Message> {
           olStyleSheet = OnloadStyleSheet.fromVRootContext(vRoot)
           this.olStyleSheets.set(msg.sheetID, olStyleSheet)
         }
-        olStyleSheet.doNext(styleSheet => {
+        olStyleSheet.whenReady(styleSheet => {
           vRoot.onNode(node => {
             // @ts-ignore
             node.adoptedStyleSheets = [...node.adoptedStyleSheets, styleSheet]
@@ -375,7 +377,7 @@ export default class DOMManager extends ListWalker<Message> {
         }
         const vRoot = this.olVRoots.get(msg.id)
         if (!vRoot) { logger.error("AdoptedSsRemoveOwner: Owner node not found", msg); return }
-        olStyleSheet.doNext(styleSheet => {
+        olStyleSheet.whenReady(styleSheet => {
           vRoot.onNode(node => {
             // @ts-ignore
             node.adoptedStyleSheets = [...vRoot.node.adoptedStyleSheets].filter(s => s !== styleSheet)
@@ -386,7 +388,7 @@ export default class DOMManager extends ListWalker<Message> {
       case MType.LoadFontFace: {
         const vRoot = this.olVRoots.get(msg.parentID)
         if (!vRoot) { logger.error("LoadFontFace: Node not found", msg); return }
-        vRoot.doNext(vNode => {
+        vRoot.whenReady(vNode => {
           if (vNode instanceof VShadowRoot) { logger.error(`Node ${vNode} expected to be a Document`, msg); return }
           let descr: Object | undefined
           try {
@@ -397,7 +399,7 @@ export default class DOMManager extends ListWalker<Message> {
           }
           const ff = new FontFace(msg.family, msg.source, descr)
           vNode.node.fonts.add(ff)
-          ff.load() // TODO: wait for this one in StylesManager in a common way with styles
+          ff.load() // TODOTODO: wait for this one in StylesManager in a common way with styles
         })
         return
       }
@@ -437,7 +439,10 @@ export default class DOMManager extends ListWalker<Message> {
    * to the one with msg.time >= `t`
    * 
    * This function autoresets pointer if necessary (better name?)
-   * */
+   * 
+   * @returns Promise that fulfulls when necessary changes get applied 
+   *   (the async part exists mostly due to styles loading)
+   */
   async moveReady(t: number): Promise<void> {
     /**
      * Basically just skipping all set attribute with attrs being "href" if user is 'jumping'
@@ -447,7 +452,7 @@ export default class DOMManager extends ListWalker<Message> {
      * */
     // http://0.0.0.0:3333/5/session/8452905874437457
     // 70 iframe, 8 create element - STYLE tag
-    await this.moveWait(t, this.applyMessage)
+    this.moveApply(t, this.applyMessage)
 
     this.attrsBacktrack.forEach(msg => {
       this.applyBacktrack(msg)
@@ -458,10 +463,12 @@ export default class DOMManager extends ListWalker<Message> {
     // Thinkabout (read): css preload
     // What if we go back before it is ready? We'll have two handlres?
     return this.stylesManager.moveReady(t).then(() => {
-      // Apply focus
+      /* Waiting for styles to be applied first */
+      /* Applying focus */
       this.focusManager.move(t)
+      /* Applying text selections */
       this.selectionManager.move(t)
-      // Apply all scrolls after the styles got applied
+      /* Applying all scrolls */
       this.nodeScrollManagers.forEach(manager => {
         const msg = manager.moveGetLast(t)
         if (msg) {
@@ -470,7 +477,7 @@ export default class DOMManager extends ListWalker<Message> {
             scrollVHost.node.scrollLeft = msg.x
             scrollVHost.node.scrollTop = msg.y
           } else if ((scrollVHost = this.olVRoots.get(msg.id))) {
-            scrollVHost.doNext(vNode => {
+            scrollVHost.whenReady(vNode => {
               if (vNode instanceof VDocument) {
                 vNode.node.defaultView?.scrollTo(msg.x, msg.y)
               }
diff --git a/frontend/app/player/web/managers/DOM/VirtualDOM.ts b/frontend/app/player/web/managers/DOM/VirtualDOM.ts
index 3c08f76dd..944130fa5 100644
--- a/frontend/app/player/web/managers/DOM/VirtualDOM.ts
+++ b/frontend/app/player/web/managers/DOM/VirtualDOM.ts
@@ -18,6 +18,7 @@ export abstract class VNode<T extends Node = Node> {
 	 * 
 	 * @returns underneath JS DOM Node
 	 * @remarks should not be called unless the real node is required since creation might be expensive
+	 * 	It is better to use `onNode` callback applicator unless in the `applyChanges` implementation
 	 */
 	get node(): T {
 		if (!this._node) {
@@ -28,6 +29,12 @@ export abstract class VNode<T extends Node = Node> {
 		return this._node
 	}
 	private nodeCallbacks: Callback<T>[] = []
+	/**
+	 * Lazy Node callback applicator
+	 *
+	 * @param callback - Callback that fires on existing JS DOM Node instantly if it exists
+	 * or whenever it gets created. Call sequence is concerned.
+	 */
 	onNode(callback: Callback<T>) {
 		if (this._node) {
 			callback(this._node)
@@ -35,6 +42,10 @@ export abstract class VNode<T extends Node = Node> {
 		}
 		this.nodeCallbacks.push(callback)
 	}
+	/**
+	 * Abstract method, should be implemented by the actual classes
+	 * It is supposed to apply virtual changes into the actual DOM
+	 */
 	public abstract applyChanges(): void
 }
 
@@ -79,7 +90,7 @@ abstract class VParent<T extends Node = Node> extends VNode<T>{
 		}
 		/* Removing tail */
 		while(realChildren.length > this.children.length) {
-			node.removeChild(node.lastChild as Node) /* realChildren.length > this.children.length >= 0 */
+			node.removeChild(node.lastChild as Node) /* realChildren.length > this.children.length >= 0 so it is not null */
 		}
 	}
 }
@@ -180,7 +191,11 @@ export class VText extends VNode<Text> {
 
 class PromiseQueue<T> {
 	constructor(private promise: Promise<T>) {}
-	doNext(cb: Callback<T>) { // Doing this with callbacks list instead might be more efficient. TODO: research
+	/**
+	 * Call sequence is concerned.
+	 */
+	// Doing this with callbacks list instead might be more efficient (but more wordy). TODO: research
+	whenReady(cb: Callback<T>) {
 		this.promise = this.promise.then(vRoot => {
 			cb(vRoot)
 			return vRoot
@@ -194,7 +209,7 @@ class PromiseQueue<T> {
 /**
  * VRoot wrapper that allows to defer all the API calls till the moment
  * when VNode CAN be created (for example, on <iframe> mount&load)
- * */
+ */
 export class OnloadVRoot extends PromiseQueue<VRoot> {
 	static fromDocumentNode(doc: Document): OnloadVRoot {
 		return new OnloadVRoot(Promise.resolve(new VDocument(() => doc)))
@@ -230,13 +245,13 @@ export class OnloadVRoot extends PromiseQueue<VRoot> {
 		}))
 	}
 	onNode(cb: Callback<Document | ShadowRoot>) {
-		this.doNext(vRoot => vRoot.onNode(cb))
+		this.whenReady(vRoot => vRoot.onNode(cb))
 	}
 	applyChanges() {
-		this.doNext(vRoot => vRoot.applyChanges())
+		this.whenReady(vRoot => vRoot.applyChanges())
 	}
 	insertChildAt(...args: Parameters<VParent['insertChildAt']>) {
-		this.doNext(vRoot => vRoot.insertChildAt(...args))
+		this.whenReady(vRoot => vRoot.insertChildAt(...args))
 	}
 }
 
@@ -245,7 +260,7 @@ export type StyleElement = HTMLStyleElement | SVGStyleElement
 /**
  * CSSStyleSheet wrapper that collects all the insertRule/deleteRule calls 
  * and then applies them when the sheet is ready
- * */
+ */
 export class OnloadStyleSheet extends PromiseQueue<CSSStyleSheet> {
 	static fromStyleElement(node: StyleElement) {
 		return new OnloadStyleSheet(new Promise((resolve, reject) => {
@@ -276,10 +291,10 @@ export class OnloadStyleSheet extends PromiseQueue<CSSStyleSheet> {
 	}
 
 	insertRule(rule: string, index: number) {
-		this.doNext(s => insertRule(s, { rule, index }))
+		this.whenReady(s => insertRule(s, { rule, index }))
 	}
 
 	deleteRule(index: number) {
-		this.doNext(s => deleteRule(s, { index }))
+		this.whenReady(s => deleteRule(s, { index }))
 	}
 }