Thomas Anderson (Gerrit)

unread,

Mar 11, 2026, 3:25:03 PM (2 days ago) Mar 11

to Lei Zhang, chromium...@chromium.org, Sadrul Chowdhury, ozone-...@chromium.org

Attention needed from Lei Zhang

Thomas Anderson voted Commit-Queue+1

Commit-Queue

+1

Open in Gerrit

Related details

Attention is currently required from:

Lei Zhang

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Lei Zhang (Gerrit)

unread,

Mar 11, 2026, 5:00:17 PM (2 days ago) Mar 11

to Thomas Anderson, Chromium LUCI CQ, Lei Zhang, chromium...@chromium.org, Sadrul Chowdhury, ozone-...@chromium.org

Attention needed from Thomas Anderson

Lei Zhang added 18 comments

File ui/base/x/selection_requestor.h

Line 46, Patchset 3 (Latest): void PerformConvertSelection(x11::Atom selection,

Lei Zhang . unresolved

Should this also have "Async" in the name?

File ui/base/x/selection_requestor.cc

Line 63, Patchset 3 (Latest): auto request = std::make_unique<Request>(selection, target, timeout,

Lei Zhang . unresolved

Just merge into line 65.

Line 85, Patchset 3 (Latest): x11::Atom target = types.front();

Lei Zhang . unresolved

Does the ordering matter? If this can grab the back element, then the next line can just pop the back element instead of copying all the other elements.

Line 198, Patchset 3 (Latest): if (!weak_this) {

Lei Zhang . unresolved

Given this, should line 167, 172, etc all have a warning comment to say "do nothing since `this` may be gone"?

Line 211, Patchset 3 (Latest): if (index >= requests_.size()) {

Lei Zhang . unresolved

Pre-existing: Can this happen?

Line 232, Patchset 3 (Latest): // 3. Update requests_ queue

Lei Zhang . unresolved

Wrap in backticks.

File ui/base/x/selection_requestor_unittest.cc

Line 108, Patchset 3 (Latest): std::vector<uint8_t> data1 = future1.Get<1>();

Lei Zhang . unresolved

Const-ref to avoid a copy.

Line 109, Patchset 3 (Latest): EXPECT_EQ("Data1", std::string(data1.begin(), data1.end()));

Lei Zhang . unresolved

Maybe string_view to avoid a copy.

Line 128, Patchset 3 (Latest): EXPECT_FALSE(future.Get<0>());

Lei Zhang . unresolved

Should this check elements 1 and 2?

Line 149, Patchset 3 (Latest): std::string data_str(span.begin(), span.end());

Lei Zhang . unresolved

string_view / merge into next line.

File ui/base/x/x11_clipboard_helper.h

Line 151, Patchset 3 (Latest): base::WeakPtrFactory<XClipboardHelper> self_ptr_factory_{this};

Lei Zhang . unresolved

Maybe just use the common "weak_ptr_factory_" name since there are no other WeakPtrFactories.

Line 59, Patchset 3 (Latest): // Returns the first of |types| offered by the current selection holder, or

Lei Zhang . unresolved

Update comment since this no longer returns.

File ui/base/x/x11_clipboard_helper.cc

Line 35, Patchset 3 (Parent):const char kClipboardManager[] = "CLIPBOARD_MANAGER";

Lei Zhang . unresolved

Curious what this was for and why it's safe to remove.

Line 274, Patchset 3 (Latest): std::sort(common_types.begin(), common_types.end());

Lei Zhang . unresolved

std::ranges::sort(common_types); ?

Line 276, Patchset 3 (Latest): std::unique(common_types.begin(), common_types.end()),

Lei Zhang . unresolved

std::ranges::unique?

Line 341, Patchset 3 (Latest): std::vector<x11::Atom> out;

Lei Zhang . unresolved

Call reserve()

Line 362, Patchset 3 (Latest): std::vector<x11::Atom> out;

Lei Zhang . unresolved

Remove and just use base::ToVector(atom_array) on line 373.

File ui/ozone/platform/x11/x11_clipboard_ozone.cc

Line 79, Patchset 3 (Parent): // TODO(crbug.com/40054419): Consider adding a way of filtering mime types and

Lei Zhang . unresolved

Is this TODO still relevant?

Open in Gerrit

Related details

Attention is currently required from:

Thomas Anderson

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

No-Unresolved-Comments

Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Thomas Anderson (Gerrit)

unread,

Mar 11, 2026, 7:04:01 PM (2 days ago) Mar 11

to Chromium LUCI CQ, Lei Zhang, chromium...@chromium.org, Sadrul Chowdhury, ozone-...@chromium.org

Attention needed from Lei Zhang

Thomas Anderson voted and added 18 comments

Votes added by Thomas Anderson

Commit-Queue

+1

18 comments

File ui/base/x/selection_requestor.h

Line 46, Patchset 3: void PerformConvertSelection(x11::Atom selection,

Lei Zhang . resolved

Should this also have "Async" in the name?

Thomas Anderson

Done

File ui/base/x/selection_requestor.cc

Line 63, Patchset 3: auto request = std::make_unique<Request>(selection, target, timeout,

Lei Zhang . resolved

Just merge into line 65.

Thomas Anderson

Done

Line 85, Patchset 3: x11::Atom target = types.front();

Lei Zhang . resolved

Does the ordering matter? If this can grab the back element, then the next line can just pop the back element instead of copying all the other elements.

Thomas Anderson

Done

Line 198, Patchset 3: if (!weak_this) {

Lei Zhang . resolved

Given this, should line 167, 172, etc all have a warning comment to say "do nothing since `this` may be gone"?

Thomas Anderson

Done

Line 211, Patchset 3: if (index >= requests_.size()) {

Lei Zhang . resolved

Pre-existing: Can this happen?

Thomas Anderson

changed to CHECK_LT

Line 232, Patchset 3: // 3. Update requests_ queue

Lei Zhang . resolved

Wrap in backticks.

Thomas Anderson

Done

File ui/base/x/selection_requestor_unittest.cc

Line 108, Patchset 3: std::vector<uint8_t> data1 = future1.Get<1>();

Lei Zhang . resolved

Const-ref to avoid a copy.

Thomas Anderson

Done

Line 109, Patchset 3: EXPECT_EQ("Data1", std::string(data1.begin(), data1.end()));

Lei Zhang . resolved

Maybe string_view to avoid a copy.

Thomas Anderson

Done

Line 128, Patchset 3: EXPECT_FALSE(future.Get<0>());

Lei Zhang . resolved

Should this check elements 1 and 2?

Thomas Anderson

Done

Line 149, Patchset 3: std::string data_str(span.begin(), span.end());

Lei Zhang . resolved

string_view / merge into next line.

Thomas Anderson

Done

File ui/base/x/x11_clipboard_helper.h

Line 151, Patchset 3: base::WeakPtrFactory<XClipboardHelper> self_ptr_factory_{this};

Lei Zhang . resolved

Maybe just use the common "weak_ptr_factory_" name since there are no other WeakPtrFactories.

Thomas Anderson

Done

Line 59, Patchset 3: // Returns the first of |types| offered by the current selection holder, or

Lei Zhang . resolved

Update comment since this no longer returns.

Thomas Anderson

Done

File ui/base/x/x11_clipboard_helper.cc

Line 35, Patchset 3 (Parent):const char kClipboardManager[] = "CLIPBOARD_MANAGER";

Lei Zhang . resolved

Curious what this was for and why it's safe to remove.

Thomas Anderson

It was used in XClipboardHelper::StoreCopyPasteDataAndWait, but that function was not called anywhere (even before this change), so it was removed.

Line 274, Patchset 3: std::sort(common_types.begin(), common_types.end());

Lei Zhang . resolved

std::ranges::sort(common_types); ?

Thomas Anderson

Done

Line 276, Patchset 3: std::unique(common_types.begin(), common_types.end()),

Lei Zhang . resolved

std::ranges::unique?

Thomas Anderson

Done

Line 341, Patchset 3: std::vector<x11::Atom> out;

Lei Zhang . resolved

Call reserve()

Thomas Anderson

Done

Line 362, Patchset 3: std::vector<x11::Atom> out;

Lei Zhang . resolved

Remove and just use base::ToVector(atom_array) on line 373.

Thomas Anderson

Done

File ui/ozone/platform/x11/x11_clipboard_ozone.cc

Line 79, Patchset 3 (Parent): // TODO(crbug.com/40054419): Consider adding a way of filtering mime types and

Lei Zhang . resolved

Is this TODO still relevant?

Thomas Anderson

it's not relevant any more since the new interface Clipboard::GetAllAvailableFormats needs to fetch them all.

Open in Gerrit

Related details

Attention is currently required from:

Lei Zhang

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Lei Zhang (Gerrit)

unread,

Mar 11, 2026, 8:25:44 PM (2 days ago) Mar 11

to Thomas Anderson, Lei Zhang, Chromium LUCI CQ, chromium...@chromium.org, Sadrul Chowdhury, ozone-...@chromium.org

Attention needed from Thomas Anderson

Lei Zhang voted and added 1 comment

Votes added by Lei Zhang

Code-Review

+1

1 comment

File ui/base/x/selection_requestor.cc

Line 80, Patchset 6 (Latest): std::reverse(remaining_types.begin(), remaining_types.end());

Lei Zhang . unresolved

Use `std::ranges::reverse_copy(types, std::back_inserter(remaining_types));` or some other way to do it in one shot.

Open in Gerrit

Related details

Attention is currently required from:

Thomas Anderson

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review
No-Unresolved-Comments
Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Thomas Anderson (Gerrit)

unread,

Mar 12, 2026, 5:03:57 PM (19 hours ago) Mar 12

to Lei Zhang, Chromium LUCI CQ, chromium...@chromium.org, Sadrul Chowdhury, ozone-...@chromium.org

Attention needed from Lei Zhang

Thomas Anderson voted and added 2 comments

Votes added by Thomas Anderson

Commit-Queue

+1

2 comments

Patchset-level comments

File-level comment, Patchset 7 (Latest):

Thomas Anderson . resolved

thestig@ ptal again. I've made a change in chrome/browser/ui/views/bookmarks/bookmark_bar_view.* to fix a failing test

File ui/base/x/selection_requestor.cc

Line 80, Patchset 6: std::reverse(remaining_types.begin(), remaining_types.end());

Lei Zhang . resolved

Use `std::ranges::reverse_copy(types, std::back_inserter(remaining_types));` or some other way to do it in one shot.

Thomas Anderson

Done

Open in Gerrit

Related details

Attention is currently required from:

Lei Zhang

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Lei Zhang (Gerrit)

unread,

Mar 12, 2026, 5:25:00 PM (19 hours ago) Mar 12

to Thomas Anderson, Lei Zhang, Chromium LUCI CQ, chromium...@chromium.org, Sadrul Chowdhury, ozone-...@chromium.org

Attention needed from Thomas Anderson

Lei Zhang added 1 comment

Patchset-level comments

File-level comment, Patchset 7 (Latest):

Thomas Anderson . resolved

thestig@ ptal again. I've made a change in chrome/browser/ui/views/bookmarks/bookmark_bar_view.* to fix a failing test

Lei Zhang

Please mention it in the CL description.

Open in Gerrit

Related details

Attention is currently required from:

Thomas Anderson

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Thomas Anderson (Gerrit)

unread,

Mar 12, 2026, 6:40:57 PM (17 hours ago) Mar 12

to Lei Zhang, Chromium LUCI CQ, chromium...@chromium.org, Sadrul Chowdhury, ozone-...@chromium.org

Attention needed from Lei Zhang

Thomas Anderson voted and added 1 comment

Votes added by Thomas Anderson

Commit-Queue

+1

1 comment

Patchset-level comments

File-level comment, Patchset 7:

Thomas Anderson . resolved

thestig@ ptal again. I've made a change in chrome/browser/ui/views/bookmarks/bookmark_bar_view.* to fix a failing test

Lei Zhang

Please mention it in the CL description.

Thomas Anderson

Done

Open in Gerrit

Related details

Attention is currently required from:

Lei Zhang

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Lei Zhang (Gerrit)

unread,

Mar 12, 2026, 7:03:45 PM (17 hours ago) Mar 12

to Thomas Anderson, AyeAye, Lei Zhang, Chromium LUCI CQ, chromium...@chromium.org, Sadrul Chowdhury, dtapuska+ch...@chromium.org, alexmo...@chromium.org, navigation...@chromium.org, creis...@chromium.org, ozone-...@chromium.org

Attention needed from Thomas Anderson

Lei Zhang added 1 comment

Patchset-level comments

File-level comment, Patchset 9 (Latest):

Lei Zhang . resolved

Still have red bots.

Open in Gerrit

Related details

Attention is currently required from:

Thomas Anderson

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Thomas Anderson (Gerrit)

unread,

Mar 12, 2026, 9:16:56 PM (15 hours ago) Mar 12

to AyeAye, Lei Zhang, Chromium LUCI CQ, chromium...@chromium.org, Sadrul Chowdhury, dtapuska+ch...@chromium.org, alexmo...@chromium.org, navigation...@chromium.org, creis...@chromium.org, ozone-...@chromium.org

Attention needed from Lei Zhang

Thomas Anderson added 1 comment

Patchset-level comments

File-level comment, Patchset 9:

Lei Zhang . resolved

Still have red bots.

Thomas Anderson

bots are green now

Open in Gerrit

Related details

Attention is currently required from:

Lei Zhang

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Lei Zhang (Gerrit)

unread,

Mar 12, 2026, 9:21:20 PM (15 hours ago) Mar 12

to Thomas Anderson, Lei Zhang, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, Sadrul Chowdhury, dtapuska+ch...@chromium.org, alexmo...@chromium.org, navigation...@chromium.org, creis...@chromium.org, ozone-...@chromium.org

Attention needed from Thomas Anderson

Lei Zhang voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Thomas Anderson

Submit Requirements:

Code-Coverage

Code-Owners
Code-Review

Review-Enforcement

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

Gerrit

satisfied_requirement

open

diffy

Thomas Anderson (Gerrit)

unread,

1:24 AM (11 hours ago) 1:24 AM

to Lei Zhang, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, Sadrul Chowdhury, dtapuska+ch...@chromium.org, alexmo...@chromium.org, navigation...@chromium.org, creis...@chromium.org, ozone-...@chromium.org

Thomas Anderson voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

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

Gerrit

satisfied_requirement

open

diffy

Thomas Anderson (Gerrit)

unread,

1:29 AM (11 hours ago) 1:29 AM

to Lei Zhang, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, Sadrul Chowdhury, dtapuska+ch...@chromium.org, alexmo...@chromium.org, navigation...@chromium.org, creis...@chromium.org, ozone-...@chromium.org

Thomas Anderson voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

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

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

1:39 AM (10 hours ago) 1:39 AM

to Thomas Anderson, Lei Zhang, AyeAye, chromium...@chromium.org, Sadrul Chowdhury, dtapuska+ch...@chromium.org, alexmo...@chromium.org, navigation...@chromium.org, creis...@chromium.org, ozone-...@chromium.org

Chromium LUCI CQ submitted the change

Unreviewed changes

11 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.

Change information

Commit message:

[X11] Convert clipboard backend to asynchronous architecture

This change refactors the X11 clipboard backend to eliminate synchronous
event dispatching and nested run-loops, which were prone to re-entrancy
issues and deadlocks.

Key changes:
- SelectionRequestor: Replaced PerformBlockingConvertSelection with an
  asynchronous PerformConvertSelection that uses callbacks. Introduced
  RequestTypesAsync for chained type requests and used base::OneShotTimer
  for timeouts.
- XClipboardHelper: Converted Read, GetTargetList, and IsFormatAvailable
  to asynchronous versions. Consolidated available type queries into
  GetAvailableMimeTypesAsync using base::BarrierCallback for parallel
  atom name resolution.
- X11ClipboardOzone: Integrated the new asynchronous helper methods into
  the Ozone platform clipboard.
- Unit Tests: Updated and re-enabled SelectionRequestorTest.NestedRequests
  to verify correct asynchronous behavior.
- Safety: Improved memory management in SelectionRequestor by ensuring
  re-entrancy safety in CompleteRequest and preventing use-after-free
  in AbortStaleRequests using WeakPtr checks.
- Performance: Optimized data transfer by using move semantics for
  clipboard data in callbacks.
- BookmarkBarView now adds the anchor highlight before obtaining the
  clipboard state to satisfy test expectations.

Bug: 40398800

Change-Id: Ifeda5e355934dfbe5bde6d38ee237f8e9b29ea9a

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7658864

Reviewed-by: Lei Zhang <the...@chromium.org>

Commit-Queue: Thomas Anderson <thomasa...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1598884}

Files:

M chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
M chrome/browser/ui/views/bookmarks/bookmark_bar_view.h
M ui/base/x/selection_requestor.cc
M ui/base/x/selection_requestor.h
M ui/base/x/selection_requestor_unittest.cc
M ui/base/x/x11_clipboard_helper.cc
M ui/base/x/x11_clipboard_helper.h
M ui/ozone/platform/x11/x11_clipboard_ozone.cc

Change size: L

Delta: 8 files changed, 431 insertions(+), 313 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Lei Zhang

Open in Gerrit

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

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

[X11] Convert clipboard backend to asynchronous architecture [chromium/src : main]

Thomas Anderson (Gerrit)

Thomas Anderson voted Commit-Queue+1

Related details

Lei Zhang (Gerrit)

Lei Zhang added 18 comments

Related details

Thomas Anderson (Gerrit)

Thomas Anderson voted and added 18 comments

Votes added by Thomas Anderson

18 comments

Related details

Lei Zhang (Gerrit)

Lei Zhang voted and added 1 comment

Votes added by Lei Zhang

1 comment

Related details

Thomas Anderson (Gerrit)

Thomas Anderson voted and added 2 comments

Votes added by Thomas Anderson

2 comments

Related details

Lei Zhang (Gerrit)

Lei Zhang added 1 comment

Related details

Thomas Anderson (Gerrit)

Thomas Anderson voted and added 1 comment

Votes added by Thomas Anderson

1 comment

Related details

Lei Zhang (Gerrit)

Lei Zhang added 1 comment

Related details

Thomas Anderson (Gerrit)

Thomas Anderson added 1 comment

Related details

Lei Zhang (Gerrit)

Lei Zhang voted Code-Review+1

Related details

Thomas Anderson (Gerrit)

Thomas Anderson voted Commit-Queue+2

Related details

Thomas Anderson (Gerrit)

Thomas Anderson voted Commit-Queue+2

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Unreviewed changes

Change information