Bug 38826 – Don't convert filenames to URLs in edit drags.

• Component: HTML Editing
• Status: RESOLVED FIXED

[Daniel Cheng]

This is on Chromium, but I believe the same will happen in Safari.

Go to any web page, set the page to be contentEditable, and then drag and drop a file. A link is created with an absolute file:// path. This is because DragController calls DragData::asURL.

[Daniel Cheng]

Created attachment 56666 [details]
Patch

[Daniel Cheng]

Darin, do you mind reviewing this patch?

[Eric Seidel (no email)]

Oliver has also worked on drag code in the past and may be interested in this change.

[Simon Fraser (smfr)]

Comment on attachment 56666 [details]
Patch

> +            String url = dragData->asURL(false, &title);

Boolean parameters suck. It's hard, at the call site, to know what the true/false means. Either use an enum, or have two separate methods.

[Jian Li]

LayoutTests/platform/qt/Skipped:5096
 +  # Filenames aren't filtered out from edit drags yet, see https://bugs.wekit.org/show_bug.cgi?id=38826
I think this test should still be marked as skipped even after filename filtering is supported because qt does not support beginDragWithFiles yet.

WebCore/platform/DragData.h:95
 +          bool containsURL(bool convertFilenames = false) const;
It will be simpler to make the default value to support the majority cases which is true. Then you do not need to change a lot of codes above.
In addition, I think convertFilenames is a little bit confusing since indeed we mean not to expose file url, maybe something like doNotExposeFilename and make the default value as false. Or even better, as Simon pointed out, switch to using enum.

WebCore/platform/DragData.h:98
 +          String asURL(bool convertFilenames = false, String* title = 0) const;
ditto.

WebCore/platform/android/DragDataAndroid.cpp:71
 +  bool DragData::containsURL(bool convertFilenames) const
Please make sure this can get compiled correctly since some platforms might complain about unused parameter.

[Daniel Cheng]

Created attachment 57959 [details]
Patch

[Early Warning System Bot]

Attachment 57959 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3090061

[WebKit Review Bot]

Attachment 57959 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3078084

[Daniel Cheng]

Created attachment 57969 [details]
Patch

[Early Warning System Bot]

Attachment 57969 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3148057

[Daniel Cheng]

Created attachment 57970 [details]
Patch

[WebKit Review Bot]

Attachment 57969 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3120078

[WebKit Review Bot]

Attachment 57970 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3113093

[Daniel Cheng]

Created attachment 58096 [details]
Patch

[WebKit Review Bot]

Attachment 58096 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3237011

[Daniel Cheng]

Created attachment 58200 [details]
Patch

[Daniel Cheng]

Created attachment 58202 [details]
Patch

[Daniel Cheng]

(In reply to comment #5)
> LayoutTests/platform/qt/Skipped:5096
>  +  # Filenames aren't filtered out from edit drags yet, see https://bugs.wekit.org/show_bug.cgi?id=38826
> I think this test should still be marked as skipped even after filename filtering is supported because qt does not support beginDragWithFiles yet.
> 

Done.

> WebCore/platform/DragData.h:95
>  +          bool containsURL(bool convertFilenames = false) const;
> It will be simpler to make the default value to support the majority cases which is true. Then you do not need to change a lot of codes above.
> In addition, I think convertFilenames is a little bit confusing since indeed we mean not to expose file url, maybe something like doNotExposeFilename and make the default value as false. Or even better, as Simon pointed out, switch to using enum.
> 
> WebCore/platform/DragData.h:98
>  +          String asURL(bool convertFilenames = false, String* title = 0) const;
> ditto.
> 

Changed the default argument value, and also switched it to using an enum.

> WebCore/platform/android/DragDataAndroid.cpp:71
>  +  bool DragData::containsURL(bool convertFilenames) const
> Please make sure this can get compiled correctly since some platforms might complain about unused parameter.

There are other unused parameters already, so I think this should be OK.

[Jian Li]

Comment on attachment 58202 [details]
Patch

LayoutTests/ChangeLog:14
 +          (moveMouseToCenterOfElement):
No need to show these new methods in js.

WebCore/platform/DragData.h:81
 +          enum ShouldConvertFilenames { DoNotConvertFilenames, ConvertFilenames };
Might be better to name ShouldConvertFilenames as FilenamesConvertPolicy.

WebCore/platform/chromium/DragDataChromium.cpp:64
 +  String DragData::asURL(ShouldConvertFilenames convertFilenames, String* title) const
ShouldConvertFilenames convertFilenames => FilenamesConvertPolicy convertPolicy

WebCore/platform/android/DragDataAndroid.cpp:71
 +  bool DragData::containsURL(ShouldConvertFilenames convertFilenames) const
The argument name can be omitted. Please also change other occurrences.

[Daniel Cheng]

Created attachment 58205 [details]
Patch

[Daniel Cheng]

(In reply to comment #19)
> (From update of attachment 58202 [details])
> LayoutTests/ChangeLog:14
>  +          (moveMouseToCenterOfElement):
> No need to show these new methods in js.
> 

This is the output from prepare-ChangeLog. I felt best to leave it that way.

> WebCore/platform/DragData.h:81
>  +          enum ShouldConvertFilenames { DoNotConvertFilenames, ConvertFilenames };
> Might be better to name ShouldConvertFilenames as FilenamesConvertPolicy.
> 

Done.

> WebCore/platform/chromium/DragDataChromium.cpp:64
>  +  String DragData::asURL(ShouldConvertFilenames convertFilenames, String* title) const
> ShouldConvertFilenames convertFilenames => FilenamesConvertPolicy convertPolicy
> 

Done.

> WebCore/platform/android/DragDataAndroid.cpp:71
>  +  bool DragData::containsURL(ShouldConvertFilenames convertFilenames) const
> The argument name can be omitted. Please also change other occurrences.


This is actually necessary--it's used by WebCore/page/DragController.cpp.

[Jian Li]

Comment on attachment 58205 [details]
Patch

> > LayoutTests/ChangeLog:14
> >  +          (moveMouseToCenterOfElement):
> > No need to show these new methods in js.
> > 
> 
> This is the output from prepare-ChangeLog. I felt best to leave it that way.
Yes, this is output from prepare-ChangeLog. But many people choose not to add new methods from the new file.

LayoutTests/editing/pasteboard/script-tests/file-drag-to-editable.js:18
 +  function dragFilesOntoEditableArea(files) {
Please move '{' to the beginning of next line.

WebCore/platform/android/DragDataAndroid.cpp:71
 +  bool DragData::containsURL(FilenameConversionPolicy filenamePolicy) const
It is better to omit the argument name "filenamePolicy" since otherwise it might break other build for "unused parameter".

WebCore/platform/android/DragDataAndroid.cpp:76
 +  String DragData::asURL(FilenameConversionPolicy filenamePolicy, String*) const
ditto.

WebCore/platform/win/ClipboardUtilitiesWin.cpp:366
 +          // FIXME: originally, this logic was so dropping files on WebKit would insert the filename.
The comment "this logic was so dropping files on WebKit" seems to be hard to understand. Could you please rephrase it?

WebCore/platform/win/ClipboardUtilitiesWin.h:64
 +  String getURL(IDataObject*, DragData::FilenameConversionPolicy filenamePolicy, bool& success, String* title = 0);
The argument name filenamePolicy can be omitted per coding style guideline.

[Daniel Cheng]

Created attachment 58289 [details]
Patch

[Daniel Cheng]

(In reply to comment #22)
> (From update of attachment 58205 [details])
> > > LayoutTests/ChangeLog:14
> > >  +          (moveMouseToCenterOfElement):
> > > No need to show these new methods in js.
> > > 
> > 
> > This is the output from prepare-ChangeLog. I felt best to leave it that way.
> Yes, this is output from prepare-ChangeLog. But many people choose not to add new methods from the new file.
> 

Done.

> LayoutTests/editing/pasteboard/script-tests/file-drag-to-editable.js:18
>  +  function dragFilesOntoEditableArea(files) {
> Please move '{' to the beginning of next line.
> 

Done.

> WebCore/platform/android/DragDataAndroid.cpp:71
>  +  bool DragData::containsURL(FilenameConversionPolicy filenamePolicy) const
> It is better to omit the argument name "filenamePolicy" since otherwise it might break other build for "unused parameter".
> 

Done.

> WebCore/platform/android/DragDataAndroid.cpp:76
>  +  String DragData::asURL(FilenameConversionPolicy filenamePolicy, String*) const
> ditto.
> 

Done.

> WebCore/platform/win/ClipboardUtilitiesWin.cpp:366
>  +          // FIXME: originally, this logic was so dropping files on WebKit would insert the filename.
> The comment "this logic was so dropping files on WebKit" seems to be hard to understand. Could you please rephrase it?
> 

Done.

> WebCore/platform/win/ClipboardUtilitiesWin.h:64
>  +  String getURL(IDataObject*, DragData::FilenameConversionPolicy filenamePolicy, bool& success, String* title = 0);
> The argument name filenamePolicy can be omitted per coding style guideline.

Done.

[Jian Li]

Comment on attachment 58289 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 58289 [details]
Patch

Clearing flags on attachment: 58289

Committed r60957: <http://trac.webkit.org/changeset/60957>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.