Tove Petersson (Gerrit)

unread,

Nov 10, 2025, 10:29:26 AM (2 days ago) Nov 10

to Johannes Kron, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, Rijubrata Bhaumik, mac-r...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, jophba...@chromium.org, mfoltz+wa...@chromium.org

Attention needed from Johannes Kron

Tove Petersson voted Commit-Queue+1

Commit-Queue

+1

Open in Gerrit

Related details

Attention is currently required from:

Johannes Kron

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

Johannes Kron (Gerrit)

unread,

Nov 11, 2025, 10:41:43 AM (yesterday) Nov 11

to Tove Petersson, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, Rijubrata Bhaumik, mac-r...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, jophba...@chromium.org, mfoltz+wa...@chromium.org

Attention needed from Tove Petersson

Johannes Kron voted and added 4 comments

Votes added by Johannes Kron

Code-Review

+1

4 comments

Patchset-level comments

File-level comment, Patchset 10 (Latest):

Johannes Kron . resolved

LGTM
Just two minor comments.

File content/browser/media/capture/pip_screen_capture_coordinator_proxy_impl.h

Line 49, Patchset 10 (Latest): mutable base::WeakPtrFactory<PipScreenCaptureCoordinatorProxyImpl>

Johannes Kron . unresolved

Is this still needed? Or is it something you anticipate we'll need to do in the future?

Line 38, Patchset 10 (Latest):

  base::WeakPtr<PipScreenCaptureCoordinatorImpl> coordinator_;
  std::optional<NativeWindowId> pip_window_id_;
  scoped_refptr<base::SequencedTaskRunner> bound_sequence_task_runner_;
  base::ObserverList<PipScreenCaptureCoordinatorProxy::Observer> observers_;
  std::unique_ptr<UiThreadObserver, base::OnTaskRunnerDeleter>
      ui_thread_observer_;

Johannes Kron . unresolved

You may add GUARDED_BY_CONTEXT(sequence_checker_) to the member variables for an extra layer of safety.

File content/browser/media/capture/pip_screen_capture_coordinator_proxy_impl.cc

Line 72, Patchset 10 (Latest):

  // `ui_thread_observer_` is automatically deleted on the UI thread via
  // `OnTaskRunnerDeleter`, which will unregister the observer.

Johannes Kron . resolved

Nice, I didn't know of this feature.

Open in Gerrit

Related details

Attention is currently required from:

Tove Petersson

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

Tove Petersson (Gerrit)

unread,

4:57 AM (13 hours ago) 4:57 AM

to Johannes Kron, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, Rijubrata Bhaumik, mac-r...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, jophba...@chromium.org, mfoltz+wa...@chromium.org

Tove Petersson added 2 comments

File content/browser/media/capture/pip_screen_capture_coordinator_proxy_impl.h

Line 49, Patchset 10: mutable base::WeakPtrFactory<PipScreenCaptureCoordinatorProxyImpl>

Johannes Kron . resolved

Is this still needed? Or is it something you anticipate we'll need to do in the future?

Tove Petersson

No, it's no longer needed.

Line 38, Patchset 10: base::WeakPtr<PipScreenCaptureCoordinatorImpl> coordinator_;


  std::optional<NativeWindowId> pip_window_id_;
  scoped_refptr<base::SequencedTaskRunner> bound_sequence_task_runner_;
  base::ObserverList<PipScreenCaptureCoordinatorProxy::Observer> observers_;
  std::unique_ptr<UiThreadObserver, base::OnTaskRunnerDeleter>
      ui_thread_observer_;

Johannes Kron . resolved

You may add GUARDED_BY_CONTEXT(sequence_checker_) to the member variables for an extra layer of safety.

Tove Petersson

Done

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

Tove Petersson (Gerrit)

unread,

5:01 AM (13 hours ago) 5:01 AM

to Johannes Kron, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, Rijubrata Bhaumik, mac-r...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, jophba...@chromium.org, mfoltz+wa...@chromium.org

Tove Petersson 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,

5:14 AM (13 hours ago) 5:14 AM

to Tove Petersson, Johannes Kron, AyeAye, chromium...@chromium.org, Rijubrata Bhaumik, mac-r...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, jophba...@chromium.org, mfoltz+wa...@chromium.org

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

10 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: content/browser/media/capture/pip_screen_capture_coordinator_proxy_impl.h
Insertions: 11, Deletions: 9.

@@ -35,19 +35,21 @@
 
   void SetPipWindowId(const std::optional<NativeWindowId>& new_pip_window_id);
 
-  base::WeakPtr<PipScreenCaptureCoordinatorImpl> coordinator_;
-  std::optional<NativeWindowId> pip_window_id_;
-  scoped_refptr<base::SequencedTaskRunner> bound_sequence_task_runner_;
-  base::ObserverList<PipScreenCaptureCoordinatorProxy::Observer> observers_;
+  base::WeakPtr<PipScreenCaptureCoordinatorImpl> coordinator_
+      GUARDED_BY_CONTEXT(sequence_checker_);
+  std::optional<NativeWindowId> pip_window_id_
+      GUARDED_BY_CONTEXT(sequence_checker_);
+  scoped_refptr<base::SequencedTaskRunner> bound_sequence_task_runner_
+      GUARDED_BY_CONTEXT(sequence_checker_);
+  base::ObserverList<PipScreenCaptureCoordinatorProxy::Observer> observers_
+      GUARDED_BY_CONTEXT(sequence_checker_);


   std::unique_ptr<UiThreadObserver, base::OnTaskRunnerDeleter>

-      ui_thread_observer_;
+      ui_thread_observer_ GUARDED_BY_CONTEXT(sequence_checker_);
 
   SEQUENCE_CHECKER(sequence_checker_);
 
-  // The weak factory is mutable to allow `GetWeakPtr()` to be called from
-  // const methods.
-  mutable base::WeakPtrFactory<PipScreenCaptureCoordinatorProxyImpl>
-      weak_factory_{this};
+  base::WeakPtrFactory<PipScreenCaptureCoordinatorProxyImpl> weak_factory_{
+      this};
 };
 
 }  // namespace content
```

Change information

Commit message:

Added PipScreenCaptureCoordinatorProxy for device-UI-thread coordination

This change introduces a proxy mechanism to enable cross-thread
communication for tracking Picture-in-Picture (PiP) window states. The
screen capturer, which operates on the device thread, needs to be aware
of PiP windows managed by the UI thread in order to exclude them from
screen captures.

The `PipScreenCaptureCoordinator`, which lives on the UI thread, can now
create a proxy. This proxy can be used by components on other threads,
such as the screen capturer on the device thread, to safely observe the
PiP window's ID.

In the first iteration, PiP-exclusion is implemented for macOS, so
PipScreenCaptureCoordinatorProxyImpl is initially compiled for macOS
only.

Bug: 445202459

Change-Id: Ifab6b17ad58864611c86582522c7c68b5981513b

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

Reviewed-by: Johannes Kron <kr...@chromium.org>

Commit-Queue: Tove Petersson <to...@chromium.org>

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

Files:

M content/browser/BUILD.gn
M content/browser/media/capture/pip_screen_capture_coordinator.cc
M content/browser/media/capture/pip_screen_capture_coordinator.h
M content/browser/media/capture/pip_screen_capture_coordinator_impl.cc
M content/browser/media/capture/pip_screen_capture_coordinator_impl.h
M content/browser/media/capture/pip_screen_capture_coordinator_impl_unittest.cc
A content/browser/media/capture/pip_screen_capture_coordinator_proxy.h
A content/browser/media/capture/pip_screen_capture_coordinator_proxy_impl.cc
A content/browser/media/capture/pip_screen_capture_coordinator_proxy_impl.h

Change size: L

Delta: 9 files changed, 344 insertions(+), 0 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Johannes Kron

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

Added PipScreenCaptureCoordinatorProxy for device-UI-thread coordination [chromium/src : main]

Tove Petersson (Gerrit)

Tove Petersson voted Commit-Queue+1

Related details

Johannes Kron (Gerrit)

Johannes Kron voted and added 4 comments

Votes added by Johannes Kron

4 comments

Related details

Tove Petersson (Gerrit)

Tove Petersson added 2 comments

Related details

Tove Petersson (Gerrit)

Tove Petersson voted Commit-Queue+2

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

Change information