Attachment 356687 Details for Bug 192388 – Patch

[patch] Patch

bug-192388-20181205175323.patch (text/plain), 7.87 KB, created by Matt Baker on 2018-12-05 17:53:31 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Matt Baker

Created: 2018-12-05 17:53:31 PST

Size: 7.87 KB

patch

obsolete

>Subversion Revision: 238917
>diff --git a/Source/WebInspectorUI/ChangeLog b/Source/WebInspectorUI/ChangeLog
>index 41d966966dc87acf4eb0e969356ef3876f8144fa..8b4eb3465341861b414dcf488f4ab044e857dd74 100644
>--- a/Source/WebInspectorUI/ChangeLog
>+++ b/Source/WebInspectorUI/ChangeLog
>@@ -1,3 +1,26 @@
>+2018-12-05  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.prototype.didRemoveItems):
>+        (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):
>+
>+        * UserInterface/Views/TreeOutline.js:
>+        (WI.TreeOutline.prototype.removeChildAtIndex):
>+        Drive-by fix: incorrect index was passed to SelectionController.
>+
> 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..77634579a2a4e56d8fb781c91ed2afb2784504f2 100644
>--- a/Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js
>+++ b/Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js
>@@ -213,14 +213,25 @@ 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;
>+
>+        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);
>+            }
>         }
>     }
> 
>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..46c6d51c740d53e1f3c25be235e7d22868ddfce7 100644
>--- a/Source/WebInspectorUI/UserInterface/Views/TreeOutline.js
>+++ b/Source/WebInspectorUI/UserInterface/Views/TreeOutline.js
>@@ -318,6 +318,7 @@ WI.TreeOutline = class TreeOutline extends WI.Object
>             return;
> 
>         let child = this.children[childIndex];
>+        let removedIndex = this.treeOutline._indexOfTreeElement(child);
>         this.children.splice(childIndex, 1);
> 
>         let parent = child.parent;
>@@ -339,7 +340,7 @@ WI.TreeOutline = class TreeOutline extends WI.Object
>         if (treeOutline) {
>             treeOutline._forgetTreeElement(child);
>             treeOutline._forgetChildrenRecursive(child);
>-            treeOutline._selectionController.didRemoveItem(childIndex);
>+            treeOutline._selectionController.didRemoveItems(new WI.IndexSet([removedIndex]));
>         }
> 
>         child._detach();
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index af0145dac57b73b09bc55ed95d346af870a13827..2e407932afe01c82622697cf54a0c4c4c5908239 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,14 @@
>+2018-12-05  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-05  Youenn Fablet  <youenn@apple.com>
> 
>         REGRESSION: Layout Test http/tests/security/cross-origin-css-resource-timing.html is flaky
>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