Bug 66381 – Web Inspector: support multiple selection/deletion of cookie records

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Trevor Cortez]

* SUMMARY
Web Inspector: Cookies/Storage Record Selection Handling Improvements

* STEPS TO REPRODUCE
1. Visit some site that will store cookies (amazon.com)
2. Open the web inspector Resources tab to view stored cookies
3. Try to delete all the cookies for www.amazon.com 

* RESULT
Only one record can be selected at a time, so all the cookies can't be deleted in bulk.

* EXPECTED RESULT
Either multiple selection should be allowed, OR when deleting a record, the next or previous indexed row should be selected, to make rapid-fire delete clicks quickly delete all the records. 

* NOTES 
I think this has been a problem since the inception of cookie listing in the inspector.  Not sure how it behaves on Windows, only tested on Mac OS 10.6.x/10.7.x

[Radar WebKit Bug Importer]

<rdar://problem/19281525>

[Matt Baker]

Adding multiple selection to the grid comes up from time to time, and this seems like a good time to add it. I'll pattern the UI on multiple selection in OS X (Finder, grid controls, etc):

Shift-Click — select a range of rows
Command-Click — toggle the row's selected state, without affecting other selected rows
Command-A — select all
Shift-Command-A — deselect all

It's tempting to use the delete key to remove rows. Using context menus or the mouse can be cumbersome (and an Accessibility consideration as well).

Note: Other ports may want to change the shortcuts to something more appropriate.

[Matt Baker]

Created attachment 350042 [details]
WIP

[Devin Rousso]

Comment on attachment 350042 [details]
WIP

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

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:-100
> -            columns.size.title = WI.UIString("Size");

Do we not want to show size information anymore?

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:86
> +        this._cookies = this._cookies.sort(comparator);

NIT: no need to reassign `this._cookies` since `.sort()` is in-place.

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:112
> +    tableSelectedRowChanged(table, rowIndex)

Oops?

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:136
> +            cell.textContent = cookie.expires ? cookie.expires.toLocaleString() : WI.UIString("Session");

Is "Session" something we want to style differently, similar to "(memory)" in the Network tab?

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:205
> +        if (!sortColumnIdentifier) {

Is this ever possible?  Shouldn't there always be a sort?

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:218
> +            // String.

This comments isn't really necessary.

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:224
> +            // Boolean.

Ditto (218).

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:229
> +            // Date.

Ditto (218).

[Matt Baker]

Created attachment 351546 [details]
WIP

[Matt Baker]

Comment on attachment 351546 [details]
WIP

Still needs refinements to column widths.

[Matt Baker]

This WIP patch includes new table delegate methods that will be introduced in https://bugs.webkit.org/show_bug.cgi?id=189803. Ignore them for now.

[Devin Rousso]

Comment on attachment 351546 [details]
WIP

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

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:58
> +            this._table.reloadData();

You should put this behind a `didInitialLayout` check, since it might not always play out nicely (`update` is called in the constructor).  Additionally, calling `reloadData` in `initialLayout` might also be needed.

[Matt Baker]

Created attachment 353102 [details]
Patch

[Matt Baker]

Created attachment 353155 [details]
Patch

[Matt Baker]

(In reply to Matt Baker from comment #10)
> Created attachment 353155 [details]
> Patch

Fixed an issue where the sort comparator wasn't being applied after reloading cookies.

[Devin Rousso]

Comment on attachment 353102 [details]
Patch

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

r-, as this doesn't re-sort when fetching new cookies

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:41
> +        this._removeSelectedCookiesNavigationItem.addEventListener(WI.ButtonNavigationItem.Event.Clicked, () => { this._table.removeSelectedRows(); });

NIT: I prefer to make class functions even for cases like this, but it's not a strong preference

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:74
> +        let comparator = this._generateSortComparator();

You should save this to a member variable so that it can be used in other places (similar to `WI.NetworkTableContentView`).

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:88
> +        contextMenu.appendSeparator();

NIT: you might want to add a separator below "Delete" as well

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:90
> +            if (table.isRowSelected(rowIndex))

NIT: we should have some sort of visual distinction for the contextmenu row, as it's not always clear what row it'll effect.  Finder uses an inset blue border.

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:168
> +        this._table.addColumn(new WI.TableColumn("name", WI.UIString("Name"), {minWidth: 150, maxWidth: 300, initialWidth: 200, resizeType: WI.TableColumn.ResizeType.Locked}));

Style: I'd prefer it if objects with many properties like this were spread out onto separate lines.  It makes it easier to read (and any future diffs cleaner).  Same is true for all the calls below.

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:170
> +        this._table.addColumn(new WI.TableColumn("domain", WI.unlocalizedString("Domain"), {}));

Style: the `{}` is not needed

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:242
> +                let aExpires = a.expires;

Style: since these values are plain members on a `WI.Cookie`, I don't think it's necessary to save them to a variable

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:253
> +            console.assert("Unexpected sort column", sortColumnIdentifier);

NIT: you should also return here

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:268
> +            this._cookies = this._filterCookies(payload.cookies);

You need to resort this based on the current sort.

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:277
> +        if (event.keyCode === 8 || event.keyCode === 46)

Can you use `WI.KeyboardShortcut.Key.Backspace.key`?  Would it also be valid to just use `event.key === "Backspace"`?

> Source/WebInspectorUI/UserInterface/Views/Table.js:1345
> +        if (!this._delegate.tableCellContextMenuClicked)

This shouldn't be necessary, `_handleContextMenu` isn't added unless `this._delegate.tableCellContextMenuClicked` is defined

[Devin Rousso]

Comment on attachment 353102 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:268
>> +            this._cookies = this._filterCookies(payload.cookies);
> 
> You need to resort this based on the current sort.

Just saw that attachment 353155 [details] fixes this.  My mistake :P

[Devin Rousso]

Comment on attachment 353155 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:42
> +        this._removeSelectedCookiesNavigationItem.addEventListener(WI.ButtonNavigationItem.Event.Clicked, () => { this._table.removeSelectedRows(); });

This seems somewhat off.  In every other case in WebInspector, the "clear" button wipes out everything in that view.  Is there a reason that we don't want to do that here?

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:81
> +        this._table.reloadData();

I'd move this inside `_updateSort`, since whenever you change the order of the rows, you'd also want to re-render them.

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:165
> +        super.initialLayout();

You should add back the FIXME for <https://webkit.org/b/151400> (or just fix it).

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:270
> +            this._cookies = this._filterCookies(payload.cookies);

Not sure if this is in the "scope" of this patch, but the previous selection isn't restored when refreshing.

Also, the `_removeSelectedCookiesNavigationItem` stays `enabled` after refreshing, even though nothing is selected.

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:272
> +            this._table.reloadData();

Ditto (81)

[Devin Rousso]

Comment on attachment 353155 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:110
> +            this._cookies.splice(index, 1);

This causes the assertion on 106 (and Table.js:1500) to fire, since you're mutating the array while trying to retrieve values from it.  You should copy/filter/separate `_cookies` based on whether the index is in `rowIndexes`.

[Matt Baker]

Comment on attachment 353102 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:74
>> +        let comparator = this._generateSortComparator();
> 
> You should save this to a member variable so that it can be used in other places (similar to `WI.NetworkTableContentView`).

This was changed in: https://bugs.webkit.org/attachment.cgi?id=353155&action=diff

>> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:90
>> +            if (table.isRowSelected(rowIndex))
> 
> NIT: we should have some sort of visual distinction for the contextmenu row, as it's not always clear what row it'll effect.  Finder uses an inset blue border.

Agree. Filed an issue for this:

Web Inspector: Table should style an unselected row when it is right-clicked
https://bugs.webkit.org/show_bug.cgi?id=191095

>> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:168
>> +        this._table.addColumn(new WI.TableColumn("name", WI.UIString("Name"), {minWidth: 150, maxWidth: 300, initialWidth: 200, resizeType: WI.TableColumn.ResizeType.Locked}));
> 
> Style: I'd prefer it if objects with many properties like this were spread out onto separate lines.  It makes it easier to read (and any future diffs cleaner).  Same is true for all the calls below.

Agreed. I'm also going to come up with some more reasonable values for min/max/initialWidth.

>> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:277
>> +        if (event.keyCode === 8 || event.keyCode === 46)
> 
> Can you use `WI.KeyboardShortcut.Key.Backspace.key`?  Would it also be valid to just use `event.key === "Backspace"`?

I changed it to:

if (event.keyCode === WI.KeyboardShortcut.Key.Backspace.keyCode || event.keyCode === WI.KeyboardShortcut.Key.Delete.keyCode) { ... }

It's a little long, but very clear.

[Matt Baker]

Created attachment 353635 [details]
Patch

[Matt Baker]

(In reply to Matt Baker from comment #17)
> Created attachment 353635 [details]
> Patch

I removed the "Remove Cookies" button. I agree it was confusing since elsewhere it means "Clear All".

[Devin Rousso]

Comment on attachment 353635 [details]
Patch

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

r=me, nice work!

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:110
> +            let cookieURL = (cookie.secure ? "https://" : "http://") + cookie.domain + cookie.path;

This should probably be a member of `WI.Cookie`.

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:164
> +            minWidth: 70,

Just out of curiosity, where did you get these values?

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:317
> +    _reloadCookies()

It might be worth adding this to a `shown()` function, so that it auto-updates whenever the view becomes visible (only once `didInitialLayout`).

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:319
> +        PageAgent.getCookies().then((payload) => {

I wish we had a manager for this so that it isn't directly invoked by view code :(

[Matt Baker]

Comment on attachment 353635 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:110
>> +            let cookieURL = (cookie.secure ? "https://" : "http://") + cookie.domain + cookie.path;
> 
> This should probably be a member of `WI.Cookie`.

Good idea. I've added a FIXME to do this in:

Web Inspector: Cookies view should use model objects instead of raw payload data
https://bugs.webkit.org/show_bug.cgi?id=189533

>> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:164
>> +            minWidth: 70,
> 
> Just out of curiosity, where did you get these values?

Experimentation. For minimum width in particular, I checked that the column header and sort arrow fit without being truncated. We may need to adjust these values after living on this.

>> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:317
>> +    _reloadCookies()
> 
> It might be worth adding this to a `shown()` function, so that it auto-updates whenever the view becomes visible (only once `didInitialLayout`).

Maybe, but for now I wanted to keep the original behavior. Currently we automatically load cookies on initialLayout only.

[Matt Baker]

Created attachment 353723 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 353723 [details]
Patch for landing

Clearing flags on attachment: 353723

Committed r237746: <https://trac.webkit.org/changeset/237746>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.