Attachment 357187 Details for Bug 192388 – Patch

[patch] Patch

bug-192388-20181212161458.patch (text/plain), 11.42 KB, created by Matt Baker on 2018-12-12 16:15:19 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Matt Baker

Created: 2018-12-12 16:15:19 PST

Size: 11.42 KB

patch

obsolete

>Subversion Revision: 239131
>diff --git a/Source/WebInspectorUI/ChangeLog b/Source/WebInspectorUI/ChangeLog
>index 183e7ff832d5336d5233b2490a732875cf557e71..ccf53915c347b28fa898459becb0bf5d61c9e990 100644
>--- a/Source/WebInspectorUI/ChangeLog
>+++ b/Source/WebInspectorUI/ChangeLog
>@@ -1,3 +1,38 @@
>+2018-12-12  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):
>+        Remove the child from the element's `children` after calling `_forgetTreeElement`,
>+        which needs to calculate the child's index to pass to the SelectionController.
>+
>+        (WI.TreeOutline.prototype.removeChildren):
>+        Remove child items during iteration so that `children` doesn't contain
>+        detached TreeElements while calling `_forgetTreeElement`.
>+
>+        (WI.TreeOutline.prototype._rememberTreeElement):
>+        (WI.TreeOutline.prototype._forgetTreeElement):
>+
> 2018-12-10  Matt Baker  <mattbaker@apple.com>
> 
>         Web Inspector: REGRESSION (r238599): unable to select specific timeline
>diff --git a/Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js b/Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js
>index 725ec3e6e8cdfef222721c9add3d5ee9b3256de5..d1e9f31a73d6823a5da8cada2e7e640f912af7ce 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,63 @@ 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;
>+
>+        let firstRemovedIndex = indexes.firstIndex;
>+        if (this._selectedIndexes.lastIndex < firstRemovedIndex)
>+            return;
>+
>+        let newSelectedIndexes = new WI.IndexSet;
>+
>+        let lastRemovedIndex = indexes.lastIndex;
>+        if (this._selectedIndexes.firstIndex < lastRemovedIndex) {
>+            let removedCount = 0;
>+            let removedIndex = firstRemovedIndex;
>+
>+            this._suppressSelectionDidChange = true;
>+
>+            // Adjust the selected indexes that are in the range between the
>+            // first and last removed index (inclusive).
>+            for (let current = this._selectedIndexes.firstIndex; current < lastRemovedIndex; current = this._selectedIndexes.indexGreaterThan(current)) {
>+                if (this.hasSelectedItem(current)) {
>+                    this.deselectItem(current);
>+                    removedCount++;
>+                    continue;
>+                }
>+
>+                while (removedIndex < current) {
>+                    removedCount++;
>+                    removedIndex = indexes.indexGreaterThan(removedIndex);
>+                }
>+
>+                let newIndex = current - removedCount;
>+                newSelectedIndexes.add(newIndex);
>+
>+                if (this._lastSelectedIndex === current)
>+                    this._lastSelectedIndex = newIndex;
>+                if (this._shiftAnchorIndex === current)
>+                    this._shiftAnchorIndex = newIndex;
>+            }
>+
>+            this._suppressSelectionDidChange = false;
>         }
>+
>+        let removedCount = indexes.size;
>+        let current = lastRemovedIndex;
>+        while (current = this._selectedIndexes.indexGreaterThan(current))
>+            newSelectedIndexes.add(current - removedCount);
>+
>+        if (this._lastSelectedIndex > lastRemovedIndex)
>+            this._lastSelectedIndex -= removedCount;
>+        if (this._shiftAnchorIndex > lastRemovedIndex)
>+            this._shiftAnchorIndex -= removedCount;
>+
>+        this._selectedIndexes = newSelectedIndexes;
>     }
> 
>     handleKeyDown(event)
>@@ -386,7 +435,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 baea55ddfbb58a0b41051a3265235f4ecd6d2713..247b6577c4736029509e78eaa0efc28f55a36cc5 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();
> 
>@@ -318,9 +312,8 @@ WI.TreeOutline = class TreeOutline extends WI.Object
>             return;
> 
>         let child = this.children[childIndex];
>-        this.children.splice(childIndex, 1);
>-
>         let parent = child.parent;
>+
>         if (child.deselect(suppressOnDeselect)) {
>             if (child.previousSibling && !suppressSelectSibling)
>                 child.previousSibling.select(true, false);
>@@ -330,18 +323,19 @@ 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;
>+
>+        this.children.splice(childIndex, 1);
>+
>         child._detach();
>         child.treeOutline = null;
>         child.parent = null;
>@@ -374,7 +368,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 +386,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 +404,10 @@ WI.TreeOutline = class TreeOutline extends WI.Object
>         // add the element
>         elements.push(element);
>         this._cachedNumberOfDescendents++;
>+
>+        let index = this._indexOfTreeElement(element);
>+        if (index >= 0)
>+            this._selectionController.didInsertItem(index);
>     }
> 
>     _forgetTreeElement(element)
>@@ -418,6 +417,10 @@ WI.TreeOutline = class TreeOutline extends WI.Object
>             this.selectedTreeElement = null;
>         }
>         if (this._knownTreeElements[element.identifier]) {
>+            let index = this._indexOfTreeElement(element);
>+            if (index >= 0)
>+                this._selectionController.didRemoveItems(new WI.IndexSet([index]));
>+
>             this._knownTreeElements[element.identifier].remove(element, true);
>             this._cachedNumberOfDescendents--;
>         }
>@@ -1030,7 +1033,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;
>     }
>

Actions: View | Formatted Diff | Diff

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