Bug 38826 - Don't convert filenames to URLs in edit drags.
Summary: Don't convert filenames to URLs in edit drags.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-09 17:49 PDT by Daniel Cheng
Modified: 2010-06-10 07:31 PDT (History)
8 users (show)

See Also:


Attachments
Patch (30.36 KB, patch)
2010-05-20 21:02 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (24.69 KB, patch)
2010-06-04 21:43 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (24.69 KB, patch)
2010-06-05 13:26 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (24.91 KB, patch)
2010-06-05 13:40 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (26.23 KB, patch)
2010-06-07 17:06 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (27.84 KB, patch)
2010-06-08 17:30 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (26.28 KB, patch)
2010-06-08 17:38 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (26.26 KB, patch)
2010-06-08 18:48 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (26.11 KB, patch)
2010-06-09 13:39 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Cheng 2010-05-09 17:49:00 PDT
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.
Comment 1 Daniel Cheng 2010-05-20 21:02:14 PDT
Created attachment 56666 [details]
Patch
Comment 2 Daniel Cheng 2010-05-20 21:10:39 PDT
Darin, do you mind reviewing this patch?
Comment 3 Eric Seidel (no email) 2010-05-28 15:53:09 PDT
Oliver has also worked on drag code in the past and may be interested in this change.
Comment 4 Simon Fraser (smfr) 2010-05-28 15:55:00 PDT
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.
Comment 5 Jian Li 2010-05-28 16:18:16 PDT
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.
Comment 6 Daniel Cheng 2010-06-04 21:43:57 PDT
Created attachment 57959 [details]
Patch
Comment 7 Early Warning System Bot 2010-06-04 21:48:54 PDT
Attachment 57959 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3090061
Comment 8 WebKit Review Bot 2010-06-05 04:07:47 PDT
Attachment 57959 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3078084
Comment 9 Daniel Cheng 2010-06-05 13:26:24 PDT
Created attachment 57969 [details]
Patch
Comment 10 Early Warning System Bot 2010-06-05 13:33:15 PDT
Attachment 57969 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3148057
Comment 11 Daniel Cheng 2010-06-05 13:40:09 PDT
Created attachment 57970 [details]
Patch
Comment 12 WebKit Review Bot 2010-06-05 14:53:06 PDT
Attachment 57969 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3120078
Comment 13 WebKit Review Bot 2010-06-05 15:17:25 PDT
Attachment 57970 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3113093
Comment 14 Daniel Cheng 2010-06-07 17:06:46 PDT
Created attachment 58096 [details]
Patch
Comment 15 WebKit Review Bot 2010-06-07 21:10:39 PDT
Attachment 58096 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3237011
Comment 16 Daniel Cheng 2010-06-08 17:30:52 PDT
Created attachment 58200 [details]
Patch
Comment 17 Daniel Cheng 2010-06-08 17:38:37 PDT
Created attachment 58202 [details]
Patch
Comment 18 Daniel Cheng 2010-06-08 17:40:00 PDT
(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.
Comment 19 Jian Li 2010-06-08 18:26:51 PDT
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.
Comment 20 Daniel Cheng 2010-06-08 18:48:36 PDT
Created attachment 58205 [details]
Patch
Comment 21 Daniel Cheng 2010-06-08 23:30:39 PDT
(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.
Comment 22 Jian Li 2010-06-09 13:24:43 PDT
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.
Comment 23 Daniel Cheng 2010-06-09 13:39:54 PDT
Created attachment 58289 [details]
Patch
Comment 24 Daniel Cheng 2010-06-09 13:41:29 PDT
(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.
Comment 25 Jian Li 2010-06-09 15:04:08 PDT
Comment on attachment 58289 [details]
Patch

r=me
Comment 26 WebKit Commit Bot 2010-06-10 07:31:45 PDT
Comment on attachment 58289 [details]
Patch

Clearing flags on attachment: 58289

Committed r60957: <http://trac.webkit.org/changeset/60957>
Comment 27 WebKit Commit Bot 2010-06-10 07:31:54 PDT
All reviewed patches have been landed.  Closing bug.