[webkit-reviews] review denied: [Bug 66381] Web Inspector: support multiple selection/deletion of cookie records : [Attachment 353102] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has denied  review:
Bug 66381: Web Inspector: support multiple selection/deletion of cookie records
https://bugs.webkit.org/show_bug.cgi?id=66381

Attachment 353102: Patch

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




--- Comment #12 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 353102
  --> https://bugs.webkit.org/attachment.cgi?id=353102
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.ButtonNavigationI
tem.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