Adding support to create a WebArchive with the correct resources as well as conversion to NSAttributedString.
Created attachment 247347 [details] Patch
Comment on attachment 247347 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247347&action=review > Source/WebCore/editing/cocoa/HTMLConverter.mm:1968 > + NSURL *url = [NSURL fileURLWithPath:[urlString stringByTrimmingCharactersInSet:[NSCharacterSet whitespaceAndNewlineCharacterSet]]]; What's up with the trimming? Though, I see the same thing happens below. Why not use _web_URLWithString: like we do below? I think it's better for some reason (other than that it takes a String instead of NSString). > Source/WebCore/editing/mac/EditorMac.mm:514 > + else I really don't like flow control that flows between #ifs like this. Maybe it could be written like this instead?: if (subresource->url().protocol() == String("webkit-attachment")) { attachments.append(sub resource); continue; } loader->addArchiveResource(subresource); > Source/WebCore/editing/mac/EditorMac.mm:524 > + for (size_t i = 0; i < nodes->length(); ++i) { No modern iterators for NodeList? > Source/WebCore/editing/mac/EditorMac.mm:525 > + ASSERT(nodes->length() == attachments.size()); I don't think this assert should be *inside* the for loop. > Source/WebCore/html/HTMLAttachmentElement.h:31 > +#include "File.h" We can put filePath() in the implementation file to avoid this include. > Source/WebCore/html/HTMLAttachmentElement.h:33 > +#include "SharedBuffer.h" A forward declaration of the SharedBuffer type would be preferable.
> Why not use _web_URLWithString: like we do below? I think it's better for > some reason (other than that it takes a String instead of NSString). > What I have from the file object is a path and I don't get a URL if I don't use fileWithURLPath. > > Source/WebCore/editing/mac/EditorMac.mm:514 > > + else > > I really don't like flow control that flows between #ifs like this. Maybe it > could be written like this instead?: > > if (subresource->url().protocol() == String("webkit-attachment")) { > attachments.append(sub resource); > continue; > } > > loader->addArchiveResource(subresource); > You're right, much better. > > Source/WebCore/editing/mac/EditorMac.mm:524 > > + for (size_t i = 0; i < nodes->length(); ++i) { > > No modern iterators for NodeList? > > > Source/WebCore/editing/mac/EditorMac.mm:525 > > + ASSERT(nodes->length() == attachments.size()); > > I don't think this assert should be *inside* the for loop. > > > Source/WebCore/html/HTMLAttachmentElement.h:31 > > +#include "File.h" > > We can put filePath() in the implementation file to avoid this include. > > > Source/WebCore/html/HTMLAttachmentElement.h:33 > > +#include "SharedBuffer.h" > > A forward declaration of the SharedBuffer type would be preferable. Will do
> > Source/WebCore/editing/mac/EditorMac.mm:524 > > + for (size_t i = 0; i < nodes->length(); ++i) { > > No modern iterators for NodeList? > No, NodeList does not have an iterator.
Created attachment 247454 [details] Patch2
Comment on attachment 247454 [details] Patch2 View in context: https://bugs.webkit.org/attachment.cgi?id=247454&action=review As discussed, let's do this with an attribute instead of an archive resource for now. > Source/WebCore/editing/cocoa/HTMLConverter.mm:1967 > + NSString *urlString = downcast<HTMLAttachmentElement>(element).filePath(); I don't think you need the filePath function. Instead, you can just do, file().path(). > Source/WebCore/editing/cocoa/HTMLConverter.mm:1968 > + NSURL *url = [NSURL fileURLWithPath:[urlString stringByTrimmingCharactersInSet:[NSCharacterSet whitespaceAndNewlineCharacterSet]]]; I don't think you need the trimming here. It is necessary for other types because they are getting the path from an attribute which is user supplied. > Source/WebCore/editing/mac/EditorMac.mm:507 > +#if ENABLE(ATTACHMENT_ELEMENT) > + Vector<RefPtr<ArchiveResource>> attachments; > +#endif Please add a comment (not necessarily here), about why you need this extra logic. > Source/WebCore/editing/mac/EditorMac.mm:512 > + if (subresource->url().protocol() == String("webkit-attachment")) { You can use url().protocolIs("webkit-attachment"). > Source/WebCore/editing/mac/EditorMac.mm:527 > + HTMLAttachmentElement& element = downcast<HTMLAttachmentElement>(*nodes->item(i)); This downcast is not safe. You need to do an is<HTMLAttachmentElement>(*nodes->item(i)) first. > Source/WebCore/editing/mac/EditorMac.mm:528 > + element.setFile(&File::create(String(attachments[i]->data()->data(), attachments[i]->data()->size())).get()); Instead of .get(), you can do .ptr(), then you don't need the &. > Source/WebCore/html/HTMLAttachmentElement.cpp:74 > + if (file) > + m_dataBuffer = SharedBuffer::create(m_file->path().characters8(), m_file->path().length()); > + else > + m_dataBuffer = nullptr; This can be created lazily when the buffer is necessary. > Source/WebCore/html/HTMLAttachmentElement.cpp:80 > +const String HTMLAttachmentElement::filePath() > +{ > + return m_file ? m_file->path() : String(); > } I don't think this helper is useful. Just have the caller grab the file. > Source/WebCore/html/HTMLAttachmentElement.cpp:102 > + addSubresourceURL(urls, resourceURL()); I don't think this called frequently enough to warrant caching the resource URL. Let's just create it here, and instead of using a UUID, just use the path as the path.
Created attachment 247533 [details] Patch2 Following different approach as discussed with Sam.
Comment on attachment 247533 [details] Patch2 View in context: https://bugs.webkit.org/attachment.cgi?id=247533&action=review > Source/WebCore/html/HTMLAttachmentElement.h:40 > + virtual bool canContainRangeEndPoint() const override { return false; } I don't think this is actually part of this patch? > LayoutTests/editing/pasteboard/copy-paste-attachment.html:5 > +<!--<script type="text/javascript" src="../../resources/dump-as-markup.js"></script>--> Let's not have random commented out stuff here.
(In reply to comment #8) > Comment on attachment 247533 [details] > Patch2 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=247533&action=review > > > Source/WebCore/html/HTMLAttachmentElement.h:40 > > + virtual bool canContainRangeEndPoint() const override { return false; } > > I don't think this is actually part of this patch? > Yes it is, it is the change to fix the selection. > > LayoutTests/editing/pasteboard/copy-paste-attachment.html:5 > > +<!--<script type="text/javascript" src="../../resources/dump-as-markup.js"></script>--> > > Let's not have random commented out stuff here. Oops!
Committed revision 180785.