Bug 142026 – Adding support for serializing HTMLAttachment elements.

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

[Enrica Casucci]

Adding support to create a WebArchive with the correct resources as well as conversion to NSAttributedString.

[Enrica Casucci]

Created attachment 247347 [details]
Patch

[Tim Horton]

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.

[Enrica Casucci]

> 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

[Enrica Casucci]

> > 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.

[Enrica Casucci]

Created attachment 247454 [details]
Patch2

[Sam Weinig]

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.

[Enrica Casucci]

Created attachment 247533 [details]
Patch2

Following different approach as discussed with Sam.

[Tim Horton]

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.

[Enrica Casucci]

(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!

[Enrica Casucci]

Committed revision 180785.