[webkit-reviews] review denied: [Bug 213986] [Win] Implement Pasteboard::writeCustomData for Web Inspector Console tab : [Attachment 403580] patch

[bugzilla-daemon at webkit.org]

Fujii Hironori <Hironori.Fujii at sony.com> has denied Tomoki Imai
<tomoki.imai at sony.com>'s request for review:
Bug 213986: [Win] Implement Pasteboard::writeCustomData for Web Inspector
Console tab
https://bugs.webkit.org/show_bug.cgi?id=213986

Attachment 403580: patch

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




--- Comment #4 from Fujii Hironori <Hironori.Fujii at sony.com> ---
Comment on attachment 403580
  --> https://bugs.webkit.org/attachment.cgi?id=403580
patch

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

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:207
> +HGLOBAL createGlobalData(const Vector<T>& vector)

I think createGlobalData should take (uint8_t* data, size_t length) as the
arguments.
Then, I don't need to add PasteboardCustomData::createBuffer().

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:209
>      HGLOBAL vm = ::GlobalAlloc(GPTR, vector.size() + 1);

According to SetClipboardData spec, it should be GMEM_MOVEABLE .
Do you know the reason why GMEM_MOVEABLE isn't given?

SetClipboardData function (winuser.h) - Win32 apps | Microsoft Docs
https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-setclipbo
arddata

> If the hMem parameter identifies a memory object, the object must
> have been allocated using the function with the GMEM_MOVEABLE
> flag.

> Source/WebCore/platform/win/PasteboardWin.cpp:257
> +	       return PasteboardCustomData::fromBuffer(vector);

This vector is used temporarily. I think it's better to use
PasteboardCustomData::fromPersistenceDecoder.
> auto customData = PasteboardCustomData::fromPersistenceDecoder({ data, size
});
> (...)
> return curtomData;