Bug 52093 - Paste and drag and drop use different code paths to interact with the pasteboard
Summary: Paste and drag and drop use different code paths to interact with the pasteboard
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Enrica Casucci
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-07 17:20 PST by Enrica Casucci
Modified: 2011-01-11 13:54 PST (History)
7 users (show)

See Also:


Attachments
Patch (114.39 KB, patch)
2011-01-07 19:14 PST, Enrica Casucci
tony: review-
Details | Formatted Diff | Diff
Patch2 (117.36 KB, patch)
2011-01-10 15:18 PST, Enrica Casucci
no flags Details | Formatted Diff | Diff
Patch3 (117.51 KB, patch)
2011-01-10 15:31 PST, Enrica Casucci
no flags Details | Formatted Diff | Diff
Patch4 (131.02 KB, patch)
2011-01-10 16:49 PST, Enrica Casucci
no flags Details | Formatted Diff | Diff
patch5 (132.60 KB, patch)
2011-01-11 08:54 PST, Enrica Casucci
no flags Details | Formatted Diff | Diff
Patch6 (133.04 KB, patch)
2011-01-11 09:04 PST, Enrica Casucci
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Enrica Casucci 2011-01-07 17:20:00 PST
The differences in behavior are visible using the test in LayoutTests/editing/pasteboard/drag-image-to-contenteditable-in-iframe.html and following the steps below:
1. launch the test on Safari on Mac
2. Drag the image inside the editable iframe. The missing image is displayed.
3. Now select the original image and copy it (cmd-c)
4. Paste it inside the editable iframe. The image is displayed correctly.
Comment 1 Enrica Casucci 2011-01-07 19:14:06 PST
Created attachment 78300 [details]
Patch
Comment 2 WebKit Review Bot 2011-01-07 19:16:18 PST
Attachment 78300 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/mac/editing/pasteboard/drag-image-to-contenteditable-in-iframe-expected.checksum', u'LayoutTests/platform/mac/editing/pasteboard/drag-image-to-contenteditable-in-iframe-expected.png', u'LayoutTests/platform/mac/editing/pasteboard/drag-image-to-contenteditable-in-iframe-expected.txt', u'WebCore/ChangeLog', u'WebCore/WebCore.exp.in', u'WebCore/loader/EmptyClients.h', u'WebCore/page/DragController.cpp', u'WebCore/page/EditorClient.h', u'WebCore/page/mac/DragControllerMac.mm', u'WebCore/page/win/DragControllerWin.cpp', u'WebCore/platform/DragData.h', u'WebCore/platform/Pasteboard.h', u'WebCore/platform/android/DragDataAndroid.cpp', u'WebCore/platform/chromium/DragDataChromium.cpp', u'WebCore/platform/gtk/DragDataGtk.cpp', u'WebCore/platform/haiku/DragDataHaiku.cpp', u'WebCore/platform/mac/DragDataMac.mm', u'WebCore/platform/mac/PasteboardMac.mm', u'WebCore/platform/qt/DragDataQt.cpp', u'WebCore/platform/win/DragDataWin.cpp', u'WebCore/platform/wince/DragDataWinCE.cpp', u'WebCore/platform/wx/DragDataWx.cpp', u'WebKit/ChangeLog', u'WebKit/WebKit.xcodeproj/project.pbxproj', u'WebKit/mac/ChangeLog', u'WebKit/mac/WebCoreSupport/WebEditorClient.h', u'WebKit/mac/WebCoreSupport/WebEditorClient.mm', u'WebKit/mac/WebCoreSupport/WebPasteboardHelper.h', u'WebKit/mac/WebCoreSupport/WebPasteboardHelper.mm', u'WebKit/mac/WebView/WebHTMLView.mm', u'WebKit/mac/WebView/WebView.mm', u'WebKit2/ChangeLog', u'WebKit2/WebProcess/WebCoreSupport/WebEditorClient.h', u'WebKit2/WebProcess/WebCoreSupport/mac/WebEditorClientMac.mm']" exit_code: 1
WebCore/page/win/DragControllerWin.cpp:50:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/platform/DragData.h:70:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/platform/DragData.h:85:  The parameter name "frame" adds no information, so it should be removed.  [readability/parameter_name] [5]
WebCore/platform/DragData.h:88:  The parameter name "frame" adds no information, so it should be removed.  [readability/parameter_name] [5]
WebCore/platform/DragData.h:89:  The parameter name "frame" adds no information, so it should be removed.  [readability/parameter_name] [5]
WebCore/platform/DragData.h:92:  The parameter name "frame" adds no information, so it should be removed.  [readability/parameter_name] [5]
WebCore/platform/win/DragDataWin.cpp:111:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/platform/Pasteboard.h:126:  The parameter name "frame" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 8 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 WebKit Review Bot 2011-01-07 19:27:22 PST
Attachment 78300 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7330463
Comment 4 Early Warning System Bot 2011-01-07 19:28:54 PST
Attachment 78300 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7448026
Comment 5 WebKit Review Bot 2011-01-07 19:48:28 PST
Attachment 78300 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7392038
Comment 6 WebKit Review Bot 2011-01-07 23:21:59 PST
Attachment 78300 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7427034
Comment 7 Tony Chang 2011-01-10 10:18:46 PST
Comment on attachment 78300 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78300&action=review

It seems unfortunate that we have to pass around a Frame* on all platforms because userVisibleString and canonicalizeURL* are in WebKit/mac.  How much work is it to move this code into WebCore?

r- just for style and compilation errors.

> WebCore/platform/mac/PasteboardMac.mm:481
> +String Pasteboard::asURL(Frame* frame)
> +{
> +    return [getBestURL(frame) absoluteString];
> +}

Nit: I'm not sure having a helper method provides much benefit here since it's only called from here.

> WebKit/mac/WebView/WebHTMLView.mm:-1721
> -// keep in sync with WebPasteboardHelper::insertablePasteboardTypes

Should this be updated to say "keep in sync with WebCore::insertablePasteboardTypes"?  Alternately, would adding this to EditorClient make it possible to share this code (maybe there's a better place for it)?
Comment 8 Enrica Casucci 2011-01-10 11:40:41 PST
> It seems unfortunate that we have to pass around a Frame* on all platforms because userVisibleString and canonicalizeURL* are in WebKit/mac.  How much work is it to move this code into WebCore?

I agree, it is just a stop gap solution. I want to move the rest of the code in a separate patch, as stated in the comment.
> r- just for style and compilation errors.

I will fix them all. Thanks for the review.

> 
> > WebCore/platform/mac/PasteboardMac.mm:481
> > +String Pasteboard::asURL(Frame* frame)
> > +{
> > +    return [getBestURL(frame) absoluteString];
> > +}
> 
> Nit: I'm not sure having a helper method provides much benefit here since it's only called from here.

I agree, I thought I was going to need it somewhere else, but I'll remove it
> 
> > WebKit/mac/WebView/WebHTMLView.mm:-1721
> > -// keep in sync with WebPasteboardHelper::insertablePasteboardTypes
> 
> Should this be updated to say "keep in sync with WebCore::insertablePasteboardTypes"?  Alternately, would adding this to EditorClient make it possible to share this code (maybe there's a better place for it)?
I removed the comment because there will be no need to keep anything in sync in the future, because in another patch I plan to remove all the pasteboard related code that is still in WebHTMLView.mm
Comment 9 Enrica Casucci 2011-01-10 15:18:54 PST
Created attachment 78458 [details]
Patch2

Fixed style issues and build breaks in other platforms.
Comment 10 WebKit Review Bot 2011-01-10 15:21:05 PST
Attachment 78458 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/platform/Pasteboard.h:126:  The parameter name "frame" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 WebKit Review Bot 2011-01-10 15:23:30 PST
Attachment 78458 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7456112
Comment 12 Enrica Casucci 2011-01-10 15:31:29 PST
Created attachment 78460 [details]
Patch3

Fixing last style issue and gtk build.
Comment 13 WebKit Review Bot 2011-01-10 15:37:19 PST
Attachment 78460 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7374123
Comment 14 Enrica Casucci 2011-01-10 16:49:57 PST
Created attachment 78471 [details]
Patch4

One more attempt at fixing gtk build.
Comment 15 WebKit Review Bot 2011-01-10 16:52:41 PST
Attachment 78471 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1
Source/WebCore/platform/DragData.h:80:  The parameter name "operation" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Enrica Casucci 2011-01-11 08:54:35 PST
Created attachment 78536 [details]
patch5

Fixed also the efl build.
Comment 17 WebKit Review Bot 2011-01-11 08:56:00 PST
Attachment 78536 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1
Source/WebCore/platform/DragData.h:80:  The parameter name "operation" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Enrica Casucci 2011-01-11 09:04:54 PST
Created attachment 78540 [details]
Patch6

One more time...
Comment 19 WebKit Review Bot 2011-01-11 09:07:48 PST
Attachment 78540 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1
Source/WebCore/platform/DragData.h:80:  The parameter name "operation" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 Alexey Proskuryakov 2011-01-11 10:45:43 PST
Comment on attachment 78540 [details]
Patch6

View in context: https://bugs.webkit.org/attachment.cgi?id=78540&action=review

This seems like moving in the right direction - hopefully, passing the frame is temporary.

> ChangeLog:17
> +        * loader/EmptyClients.h: Added two Mac only methods to call into WebKit to use functionality
> +        that is in NSURLExtras.
> +        (WebCore::EmptyEditorClient::canonicalizeURL):
> +        (WebCore::EmptyEditorClient::canonicalizeURLString):

This doesn't sound like the kind of functionality we should call out to WebKit for. Would there be any known breakage if KURL canonicalization was used?

Even if we need NSURL canonicalization, it would probably be better and easier to move the implementation from WebNSURLExtras.mm to KURLMac.mm instead of adding temporary code to EditorClient.

> ChangeLog:88
> +2011-01-10  Enrica Casucci  <enrica@apple.com>

ChangeLogs are quite messed up (looks like there are several copies in root ChangeLog, and then in Sources/WebCore/ChangeLog?)

> Source/WebCore/page/qt/DragControllerQt.cpp:54
> -    if (dragData->containsURL())
> +    if (dragData->containsURL(0))

Is this going away soon? These zeroes are infuriating.

> Source/WebCore/platform/DragData.h:77
> +class DragData {
>      public:
>          enum FilenameConversionPolicy { DoNotConvertFilenames, ConvertFilenames };

Style is wrong here - I think that you should just keep the whole class indented, rather than indent its content by 8 spaces.

> Source/WebCore/platform/DragData.h:104
> -} //namespace WebCore
> +} // namespace WebCore

I generally suggest omitting these comments. It's more reliable to double-click on a closing brace to really check what it corresponds to - and generally, one just ignores this boilerplate code, so comments are visual noise.

> Source/WebCore/platform/Pasteboard.h:127
> +    NSURL *getBestURL(Frame *);

Style: should be "Frame*".

> Source/WebCore/platform/mac/DragDataMac.mm:122
> +bool DragData::containsURL(Frame *frame, FilenameConversionPolicy filenamePolicy) const

Frame*

> Source/WebCore/platform/mac/PasteboardMac.mm:440
> +NSURL *Pasteboard::getBestURL(Frame *frame)

Frame*

> Source/WebCore/platform/win/DragDataWin.cpp:41
> +bool DragData::containsURL(Frame *, FilenameConversionPolicy filenamePolicy) const

Frame*

> Source/WebCore/platform/win/DragDataWin.cpp:50
> +String DragData::asURL(Frame *, FilenameConversionPolicy filenamePolicy, String* title) const

Frame*

> Source/WebCore/platform/win/DragDataWin.cpp:93
> +String DragData::asPlainText(Frame *) const

Frame*
Comment 21 Enrica Casucci 2011-01-11 11:21:15 PST
(In reply to comment #20)
> This seems like moving in the right direction - hopefully, passing the frame is temporary.
yes.
 
> > ChangeLog:17
> > +        * loader/EmptyClients.h: Added two Mac only methods to call into WebKit to use functionality
> > +        that is in NSURLExtras.
> > +        (WebCore::EmptyEditorClient::canonicalizeURL):
> > +        (WebCore::EmptyEditorClient::canonicalizeURLString):
> 
> This doesn't sound like the kind of functionality we should call out to WebKit for. Would there be any known breakage if KURL canonicalization was used?
> 
> Even if we need NSURL canonicalization, it would probably be better and easier to move the implementation from WebNSURLExtras.mm to KURLMac.mm instead of adding temporary code to EditorClient.

I will do everything with that patch.
> ChangeLogs are quite messed up (looks like there are several copies in root ChangeLog, and then in Sources/WebCore/ChangeLog?)

I'll fix it. 
> Is this going away soon? These zeroes are infuriating.
With the patch that moves the NSUrlExtras stuff.
 
> > Source/WebCore/platform/DragData.h:77
> > +class DragData {
> >      public:
> >          enum FilenameConversionPolicy { DoNotConvertFilenames, ConvertFilenames };
> 
> Style is wrong here - I think that you should just keep the whole class indented, rather than indent its content by 8 spaces.
I did it because the style checked complained.

> > Source/WebCore/platform/DragData.h:104
> > -} //namespace WebCore
> > +} // namespace WebCore
> 
> I generally suggest omitting these comments. It's more reliable to double-click on a closing brace to really check what it corresponds to - and generally, one just ignores this boilerplate code, so comments are visual noise.

Ok.
I've addressed all the "Frame *" to "Frame*" changes.
Comment 22 Alexey Proskuryakov 2011-01-11 11:31:19 PST
> I did it because the style checked complained.

Understood - however the result is worse than before. Indented classes is our earlier coding style, but double indentation for class insides only would be unique. You can outdent the whole class if you want to fix the style with this patch.
Comment 23 Enrica Casucci 2011-01-11 11:38:53 PST
(In reply to comment #22)
> > I did it because the style checked complained.
> 
> Understood - however the result is worse than before. Indented classes is our earlier coding style, but double indentation for class insides only would be unique. You can outdent the whole class if you want to fix the style with this patch.

Sorry I missed the 8 characters indentation when fixing the style. I see what you mean now, and I've fixed it.
Comment 24 Enrica Casucci 2011-01-11 11:48:10 PST
http://trac.webkit.org/changeset/75523