Bug 52093 – Paste and drag and drop use different code paths to interact with the pasteboard

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Enrica Casucci]

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.

[Enrica Casucci]

Created attachment 78300 [details]
Patch

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 78300 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7330463

[Early Warning System Bot]

Attachment 78300 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7448026

[WebKit Review Bot]

Attachment 78300 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7392038

[WebKit Review Bot]

Attachment 78300 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7427034

[Tony Chang]

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)?

[Enrica Casucci]

> 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

[Enrica Casucci]

Created attachment 78458 [details]
Patch2

Fixed style issues and build breaks in other platforms.

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 78458 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7456112

[Enrica Casucci]

Created attachment 78460 [details]
Patch3

Fixing last style issue and gtk build.

[WebKit Review Bot]

Attachment 78460 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7374123

[Enrica Casucci]

Created attachment 78471 [details]
Patch4

One more attempt at fixing gtk build.

[WebKit Review Bot]

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.

[Enrica Casucci]

Created attachment 78536 [details]
patch5

Fixed also the efl build.

[WebKit Review Bot]

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.

[Enrica Casucci]

Created attachment 78540 [details]
Patch6

One more time...

[WebKit Review Bot]

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.

[Alexey Proskuryakov]

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
> [phone]  Enrica Casucci  <[e-mail]>

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*

[Enrica Casucci]

(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.

[Alexey Proskuryakov]

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

[Enrica Casucci]

(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.

[Enrica Casucci]

http://trac.webkit.org/changeset/75523

[Patrick R. Gansterer]

(In reply to comment #24)
> http://trac.webkit.org/changeset/75523

Broke WinCE <http://build.webkit.org/builders/WinCE%20Release%20%28Build%29/builds/432>
Fixed with http://trac.webkit.org/changeset/75549