[webkit-reviews] review granted: [Bug 179586] [Attachment Support] Implement SPI for clients to request data for a given attachment : [Attachment 326710] Fix WinCairo build

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 179586: [Attachment Support] Implement SPI for clients to request data for
a given attachment
https://bugs.webkit.org/show_bug.cgi?id=179586

Attachment 326710: Fix WinCairo build

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 326710
  --> https://bugs.webkit.org/attachment.cgi?id=326710
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!