Bug 177728 – Merge readFilenames() and read(PasteboardFileReader)

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

In https://trac.webkit.org/r222656, we introduced PasteboardFileReader which reads a single data-like file out of pasteboard.
Generalizing this class to support reading multiple files off of pasteboard allows us to eliminate
both Pasteboard::readFilenames() and Pasteboard::typesTreatedAsFiles now that we have Pasteboard::containsFiles.

[Radar WebKit Bug Importer]

<rdar://problem/34761725>

[Ryosuke Niwa]

Created attachment 322337 [details]
Cleanup

[Ryosuke Niwa]

Created attachment 322339 [details]
Fixed builds

[Ryosuke Niwa]

Created attachment 322340 [details]
Fixed Windows build

[Darin Adler]

Comment on attachment 322339 [details]
Fixed builds

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

Not sure if the interface is quite right. The function doing the pasteboard processing needs to know if the client is going to need things or not.

> Source/WebCore/platform/cocoa/PasteboardCocoa.mm:106
> +        return nullptr; // Don't support TIFF on iOS for now.

This comment doesn’t make sense because the line above says ASSERT_NOT_REACHED. Which is it? Not reached, or reached but not supported?

> Source/WebCore/platform/cocoa/PasteboardCocoa.mm:163
> +    Vector<String> types;
>      copyToVector(result, types);
>      return types;

This is so lame. We should have a way to convert a ListHashSet to a vector with a return value rather than an out argument.

> Source/WebCore/platform/cocoa/PasteboardCocoa.mm:185
> +    HashSet<const char*> existingMIMEs;

This HashSet is going to use the addresses of the string literals to define equality. Is that OK, or do we need a set that will treat equal strings that might not be the same literal at different addresses as equal too?

If we do want to use the pointers, we should add a comment to imageTypeToMIMEType cautioning it to only use each string literal once since they are not guaranteed to be coalesced.

> Source/WebCore/platform/cocoa/PasteboardCocoa.mm:191
> +        if (imageType == ImageType::Invalid || existingMIMEs.contains(mimeType))
> +            continue;
> +        existingMIMEs.add(mimeType);

Double hash table lookup here. Need to combine the contains and the add by using the return value of "add" to avoid doing it twice.

> Source/WebCore/platform/win/PasteboardWin.cpp:312
> +struct PasteboardFileCounter final : public PasteboardFileReader {

No need for "public" here.

> Source/WebCore/platform/win/PasteboardWin.cpp:313
> +

Extra blank line here.

> Source/WebCore/platform/win/PasteboardWin.cpp:315
> +    void readBuffer(const String&, const String&, Ref<SharedBuffer>&&) final { ASSERT_NOT_REACHED(); }

This is ugly. It makes me think that we are using one type for two different purposes.

> Source/WebCore/platform/win/PasteboardWin.cpp:355
>      if (!m_dragDataMap.contains(cfHDropFormat()->cfFormat))
> -        return fileNames;
> -    return m_dragDataMap.get(cfHDropFormat()->cfFormat);
> +        return;
> +
> +    Vector<String> list = m_dragDataMap.get(cfHDropFormat()->cfFormat);

Double hash table lookup here. Should use find instead.

[Sam Weinig]

Comment on attachment 322340 [details]
Fixed Windows build

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

> Source/WebCore/editing/WebCorePasteboardFileReader.cpp:45
> +    files.append(File::create(Blob::create(WTFMove(data), type), filename));

Is it really necessary to do this File wrapping a Blob dance? Given that a File isa Blob, I think you should be able to avoid it.

> Source/WebCore/platform/cocoa/PasteboardCocoa.mm:107
> +#if PLATFORM(MAC)
> +        return "image/png"; // For Web compatibility, we pretend to have PNG instead.
> +#else
>          ASSERT_NOT_REACHED();
> -        return nullptr;
> +        return nullptr; // Don't support TIFF on iOS for now.
> +#endif

This would be slightly nicer as a #if HAVE(TIFF)

> Source/WebCore/platform/cocoa/PasteboardCocoa.mm:153
> +    Vector<String> cocoaTypes = readTypesWithSecurityCheck();

auto?

> Source/WebCore/platform/cocoa/PasteboardCocoa.mm:154
> +    if (!cocoaTypes.size())

I prefer this as cocoaTypes.isEmpty()

> Source/WebCore/platform/cocoa/PasteboardCocoa.mm:184
> +    Vector<String> cocoaTypes = readTypesWithSecurityCheck();

auto?

> Source/WebCore/platform/cocoa/PasteboardCocoa.mm:188
> +        const char* mimeType = imageTypeToMIMEType(imageType);

auto?

> Source/WebCore/platform/win/PasteboardWin.cpp:314
> +struct PasteboardFileCounter final : public PasteboardFileReader {
> +
> +    void readFilename(const String&) final { ++count; }

Extra newline.

> Source/WebCore/platform/win/PasteboardWin.cpp:355
> +    Vector<String> list = m_dragDataMap.get(cfHDropFormat()->cfFormat);

auto? Or just put the call to get in loop header:

for (auto& filename : m_dragDataMap.get(cfHDropFormat()->cfFormat))

[Ryosuke Niwa]

(In reply to Darin Adler from comment #5)
> Comment on attachment 322339 [details]
> Fixed builds
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322339&action=review
> 
> Not sure if the interface is quite right. The function doing the pasteboard
> processing needs to know if the client is going to need things or not.

I don't follow. Any suggestions on how to improve it?

> > Source/WebCore/platform/cocoa/PasteboardCocoa.mm:106
> > +        return nullptr; // Don't support TIFF on iOS for now.
> 
> This comment doesn’t make sense because the line above says
> ASSERT_NOT_REACHED. Which is it? Not reached, or reached but not supported?

What I meant is that we don't expect reach there since we don't support TIFF on iOS. Maybe I can just get rid of the comment since we already have a comment about it in imageTypeToMIMEType.

> > Source/WebCore/platform/cocoa/PasteboardCocoa.mm:163
> > +    Vector<String> types;
> >      copyToVector(result, types);
> >      return types;
> 
> This is so lame. We should have a way to convert a ListHashSet to a vector
> with a return value rather than an out argument.

Indeed. What's the canonical way to get a Vector out of HashSet, etc...? I was thinking that we can add Vector::from(~) which supports any iterable type.

> > Source/WebCore/platform/cocoa/PasteboardCocoa.mm:185
> > +    HashSet<const char*> existingMIMEs;
> 
> This HashSet is going to use the addresses of the string literals to define
> equality. Is that OK, or do we need a set that will treat equal strings that
> might not be the same literal at different addresses as equal too?

Yes, this is indeed intended as imageTypeToMIMEType always returns a const char* to a static string literal.

> If we do want to use the pointers, we should add a comment to
> imageTypeToMIMEType cautioning it to only use each string literal once since
> they are not guaranteed to be coalesced.

Sure. I can add a comment.

> > Source/WebCore/platform/cocoa/PasteboardCocoa.mm:191
> > +        if (imageType == ImageType::Invalid || existingMIMEs.contains(mimeType))
> > +            continue;
> > +        existingMIMEs.add(mimeType);
> 
> Double hash table lookup here. Need to combine the contains and the add by
> using the return value of "add" to avoid doing it twice.

Nice catch. Fixed.

> > Source/WebCore/platform/win/PasteboardWin.cpp:312
> > +struct PasteboardFileCounter final : public PasteboardFileReader {
> 
> No need for "public" here.

Fixed.

> > Source/WebCore/platform/win/PasteboardWin.cpp:313
> > +
> 
> Extra blank line here.

Removed.

> > Source/WebCore/platform/win/PasteboardWin.cpp:315
> > +    void readBuffer(const String&, const String&, Ref<SharedBuffer>&&) final { ASSERT_NOT_REACHED(); }
> 
> This is ugly. It makes me think that we are using one type for two different
> purposes.

Well, so it's possible that Windows would eventually support adding buffer types. Right now, we only support filenames. I'm gonna just replace this with ++count since that'd make this code future proof.

> > Source/WebCore/platform/win/PasteboardWin.cpp:355
> >      if (!m_dragDataMap.contains(cfHDropFormat()->cfFormat))
> > -        return fileNames;
> > -    return m_dragDataMap.get(cfHDropFormat()->cfFormat);
> > +        return;
> > +
> > +    Vector<String> list = m_dragDataMap.get(cfHDropFormat()->cfFormat);
> 
> Double hash table lookup here. Should use find instead.

Fixed.

[Ryosuke Niwa]

(In reply to Sam Weinig from comment #6)
> Comment on attachment 322340 [details]
> Fixed Windows build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322340&action=review
> 
> > Source/WebCore/editing/WebCorePasteboardFileReader.cpp:45
> > +    files.append(File::create(Blob::create(WTFMove(data), type), filename));
> 
> Is it really necessary to do this File wrapping a Blob dance? Given that a
> File isa Blob, I think you should be able to avoid it.

We don't currently have File::create variant that takes Vector<uint8_t>.

> > Source/WebCore/platform/cocoa/PasteboardCocoa.mm:107
> > +#if PLATFORM(MAC)
> > +        return "image/png"; // For Web compatibility, we pretend to have PNG instead.
> > +#else
> >          ASSERT_NOT_REACHED();
> > -        return nullptr;
> > +        return nullptr; // Don't support TIFF on iOS for now.
> > +#endif
> 
> This would be slightly nicer as a #if HAVE(TIFF)

Do we even have HAVE(TIFF)? Anyhow, we do support TIFF format elsewhere on iOS. It's just that I didn't add the support for converting TIFF to PNG on iOS for now since that's rather involved.

> > Source/WebCore/platform/cocoa/PasteboardCocoa.mm:153
> > +    Vector<String> cocoaTypes = readTypesWithSecurityCheck();
> 
> auto?

Fixed.

> > Source/WebCore/platform/cocoa/PasteboardCocoa.mm:154
> > +    if (!cocoaTypes.size())
> 
> I prefer this as cocoaTypes.isEmpty()

Sure. Replaced.

> > Source/WebCore/platform/cocoa/PasteboardCocoa.mm:184
> > +    Vector<String> cocoaTypes = readTypesWithSecurityCheck();
> 
> auto?

Fixed.

> > Source/WebCore/platform/cocoa/PasteboardCocoa.mm:188
> > +        const char* mimeType = imageTypeToMIMEType(imageType);
> 
> auto?

Hm... I think I'd prefer keeping const char* so that it's clear the function itself returns that given Darin's comment about existingMIMEs being HashSet<const char*>.

> > Source/WebCore/platform/win/PasteboardWin.cpp:314
> > +struct PasteboardFileCounter final : public PasteboardFileReader {
> > +
> > +    void readFilename(const String&) final { ++count; }
> 
> Extra newline.

Fixed.

> > Source/WebCore/platform/win/PasteboardWin.cpp:355
> > +    Vector<String> list = m_dragDataMap.get(cfHDropFormat()->cfFormat);
> 
> auto? Or just put the call to get in loop header:
> 
> for (auto& filename : m_dragDataMap.get(cfHDropFormat()->cfFormat))

Sure. In fact, after addressing Darin's comment, this looks like:
    auto list = m_dragDataMap.find(cfHDropFormat()->cfFormat);
    if (list == m_dragDataMap.end())
        return;

    for (auto& filename : *list)
        reader.readFilename(filename);

[Ryosuke Niwa]

Created attachment 322342 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 322342 [details]
Patch for landing

Wait for EWS.

[Ryosuke Niwa]

Created attachment 322344 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 322344 [details]
Patch for landing

Wait for EWS again.

[Sam Weinig]

(In reply to Darin Adler from comment #5)
> Comment on attachment 322339 [details]
> 
> > Source/WebCore/platform/cocoa/PasteboardCocoa.mm:163
> > +    Vector<String> types;
> >      copyToVector(result, types);
> >      return types;
> 
> This is so lame. We should have a way to convert a ListHashSet to a vector
> with a return value rather than an out argument.

Totally agree. Filed https://bugs.webkit.org/show_bug.cgi?id=177732 with some thoughts.

[WebKit Commit Bot]

Comment on attachment 322344 [details]
Patch for landing

Clearing flags on attachment: 322344

Committed r222699: <http://trac.webkit.org/changeset/222699>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.