WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
213986
[Win] Implement Pasteboard::writeCustomData for Web Inspector Console tab
https://bugs.webkit.org/show_bug.cgi?id=213986
Summary
[Win] Implement Pasteboard::writeCustomData for Web Inspector Console tab
Tomoki Imai
Reported
2020-07-06 00:54:03 PDT
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.
Attachments
patch
(15.17 KB, patch)
2020-07-06 01:06 PDT
,
Tomoki Imai
no flags
Details
Formatted Diff
Diff
patch
(15.17 KB, patch)
2020-07-06 01:19 PDT
,
Tomoki Imai
fujii.hironori
: review-
Details
Formatted Diff
Diff
Where we failed to copy in WebInspector
(24.63 KB, image/png)
2020-07-06 01:33 PDT
,
Tomoki Imai
no flags
Details
patch
(13.53 KB, patch)
2020-07-06 23:18 PDT
,
Tomoki Imai
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Tomoki Imai
Comment 1
2020-07-06 01:06:34 PDT
Created
attachment 403577
[details]
patch Patch to implement Pasteboard::writeCustomData for Windows.
Tomoki Imai
Comment 2
2020-07-06 01:19:46 PDT
Created
attachment 403580
[details]
patch Fixed style check error
Tomoki Imai
Comment 3
2020-07-06 01:33:45 PDT
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 4
2020-07-06 14:36:24 PDT
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 5
2020-07-06 14:53:49 PDT
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 6
2020-07-06 14:56:41 PDT
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 7
2020-07-06 15:39:24 PDT
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
Comment 8
2020-07-06 23:18:44 PDT
Created
attachment 403668
[details]
patch Applied Fujii -san's comments
Tomoki Imai
Comment 9
2020-07-06 23:35:25 PDT
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
Comment 10
2020-07-07 03:54:14 PDT
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
Comment 11
2020-07-07 04:31:31 PDT
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
Comment 12
2020-07-07 04:32:14 PDT
<
rdar://problem/65171948
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug