Bug 191482

Summary: Web Inspector: Cookies table needs copy keyboard shortcut and context menu support
Product: WebKit Reporter: Matt Baker <mattbaker>
Component: Web InspectorAssignee: Matt Baker <mattbaker>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, hi, inspector-bugzilla-changes, joepeck, webkit-bug-importer
Priority: P3 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Matt Baker 2018-11-09 12:41:34 PST
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.
Comment 1 Radar WebKit Bug Importer 2018-11-09 12:41:53 PST
<rdar://problem/45953002>
Comment 2 Matt Baker 2018-11-14 16:05:46 PST
Created attachment 354865 [details]
Patch
Comment 3 Matt Baker 2018-11-16 14:39:12 PST
Created attachment 355127 [details]
Patch
Comment 4 Devin Rousso 2018-11-20 21:18:03 PST
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.
Comment 5 Matt Baker 2018-11-25 17:33:52 PST
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).
Comment 6 Matt Baker 2018-11-25 18:48:06 PST
Created attachment 355612 [details]
Patch
Comment 7 Joseph Pecoraro 2018-11-27 12:03:43 PST
Comment on attachment 355612 [details]
Patch

r=me
Comment 8 WebKit Commit Bot 2018-11-27 13:06:59 PST
Comment on attachment 355612 [details]
Patch

Clearing flags on attachment: 355612

Committed r238569: <https://trac.webkit.org/changeset/238569>
Comment 9 WebKit Commit Bot 2018-11-27 13:07:00 PST
All reviewed patches have been landed.  Closing bug.