[webkit-reviews] review granted: [Bug 190299] Web Inspector: Table should support select all (Cmd-A) : [Attachment 354671] 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 190299: Web Inspector: Table should support select all (Cmd-A)
https://bugs.webkit.org/show_bug.cgi?id=190299

Attachment 354671: Patch

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




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

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

r=me, one thing I'm "concerned" about is that ⌘-a will only work if the
focused/receiving element is within the `WI.Table` (e.g. if the user tries to
⌘-a on the cookies view immediately after opening the tab, would it still
select all?).  It's fair to say that the owner of the `WI.Table` should be
responsible for that, but we might want to think about how to make this better
(assuming it's actually an issue).

> Source/WebInspectorUI/UserInterface/Views/Table.js:1301
> +	   if (event.key === "a" && event.metaKey) {

Please add a FIXME for `event.commandOrControlKey`, as this will not work right
on non-mac platforms.

> LayoutTests/inspector/table/table-selection.html:156
> +	       InspectorTest.expectEqual(table.selectedRows.length, 0, "Should
have no selected rows.");

I think this test would be more demonstrative if you had an initially selected
row (e.g. 1), called `selectAll`, and then checked that the only selected row
was the one initially selected (e.g. 1)

> LayoutTests/inspector/table/table-selection.html:170
> +	       InspectorTest.expectEqual(table.selectedRows.length,
table.numberOfRows, "Should have selected all rows.");

Ditto (156), but you'd instead have the check that you have now