Bug 189803 – Web Inspector: Table should support deleting rows

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

Summary:
Table should support deleting rows. This can go through the table delegate with some new delegate methods:

TableDelegate
{
    tableShouldDeleteRow(table, rowIndex) => bool
    tableDidDeleteRow(table, rowIndex)
}

Implementing tableShouldDeleteRow will enable deleting table rows with the delete key, but since the table context menu is handled by the delegate it may take a separate path.

This is needed to use Table in CookieStorageContentView.

[Radar WebKit Bug Importer]

<rdar://problem/44655709>

[Matt Baker]

Created attachment 352992 [details]
Patch

[Matt Baker]

I missed a row removal use case. When right-clicking in a macOS tree or table that supports item removal (Finder, Mail, Xcode) brings up a context menu with a "Delete" menu item. When a selection exists in the tree/table, the target of the delete will be:

a) All the selected items, if the target item is selected
b) The target item only, if the target item is not selected

Supporting scenario b) requires an additional method, `Table.prototype.removeRow(rowIndex)`.

[Matt Baker]

Created attachment 353069 [details]
Patch

[Matt Baker]

(In reply to Matt Baker from comment #4)
> Created attachment 353069 [details]
> Patch

This can be tested by applying https://bugs.webkit.org/show_bug.cgi?id=66381.

[Devin Rousso]

Comment on attachment 353069 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353069&action=review

r=me, with some NITs

I'd love to see a test (if you can) of what would happen when selection/deletion occurs outside of what's been cached (see Table.js:1432).

> Source/WebInspectorUI/UserInterface/Base/IndexSet.js:99
> +    clone()

NIT: I think we've preferred `copy` in the past

> Source/WebInspectorUI/UserInterface/Views/Table.js:388
> +        if (rowIndex >= this.numberOfRows)

While we're at it, should we also check that it's non-negative?  Maybe an assertion too?

> Source/WebInspectorUI/UserInterface/Views/Table.js:406
> +

Style: extra newline

> Source/WebInspectorUI/UserInterface/Views/Table.js:1432
> +            let newSelectedRow = this._cachedRows.get(rowIndex);

So this means that if we don't have a cached row for the index, we don't select it?  Is it possible for us to be in a situation where we don't have a cached row (e.g. it's far off the screen) and try to select it?  It seems like we should do some sort of "generate rows if necessary" here to make sure that we never "lose" a selection.

> Source/WebInspectorUI/UserInterface/Views/Table.js:1442
> +    _removeRows(rowIndexes)

NIT: just `indexes` would be clear enough IMO

> Source/WebInspectorUI/UserInterface/Views/Table.js:1446
> +        let adjustRowAtIndex = (rowIndex) => {

Ditto (1442)

> LayoutTests/inspector/table/resources/table-utilities.js:27
> +            for (let i = rowIndexes.length - 1; i >= 0; --i)

NIT: I'd call this `index` instead of `i` since it's a value, not "just" an index in an array.

[Matt Baker]

Comment on attachment 353069 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353069&action=review

>> Source/WebInspectorUI/UserInterface/Base/IndexSet.js:99
>> +    clone()
> 
> NIT: I think we've preferred `copy` in the past

Will change.

>> Source/WebInspectorUI/UserInterface/Views/Table.js:388
>> +        if (rowIndex >= this.numberOfRows)
> 
> While we're at it, should we also check that it's non-negative?  Maybe an assertion too?

I took another look at public Table methods taking a row index: deselectRow, selectRow, and removeRow. Only removeRow has any kind of validation. cellForRowAndColumn returns early if the row isn't visible, so an invalid index won't matter. I'll remove the check from removeRow, and add an assertion to all three:

console.assert(rowIndex >= 0 && rowIndex < this.numberOfRows);

>> Source/WebInspectorUI/UserInterface/Views/Table.js:1432
>> +            let newSelectedRow = this._cachedRows.get(rowIndex);
> 
> So this means that if we don't have a cached row for the index, we don't select it?  Is it possible for us to be in a situation where we don't have a cached row (e.g. it's far off the screen) and try to select it?  It seems like we should do some sort of "generate rows if necessary" here to make sure that we never "lose" a selection.

It shouldn't be possible for us to to get in this situation currently, since a row needs to be visible (cached) to be interacted with/selected. Adding the index to `this._selectedRows` shouldn't be conditional though; good catch!

[Matt Baker]

(In reply to Devin Rousso from comment #6)
> Comment on attachment 353069 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353069&action=review
> 
> r=me, with some NITs
> 
> I'd love to see a test (if you can) of what would happen when
> selection/deletion occurs outside of what's been cached (see Table.js:1432).

Good idea; I tested deselectRow, selectRow, and removeRow with an uncached row (row 999 of 1000). deselectRow fails but the others work as expected. I added a test to table-remove.rows.html, and will fix deselectRow (and add tests) in a follow-up:

Web Inspector: Table selection should not require that rows be in the cache
https://bugs.webkit.org/show_bug.cgi?id=190993

[Matt Baker]

Created attachment 353248 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 353248 [details]
Patch for landing

Clearing flags on attachment: 353248

Committed r237495: <https://trac.webkit.org/changeset/237495>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.