[webkit-reviews] review granted: [Bug 189718] Web Inspector: Table should support shift-extending the row selection : [Attachment 354623] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted  review:
Bug 189718: Web Inspector: Table should support shift-extending the row
selection
https://bugs.webkit.org/show_bug.cgi?id=189718

Attachment 354623: Patch

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




--- Comment #31 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 354623
  --> https://bugs.webkit.org/attachment.cgi?id=354623
Patch

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

r=me, please wait for EWS

> Source/WebInspectorUI/UserInterface/Base/IndexSet.js:112
> +	   if (!this.size || (this.firstIndex >= range[0] && this.lastIndex <=
range.lastValue)) {

It's times like these that I wish we had a `firstIndex` too, but I also realize
how completely unnecessary that would be :|

> Source/WebInspectorUI/UserInterface/Base/IndexSet.js:140
> +	   if (startIndex <= this.firstIndex && lastIndex >= this.lastIndex) {

Style: this is the reverse order of `addRange` (e.g. whether you use
`this._<blah>` on the left or the right)

> Source/WebInspectorUI/UserInterface/Base/IndexSet.js:174
> +    difference(indexSet)

NIT: I'd personally call this `subtract`

> Source/WebInspectorUI/UserInterface/Base/IndexSet.js:179
> +	       return [];

NIT: should this be `return new IndexSet` to match the return type of the other
`return`?

> Source/WebInspectorUI/UserInterface/Views/Table.js:232
>  

Style: remove newline

> Source/WebInspectorUI/UserInterface/Views/Table.js:1288
> +	   if (event.metaKey || event.ctrlKey)

`event.altKey`?

> Source/WebInspectorUI/UserInterface/Views/Table.js:1370
> +	   if (event.metaKey) {

This should be `event.ctrlKey` on windows.  I think @Nikita added an
`Event.prototype.commandOrControl`?

> Source/WebInspectorUI/UserInterface/Views/Table.js:1388
> +	   if (isNaN(this._selectedRowIndex)) {

Alternatively, you could check/assert that `newSelectedRows` is empty

> Source/WebInspectorUI/UserInterface/Views/Table.js:1415
> +	   newSelectedRows.addRange(startIndex, endIndex - startIndex + 1);

It's cases like this that make me consider whether `addRange` should take a
`endIndex` instead of `count` 🤔

> Source/WebInspectorUI/UserInterface/Views/Table.js:1592
> +		   row.classList.toggle("selected", flag);

NIT: due to the nature of `undefined`, I always add `!!` before the flag
argument for `classList.toggle` (passing `undefined` makes `classList.toggle`
act like a flip-flop)

> LayoutTests/inspector/unit-tests/index-set.html:361
> +	   name: "IndexSet.prototype.equals",

You're missing the expected results for this

> LayoutTests/inspector/unit-tests/index-set.html:376
> +	   name: "IndexSet.prototype.difference",

Ditto (361)