[chromium] Fix ClipboardChromium::validateFilename to actually operate on extensions
Created attachment 123905 [details] Patch
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?
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).
Created attachment 123994 [details] Patch
Comment on attachment 123994 [details] Patch Need to update ChangeLogs. Please ignore.
Created attachment 123995 [details] Patch
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.
Created attachment 124008 [details] Patch
(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.
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?
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'
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.
Comment on attachment 124008 [details] Patch Clearing flags on attachment: 124008 Committed r105962: <http://trac.webkit.org/changeset/105962>
All reviewed patches have been landed. Closing bug.