RESOLVED FIXED 76996
[chromium] Fix ClipboardChromium::validateFilename to actually operate on extensions
https://bugs.webkit.org/show_bug.cgi?id=76996
Summary [chromium] Fix ClipboardChromium::validateFilename to actually operate on ext...
Daniel Cheng
Reported 2012-01-25 02:31:19 PST
[chromium] Fix ClipboardChromium::validateFilename to actually operate on extensions
Attachments
Patch (8.38 KB, patch)
2012-01-25 02:33 PST, Daniel Cheng
no flags
Patch (13.07 KB, patch)
2012-01-25 13:03 PST, Daniel Cheng
no flags
Patch (13.82 KB, patch)
2012-01-25 13:12 PST, Daniel Cheng
no flags
Patch (19.92 KB, patch)
2012-01-25 14:18 PST, Daniel Cheng
no flags
Daniel Cheng
Comment 1 2012-01-25 02:33:59 PST
Daniel Cheng
Comment 2 2012-01-25 02:39:32 PST
I left the test oops for now because I'm not sure what your opinion here is. Unless I'm missing something very clever, this isn't currently testable with layout tests. However... we could probably write a webkit unit test for it since it's a static helper; any idea on the preferred way to handle platform differences Separate ClipboardChromiumWinTest.cpp and ClipboardChromiumMacTest.cpp?
Tony Chang
Comment 3 2012-01-25 10:10:41 PST
Comment on attachment 123905 [details] Patch The code change looks fine. Can you use eventSender.beginDragWithFiles to test? I would try all file names on all platforms and check in different results for each platform. If that doesn't work, a unittest is fine (probably with some #ifs in the cpp file).
Daniel Cheng
Comment 4 2012-01-25 13:03:45 PST
Daniel Cheng
Comment 5 2012-01-25 13:04:27 PST
Comment on attachment 123994 [details] Patch Need to update ChangeLogs. Please ignore.
Daniel Cheng
Comment 6 2012-01-25 13:12:18 PST
Tony Chang
Comment 7 2012-01-25 13:24:55 PST
Comment on attachment 123995 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123995&action=review > Source/WebCore/platform/chromium/ClipboardChromiumLinux.cpp:36 > +void ClipboardChromium::validateFilename(String& name, String& extension) > { > notImplemented(); Aren't null and forward slash not allowed? > Source/WebKit/chromium/WebKit.gypi:150 > + ['OS!="linux"', { > + 'webkit_unittest_files': [ > + 'tests/ClipboardChromiumTest.cpp', We should run this test on Linux too.
Daniel Cheng
Comment 8 2012-01-25 14:18:19 PST
Daniel Cheng
Comment 9 2012-01-25 14:20:10 PST
(In reply to comment #7) > (From update of attachment 123995 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=123995&action=review > > > Source/WebCore/platform/chromium/ClipboardChromiumLinux.cpp:36 > > +void ClipboardChromium::validateFilename(String& name, String& extension) > > { > > notImplemented(); > > Aren't null and forward slash not allowed? I was opting to keep the same implementation for simplicity. Examining the FS limits though, it seems easier to just share the Linux/Mac code for this. > > > Source/WebKit/chromium/WebKit.gypi:150 > > + ['OS!="linux"', { > > + 'webkit_unittest_files': [ > > + 'tests/ClipboardChromiumTest.cpp', > > We should run this test on Linux too. I omitted it because we didn't have any implementation before. While I was here, I also adjusted the Win implementation to be more correct--paths are limited to MAX_PATH length with a null terminator, but individual path components are a max of 255 without null.
Tony Chang
Comment 10 2012-01-25 15:00:57 PST
Comment on attachment 124008 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124008&action=review > Source/WebCore/platform/chromium/ClipboardChromiumPosix.cpp:40 > + // HFS+ disallows '/' and Linux systems also disallow null. For sanity's sake we'll also > + // disallow control characters. Why would we disallow control characters? What if the user is trying to drag&drop a file with a control character in it?
Daniel Cheng
Comment 11 2012-01-25 15:06:21 PST
Comment on attachment 124008 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124008&action=review >> Source/WebCore/platform/chromium/ClipboardChromiumPosix.cpp:40 >> + // disallow control characters. > > Why would we disallow control characters? What if the user is trying to drag&drop a file with a control character in it? This is only used when dragging images out of WebKit, IIRC. I don't know why we disallow, other than whoever wrote the code originally thought it'd be nice. I guess it's nice to have filenames without too many strange characters in it or causing your terminal to behave strangely after you 'ls'
Tony Chang
Comment 12 2012-01-25 16:42:04 PST
Comment on attachment 124008 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124008&action=review >>> Source/WebCore/platform/chromium/ClipboardChromiumPosix.cpp:40 >>> + // disallow control characters. >> >> Why would we disallow control characters? What if the user is trying to drag&drop a file with a control character in it? > > This is only used when dragging images out of WebKit, IIRC. I don't know why we disallow, other than whoever wrote the code originally thought it'd be nice. I guess it's nice to have filenames without too many strange characters in it or causing your terminal to behave strangely after you 'ls' Ah, I see. I guess that's OK.
Daniel Cheng
Comment 13 2012-01-25 21:02:19 PST
Comment on attachment 124008 [details] Patch Clearing flags on attachment: 124008 Committed r105962: <http://trac.webkit.org/changeset/105962>
Daniel Cheng
Comment 14 2012-01-25 21:02:23 PST
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.