Bug 179586 – [Attachment Support] Implement SPI for clients to request data for a given attachment

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Wenson Hsieh]

<rdar://problem/35355720>

[Wenson Hsieh]

Created attachment 326709 [details]
First pass

[Wenson Hsieh]

Created attachment 326710 [details]
Fix WinCairo build

[Darin Adler]

Comment on attachment 326710 [details]
Fix WinCairo build

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

> Source/WebCore/dom/Document.cpp:7492
> +    m_attachmentIdentifierToElementMap.set(identifier, makeRef(attachment));

Surprised the makeRef is required.

> Source/WebCore/dom/Document.cpp:7516
> +    auto iter = m_attachmentIdentifierToElementMap.find(identifier);
> +    if (iter == m_attachmentIdentifierToElementMap.end())
> +        return nullptr;
> +
> +    return iter->value.copyRef();

Should be able to write:

    return m_attachmentIdentifierToElementMap.get(identifier);

Should not need find/end and explicit nullptr.

> Source/WebCore/html/HTMLAttachmentElement.cpp:83
> -HTMLAttachmentElement::~HTMLAttachmentElement() = default;
> +HTMLAttachmentElement::~HTMLAttachmentElement()
> +{
> +    m_attachmentReaders.clear();
> +}

I don’t understand why this change is needed. Why do we need to explicitly clear out the vector of unique_ptr before it gets destroyed?

> Source/WebCore/html/HTMLAttachmentElement.cpp:191
> +        invokeCallbackAndFinishReading(SharedBuffer::create(reinterpret_cast<uint8_t*>(arrayBuffer->data()), arrayBuffer->byteLength()));

Really annoying that we need the reinterpret_cast. Need to rearrange things so that does not happen!

[Wenson Hsieh]

Comment on attachment 326710 [details]
Fix WinCairo build

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

Thanks for taking a look!

>> Source/WebCore/dom/Document.cpp:7492
>> +    m_attachmentIdentifierToElementMap.set(identifier, makeRef(attachment));
> 
> Surprised the makeRef is required.

Whoops — the makeRef isn't necessary here. Removed.

>> Source/WebCore/dom/Document.cpp:7516
>> +    return iter->value.copyRef();
> 
> Should be able to write:
> 
>     return m_attachmentIdentifierToElementMap.get(identifier);
> 
> Should not need find/end and explicit nullptr.

Good catch — simplified to just get() from the HashMap.

>> Source/WebCore/html/HTMLAttachmentElement.cpp:83
>> +}
> 
> I don’t understand why this change is needed. Why do we need to explicitly clear out the vector of unique_ptr before it gets destroyed?

At second glance, I don't think it's needed. I originally put it here so that if the attachment element is destroyed with active attachment data readers, the attachment data readers will still tear down and invoke callbacks within the lifetime of the owning attachment element, since ~AttachmentDataReader called back into its HTMLAttachmentElement&. I moved a bit of logic around to avoid this dance, so the destructor of AttachmentDataReader doesn't rely on the existence of the HTMLAttachmentElement.

>> Source/WebCore/html/HTMLAttachmentElement.cpp:191
>> +        invokeCallbackAndFinishReading(SharedBuffer::create(reinterpret_cast<uint8_t*>(arrayBuffer->data()), arrayBuffer->byteLength()));
> 
> Really annoying that we need the reinterpret_cast. Need to rearrange things so that does not happen!

Ok. I'll look into this...

[Darin Adler]

Comment on attachment 326710 [details]
Fix WinCairo build

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

>>> Source/WebCore/html/HTMLAttachmentElement.cpp:191
>>> +        invokeCallbackAndFinishReading(SharedBuffer::create(reinterpret_cast<uint8_t*>(arrayBuffer->data()), arrayBuffer->byteLength()));
>> 
>> Really annoying that we need the reinterpret_cast. Need to rearrange things so that does not happen!
> 
> Ok. I'll look into this...

Oh, I mean long term. Not right now. This is expedient for the moment.

I believe the problem comes from mixing char and unsigned char in our various buffers, particularly in Vector<>. We also sometimes use the other name for unsigned char, uint8_t. We need to choose one and convert as much as possible to the same so there is no casting needed. I think people are likely leaning towards uint8_t.

Comment on attachment 326710 [details]
Fix WinCairo build

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

> Source/WebKit/UIProcess/API/Cocoa/_WKAttachment.h:32
> +typedef void(^_WKAttachmentDataCompletionHandler)(NSData *, NSError *);

Not sure what the extra indirection through this type adds. I think we normally avoid such typedefs.

> Source/WebKit/UIProcess/API/Cocoa/_WKAttachment.mm:70
> +        if (buffer && buffer->size() && error == CallbackBase::Error::None)

Why is the empty data case considered an error? Can there not be, say, and empty text file as an attachment?

[Wenson Hsieh]

Comment on attachment 326710 [details]
Fix WinCairo build

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

>>>> Source/WebCore/html/HTMLAttachmentElement.cpp:191
>>>> +        invokeCallbackAndFinishReading(SharedBuffer::create(reinterpret_cast<uint8_t*>(arrayBuffer->data()), arrayBuffer->byteLength()));
>>> 
>>> Really annoying that we need the reinterpret_cast. Need to rearrange things so that does not happen!
>> 
>> Ok. I'll look into this...
> 
> Oh, I mean long term. Not right now. This is expedient for the moment.
> 
> I believe the problem comes from mixing char and unsigned char in our various buffers, particularly in Vector<>. We also sometimes use the other name for unsigned char, uint8_t. We need to choose one and convert as much as possible to the same so there is no casting needed. I think people are likely leaning towards uint8_t.

Got it – thanks for the explanation!

>> Source/WebKit/UIProcess/API/Cocoa/_WKAttachment.h:32
>> +typedef void(^_WKAttachmentDataCompletionHandler)(NSData *, NSError *);
> 
> Not sure what the extra indirection through this type adds. I think we normally avoid such typedefs.

I generally find it more readable to use a typedef for these block types, especially if I need to declare one as a local or instance variable, but this isn't really a strong preference.

Removed this typedef.

>> Source/WebKit/UIProcess/API/Cocoa/_WKAttachment.mm:70
>> +        if (buffer && buffer->size() && error == CallbackBase::Error::None)
> 
> Why is the empty data case considered an error? Can there not be, say, and empty text file as an attachment?

Good point. Do you think Mail will have a need for supplying empty data through this SPI?

At any rate, it's probably expected that one should be able to create an attachment with an empty NSData, and then -requestData: and get an empty data blob back. I'll do this and add a new test for this case.

(In reply to Wenson Hsieh from comment #7)

> >> Source/WebKit/UIProcess/API/Cocoa/_WKAttachment.mm:70
> >> +        if (buffer && buffer->size() && error == CallbackBase::Error::None)
> > 
> > Why is the empty data case considered an error? Can there not be, say, and empty text file as an attachment?
> 
> Good point. Do you think Mail will have a need for supplying empty data
> through this SPI?

It may be unnecessary to *set* the data to be empty, assuming that’s the initial value, but attachments with empty data exist, and retrieving their data should work.

[Wenson Hsieh]

Created attachment 326740 [details]
Wait for EWS

[WebKit Commit Bot]

Comment on attachment 326740 [details]
Wait for EWS

Clearing flags on attachment: 326740

Committed r224755: <https://trac.webkit.org/changeset/224755>