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

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has denied Matt Baker <mattbaker at apple.com>'s
request for review:
Bug 189718: Web Inspector: Table should support shift-extending the row
selection
https://bugs.webkit.org/show_bug.cgi?id=189718

Attachment 353938: Patch

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




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

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

r- due to the fact that shift-selection doesn't work when there is no previous
selection

Doing this in Finder selects from the first item to wherever you clicked, which
is (I guess) what you're trying to match.

> Source/WebInspectorUI/UserInterface/Base/IndexSet.js:96
> +	   this._validateIndex(startIndex);

Should this return if this validation fails?

> Source/WebInspectorUI/UserInterface/Base/IndexSet.js:99
> +	   if (!count)

NIT: in order to prevent an infinite loop, you should make this `count <= 0`,
not just that it's falsy

> Source/WebInspectorUI/UserInterface/Base/IndexSet.js:123
> +	   this._validateIndex(startIndex);

Ditto (96)

> Source/WebInspectorUI/UserInterface/Base/IndexSet.js:126
> +	   if (!count)

Ditto (99)

> Source/WebInspectorUI/UserInterface/Views/Table.js:90
> +	   this._anchorRowIndex = NaN;

So, assuming I am understanding this correctly, this variable holds the last
specifically selected index before the most recent shift-selection?  In that
case, this needs a much more specific name, as I spent a good amount of time
trying to figure out how it played into everything (e.g. I was wondering why
it's set to `NaN` inside `selectRow` when `_selectedRowIndex` isn't).

> Source/WebInspectorUI/UserInterface/Views/Table.js:233
> +	   if (this._selectedRows.size) {

Not that I have anything against this change, but is there a reason for it
(other than avoiding work when unnecessary)?

> Source/WebInspectorUI/UserInterface/Views/Table.js:1288
> +	   if (!this.numberOfRows)

This should be the first early return, not the last.

> Source/WebInspectorUI/UserInterface/Views/Table.js:1307
> +    _selectRowsFromArrowKey(goingUp, shiftKey)

NIT: `goingUp` is only used once, so I'd rather you call it `direction` and
check if it's equal to "up" or "down", rather than pass a boolean
NIT: considering that this only deals with up/down, this might be better named
as `_selectRowsFromVerticalArrowKey` (if we ever add nesting to `WI.Table` in
the future, left/right might also need to be supported)

> Source/WebInspectorUI/UserInterface/Views/Table.js:1324
> +	   let priorRowIndex = this._selectedRowIndex - rowIncrement;

This seems wrong.  If I've selected the first row and try to move down, there
is no `priorRowIndex` as it'd be `-1`.	I think we just want to check whether
the `_selectedRowIndex` is selected or not, and base our determination off of
that.

> Source/WebInspectorUI/UserInterface/Views/Table.js:1377
> +	   function normalizeRangeExcludingAnchor(anchorIndex, rowIndex) {

NIT: `anchorIndex` is always passed in as `_anchorRowIndex`, so why not just
make this an arrow function and use that instead?

> Source/WebInspectorUI/UserInterface/Views/Table.js:1380
> +	       let startIndex;
> +	       let endIndex;

I tend to prefer to always giving variables a starting value (in this case,
probably NaN)

> Source/WebInspectorUI/UserInterface/Views/Table.js:1400
> +	   if (isNaN(this._anchorRowIndex))
> +	       this._anchorRowIndex = this._selectedRowIndex;

This doesn't handle the case that we shift-select before anything has been
selected.  If that's not covered by this patch, a FIXME should be added, as
well as an early return somewhere.

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

Style: `endIndex - startIndex + 1`

> Source/WebInspectorUI/UserInterface/Views/Table.js:1417
> +	   this._notifySelectionDidChange();

Should this also be called when the shift-selection goes back go
`_anchorRowIndex`?

1. Select row 2
2. Shift-Select row 4 (2-4 selected)
3. Shift-Select row 2 (2 selected)
 => I'd expect a "selection changed" event to be fired

> LayoutTests/inspector/unit-tests/index-set-expected.txt:79
> +Given an IndexSet with values [10,11,12]:

Style: these could use newlines after each sub-test for readability

> LayoutTests/inspector/unit-tests/index-set.html:204
> +	   }

This test is missing a `return true`

> LayoutTests/inspector/unit-tests/index-set.html:207
> +    function rangeToArray(startIndex, count) {

Style: we usually put utility functions before the suite (top of `test`
function)

> LayoutTests/inspector/unit-tests/index-set.html:258
> +		   description: "Add range overlapping the middle.",

Can you add a version of this test for `remove` too?

> LayoutTests/inspector/unit-tests/index-set.html:343
> +	   }

Ditto (204)