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.
<rdar://problem/34761725>
Created attachment 322337 [details] Cleanup
Created attachment 322339 [details] Fixed builds
Created attachment 322340 [details] Fixed Windows build
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.
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))
(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.
(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);
Created attachment 322342 [details] Patch for landing
Comment on attachment 322342 [details] Patch for landing Wait for EWS.
Created attachment 322344 [details] Patch for landing
Comment on attachment 322344 [details] Patch for landing Wait for EWS again.
(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.
Comment on attachment 322344 [details] Patch for landing Clearing flags on attachment: 322344 Committed r222699: <http://trac.webkit.org/changeset/222699>
All reviewed patches have been landed. Closing bug.