Summary: | Don't convert filenames to URLs in edit drags. | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Cheng <dcheng> | ||||||||||||||||||||
Component: | HTML Editing | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | ap, commit-queue, darin, eric, jianli, oliver, webkit-ews, webkit.review.bot | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||||||
Attachments: |
|
Description
Daniel Cheng
2010-05-09 17:49:00 PDT
Created attachment 56666 [details]
Patch
Darin, do you mind reviewing this patch? Oliver has also worked on drag code in the past and may be interested in this change. 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. 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. Created attachment 57959 [details]
Patch
Attachment 57959 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/3090061 Attachment 57959 [details] did not build on win: Build output: http://webkit-commit-queue.appspot.com/results/3078084 Created attachment 57969 [details]
Patch
Attachment 57969 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/3148057 Created attachment 57970 [details]
Patch
Attachment 57969 [details] did not build on win: Build output: http://webkit-commit-queue.appspot.com/results/3120078 Attachment 57970 [details] did not build on win: Build output: http://webkit-commit-queue.appspot.com/results/3113093 Created attachment 58096 [details]
Patch
Attachment 58096 [details] did not build on win: Build output: http://webkit-commit-queue.appspot.com/results/3237011 Created attachment 58200 [details]
Patch
Created attachment 58202 [details]
Patch
(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 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.
Created attachment 58205 [details]
Patch
(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 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. Created attachment 58289 [details]
Patch
(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 on attachment 58289 [details]
Patch
r=me
Comment on attachment 58289 [details] Patch Clearing flags on attachment: 58289 Committed r60957: <http://trac.webkit.org/changeset/60957> All reviewed patches have been landed. Closing bug. |