Bug 66381

Summary: Web Inspector: support multiple selection/deletion of cookie records
Product: WebKit Reporter: Trevor Cortez <trevor.cortez>
Component: Web InspectorAssignee: Matt Baker <mattbaker>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bugzilla, bweinstein, commit-queue, graouts, hi, inspector-bugzilla-changes, jonowells, keishi, loislo, pfeldman, pmuellr, rik, webkit-bug-importer, yurys
Priority: P5 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 189705, 189803    
Bug Blocks: 191095    
Attachments:
Description Flags
WIP
none
WIP
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Trevor Cortez 2011-08-17 08:30:59 PDT
* 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
Comment 1 Radar WebKit Bug Importer 2014-12-17 11:23:02 PST
<rdar://problem/19281525>
Comment 2 Matt Baker 2016-04-26 23:28:26 PDT
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.
Comment 3 Matt Baker 2018-09-18 13:00:09 PDT
Created attachment 350042 [details]
WIP
Comment 4 Devin Rousso 2018-09-18 22:57:10 PDT
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).
Comment 5 Matt Baker 2018-10-03 14:14:34 PDT
Created attachment 351546 [details]
WIP
Comment 6 Matt Baker 2018-10-03 14:15:34 PDT
Comment on attachment 351546 [details]
WIP

Still needs refinements to column widths.
Comment 7 Matt Baker 2018-10-03 14:17:48 PDT
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.
Comment 8 Devin Rousso 2018-10-03 15:19:16 PDT
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.
Comment 9 Matt Baker 2018-10-25 13:19:48 PDT
Created attachment 353102 [details]
Patch
Comment 10 Matt Baker 2018-10-25 21:26:41 PDT
Created attachment 353155 [details]
Patch
Comment 11 Matt Baker 2018-10-25 21:27:31 PDT
(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.
Comment 12 Devin Rousso 2018-10-25 22:36:59 PDT
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
Comment 13 Devin Rousso 2018-10-25 22:40:32 PDT
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
Comment 14 Devin Rousso 2018-10-25 22:45:40 PDT
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)
Comment 15 Devin Rousso 2018-10-25 22:50:11 PDT
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`.
Comment 16 Matt Baker 2018-10-30 17:53:38 PDT
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.
Comment 17 Matt Baker 2018-11-01 12:26:27 PDT
Created attachment 353635 [details]
Patch
Comment 18 Matt Baker 2018-11-01 12:27:47 PDT
(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".
Comment 19 Devin Rousso 2018-11-02 11:44:37 PDT
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 :(
Comment 20 Matt Baker 2018-11-02 12:10:47 PDT
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.
Comment 21 Matt Baker 2018-11-02 12:13:59 PDT
Created attachment 353723 [details]
Patch for landing
Comment 22 WebKit Commit Bot 2018-11-02 12:39:11 PDT
Comment on attachment 353723 [details]
Patch for landing

Clearing flags on attachment: 353723

Committed r237746: <https://trac.webkit.org/changeset/237746>
Comment 23 WebKit Commit Bot 2018-11-02 12:39:13 PDT
All reviewed patches have been landed.  Closing bug.