In order to make DataTransfer support text/html properly, we need to use WebContentReader object, which is currently defined in EditorMac.mm and EditorIOS.mm Extract these into separate files so that we can use them in DataTransfer.
Created attachment 320530 [details] Refactoring
Comment on attachment 320530 [details] Refactoring View in context: https://bugs.webkit.org/attachment.cgi?id=320530&action=review > Source/WebCore/editing/WebContentReader.h:38 > + Frame& frame; This really feels like it should have a Document& not a frame, given how many operations are going to require a document. > Source/WebCore/editing/WebContentReader.h:55 > +#if PLATFORM(IOS) > + void addFragment(RefPtr<DocumentFragment>&&); > +#endif Given this doesn't use any iOS specific types, it is surprising that this is #if PLATFORM(IOS). What is the reason for making this iOS only? > Source/WebCore/editing/WebContentReader.h:66 > +#if !(PLATFORM(GTK) || PLATFORM(WIN)) > + bool readWebArchive(SharedBuffer*) override; > + bool readFilenames(const Vector<String>&) override; > + bool readHTML(const String&) override; > + bool readRTFD(SharedBuffer&) override; > + bool readRTF(SharedBuffer&) override; > + bool readImage(Ref<SharedBuffer>&&, const String& type) override; > + bool readURL(const URL&, const String& title) override; > +#endif Is it really necessary for these to #ifdef'd out? Usually we just add stubs for platforms that don't support something like this. > Source/WebCore/editing/WebContentReader.h:76 > +struct FragmentAndResources { > + RefPtr<DocumentFragment> fragment; > + Vector<Ref<ArchiveResource>> resources; > +}; > + > +RefPtr<DocumentFragment> createFragmentForImageResourceAndAddResource(Frame&, RefPtr<ArchiveResource>&&); Again, these feel platform agnostic. > Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm:51 > +#if PLATFORM(IOS) > +SOFT_LINK_PRIVATE_FRAMEWORK(WebKitLegacy) > +#endif > + > +#if PLATFORM(MAC) > +SOFT_LINK_FRAMEWORK_IN_UMBRELLA(WebKit, WebKitLegacy) > +#endif > + > +// FIXME: Get rid of this and change NSAttributedString conversion so it doesn't use WebKitLegacy (cf. rdar://problem/30597352). > +SOFT_LINK(WebKitLegacy, _WebCreateFragment, void, (WebCore::Document& document, NSAttributedString *string, WebCore::FragmentAndResources& result), (document, string, result)) > + > +#if PLATFORM(COCOA) 😭 > Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm:55 > +RefPtr<DocumentFragment> createFragmentForImageResourceAndAddResource(Frame& frame, RefPtr<ArchiveResource>&& resource) It looks like all the callers pass a non-null ArchiveResource, so this should probably take a Ref<ArchiveResource> and then also return a Ref<DocumentFragment> > Source/WebCore/editing/mac/WebContentReaderMac.mm:52 > +// Defined in EditorCocoa.mm > +RefPtr<DocumentFragment> createFragmentForImageResourceAndAddResource(Frame&, RefPtr<ArchiveResource>&&); > +RefPtr<DocumentFragment> createFragmentAndAddResources(Frame&, NSAttributedString *); I don't think you need to forward declare these again. They are already declared in the header. Also, the comment is wrong.
Comment on attachment 320530 [details] Refactoring View in context: https://bugs.webkit.org/attachment.cgi?id=320530&action=review >> Source/WebCore/editing/WebContentReader.h:38 >> + Frame& frame; > > This really feels like it should have a Document& not a frame, given how many operations are going to require a document. We do access FrameLoader & Editor (e.g. readWebArchive) so we need both Document & Frame. I think keeping a reference to Frame probably makes sense at least for now. >> Source/WebCore/editing/WebContentReader.h:55 >> +#endif > > Given this doesn't use any iOS specific types, it is surprising that this is #if PLATFORM(IOS). What is the reason for making this iOS only? It's only used in iOS code. The only reason we can't make this a static local function is EditorIOS calls it. Not sure if it's better to have a function not used in all other ports or hide it behind a build flag. I hid it behind a build flag for now since I'd have to add WebContentReader.cpp to have it everywhere. >> Source/WebCore/editing/WebContentReader.h:66 >> +#endif > > Is it really necessary for these to #ifdef'd out? Usually we just add stubs for platforms that don't support something like this. I tend to agree. One weird thing about this class is Pasteboard.h has defines PasteboardWebContentReader and uses the same if-def. I think one simple approach would be to add empty implementations in that abstract class, and then only override these on platforms where they're defined. >> Source/WebCore/editing/WebContentReader.h:76 >> +RefPtr<DocumentFragment> createFragmentForImageResourceAndAddResource(Frame&, RefPtr<ArchiveResource>&&); > > Again, these feel platform agnostic. Will move this to markup.cpp >> Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm:51 >> +#if PLATFORM(COCOA) > > 😭 I guess we don't need this if since it's only used in Cocoa. >> Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm:55 >> +RefPtr<DocumentFragment> createFragmentForImageResourceAndAddResource(Frame& frame, RefPtr<ArchiveResource>&& resource) > > It looks like all the callers pass a non-null ArchiveResource, so this should probably take a Ref<ArchiveResource> and then also return a Ref<DocumentFragment> Will change. >> Source/WebCore/editing/mac/WebContentReaderMac.mm:52 >> +RefPtr<DocumentFragment> createFragmentAndAddResources(Frame&, NSAttributedString *); > > I don't think you need to forward declare these again. They are already declared in the header. Also, the comment is wrong. Oops, this is a left over from earlier cleanups. Will remove.
Created attachment 320608 [details] Patch for landing
Comment on attachment 320608 [details] Patch for landing Wait for EWS.
Created attachment 320616 [details] Patch for landing
Created attachment 320619 [details] Patch for landing
Comment on attachment 320619 [details] Patch for landing Wait for EWS.
Comment on attachment 320619 [details] Patch for landing Attachment 320619 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4530986 New failing tests: editing/pasteboard/drag-files-to-editable-element-as-URLs.html
Created attachment 320621 [details] Archive of layout-test-results from ews100 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 320622 [details] Fixed the tests
Apparently, I was inadvertently reverting my change in https://trac.webkit.org/changeset/221944 LOL.
Comment on attachment 320622 [details] Fixed the tests Clearing flags on attachment: 320622 Committed r221960: <http://trac.webkit.org/changeset/221960>
All reviewed patches have been landed. Closing bug.
<rdar://problem/34694481>