Bug 176770 – Refactor WebContentReader out of EditorMac and EditorIOS

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 320530 [details]
Refactoring

[Sam Weinig]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 320608 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 320608 [details]
Patch for landing

Wait for EWS.

[Ryosuke Niwa]

Created attachment 320616 [details]
Patch for landing

[Ryosuke Niwa]

Created attachment 320619 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 320619 [details]
Patch for landing

Wait for EWS.

[Build Bot]

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

[Build Bot]

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

[Ryosuke Niwa]

Created attachment 320622 [details]
Fixed the tests

[Ryosuke Niwa]

Apparently, I was inadvertently reverting my change in https://trac.webkit.org/changeset/221944 LOL.

[Ryosuke Niwa]

Comment on attachment 320622 [details]
Fixed the tests

Clearing flags on attachment: 320622

Committed r221960: <http://trac.webkit.org/changeset/221960>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34694481>