Bug 213986 – [Win] Implement Pasteboard::writeCustomData for Web Inspector Console tab

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Tomoki Imai]

Like GTK bug 177633, Windows should have Pasteboard::writeCustomData implemented.

Pasteboard::writeCustomData is required to support "copy" or "Ctrl-C" in web inspector Console tab.
Currently, "Ctrl-C" in Console tab doesn't have any effect.

[Tomoki Imai]

Created attachment 403577 [details]
patch

Patch to implement Pasteboard::writeCustomData for Windows.

[Tomoki Imai]

Created attachment 403580 [details]
patch

Fixed style check error

[Tomoki Imai]

Created attachment 403581 [details]
Where we failed to copy in WebInspector

How to reproduce the web inspector copy issue.

1. Open MiniBrowser, with arbitrarily URL.
2. Go to Console tab
3. Input some JS statement and execute
4. Try to copy the output or previous input by select the cell and Ctrl-C.

Actual result:
Nothing is copied

Expected result:
The previous input or output should be copied to the pasteboard.

[Fujii Hironori]

Comment on attachment 403580 [details]
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-setclipboarddata

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

[Fujii Hironori]

Comment on attachment 403580 [details]
patch

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

> Source/WebCore/platform/win/PasteboardWin.cpp:1160
> +            HGLOBAL cbData = createGlobalData(customData.createBuffer());

If you remove PasteboardCustomData::createBuffer() and change the arguments of createGlobalData, this code would look like:

auto sharedBuffer = customData.createSharedBuffer();
HGLOBAL cbData = createGlobalData(sharedBuffer.data(), sharedBuffer.size());

[Fujii Hironori]

Comment on attachment 403580 [details]
patch

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

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:212
> +    T* buffer = static_cast<T*>(GlobalLock(vm));

Ditto.

> Source/WebCore/platform/win/PasteboardWin.cpp:250
> +            uint8_t* data = static_cast<uint8_t*>(GlobalLock(cbData));

uint8_t* data = static_cast<uint8_t*>(...);
I think these code seems redundant because there are the same types both sides.
auto data = static_cast<uint8_t*>(...);

[Fujii Hironori]

Comment on attachment 403580 [details]
patch

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

> Source/WebCore/ChangeLog:14
> +        * platform/PasteboardCustomData.cpp: Currently Windows ports don't use SharedBuffer, because PasteboardWin uses the native APIs in WebProcess.

IIUC, WebCore::SharedBuffer is a platform specifc data backed segmented ref-counted buffer. It doens't imply multi-process shared memory like WebKit::SharedMemory.

[Tomoki Imai]

Created attachment 403668 [details]
patch

Applied Fujii -san's comments

[Tomoki Imai]

Thanks for your review!

(In reply to Fujii Hironori from comment #4)
> Comment on attachment 403580 [details]
> 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().

I agree, fixed.

> 
> > 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-
> setclipboarddata
> 
> > If the hMem parameter identifies a memory object, the object must
> > have been allocated using the function with the GMEM_MOVEABLE
> > flag.
> 

Good point, but I don't know.
Existing code also has the same issue. `createGlobalData` variants called GlobalAlloc with argument GPTR.
GPTR is equal to GMEM_FIXED | GMEM_ZEROINIT according to https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-globalalloc.

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

Replaced with your suggestion.
The reason why I created a vector temporarily was to minimize the duration between GlobalLock and GlobalUnlock.
But I don't think decoding PasteboardCustomData takes so long.

(In reply to Fujii Hironori from comment #5)
> Comment on attachment 403580 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403580&action=review
> 
> > Source/WebCore/platform/win/PasteboardWin.cpp:1160
> > +            HGLOBAL cbData = createGlobalData(customData.createBuffer());
> 
> If you remove PasteboardCustomData::createBuffer() and change the arguments
> of createGlobalData, this code would look like:
> 
> auto sharedBuffer = customData.createSharedBuffer();
> HGLOBAL cbData = createGlobalData(sharedBuffer.data(), sharedBuffer.size());

Thanks, I had to reinterpret_cast sharedBuffer.data() because it returns char*.
Doing reinterpret_cast looks safe because SharedBuffer::decoder() also does.

(In reply to Fujii Hironori from comment #7)
> Comment on attachment 403580 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403580&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        * platform/PasteboardCustomData.cpp: Currently Windows ports don't use SharedBuffer, because PasteboardWin uses the native APIs in WebProcess.
> 
> IIUC, WebCore::SharedBuffer is a platform specifc data backed segmented
> ref-counted buffer. It doens't imply multi-process shared memory like
> WebKit::SharedMemory.

Thanks, it has helper functions, for example decoder(), but not limited to shared memory usage.

[Tomoki Imai]

Thanks for your review!

I'll cq+.
I see some test failures on Win about webanimations according to EWS, but I believe they are not related to this patch.
GTK and WPE reported the same issue in following bugs.
- bug 211948 
- bug 212020 
- bug 213783

[EWS]

Committed r264014: <https://trac.webkit.org/changeset/264014>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403668 [details].

[Radar WebKit Bug Importer]

<rdar://problem/65171948>