[webkit-reviews] review granted: [Bug 176770] Refactor WebContentReader out of EditorMac and EditorIOS : [Attachment 320530] Refactoring

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 176770: Refactor WebContentReader out of EditorMac and EditorIOS
https://bugs.webkit.org/show_bug.cgi?id=176770

Attachment 320530: Refactoring

https://bugs.webkit.org/attachment.cgi?id=320530&action=review




--- Comment #2 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 320530
  --> https://bugs.webkit.org/attachment.cgi?id=320530
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.