WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
177728
Merge readFilenames() and read(PasteboardFileReader)
https://bugs.webkit.org/show_bug.cgi?id=177728
Summary
Merge readFilenames() and read(PasteboardFileReader)
Ryosuke Niwa
Reported
2017-10-01 16:28:08 PDT
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.
Attachments
Cleanup
(22.70 KB, patch)
2017-10-01 16:51 PDT
,
Ryosuke Niwa
no flags
Details
Formatted Diff
Diff
Fixed builds
(24.88 KB, patch)
2017-10-01 17:19 PDT
,
Ryosuke Niwa
no flags
Details
Formatted Diff
Diff
Fixed Windows build
(24.87 KB, patch)
2017-10-01 18:32 PDT
,
Ryosuke Niwa
no flags
Details
Formatted Diff
Diff
Patch for landing
(25.47 KB, patch)
2017-10-01 19:04 PDT
,
Ryosuke Niwa
no flags
Details
Formatted Diff
Diff
Patch for landing
(25.47 KB, patch)
2017-10-01 19:40 PDT
,
Ryosuke Niwa
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2017-10-01 16:28:44 PDT
<
rdar://problem/34761725
>
Ryosuke Niwa
Comment 2
2017-10-01 16:51:53 PDT
Created
attachment 322337
[details]
Cleanup
Ryosuke Niwa
Comment 3
2017-10-01 17:19:21 PDT
Created
attachment 322339
[details]
Fixed builds
Ryosuke Niwa
Comment 4
2017-10-01 18:32:54 PDT
Created
attachment 322340
[details]
Fixed Windows build
Darin Adler
Comment 5
2017-10-01 18:36:32 PDT
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 6
2017-10-01 18:52:16 PDT
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
Comment 7
2017-10-01 18:54:23 PDT
(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
Comment 8
2017-10-01 19:00:28 PDT
(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
Comment 9
2017-10-01 19:04:06 PDT
Created
attachment 322342
[details]
Patch for landing
Ryosuke Niwa
Comment 10
2017-10-01 19:04:18 PDT
Comment on
attachment 322342
[details]
Patch for landing Wait for EWS.
Ryosuke Niwa
Comment 11
2017-10-01 19:40:06 PDT
Created
attachment 322344
[details]
Patch for landing
Ryosuke Niwa
Comment 12
2017-10-01 19:44:30 PDT
Comment on
attachment 322344
[details]
Patch for landing Wait for EWS again.
Sam Weinig
Comment 13
2017-10-01 20:11:28 PDT
(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 14
2017-10-02 00:03:48 PDT
Comment on
attachment 322344
[details]
Patch for landing Clearing flags on attachment: 322344 Committed
r222699
: <
http://trac.webkit.org/changeset/222699
>
WebKit Commit Bot
Comment 15
2017-10-02 00:03:50 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug