Attachment 356772 Details for Bug 192388 – Patch

[patch] Patch

bug-192388-20181206174056.patch (text/plain), 12.33 KB, created by Matt Baker on 2018-12-06 17:41:07 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Matt Baker

Created: 2018-12-06 17:41:07 PST

Size: 12.33 KB

patch

obsolete

>Subversion Revision: 238936
>diff --git a/Source/WebInspectorUI/ChangeLog b/Source/WebInspectorUI/ChangeLog
>index 41d966966dc87acf4eb0e969356ef3876f8144fa..7a446ea3666587ff9afbf06e29aba1f37d7b04e5 100644
>--- a/Source/WebInspectorUI/ChangeLog
>+++ b/Source/WebInspectorUI/ChangeLog
>@@ -1,3 +1,34 @@
>+2018-12-06  Matt Baker  <mattbaker@apple.com>
>+
>+        Web Inspector: Table selection becomes corrupted when deleting selected cookies
>+        https://bugs.webkit.org/show_bug.cgi?id=192388
>+        <rdar://problem/46472364>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * UserInterface/Controllers/SelectionController.js:
>+        (WI.SelectionController):
>+        (WI.SelectionController.prototype.didRemoveItems):
>+        (WI.SelectionController.prototype._updateSelectedItems):
>+        (WI.SelectionController.prototype.didRemoveItem): Deleted.
>+        Replace `didRemoveItem` with a method taking an IndexSet. Calling the
>+        single-index version while iterating over multiple rows in ascending
>+        order is unsafe, a detail best left to the SelectionController.
>+
>+        * UserInterface/Views/Table.js:
>+        (WI.Table.prototype.removeRow):
>+        (WI.Table.prototype._removeRows):
>+        Notify SelectionController of removed rows.
>+
>+        * UserInterface/Views/TreeOutline.js:
>+        (WI.TreeOutline.prototype.insertChild):
>+        (WI.TreeOutline.prototype.removeChildAtIndex):
>+        Drive-by fix: incorrect index was passed to SelectionController.
>+        (WI.TreeOutline.prototype.removeChildren):
>+        Make sure `children` doesn't contain detached TreeElements during iteration.
>+        (WI.TreeOutline.prototype._rememberTreeElement):
>+        (WI.TreeOutline.prototype._forgetTreeElement):
>+
> 2018-12-05  Matt Baker  <mattbaker@apple.com>
> 
>         Web Inspector: SelectionController should not extend the selection when allowsMultipleSelection is false
>diff --git a/Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js b/Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js
>index 725ec3e6e8cdfef222721c9add3d5ee9b3256de5..e0d5ac9ae401e8e7215555e3763e35e13cc24aac 100644
>--- a/Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js
>+++ b/Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js
>@@ -37,6 +37,7 @@ WI.SelectionController = class SelectionController extends WI.Object
>         this._lastSelectedIndex = NaN;
>         this._shiftAnchorIndex = NaN;
>         this._selectedIndexes = new WI.IndexSet;
>+        this._suppressSelectionDidChange = false;
> 
>         console.assert(this._delegate.selectionControllerNumberOfItems, "SelectionController delegate must implement selectionControllerNumberOfItems.");
>         console.assert(this._delegate.selectionControllerNextSelectableIndex, "SelectionController delegate must implement selectionControllerNextSelectableIndex.");
>@@ -213,15 +214,30 @@ WI.SelectionController = class SelectionController extends WI.Object
>         }
>     }
> 
>-    didRemoveItem(index)
>+    didRemoveItems(indexes)
>     {
>-        if (this.hasSelectedItem(index))
>-            this.deselectItem(index);
>+        console.assert(indexes instanceof WI.IndexSet);
> 
>-        while (index = this._selectedIndexes.indexGreaterThan(index)) {
>-            this._selectedIndexes.delete(index);
>-            this._selectedIndexes.add(index - 1);
>+        if (!this._selectedIndexes.size)
>+            return;
>+
>+        if (this._selectedIndexes.lastIndex < indexes.firstIndex)
>+            return;
>+
>+        this._suppressSelectionDidChange = true;
>+
>+        for (let index = indexes.lastIndex; index >= indexes.firstIndex; index = indexes.indexLessThan(index)) {
>+            if (this.hasSelectedItem(index))
>+                this.deselectItem(index);
>+
>+            let nextIndex = index;
>+            while (nextIndex = this._selectedIndexes.indexGreaterThan(nextIndex)) {
>+                this._selectedIndexes.delete(nextIndex);
>+                this._selectedIndexes.add(nextIndex - 1);
>+            }
>         }
>+
>+        this._suppressSelectionDidChange = false;
>     }
> 
>     handleKeyDown(event)
>@@ -386,7 +402,7 @@ WI.SelectionController = class SelectionController extends WI.Object
>         let oldSelectedIndexes = this._selectedIndexes.copy();
>         this._selectedIndexes = indexes;
> 
>-        if (!this._delegate.selectionControllerSelectionDidChange)
>+        if (this._suppressSelectionDidChange || !this._delegate.selectionControllerSelectionDidChange)
>             return;
> 
>         let deselectedItems = oldSelectedIndexes.difference(indexes);
>diff --git a/Source/WebInspectorUI/UserInterface/Views/Table.js b/Source/WebInspectorUI/UserInterface/Views/Table.js
>index acd0cf4300d385b50c9103f081ac4840aed6c813..2bc6f34b602bd2425d889f790bec563c99e5f63d 100644
>--- a/Source/WebInspectorUI/UserInterface/Views/Table.js
>+++ b/Source/WebInspectorUI/UserInterface/Views/Table.js
>@@ -338,8 +338,8 @@ WI.Table = class Table extends WI.View
>         if (this.isRowSelected(rowIndex))
>             this.deselectRow(rowIndex);
> 
>-        this._removeRows(new WI.IndexSet([rowIndex]));
>-        this._selectionController.didRemoveItem(rowIndex);
>+        let rowIndexes = new WI.IndexSet([rowIndex]);
>+        this._removeRows(rowIndexes);
>     }
> 
>     removeSelectedRows()
>@@ -1393,6 +1393,8 @@ WI.Table = class Table extends WI.View
>         this._cachedNumberOfRows -= removed;
>         console.assert(this._cachedNumberOfRows >= 0);
> 
>+        this._selectionController.didRemoveItems(rowIndexes);
>+
>         if (this._delegate.tableDidRemoveRows) {
>             this._delegate.tableDidRemoveRows(this, Array.from(rowIndexes));
>             console.assert(this._cachedNumberOfRows === this._dataSource.tableNumberOfRows(this), "Table data source should update after removing rows.");
>diff --git a/Source/WebInspectorUI/UserInterface/Views/TreeOutline.js b/Source/WebInspectorUI/UserInterface/Views/TreeOutline.js
>index 72f2cfdaf17e6266b475ae9d417054d30edb33dc..ed7b5538b461bb21a572af922c6d72c971ce8249 100644
>--- a/Source/WebInspectorUI/UserInterface/Views/TreeOutline.js
>+++ b/Source/WebInspectorUI/UserInterface/Views/TreeOutline.js
>@@ -295,12 +295,6 @@ WI.TreeOutline = class TreeOutline extends WI.Object
>         if (child.hasChildren && child.treeOutline._treeElementsExpandedState[child.identifier] !== undefined)
>             child.expanded = child.treeOutline._treeElementsExpandedState[child.identifier];
> 
>-        // Update the SelectionController before attaching the TreeElement.
>-        // Attaching the TreeElement can cause it to become selected, and
>-        // added to the SelectionController.
>-        let insertionIndex = this.treeOutline._indexOfTreeElement(child) || 0;
>-        this.treeOutline._selectionController.didInsertItem(insertionIndex);
>-
>         if (this._childrenListNode)
>             child._attach();
> 
>@@ -330,18 +324,17 @@ WI.TreeOutline = class TreeOutline extends WI.Object
>                 parent.select(true, false);
>         }
> 
>-        if (child.previousSibling)
>-            child.previousSibling.nextSibling = child.nextSibling;
>-        if (child.nextSibling)
>-            child.nextSibling.previousSibling = child.previousSibling;
>-
>         let treeOutline = child.treeOutline;
>         if (treeOutline) {
>             treeOutline._forgetTreeElement(child);
>             treeOutline._forgetChildrenRecursive(child);
>-            treeOutline._selectionController.didRemoveItem(childIndex);
>         }
> 
>+        if (child.previousSibling)
>+            child.previousSibling.nextSibling = child.nextSibling;
>+        if (child.nextSibling)
>+            child.nextSibling.previousSibling = child.previousSibling;
>+
>         child._detach();
>         child.treeOutline = null;
>         child.parent = null;
>@@ -374,7 +367,8 @@ WI.TreeOutline = class TreeOutline extends WI.Object
> 
>     removeChildren(suppressOnDeselect)
>     {
>-        for (let child of this.children) {
>+        while (this.children.length) {
>+            let child = this.children[0];
>             child.deselect(suppressOnDeselect);
> 
>             let treeOutline = child.treeOutline;
>@@ -391,9 +385,9 @@ WI.TreeOutline = class TreeOutline extends WI.Object
> 
>             if (treeOutline)
>                 treeOutline.dispatchEventToListeners(WI.TreeOutline.Event.ElementRemoved, {element: child});
>-        }
> 
>-        this.children = [];
>+            this.children.shift();
>+        }
>     }
> 
>     _rememberTreeElement(element)
>@@ -409,6 +403,12 @@ WI.TreeOutline = class TreeOutline extends WI.Object
>         // add the element
>         elements.push(element);
>         this._cachedNumberOfDescendents++;
>+
>+        // Update the SelectionController before attaching the TreeElement.
>+        // Attaching the TreeElement can cause it to become selected, and
>+        // added to the SelectionController.
>+        let index = this._indexOfTreeElement(element) || 0;
>+        this._selectionController.didInsertItem(index);
>     }
> 
>     _forgetTreeElement(element)
>@@ -418,6 +418,9 @@ WI.TreeOutline = class TreeOutline extends WI.Object
>             this.selectedTreeElement = null;
>         }
>         if (this._knownTreeElements[element.identifier]) {
>+            let index = this._indexOfTreeElement(element);
>+            this._selectionController.didRemoveItems(new WI.IndexSet([index]));
>+
>             this._knownTreeElements[element.identifier].remove(element, true);
>             this._cachedNumberOfDescendents--;
>         }
>@@ -1024,7 +1027,7 @@ WI.TreeOutline = class TreeOutline extends WI.Object
>             ++index;
>         }
> 
>-        console.assert(false, "Unable to get index for tree element.", treeElement, treeOutline);
>+        console.assert(false, "Unable to get index for tree element.", treeElement);
>         return NaN;
>     }
> 
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 3dfd5d95fdfb9ad2ac5102adc394a60ead42685c..55a4104dff5582299d0d88296819bdae71ac4ae1 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,14 @@
>+2018-12-06  Matt Baker  <mattbaker@apple.com>
>+
>+        Web Inspector: Table selection becomes corrupted when deleting selected cookies
>+        https://bugs.webkit.org/show_bug.cgi?id=192388
>+        <rdar://problem/46472364>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * inspector/table/table-remove-rows-expected.txt:
>+        * inspector/table/table-remove-rows.html:
>+
> 2018-12-06  Jiewen Tan  <jiewen_tan@apple.com>
> 
>         Layout Test http/tests/misc/resource-timing-resolution.html is a flaky failure
>diff --git a/LayoutTests/inspector/table/table-remove-rows-expected.txt b/LayoutTests/inspector/table/table-remove-rows-expected.txt
>index 92e34ab0d32229b18bed6cae8fc1f241b040819e..5fa87ba1283f54f15c245709c0130dbbf371a163 100644
>--- a/LayoutTests/inspector/table/table-remove-rows-expected.txt
>+++ b/LayoutTests/inspector/table/table-remove-rows-expected.txt
>@@ -13,6 +13,11 @@ Given a Table with selected rows [0], remove row 0.
> Selection changed to [] before removing row 0.
> PASS: Should remove row 0.
> 
>+-- Running test case: Table.RemoveRow.PrecedingSelected
>+Given a Table with selected rows [1,3], remove row 0.
>+PASS: Should remove row 0.
>+PASS: Selected row indexes should be adjusted.
>+
> -- Running test case: Table.RemoveSelectedRows.Single.SelectFollowing
> Given a Table with selected rows [0]:
>  * Row 0
>diff --git a/LayoutTests/inspector/table/table-remove-rows.html b/LayoutTests/inspector/table/table-remove-rows.html
>index c4615033f822c03de16e33b7196691f26a460bef..358e0491bd5a480ed5b872a0dd57d8534039d192 100644
>--- a/LayoutTests/inspector/table/table-remove-rows.html
>+++ b/LayoutTests/inspector/table/table-remove-rows.html
>@@ -134,6 +134,24 @@ function test()
>         }
>     });
> 
>+    suite.addTestCase({
>+        name: "Table.RemoveRow.PrecedingSelected",
>+        description: "Remove a row preceding the selection, causing the selection to shift up.",
>+        test() {
>+            let testDelegate = new RemoveRowTestDelegate;
>+            let table = InspectorTest.createTableWithDelegate(testDelegate, numberOfRows);
>+            table.allowsMultipleSelection = true;
>+
>+            table.selectRow(1);
>+            table.selectRow(3, true);
>+            testDelegate.triggerRemoveRow(table, 0);
>+
>+            InspectorTest.expectShallowEqual(table.selectedRows, [0, 2], "Selected row indexes should be adjusted.");
>+
>+            return true;
>+        }
>+    });
>+
>     function addTestCase({name, description, rowIndexes}) {
>         suite.addTestCase({
>             name, description,

Actions: View | Formatted Diff | Diff

Attachments on bug 192388: 356619 | 356636 | 356637 | 356642 | 356687 | 356692 | 356772 | 357161 | 357162 | 357168 | 357169 | 357175 | 357181 | 357187 | 357203