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.
Created attachment 78300 [details] Patch
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.
Attachment 78300 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7330463
Attachment 78300 [details] did not build on qt: Build output: http://queues.webkit.org/results/7448026
Attachment 78300 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7392038
Attachment 78300 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7427034
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)?
> 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
Created attachment 78458 [details] Patch2 Fixed style issues and build breaks in other platforms.
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.
Attachment 78458 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7456112
Created attachment 78460 [details] Patch3 Fixing last style issue and gtk build.
Attachment 78460 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7374123
Created attachment 78471 [details] Patch4 One more attempt at fixing gtk build.
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.
Created attachment 78536 [details] patch5 Fixed also the efl build.
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.
Created attachment 78540 [details] Patch6 One more time...
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 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*
(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.
> 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.
(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.
http://trac.webkit.org/changeset/75523
(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