Bug 183485 - [macOS] Copying a table from the Numbers app and pasting into iCloud Numbers fails
Summary: [macOS] Copying a table from the Numbers app and pasting into iCloud Numbers ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-03-08 15:16 PST by Wenson Hsieh
Modified: 2018-03-10 13:27 PST (History)
12 users (show)

See Also:


Attachments
Patch (25.64 KB, patch)
2018-03-09 12:38 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Fix macOS 10.11 builds (25.64 KB, patch)
2018-03-09 13:57 PST, Wenson Hsieh
rniwa: review+
Details | Formatted Diff | Diff
One more EWS run (25.78 KB, patch)
2018-03-10 01:09 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
One more EWS run (25.75 KB, patch)
2018-03-10 01:43 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2018-03-08 15:16:54 PST
<rdar://problem/38041984>
Comment 1 Wenson Hsieh 2018-03-09 12:38:23 PST
Created attachment 335450 [details]
Patch
Comment 2 Wenson Hsieh 2018-03-09 13:57:57 PST
Created attachment 335458 [details]
Fix macOS 10.11 builds
Comment 3 Ryosuke Niwa 2018-03-09 17:09:41 PST
Comment on attachment 335458 [details]
Fix macOS 10.11 builds

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

> Source/WebCore/ChangeLog:12
> +        r222688) this means we now don't allow web pages to access "text/plain" in the case where the user copies part

End the previous sentence and capitalize "this"?

> Source/WebCore/ChangeLog:13
> +        of a table from the native Numbers app, since Numbers additionally writes a snapshot of the table to the

No need for , here.

> Source/WebCore/ChangeLog:16
> +        In the first place, this restriction on getData/setData was intended to prevent web pages from extracting users'

I don't think "In the first place" as much value to the description here.

> Source/WebCore/ChangeLog:37
> +        Only whitelist "text/html" or "text/uri-list" in the case where there are actual files in the pasteboard. If

Only allow*. We should avoid using terms such as whitelist and blacklist going forward.

> Source/WebCore/ChangeLog:47
> +        -   MayContainFilePaths indicates that there are real might be real file paths on the pasteboard. This means

Nit: there are real might be real file paths -> there might be file paths.

> Source/WebCore/dom/DataTransfer.cpp:207
> +    return m_pasteboard->containsFiles() == Pasteboard::ContainsFilesResult::MayContainFilePaths;

Now that this function returns an enum, it's strange to call this function "containsFile".
How about m_pasteboard->fileContentState()?

> Source/WebCore/platform/Pasteboard.h:224
> +    enum class ContainsFilesResult { NoImageDataOrFilePaths, OnlyImageData, MayContainFilePaths };
> +    virtual WEBCORE_EXPORT ContainsFilesResult containsFiles();

How about FileContentState { NoFileOrImageData, InMemoryImage, MayContainFilePaths } and fileContentState().
I don't think "OnlyImageData" quite communicate as to why that's relevant to the presence of a file.
Comment 4 Wenson Hsieh 2018-03-10 00:56:36 PST
Comment on attachment 335458 [details]
Fix macOS 10.11 builds

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

>> Source/WebCore/ChangeLog:12
>> +        r222688) this means we now don't allow web pages to access "text/plain" in the case where the user copies part
> 
> End the previous sentence and capitalize "this"?

Fixed!

>> Source/WebCore/ChangeLog:13
>> +        of a table from the native Numbers app, since Numbers additionally writes a snapshot of the table to the
> 
> No need for , here.

Removed the ","

>> Source/WebCore/ChangeLog:16
>> +        In the first place, this restriction on getData/setData was intended to prevent web pages from extracting users'
> 
> I don't think "In the first place" as much value to the description here.

Fair enough — removed!

>> Source/WebCore/ChangeLog:37
>> +        Only whitelist "text/html" or "text/uri-list" in the case where there are actual files in the pasteboard. If
> 
> Only allow*. We should avoid using terms such as whitelist and blacklist going forward.

Ah, right! Fixed.

>> Source/WebCore/ChangeLog:47
>> +        -   MayContainFilePaths indicates that there are real might be real file paths on the pasteboard. This means
> 
> Nit: there are real might be real file paths -> there might be file paths.

Whoops, fixed.

>> Source/WebCore/dom/DataTransfer.cpp:207
>> +    return m_pasteboard->containsFiles() == Pasteboard::ContainsFilesResult::MayContainFilePaths;
> 
> Now that this function returns an enum, it's strange to call this function "containsFile".
> How about m_pasteboard->fileContentState()?

Good point — changed Pasteboard::containsFile() to Pasteboard::fileContentState()

>> Source/WebCore/platform/Pasteboard.h:224
>> +    virtual WEBCORE_EXPORT ContainsFilesResult containsFiles();
> 
> How about FileContentState { NoFileOrImageData, InMemoryImage, MayContainFilePaths } and fileContentState().
> I don't think "OnlyImageData" quite communicate as to why that's relevant to the presence of a file.

Yeah, this sounds reasonable.

Renamed enum type and values to FileContentState { NoFileOrImageData, InMemoryImage, MayContainFilePaths } (And also renamed the method to fileContentState).
Comment 5 Wenson Hsieh 2018-03-10 01:09:50 PST
Created attachment 335506 [details]
One more EWS run
Comment 6 Wenson Hsieh 2018-03-10 01:43:15 PST
Created attachment 335507 [details]
One more EWS run
Comment 7 WebKit Commit Bot 2018-03-10 03:33:25 PST
Comment on attachment 335507 [details]
One more EWS run

Clearing flags on attachment: 335507

Committed r229503: <https://trac.webkit.org/changeset/229503>