Bug 76996 – [chromium] Fix ClipboardChromium::validateFilename to actually operate on extensions

• Component: New Bugs
• Status: RESOLVED FIXED

[Daniel Cheng]

[chromium] Fix ClipboardChromium::validateFilename to actually operate on extensions

[Daniel Cheng]

Created attachment 123905 [details]
Patch

[Daniel Cheng]

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 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]

Created attachment 123994 [details]
Patch

[Daniel Cheng]

Comment on attachment 123994 [details]
Patch

Need to update ChangeLogs. Please ignore.

[Daniel Cheng]

Created attachment 123995 [details]
Patch

[Tony Chang]

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]

Created attachment 124008 [details]
Patch

[Daniel Cheng]

(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 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 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 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 on attachment 124008 [details]
Patch

Clearing flags on attachment: 124008

Committed r105962: <http://trac.webkit.org/changeset/105962>

[Daniel Cheng]

All reviewed patches have been landed.  Closing bug.