[webkit-reviews] review denied: [Bug 191482] Web Inspector: Cookies table needs copy keyboard shortcut and context menu support : [Attachment 355127] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has denied Matt Baker <mattbaker at apple.com>'s
request for review:
Bug 191482: Web Inspector: Cookies table needs copy keyboard shortcut and
context menu support
https://bugs.webkit.org/show_bug.cgi?id=191482

Attachment 355127: Patch

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




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

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

r-, due to the `emDash` and `zeroWidthSpace` concerns.	The logic looks fine.

> Source/WebInspectorUI/ChangeLog:20
> +	   (WI.CookieStorageContentView):

NIT: this should go first in the ChangeLog

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:352
> +	       return values.join("\t");

Is the reason you aren't using a CSV format because some of the `WI.Cookie`
values might have commas?  In that case, perhaps we should expose this as a
cookie string instead (e.g. `${key}=${value}` joined by ";")?

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:368
> +	       return cookie.domain || emDash;

We shouldn't expose `emDash` as clipboard content.  That's really just a
formatting tool for WebInspector.  See (352) for an alternative

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:370
> +	       return cookie.path || emDash;

Ditto (368)

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:376
> +	       return cookie.secure ? checkmark : zeroWidthSpace;

We shouldn't expose `zeroWidthSpace` as clipboard content.  That's really just
a formatting tool for WebInspector.  See (352) for an alternative

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:378
> +	       return cookie.httpOnly ? checkmark : zeroWidthSpace;

Ditto (376)

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:380
> +	       return cookie.sameSite === WI.Cookie.SameSiteType.None ? emDash
: WI.Cookie.displayNameForSameSiteType(cookie.sameSite);

Ditto (368)

> Source/WebInspectorUI/UserInterface/Views/Table.js:244
> +	   return this._columnSpecs.values();

This should probably just call `Array.from` by default.  I don't think we ever
expose an iterator unless it's via a `Symbol.iterator` function.