[webkit-reviews] review granted: [Bug 189803] Web Inspector: Table should support deleting rows : [Attachment 353069] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Matt Baker <mattbaker at apple.com>'s
request for review:
Bug 189803: Web Inspector: Table should support deleting rows
https://bugs.webkit.org/show_bug.cgi?id=189803

Attachment 353069: Patch

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




--- Comment #6 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 353069
  --> https://bugs.webkit.org/attachment.cgi?id=353069
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.