Bug 176884 – WebContentReader::readWebArchive doesn't need to handle image MIME type

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

Right now, WebContentReader::readWebArchive for Mac supports having image MIME type but this doesn't seem be ever used in practice.
We have an explicit code path to handle image files/content in the pasteboard as well.

Remove this code so that iOS & Mac can share code.

[Ryosuke Niwa]

Created attachment 320713 [details]
Cleanup

[Ryosuke Niwa]

Created attachment 320722 [details]
Added missing includes

[Sam Weinig]

Comment on attachment 320722 [details]
Added missing includes

View in context: https://bugs.webkit.org/attachment.cgi?id=320722&action=review

> Source/WebCore/editing/WebContentReader.h:55
> +#if PLATFORM(COCOA)
>      void addFragment(RefPtr<DocumentFragment>&&);
>  #endif

I still think this could be available to all platforms. There really isn't anything cocoa specific about it.

> Source/WebCore/editing/WebContentReader.h:59
>      bool readWebArchive(SharedBuffer*) override;

Perhaps this should be in a #if ENABLE(LEGACY_WEB_ARCHIVE).

[Ryosuke Niwa]

Comment on attachment 320722 [details]
Added missing includes

View in context: https://bugs.webkit.org/attachment.cgi?id=320722&action=review

>> Source/WebCore/editing/WebContentReader.h:55
>>  #endif
> 
> I still think this could be available to all platforms. There really isn't anything cocoa specific about it.

Yeah, let's just do that.

>> Source/WebCore/editing/WebContentReader.h:59
>>      bool readWebArchive(SharedBuffer*) override;
> 
> Perhaps this should be in a #if ENABLE(LEGACY_WEB_ARCHIVE).

Perhaps. I think Darin's intention was implementing these functions on all platforms.
I'm gonna leave them alone for now.

[Ryosuke Niwa]

Created attachment 320736 [details]
Refactoring

[Ryosuke Niwa]

Created attachment 320742 [details]
Fixed API tests

[Sam Weinig]

Comment on attachment 320742 [details]
Fixed API tests

View in context: https://bugs.webkit.org/attachment.cgi?id=320742&action=review

> Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm:119
> +    const String& type = mainResource->mimeType();

auto?

> Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm:125
> +    if (DocumentLoader* loader = frame.loader().documentLoader())

auto?

> Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm:128
> +    String markupString = String::fromUTF8(mainResource->data().data(), mainResource->data().size());

auto?

> Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm:164
> +    addFragment(createFragmentFromText(context, [text precomposedStringWithCanonicalMapping]));

Should use dot notation since precomposedStringWithCanonicalMapping is a property.

> Source/WebCore/editing/ios/WebContentReaderIOS.mm:102
> -    auto newFragment = frame.document()->createDocumentFragment();
> +    Ref<DocumentFragment> newFragment = frame.document()->createDocumentFragment();

Why remove the auto?

[Ryosuke Niwa]

Created attachment 320816 [details]
Patch for landing

[Ryosuke Niwa]

Thanks for the review. Fixed them all.

[Ryosuke Niwa]

Comment on attachment 320816 [details]
Patch for landing

Wait for EWS.

[Ryosuke Niwa]

Created attachment 320826 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 320826 [details]
Patch for landing

Wait for EWS.

[Ryosuke Niwa]

(In reply to Sam Weinig from comment #7)
> Comment on attachment 320742 [details]
> Fixed API tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320742&action=review
>
> > Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm:164
> > +    addFragment(createFragmentFromText(context, [text precomposedStringWithCanonicalMapping]));
> 
> Should use dot notation since precomposedStringWithCanonicalMapping is a
> property.

Ugh... apparently text is a String instances. I'd leave square brackets for now instead of explicitly casing it to NSString.

[Ryosuke Niwa]

Created attachment 320830 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 320830 [details]
Patch for landing

Clearing flags on attachment: 320830

Committed r222062: <http://trac.webkit.org/changeset/222062>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34693428>