Bug 176884

Summary: WebContentReader::readWebArchive doesn't need to handle image MIME type
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, sam, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 176772    
Attachments:
Description Flags
Cleanup
none
Added missing includes
none
Refactoring
none
Fixed API tests
none
Patch for landing
none
Patch for landing
none
Patch for landing none

Ryosuke Niwa
Reported 2017-09-13 17:18:29 PDT
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.
Attachments
Cleanup (7.33 KB, patch)
2017-09-13 17:24 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Added missing includes (7.63 KB, patch)
2017-09-13 18:40 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Refactoring (21.39 KB, patch)
2017-09-14 00:00 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Fixed API tests (21.82 KB, patch)
2017-09-14 00:48 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Patch for landing (20.55 KB, patch)
2017-09-14 13:55 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Patch for landing (20.55 KB, patch)
2017-09-14 14:33 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Patch for landing (20.55 KB, patch)
2017-09-14 15:11 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1 2017-09-13 17:24:18 PDT
Created attachment 320713 [details] Cleanup
Ryosuke Niwa
Comment 2 2017-09-13 18:40:18 PDT
Created attachment 320722 [details] Added missing includes
Sam Weinig
Comment 3 2017-09-13 21:46:22 PDT
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 4 2017-09-13 23:45:13 PDT
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
Comment 5 2017-09-14 00:00:40 PDT
Created attachment 320736 [details] Refactoring
Ryosuke Niwa
Comment 6 2017-09-14 00:48:28 PDT
Created attachment 320742 [details] Fixed API tests
Sam Weinig
Comment 7 2017-09-14 09:15:53 PDT
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
Comment 8 2017-09-14 13:55:06 PDT
Created attachment 320816 [details] Patch for landing
Ryosuke Niwa
Comment 9 2017-09-14 13:55:10 PDT
Thanks for the review. Fixed them all.
Ryosuke Niwa
Comment 10 2017-09-14 13:55:25 PDT
Comment on attachment 320816 [details] Patch for landing Wait for EWS.
Ryosuke Niwa
Comment 11 2017-09-14 14:33:10 PDT
Created attachment 320826 [details] Patch for landing
Ryosuke Niwa
Comment 12 2017-09-14 14:33:22 PDT
Comment on attachment 320826 [details] Patch for landing Wait for EWS.
Ryosuke Niwa
Comment 13 2017-09-14 15:10:14 PDT
(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
Comment 14 2017-09-14 15:11:13 PDT
Created attachment 320830 [details] Patch for landing
WebKit Commit Bot
Comment 15 2017-09-14 16:43:59 PDT
Comment on attachment 320830 [details] Patch for landing Clearing flags on attachment: 320830 Committed r222062: <http://trac.webkit.org/changeset/222062>
WebKit Commit Bot
Comment 16 2017-09-14 16:44:01 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 17 2017-09-27 12:31:39 PDT
<rdar://problem/34693428>
Note You need to log in before you can comment on or make changes to this bug.