Bug 210876 - Web Inspector: Storage: cannot select multiple local storage entries
Summary: Web Inspector: Storage: cannot select multiple local storage entries
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-22 14:51 PDT by Jon Lee
Modified: 2020-04-23 18:06 PDT (History)
5 users (show)

See Also:


Attachments
Patch (42.54 KB, patch)
2020-04-22 18:22 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (50.57 KB, patch)
2020-04-23 17:32 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Lee 2020-04-22 14:51:30 PDT
Split off from bug 209867.

I'd like to be able to delete multiple key/values pairs in local storage in WI.

This bug tracks the following requests:
- Allow selection of multiple rows
- Command-A to select all rows

Devin says:
> These both should be solved if we switch from `WI.DataGrid` to `WI.Table`.  We'd need to either add logic to `WI.Table` to support editing by default, or add custom logic to `WI.DOMStorageContentView` for editing.
Comment 1 Devin Rousso 2020-04-22 18:22:26 PDT
Created attachment 397299 [details]
Patch
Comment 2 Devin Rousso 2020-04-22 20:03:14 PDT Comment hidden (obsolete)
Comment 3 BJ Burg 2020-04-23 12:22:00 PDT
Comment on attachment 397299 [details]
Patch

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

r=me, nice work!

> Source/WebInspectorUI/ChangeLog:59
> +        Introduce a `WI.SelectionController.Reason` which is used to tell the `_delegate` about why

I think this should be called Operation not reason. It's possible to select new elements in 3 operations: direct selection, all, and range extension.

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:53
> +
> +    static createTreeComparator(itemForRepresentedObject)

This comparator is begging to be tested somehow. At the very least, I'd like a comment to explain the general heuristic, which seems to be to "find the first common ancestor and compare the ordinal of a and b under the common parent".

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:63
> +            let getLevel = (item) => {

Nit: depth?

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:192
> +                const reason = WI.SelectionController.Reason.Extend;

So.. Reason.Extend also covers shrinking the range?

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:224
> +        const reason = WI.SelectionController.Reason.All;

Why no reset()?

> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:71
> +        let selectionComparator = WI.SelectionController.createTreeComparator(itemForRepresentedObject);

This is elegant. Good job.

> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:1419
> +        let collapseKeyIdentifier = isRTL ? "Right" : "Left";

I'm unsure if this is correct. Does Mac system UI reverse the keys for collapsing when in RTL?

> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:1438
> +                        handled = nextSelectedNode ? true : false;

Nit: !!nextSelectedNode (I realise it's moved from earlier)

> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:2051
> +        return dataGridNode;

lol @ this code
Comment 4 Devin Rousso 2020-04-23 12:42:23 PDT
Comment on attachment 397299 [details]
Patch

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

>> Source/WebInspectorUI/ChangeLog:59
>> +        Introduce a `WI.SelectionController.Reason` which is used to tell the `_delegate` about why
> 
> I think this should be called Operation not reason. It's possible to select new elements in 3 operations: direct selection, all, and range extension.

Yeah that's a better name :)

>> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:53
>> +    static createTreeComparator(itemForRepresentedObject)
> 
> This comparator is begging to be tested somehow. At the very least, I'd like a comment to explain the general heuristic, which seems to be to "find the first common ancestor and compare the ordinal of a and b under the common parent".

This code has just been moved from `WI.TreeOutline` (and from `WI.Table` below) so the logic hasn't changed at all.

I can add some test cases to LayoutTests/inspector/tree-outline/tree-outline-selection.html for the parent/child cases.

>> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:192
>> +                const reason = WI.SelectionController.Reason.Extend;
> 
> So.. Reason.Extend also covers shrinking the range?

Yeah.  I consider that to be a kind of "negative" extension.

>> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:224
>> +        const reason = WI.SelectionController.Reason.All;
> 
> Why no reset()?

`reset` will clear the list of selected items, meaning that we won't deselect any previously selected items.  This wasn't a problem before as `selectAll` would always select everything, whereas now we ignore `WI.PlaceholderDataGridNode` when `selectAll`-ing.

>> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:1419
>> +        let collapseKeyIdentifier = isRTL ? "Right" : "Left";
> 
> I'm unsure if this is correct. Does Mac system UI reverse the keys for collapsing when in RTL?

Yes, I believe so.  Keep in mind that the UI is also reversed, so the disclosure arrow will point in the other direction :)

>> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:1438
>> +                        handled = nextSelectedNode ? true : false;
> 
> Nit: !!nextSelectedNode (I realise it's moved from earlier)

lol sure :P

>> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:2051
>> +        return dataGridNode;
> 
> lol @ this code

I did this just in case we make a change in the future.  Plus I think it makes the code easier to read/follow, instead of having to implicitly know that the `WI.DataGridNode` is its own `representedObject`.
Comment 5 Devin Rousso 2020-04-23 17:32:22 PDT
Created attachment 397403 [details]
Patch
Comment 6 EWS 2020-04-23 18:05:38 PDT
Committed r260613: <https://trac.webkit.org/changeset/260613>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397403 [details].
Comment 7 Radar WebKit Bug Importer 2020-04-23 18:06:18 PDT
<rdar://problem/62273396>