Bug 183485 – [macOS] Copying a table from the Numbers app and pasting into iCloud Numbers fails

• Component: HTML Editing
• Status: RESOLVED FIXED

[Wenson Hsieh]

<rdar://problem/38041984>

[Wenson Hsieh]

Created attachment 335450 [details]
Patch

[Wenson Hsieh]

Created attachment 335458 [details]
Fix macOS 10.11 builds

[Ryosuke Niwa]

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.

[Wenson Hsieh]

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

[Wenson Hsieh]

Created attachment 335506 [details]
One more EWS run

[Wenson Hsieh]

Created attachment 335507 [details]
One more EWS run

[WebKit Commit Bot]

Comment on attachment 335507 [details]
One more EWS run

Clearing flags on attachment: 335507

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