It is a layering violation for DragData to depend on Clipboard, DocumentFragment, and Document.
Created attachment 236510 [details] Patch
Created attachment 236511 [details] Try to fix EFL and Win build
Created attachment 236519 [details] Try to fix EFL and win build
Comment on attachment 236519 [details] Try to fix EFL and win build View in context: https://bugs.webkit.org/attachment.cgi?id=236519&action=review > Source/WebCore/page/gtk/DragControllerGtk.cpp:83 > + if (!dragData.platformData()->hasMarkup() || !frame->document()) Since we are not null-checking the Frame here, can we pass it by reference? Or do we need to null check it?
(In reply to comment #4) > (From update of attachment 236519 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=236519&action=review > > > Source/WebCore/page/gtk/DragControllerGtk.cpp:83 > > + if (!dragData.platformData()->hasMarkup() || !frame->document()) > > Since we are not null-checking the Frame here, can we pass it by reference? Or do we need to null check it? We can indeed pass it by reference, the caller should ensure a valid a frame is passed, and currently the only caller does it: RefPtr<Frame> innerFrame = element->document().frame(); ASSERT(innerFrame);
Created attachment 236580 [details] Updated patch Pass the Frame by reference
Created attachment 237217 [details] Rebased patch Rebased to make it build on top of current git master
Comment on attachment 237217 [details] Rebased patch View in context: https://bugs.webkit.org/attachment.cgi?id=237217&action=review We should make sure we are adding the minimum includes here. > Source/WebCore/page/DragController.h:119 > + static PassRefPtr<DocumentFragment> documentFragmentFromDragData(DragData&, Frame&, Range&, bool allowPlainText, bool& chosePlainText); > + static PassRefPtr<DocumentFragment> createFragmentFromDragData(DragData&, Frame&, Range&, bool allowPlainText, bool& chosePlainText); The names of these two functions don’t make it at all clear why there are two of them and how they differ. I think the second one exists only to have a platform-dependent hook to do things differently on every platform. Long term, I want to refactor this code so that it’s shared and not platform-specific. I think it can be done. In the meantime, I suggest a name that is clearer. I’m also not sure that the first function needs to be a class member. Maybe it can just be a function with internal linkages inside the .cpp file and not in the header at all. > Source/WebCore/page/win/DragControllerWin.cpp:93 > + * Order is richest format first. On OSX this is: You just moved this code, but this is a Windows-specific file. Not sure why a list of OS X formats is relevant. Also, it’s “OS X”, not “OSX”. I suggest we just drop the comment entirely. > Source/WebCore/page/win/DragControllerWin.cpp:122 > + if (DragDataRef platformDragData = dragData.platformData()) { > + if (containsFilenames(platformDragData)) { > + if (PassRefPtr<DocumentFragment> fragment = fragmentFromFilenames(frame.document(), platformDragData)) > + return fragment; > + } > + > + if (containsHTML(platformDragData)) { > + if (PassRefPtr<DocumentFragment> fragment = fragmentFromHTML(frame.document(), platformDragData)) > + return fragment; > + } > + } else { > + if (containsFilenames(&dragData.dragDataMap())) { > + if (PassRefPtr<DocumentFragment> fragment = fragmentFromFilenames(frame.document(), &dragData.dragDataMap())) > + return fragment; > + } > + > + if (containsHTML(&dragData.dragDataMap())) { > + if (PassRefPtr<DocumentFragment> fragment = fragmentFromHTML(frame.document(), &dragData.dragDataMap())) > + return fragment; > + } > + } You just moved this code, but sure would be nice to factor this so we don’t have to repeat everything twice. > Source/WebCore/platform/DragData.h:95 > + bool containsURL(FilenameConversionPolicy filenamePolicy = ConvertFilenames) const; The argument name filenamePolicy doesn’t add anything and should be removed. > Source/WebCore/platform/DragData.h:98 > + String asURL(FilenameConversionPolicy filenamePolicy = ConvertFilenames, String* title = 0) const; The argument name filenamePolicy doesn’t add anything and should be removed. Should use nullptr rather than 0. > Source/WebCore/platform/mac/DragDataMac.mm:111 > + // FIXME: It's not clear this is 100% correct since we know -[NSURL URLWithString:] does not handle > + // all the same cases we handle well in the URL code for creating an NSURL. Where was this code moved from? It’s easy for us to rewrite this use WebCore::URL to make the NSURL.
(In reply to comment #8) > (From update of attachment 237217 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=237217&action=review Thanks for the review. > We should make sure we are adding the minimum includes here. I tried it for GTK+ port at least since it's the only port I can compile. > > Source/WebCore/page/DragController.h:119 > > + static PassRefPtr<DocumentFragment> documentFragmentFromDragData(DragData&, Frame&, Range&, bool allowPlainText, bool& chosePlainText); > > + static PassRefPtr<DocumentFragment> createFragmentFromDragData(DragData&, Frame&, Range&, bool allowPlainText, bool& chosePlainText); > > The names of these two functions don’t make it at all clear why there are two of them and how they differ. > > I think the second one exists only to have a platform-dependent hook to do things differently on every platform. Long term, I want to refactor this code so that it’s shared and not platform-specific. I think it can be done. In the meantime, I suggest a name that is clearer. We can create a Pasteboard for drag and drop with the given DragData, and use something like Editor::webContentFromPasteboard() implementing that for other platforms (it's mac specific now). But that will be easier once I land patch in bug #136802, so I'll leave just a FIXME here and will submit a new patch once this and #136802 are both fixed. > I’m also not sure that the first function needs to be a class member. Maybe it can just be a function with internal linkages inside the .cpp file and not in the header at all. Yes, I made it a member only because I needed to call the second one, that is implemented in port specific files. I could also make it public and pass the drag controller to the static function, but I thought it was better to keep them private. In any case, I'll remove the first one and make the second one static function again in a follow up patch, when moving the implementation to the editor. > > Source/WebCore/page/win/DragControllerWin.cpp:93 > > + * Order is richest format first. On OSX this is: > > You just moved this code, but this is a Windows-specific file. Not sure why a list of OS X formats is relevant. Also, it’s “OS X”, not “OSX”. > > I suggest we just drop the comment entirely. Ok. > > Source/WebCore/page/win/DragControllerWin.cpp:122 > > + if (DragDataRef platformDragData = dragData.platformData()) { > > + if (containsFilenames(platformDragData)) { > > + if (PassRefPtr<DocumentFragment> fragment = fragmentFromFilenames(frame.document(), platformDragData)) > > + return fragment; > > + } > > + > > + if (containsHTML(platformDragData)) { > > + if (PassRefPtr<DocumentFragment> fragment = fragmentFromHTML(frame.document(), platformDragData)) > > + return fragment; > > + } > > + } else { > > + if (containsFilenames(&dragData.dragDataMap())) { > > + if (PassRefPtr<DocumentFragment> fragment = fragmentFromFilenames(frame.document(), &dragData.dragDataMap())) > > + return fragment; > > + } > > + > > + if (containsHTML(&dragData.dragDataMap())) { > > + if (PassRefPtr<DocumentFragment> fragment = fragmentFromHTML(frame.document(), &dragData.dragDataMap())) > > + return fragment; > > + } > > + } > > You just moved this code, but sure would be nice to factor this so we don’t have to repeat everything twice. Agree. > > Source/WebCore/platform/DragData.h:95 > > + bool containsURL(FilenameConversionPolicy filenamePolicy = ConvertFilenames) const; > > The argument name filenamePolicy doesn’t add anything and should be removed. > > > Source/WebCore/platform/DragData.h:98 > > + String asURL(FilenameConversionPolicy filenamePolicy = ConvertFilenames, String* title = 0) const; > > The argument name filenamePolicy doesn’t add anything and should be removed. Should use nullptr rather than 0. > > > Source/WebCore/platform/mac/DragDataMac.mm:111 > > + // FIXME: It's not clear this is 100% correct since we know -[NSURL URLWithString:] does not handle > > + // all the same cases we handle well in the URL code for creating an NSURL. > > Where was this code moved from? It’s easy for us to rewrite this use WebCore::URL to make the NSURL. From Editor::plainTextFromPasteboard() in EditorMac.mm
Created attachment 238114 [details] Patch for landing Addressed review comments
(In reply to comment #9) > (In reply to comment #8) > > > Source/WebCore/page/DragController.h:119 > > > + static PassRefPtr<DocumentFragment> documentFragmentFromDragData(DragData&, Frame&, Range&, bool allowPlainText, bool& chosePlainText); > > > + static PassRefPtr<DocumentFragment> createFragmentFromDragData(DragData&, Frame&, Range&, bool allowPlainText, bool& chosePlainText); > > > > The names of these two functions don’t make it at all clear why there are two of them and how they differ. > > > > I think the second one exists only to have a platform-dependent hook to do things differently on every platform. Long term, I want to refactor this code so that it’s shared and not platform-specific. I think it can be done. In the meantime, I suggest a name that is clearer. > > We can create a Pasteboard for drag and drop with the given DragData, and use something like Editor::webContentFromPasteboard() implementing that for other platforms (it's mac specific now). But that will be easier once I land patch in bug #136802, so I'll leave just a FIXME here and will submit a new patch once this and #136802 are both fixed. See https://bugs.webkit.org/show_bug.cgi?id=136819
Created attachment 238158 [details] Try to fix windows build Ok, so it seems I can't use the ternary operator for the windows refactoring so I'll try my luck again using a template.
Created attachment 238181 [details] Try to fix windows build again
Committed r173686: <http://trac.webkit.org/changeset/173686>