Web Inspector: Timelines Tab: Screenshots: images flash while recordi…

…ng is still active

https://bugs.webkit.org/show_bug.cgi?id=240852

Reviewed by Patrick Angle.

* Source/WebInspectorUI/UserInterface/Views/ScreenshotsTimelineOverviewGraph.js:
(WI.ScreenshotsTimelineOverviewGraph):
(WI.ScreenshotsTimelineOverviewGraph.prototype.reset):
(WI.ScreenshotsTimelineOverviewGraph.prototype.layout):
* Source/WebInspectorUI/UserInterface/Views/ScreenshotsTimelineView.js:
(WI.ScreenshotsTimelineView):
(WI.ScreenshotsTimelineView.prototype.reset):
(WI.ScreenshotsTimelineView.prototype.layout):
(WI.ScreenshotsTimelineView.prototype.selectRecord):
(WI.ScreenshotsTimelineView.prototype.clear): Deleted.
Keep a `WeakMap` of `<img>` for each `WI.ScreenshotsTimelineRecord` so that we don't recreate it
over and over in `layout`. Also, don't show the `<img>` until after it `"load"`.

* Source/WebInspectorUI/UserInterface/Views/ScreenshotsTimelineView.css:
(.timeline-view.screenshots):
(.timeline-view.screenshots > img.selected):
Remove scroll snapping as it prevents the selected image from being scrolled offscreen, which is
unnecessarily hostile. The changing border color is enough to identify it as the selected image.

* Source/WebInspectorUI/UserInterface/Base/Utilities.js:
(Map.prototype.getOrInitialize):
(WeakMap.prototype.getOrInitialize): Added.
Adjust utility function to allow for `value` to be a function that's only executed if the `key` does
not exist in the `Map`/`WeakMap`.

* LayoutTests/inspector/unit-tests/map-utilities.html:
* LayoutTests/inspector/unit-tests/map-utilities-expected.txt:
* LayoutTests/inspector/unit-tests/weakmap-utilities.html: Added.
* LayoutTests/inspector/unit-tests/weakmap-utilities-expected.txt: Added.

Canonical link: https://commits.webkit.org/250939@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@294775 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/inspector/unit-tests/map-utilities-expected.txt

@@ -14,7 +14,14 @@ PASS: Map does not have `key`.
 PASS: Map has `key` => `undefined`.
 PASS: Map take `doesNotExistKey` => `undefined`.
 
--- Running test case: Map.prototype.getOrInitialize
+-- Running test case: Map.prototype.getOrInitialize.Raw
+PASS: Map does not have `key`.
+PASS: Map should have initialized `key` with `value`.
+PASS: Map does have `key` => `value`.
+PASS: Map should get `key` with `value` without re-initializing.
+PASS: Map still has `key` => `value`.
+
+-- Running test case: Map.prototype.getOrInitialize.Function
 PASS: Map does not have `key`.
 PASS: Map should have initialized `key` with `value`.
 PASS: Map does have `key` => `value`.

LayoutTests/inspector/unit-tests/map-utilities.html

@@ -47,7 +47,7 @@
     });
 
     suite.addTestCase({
-        name: "Map.prototype.getOrInitialize",
+        name: "Map.prototype.getOrInitialize.Raw",
         test() {
             const key = "key";
             const value = "value";
@@ -62,6 +62,22 @@
         }
     });
 
+    suite.addTestCase({
+        name: "Map.prototype.getOrInitialize.Function",
+        test() {
+            const key = "key";
+            const value = () => "value";
+            const value2 = () => InspectorTest.fail("Should not be reached.");
+
+            let map = new Map;
+            InspectorTest.expectEqual(map.get(key), undefined, "Map does not have `key`.");
+            InspectorTest.expectEqual(map.getOrInitialize(key, value), value(), "Map should have initialized `key` with `value`.");
+            InspectorTest.expectEqual(map.get(key), value(), "Map does have `key` => `value`.");
+            InspectorTest.expectEqual(map.getOrInitialize(key, value2), value(), "Map should get `key` with `value` without re-initializing.");
+            InspectorTest.expectEqual(map.get(key), value(), "Map still has `key` => `value`.");
+        }
+    });
+
     suite.runTestCasesAndFinish();
 }
 </script>

LayoutTests/inspector/unit-tests/weakmap-utilities-expected.txt

@@ -0,0 +1,16 @@
+
+== Running test suite: WeakMap
+-- Running test case: WeakMap.prototype.getOrInitialize.Raw
+PASS: Map does not have `key`.
+PASS: Map should have initialized `key` with `value`.
+PASS: Map does have `key` => `value`.
+PASS: Map should get `key` with `value` without re-initializing.
+PASS: Map still has `key` => `value`.
+
+-- Running test case: WeakMap.prototype.getOrInitialize.Function
+PASS: Map does not have `key`.
+PASS: Map should have initialized `key` with `value`.
+PASS: Map does have `key` => `value`.
+PASS: Map should get `key` with `value` without re-initializing.
+PASS: Map still has `key` => `value`.
+

LayoutTests/inspector/unit-tests/weakmap-utilities.html

@@ -0,0 +1,48 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../../http/tests/inspector/resources/inspector-test.js"></script>
+<script>
+function test()
+{
+    let suite = InspectorTest.createSyncSuite("WeakMap");
+
+    suite.addTestCase({
+        name: "WeakMap.prototype.getOrInitialize.Raw",
+        test() {
+            const key = {value: "key"};
+            const value = "value";
+            const value2 = "value2";
+
+            let map = new WeakMap;
+            InspectorTest.expectEqual(map.get(key), undefined, "Map does not have `key`.");
+            InspectorTest.expectEqual(map.getOrInitialize(key, value), value, "Map should have initialized `key` with `value`.");
+            InspectorTest.expectEqual(map.get(key), value, "Map does have `key` => `value`.");
+            InspectorTest.expectEqual(map.getOrInitialize(key, value2), value, "Map should get `key` with `value` without re-initializing.");
+            InspectorTest.expectEqual(map.get(key), value, "Map still has `key` => `value`.");
+        }
+    });
+
+    suite.addTestCase({
+        name: "WeakMap.prototype.getOrInitialize.Function",
+        test() {
+            const key = {value: "key"};
+            const value = () => "value";
+            const value2 = () => InspectorTest.fail("Should not be reached.");
+
+            let map = new WeakMap;
+            InspectorTest.expectEqual(map.get(key), undefined, "Map does not have `key`.");
+            InspectorTest.expectEqual(map.getOrInitialize(key, value), value(), "Map should have initialized `key` with `value`.");
+            InspectorTest.expectEqual(map.get(key), value(), "Map does have `key` => `value`.");
+            InspectorTest.expectEqual(map.getOrInitialize(key, value2), value(), "Map should get `key` with `value` without re-initializing.");
+            InspectorTest.expectEqual(map.get(key), value(), "Map still has `key` => `value`.");
+        }
+    });
+
+    suite.runTestCasesAndFinish();
+}
+</script>
+</head>
+<body onLoad="runTest()">
+</body>
+</html>

Source/WebInspectorUI/UserInterface/Base/Utilities.js

@@ -148,6 +148,31 @@ Object.defineProperty(Map.prototype, "getOrInitialize",
         if (value)
             return value;
 
+        if (typeof initialValue === "function")
+            initialValue = initialValue();
+
+        console.assert(initialValue !== undefined, "getOrInitialize should not be used with undefined.");
+
+        this.set(key, initialValue);
+        return initialValue;
+    }
+});
+
+Object.defineProperty(WeakMap.prototype, "getOrInitialize",
+{
+    value(key, initialValue)
+    {
+        console.assert(initialValue !== undefined, "getOrInitialize should not be used with undefined.");
+
+        let value = this.get(key);
+        if (value)
+            return value;
+
+        if (typeof initialValue === "function")
+            initialValue = initialValue();
+
+        console.assert(initialValue !== undefined, "getOrInitialize should not be used with undefined.");
+
         this.set(key, initialValue);
         return initialValue;
     }

Source/WebInspectorUI/UserInterface/Views/ScreenshotsTimelineOverviewGraph.js

@@ -37,6 +37,8 @@ WI.ScreenshotsTimelineOverviewGraph = class ScreenshotsTimelineOverviewGraph ext
         this._screenshotsTimeline = timeline;
 
         this._lastSelectedRecordInLayout = null;
+
+        this.reset();
     }
 
     // Protected
@@ -46,6 +48,13 @@ WI.ScreenshotsTimelineOverviewGraph = class ScreenshotsTimelineOverviewGraph ext
         return 60;
     }
 
+    reset()
+    {
+        super.reset();
+
+        this._imageElementForRecord = new WeakMap;
+    }
+
     layout()
     {
         super.layout();
@@ -57,27 +66,37 @@ WI.ScreenshotsTimelineOverviewGraph = class ScreenshotsTimelineOverviewGraph ext
 
         let secondsPerPixel = this.timelineOverview.secondsPerPixel;
 
-        this._lastSelectedRecordInLayout = this.selectedRecord;
-
         for (let record of this._visibleRecords()) {
-            let imageElement = this.element.appendChild(document.createElement("img"));
-            imageElement.src = record.imageData;
-            if (record === this.selectedRecord)
-                imageElement.classList.add("selected");
-
-            imageElement.height = this.height;
-            let widthScale = this.height / record.height;
-            let pixelWidth = record.width * widthScale;
-            imageElement.width = pixelWidth;
-
-            imageElement.style.left = (record.startTime - this.startTime) / secondsPerPixel + "px";
-            imageElement.addEventListener("click", (event) => {
-                // Ensure that the container "click" listener added by `WI.TimelineOverview` isn't called.
-                event.__timelineRecordClickEventHandled = true;
-
-                this.selectedRecord = record;
-            });
+            this.element.appendChild(this._imageElementForRecord.getOrInitialize(record, () => {
+                let imageElement = document.createElement("img");
+                imageElement.width = record.width * (this.height / record.height);
+                imageElement.height = this.height;
+                imageElement.style.left = (record.startTime - this.startTime) / secondsPerPixel + "px";
+
+                imageElement.hidden = true;
+                imageElement.addEventListener("load", (event) => {
+                    imageElement.hidden = false;
+                }, {once: true});
+                imageElement.src = record.imageData;
+
+                imageElement.addEventListener("click", (event) => {
+                    // Ensure that the container "click" listener added by `WI.TimelineOverview` isn't called.
+                    event.__timelineRecordClickEventHandled = true;
+
+                    this.selectedRecord = record;
+                });
+
+                return imageElement;
+            }));
         }
+
+        if (this._lastSelectedRecordInLayout)
+            this._imageElementForRecord.get(this._lastSelectedRecordInLayout)?.classList.remove("selected");
+
+        this._lastSelectedRecordInLayout = this.selectedRecord;
+
+        if (this._lastSelectedRecordInLayout)
+            this._imageElementForRecord.get(this._lastSelectedRecordInLayout)?.classList.add("selected");
     }
 
     updateSelectedRecord()

Source/WebInspectorUI/UserInterface/Views/ScreenshotsTimelineView.css

@@ -26,7 +26,6 @@
 .timeline-view.screenshots {
     white-space: nowrap; 
     overflow-x: auto;
-    scroll-snap-type: x mandatory;
     background-color: hsl(0, 0%, 90%);
 }
 
@@ -46,7 +45,6 @@
 
 .timeline-view.screenshots > img.selected {
     box-shadow: 0 0 0 2px var(--glyph-color-active), var(--shadow);
-    scroll-snap-align: center;
 }
 
 @media (prefers-color-scheme: dark) {

Source/WebInspectorUI/UserInterface/Views/ScreenshotsTimelineView.js

@@ -36,6 +36,8 @@ WI.ScreenshotsTimelineView = class ScreenshotsTimelineView extends WI.TimelineVi
         this.element.classList.add("screenshots");
 
         this._selectedRecord = null;
+
+        this._imageElementForRecord = new WeakMap;
     }
 
     // Public
@@ -44,12 +46,9 @@ WI.ScreenshotsTimelineView = class ScreenshotsTimelineView extends WI.TimelineVi
     {
         super.reset();
 
-        this.clear();
-    }
-
-    clear()
-    {
         this.selectRecord(null);
+
+        this._imageElementForRecord = new WeakMap;
     }
 
     // Protected
@@ -68,26 +67,57 @@ WI.ScreenshotsTimelineView = class ScreenshotsTimelineView extends WI.TimelineVi
 
         this.element.removeChildren();
 
+        let selectedElement = null;
+
         for (let record of this._visibleRecords()) {
-            let imageElement = this.element.appendChild(document.createElement("img"));
-            imageElement.src = record.imageData;
+            this.element.appendChild(this._imageElementForRecord.getOrInitialize(record, () => {
+                let imageElement = document.createElement("img");
+
+                imageElement.hidden = true;
+                imageElement.addEventListener("load", (event) => {
+                    imageElement.hidden = false;
+                }, {once: true});
+                imageElement.src = record.imageData;
 
-            if (record === this._selectedRecord)
-                imageElement.classList.add("selected");
+                imageElement.addEventListener("click", (event) => {
+                    this._selectTimelineRecord(record);
+                });
 
-            imageElement.addEventListener("click", (event) => {
-                this._selectTimelineRecord(record);
-            });
+                if (record === this._selectedRecord)
+                    selectedElement = imageElement;
+
+                return imageElement;
+            }));
         }        
 
+        if (selectedElement) {
+            selectedElement.classList.add("selected");
+
+            selectedElement.scrollIntoView({inline: "center"});
+        }
+
         if (!this.element.childNodes.length)
             this.element.appendChild(WI.createMessageTextView(WI.UIString("No screenshots", "No screenshots @ Screenshots Timeline", "Placeholder text shown when there are no images to display in the Screenshots timeline.")));
     }
 
     selectRecord(record)
     {
+        if (record === this._selectedRecord)
+            return;
+
+        if (this._selectedRecord)
+            this._imageElementForRecord.get(this._selectedRecord)?.classList.remove("selected");
+
         this._selectedRecord = record;
-        this.needsLayout();
+
+        if (this._selectedRecord) {
+            let element = this._imageElementForRecord.get(this._selectedRecord);
+            if (element) {
+                element.classList.add("selected");
+
+                element.scrollIntoView({inline: "center"});
+            }
+        }
     }
 
     // Private