[webkit-reviews] review denied: [Bug 182636] Pasting from Excel no longer provides text/html data : [Attachment 333499] Adjust type order & test expectations.

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 182636: Pasting from Excel no longer provides text/html data
https://bugs.webkit.org/show_bug.cgi?id=182636

Attachment 333499: Adjust type order & test expectations.

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




--- Comment #8 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 333499
  --> https://bugs.webkit.org/attachment.cgi?id=333499
Adjust type order & test expectations.

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

> Source/WebCore/ChangeLog:57
> +	   "text/plain". I opted for this because it seems less hacky than
appending "text/html" separately, on top of
> +	   "text/uri-list".

If you're doing that here, then it's weird that getDataForItem has special code
paths for "text/uri-list" and "text/html".
Either we should always special case those two, or always remove plain text.
Using two different strategies in those two places should lead to bugs.
r- because I think we need to at least address this problem before landing
this.

> Source/WebCore/dom/DataTransfer.cpp:158
> +	   if (lowercaseType == "text/html" &&
RuntimeEnabledFeatures::sharedFeatures().customPasteboardDataEnabled()) {

Why do we need to check
RuntimeEnabledFeatures::sharedFeatures().customPasteboardDataEnabled() here?

> Source/WebCore/dom/DataTransfer.cpp:175
> +String DataTransfer::readPasteboardDataForBindingsRespectingOrigin(Document&
document, const String& lowercaseType, WebContentReadingPolicy policy) const

DataTransfer object is for bindings only. There is no need to say that. Just
say readStringFromPasteboard or something.

> Source/WebCore/dom/DataTransfer.cpp:310
> +	   // We don't expose 'text/plain' to the page if the pasteboard
contains files, because plain text data may contain file paths.

Just say "Remove "text/plain" to avoid exposing local file paths".
No need to say we don't expose it. It's obvious from the code.

> Source/WebCore/platform/Pasteboard.h:67
> +enum class WebContentReadingPolicy { ReadFromAnyType,
OnlyReadFromRichTextTypes };

Since the enum says "reading policy", we probably don't need to say "ReadFrom".
Just AnyTime, OnlyRichTextTypes would suffice.

> Source/WebCore/platform/ios/PasteboardIOS.mm:166
> +static bool typeIsAppropriateForWebContentReadingPolicy(NSString *type,
WebContentReadingPolicy policy)

How about just isTypeAllowedByReadingPolicy? I don't think we need to spell out
the full enum name since it's in the argument list.

> Source/WebCore/platform/ios/PasteboardIOS.mm:168
> +    return policy == WebContentReadingPolicy::ReadFromAnyType || [type
isEqualToString:WebArchivePboardType] || [type isEqualToString:(NSString
*)kUTTypeHTML] || [type isEqualToString:(NSString *)kUTTypeRTF] || [type
isEqualToString:(NSString *)kUTTypeFlatRTFD];

Please wrap this line at some point.