WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(13.07 KB, patch)
2012-01-25 13:03 PST
,
Daniel Cheng
no flags
Details
Formatted Diff
Diff
Patch
(13.82 KB, patch)
2012-01-25 13:12 PST
,
Daniel Cheng
no flags
Details
Formatted Diff
Diff
Patch
(19.92 KB, patch)
2012-01-25 14:18 PST
,
Daniel Cheng
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Daniel Cheng
Comment 1
2012-01-25 02:33:59 PST
Created
attachment 123905
[details]
Patch
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
Created
attachment 123994
[details]
Patch
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
Created
attachment 123995
[details]
Patch
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
Created
attachment 124008
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug