fix: clipboard.writeBuffer raw format access #31116

henrit · 2021-09-24T19:41:29Z

Description of Change

This PR addresses #31115.

It includes a chromium patch to re-expose a mechanism to set raw data on the clipboard that was recently removed.

Checklist

PR description included and stakeholders cc'd
npm test passes
tests are changed or added
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Fixed clipboard.writeBuffer(), clipboard.readBuffer() and clipboard.read()'s ability to manipulate platform-specific clipboard formats.

welcome · 2021-09-24T19:41:31Z

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

fix: don't overwrite prevent_default if default wasn't prevented
feat: add app.isPackaged() method
docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

Follow the JavaScript, C++, and Python coding style.
Run npm run lint locally to catch formatting errors earlier.
Document any user-facing changes you've made following the documentation styleguide.
Include tests when adding/changing behavior.
Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

codebytere

Hey @henrit - thanks for this!

Given that the underlying functionality you're adding via patch is something Chromium removed with no intention of re-adding, the approach contained in the patch is something that we'd need to float forever. Generally speaking, we try to keep our patch count to a minimum as each adds nontrivial maintenance burden, and prefer only to add indefinite lifetime patches as a last resort.

I'd prefer we first try to see if we can avoid doing so before trying to move this PR forward (e.g. by subclassing ScopedClipboardWriter to add the missing functionality to Electron's own source tree).

codebytere · 2021-09-24T20:03:47Z

patches/chromium/add_ui_scopedcliboardwriter_writeunsaferawdata.patch

+
+void ScopedClipboardWriter::WriteUnsafeRawData(const std::u16string& format,
+                                               mojo_base::BigBuffer data) {
+  RecordWrite(ClipboardFormatMetric::kUnsafeRawData);


Chromium only uses this to record perf metrics - we can remove this and the added enum class value as we don't make use of them in Electron

Thanks for the quick feedback. That makes sense.
What would be a good folder to put the subclass?

Should it go next to the rest of the code in shell/common/api/, or maybe one-level up in shell/common/ since it doesn't itself define apis?

let's just go with shell/common/ for now!

Unfortunately I'm not seeing a path to subclass the chromium clipboard code into something useful.
ui::ScopedClipboardWriter keeps its reference to a ui::Clipboard subclass private, and ui::Clipboard itself keeps
all its clipboard writing methods protected, which ScopedClipboardWriter can only access because it's
marked as a friend class.
If I understand this correctly, this code shape limits us to either copying large chunks of clipboard code
into electron, or patch it on the chromium side.

@ckerr

@henrit I looked into this and I agree with you on your assessment. Given that we would have to bring over a large amount of the the clipboard code, we should instead proceed with the patch instead. @henrit are you willing to continue working on this? If not, I'd be happy to take it over, but since it's your PR, I'd prefer to defer to you.

Looking at the PR, I think if it is rebased to resolve the conflict and perhaps cleanup the patch with @codebytere's suggestions, we can move this PR forward.

cc @ckerr @codebytere

I'm a little tardy with this response, but FWIW I've looked into this wrt pickling and also agree with going the patch route first, since it looks like we'd need to copy a decent amount of code if we copied instead of patching.

Some of my review comments don't make sense in a patch context. Feel free to mark resolved at will.

I'm still happy to help via followup review and/or pairing if anyone wants it, whether it's @henrit or @jkleinsc.

patches/chromium/add_ui_scopedcliboardwriter_writeunsaferawdata.patch

shell/common/api/electron_api_clipboard.cc

spec/api-clipboard-spec.js

ckerr

To clarify my review from yesterday a bit -- @codebytere is right that it makes more sense to have this live as standalone code as possible instead of as a patch.

My review comments yesterday are for changes after the code is moved to a subclass, but I didn't make that explicit and looking at my comments again today I don't want to give the impression that I was asking for an updated file in .patches/

welcome · 2021-11-04T18:19:32Z

Congrats on merging your first pull request! 🎉🎉🎉

release-clerk · 2021-11-04T18:19:33Z

Release Notes Persisted

Fixed clipboard.writeBuffer(), clipboard.readBuffer() and clipboard.read()'s ability to manipulate platform-specific clipboard formats.

* fix: clipboard.writeBuffer raw format access * test: clipboard.writeBuffer raw format access * test: clipboard win32 test skip * fixup spec * cleanup patch Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>

trop · 2021-11-04T18:20:02Z

I have automatically backported this PR to "16-x-y", please check out #31719

trop · 2021-11-04T18:20:05Z

I have automatically backported this PR to "15-x-y", please check out #31720

* fix: clipboard.writeBuffer raw format access * test: clipboard.writeBuffer raw format access * test: clipboard win32 test skip * fixup spec * cleanup patch Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org> Co-authored-by: henrit <henrit@gmail.com> Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>

* fix: clipboard.writeBuffer raw format access (#31116) * fix: clipboard.writeBuffer raw format access * test: clipboard.writeBuffer raw format access * test: clipboard win32 test skip * fixup spec * cleanup patch Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org> * fixup .patches file Co-authored-by: henrit <henrit@gmail.com> Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>

* fix: clipboard.writeBuffer raw format access * test: clipboard.writeBuffer raw format access * test: clipboard win32 test skip * fixup spec * cleanup patch Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>

henrit added 2 commits September 23, 2021 21:31

fix: clipboard.writeBuffer raw format access

7b17ede

test: clipboard.writeBuffer raw format access

0a583e7

henrit requested a review from a team as a code owner September 24, 2021 19:41

electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Sep 24, 2021

codebytere requested changes Sep 24, 2021

View reviewed changes

electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Oct 1, 2021

henrit added 2 commits October 18, 2021 03:51

test: clipboard win32 test skip

db24107

Merge branch 'main' into raw-clipboard-support

81e2326

ckerr reviewed Oct 18, 2021

View reviewed changes

ckerr reviewed Oct 19, 2021

View reviewed changes

jkleinsc added semver/patch backwards-compatible bug fixes target/15-x-y labels Nov 2, 2021

jkleinsc requested a review from codebytere November 2, 2021 15:41

jkleinsc added 3 commits November 3, 2021 13:24

Merge branch 'main' into raw-clipboard-support

6614263

fixup spec

8c281c7

cleanup patch

2935923

jkleinsc approved these changes Nov 4, 2021

View reviewed changes

jkleinsc merged commit 3c33d70 into electron:main Nov 4, 2021

trop bot mentioned this pull request Nov 4, 2021

fix: clipboard.writeBuffer raw format access #31719

Merged

trop bot mentioned this pull request Nov 4, 2021

fix: clipboard.writeBuffer raw format access #31720

Merged

trop bot added in-flight/16-x-y and removed target/16-x-y labels Nov 4, 2021

trop bot added in-flight/15-x-y and removed target/15-x-y labels Nov 4, 2021

jkleinsc mentioned this pull request Nov 4, 2021

[Bug]: clipboard.writeBuffer() regression #31115

Closed

3 tasks

trop bot added merged/16-x-y and removed in-flight/16-x-y labels Nov 4, 2021

trop bot added merged/15-x-y and removed in-flight/15-x-y labels Nov 8, 2021

commit-lint bot mentioned this pull request Feb 7, 2022

[pull] main from electron:main Blaquez-home/electron#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: clipboard.writeBuffer raw format access #31116

fix: clipboard.writeBuffer raw format access #31116

henrit commented Sep 24, 2021

welcome bot commented Sep 24, 2021

codebytere left a comment •

edited

codebytere Sep 24, 2021

henrit Sep 24, 2021

codebytere Sep 27, 2021

henrit Oct 19, 2021 •

edited

jkleinsc Nov 2, 2021

ckerr Nov 2, 2021 •

edited

ckerr left a comment

welcome bot commented Nov 4, 2021

release-clerk bot commented Nov 4, 2021

trop bot commented Nov 4, 2021

trop bot commented Nov 4, 2021

fix: clipboard.writeBuffer raw format access #31116

fix: clipboard.writeBuffer raw format access #31116

Conversation

henrit commented Sep 24, 2021

Description of Change

Checklist

Release Notes

welcome bot commented Sep 24, 2021

codebytere left a comment • edited

Choose a reason for hiding this comment

codebytere Sep 24, 2021

Choose a reason for hiding this comment

henrit Sep 24, 2021

Choose a reason for hiding this comment

codebytere Sep 27, 2021

Choose a reason for hiding this comment

henrit Oct 19, 2021 • edited

Choose a reason for hiding this comment

jkleinsc Nov 2, 2021

Choose a reason for hiding this comment

ckerr Nov 2, 2021 • edited

Choose a reason for hiding this comment

ckerr left a comment

Choose a reason for hiding this comment

welcome bot commented Nov 4, 2021

release-clerk bot commented Nov 4, 2021

trop bot commented Nov 4, 2021

trop bot commented Nov 4, 2021

codebytere left a comment •

edited

henrit Oct 19, 2021 •

edited

ckerr Nov 2, 2021 •

edited