[webkit-reviews] review granted: [Bug 183485] [macOS] Copying a table from the Numbers app and pasting into iCloud Numbers fails : [Attachment 335458] Fix macOS 10.11 builds

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 183485: [macOS] Copying a table from the Numbers app and pasting into
iCloud Numbers fails
https://bugs.webkit.org/show_bug.cgi?id=183485

Attachment 335458: Fix macOS 10.11 builds

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




--- Comment #3 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 335458
  --> https://bugs.webkit.org/attachment.cgi?id=335458
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.