Bug 176770

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>