Bug 191482 – Web Inspector: Cookies table needs copy keyboard shortcut and context menu support

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

[Matt Baker]

Summary:
Cookies table needs copy keyboard shortcut and context menu support.

DataGrid supports `handleCopyEvent` and provides "Copy Row" & "Copy Table" context menu items as needed. Table doesn't need to provide this level of support, but at the very least clients need to query column visibility, so the clipboard text can reflect what is shown in the table.

CookieStorageContentView can reuse the formatting from tablePopulateCell to generate clipboard text.

[Radar WebKit Bug Importer]

<rdar://problem/45953002>

[Matt Baker]

Created attachment 354865 [details]
Patch

[Matt Baker]

Created attachment 355127 [details]
Patch

[Devin Rousso]

Comment on attachment 355127 [details]
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.

[Matt Baker]

Comment on attachment 355127 [details]
Patch

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

>> 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 ";")?

This matches the original DataGrid behavior. I haven't found many native apps with tables to use for comparison, but the Instruments app (which doesn't allow multiple selection) does use \t to delimit fields. We could always add other copy options if there is a need.

>> 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

I'll make the content depend on a `formatForClipboard` flag (or something like that).

[Matt Baker]

Created attachment 355612 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 355612 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 355612 [details]
Patch

Clearing flags on attachment: 355612

Committed r238569: <https://trac.webkit.org/changeset/238569>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.