[ClipboardChange event] Add `changeId` implementation [chromium/src : main]

0 views

Skip to first unread message

Simon Hangl (Gerrit)

unread,

Sep 23, 2025, 4:45:57 AM (13 days ago) Sep 23

to Luke Klimek, chromium...@chromium.org, Kentaro Hara, Raphael Kubo da Costa, James Maclean, alexmo...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, creis...@chromium.org, dcheng+c...@chromium.org, ipc-securi...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, navigation...@chromium.org

Attention needed from Luke Klimek

Simon Hangl voted and added 6 comments

Votes added by Simon Hangl

Code-Review

6 comments

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Simon Hangl . resolved

high-level lgtm

Commit Message

Line 9, Patchset 1 (Latest):Context:

Simon Hangl . unresolved

please add a basic description of the change. providing a link to a link to a link ... isn't exactly reviewer friendly :P

File content/browser/renderer_host/clipboard_host_impl.cc

Line 168, Patchset 1 (Latest): ui::ClipboardBuffer clipboard_buffer,

Simon Hangl . unresolved

nit: const

Line 173, Patchset 1 (Latest): render_frame_host().GetProcess()->GetStoragePartition());

Simon Hangl . unresolved

you may very well inline this.

File content/browser/storage_partition_impl.h

Line 556, Patchset 1 (Latest): const base::Uuid& GetPartitionUUIDPerStorageKey(

Simon Hangl . unresolved

does this have to be declared in the impl class? can it be added to the interface? then you don't have to do a static cast where you need to use it.

File third_party/blink/renderer/modules/clipboard/clipboard_change_event.h

Line 24, Patchset 1 (Latest): ClipboardChangeEvent(const ClipboardChangeEventInit* initializer);

Simon Hangl . unresolved

drive-by fix suggestion:

Please fix this WARNING reported by ClangTidy: check: google-explicit-constructor

single-argument constructors must be marked ...

check: google-explicit-constructor

single-argument constructors must be marked explicit to avoid unintentional implicit conversions (https://clang.llvm.org/extra/clang-tidy/checks/google/explicit-constructor.html)

(Note: You can add `Skip-Clang-Tidy-Checks: google-explicit-constructor` footer to the CL description to skip the check)

(Lint observed on `android-clang-tidy-rel` and `linux-clang-tidy-rel`)

Open in Gerrit

Related details

Attention is currently required from:

Luke Klimek

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Luke Klimek (Gerrit)

unread,

Sep 23, 2025, 9:55:53 AM (13 days ago) Sep 23

to Simon Hangl, chromium...@chromium.org, Kentaro Hara, Raphael Kubo da Costa, James Maclean, pwa-com...@google.com, alexmo...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, creis...@chromium.org, dcheng+c...@chromium.org, ipc-securi...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, navigation...@chromium.org

Attention needed from Simon Hangl

Luke Klimek added 5 comments

Commit Message

Line 9, Patchset 1:Context:

Simon Hangl . resolved

please add a basic description of the change. providing a link to a link to a link ... isn't exactly reviewer friendly :P

Luke Klimek

Yes, I was planning on doing this when toggling the WIP bit later. Added something.

File content/browser/renderer_host/clipboard_host_impl.cc

Line 168, Patchset 1: ui::ClipboardBuffer clipboard_buffer,

Simon Hangl . resolved

nit: const

Luke Klimek

Hm, this is a primitive type anyway, I feel it would make the signature just *a bit* less readable. And for consistency require changing all of the places in this file, which would make the CL less readable. I'll maybe not do that this time.

Line 173, Patchset 1: render_frame_host().GetProcess()->GetStoragePartition());

Simon Hangl . resolved

you may very well inline this.

Luke Klimek

Done

File content/browser/storage_partition_impl.h

Line 556, Patchset 1: const base::Uuid& GetPartitionUUIDPerStorageKey(

Simon Hangl . resolved

does this have to be declared in the impl class? can it be added to the interface? then you don't have to do a static cast where you need to use it.

Luke Klimek

Hm, I vaguely recall some reason. But cannot remember it now, let's put it in the interface.

File third_party/blink/renderer/modules/clipboard/clipboard_change_event.h

Line 24, Patchset 1: ClipboardChangeEvent(const ClipboardChangeEventInit* initializer);

Simon Hangl . resolved

drive-by fix suggestion:
Please fix this WARNING reported by ClangTidy: check: google-explicit-constructor
single-argument constructors must be marked ...
check: google-explicit-constructor
single-argument constructors must be marked explicit to avoid unintentional implicit conversions (https://clang.llvm.org/extra/clang-tidy/checks/google/explicit-constructor.html)
(Note: You can add `Skip-Clang-Tidy-Checks: google-explicit-constructor` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel` and `linux-clang-tidy-rel`)

Luke Klimek

Done

Open in Gerrit

Related details

Attention is currently required from:

Simon Hangl

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

0 new messages