[Media Capture] new picker invoker use delegate impl from clank [chromium/src : main]

0 views
Skip to first unread message

Grace Cham (Gerrit)

unread,
Dec 12, 2025, 12:22:56 AMDec 12
to Calder Kitagawa, chromium...@chromium.org, feature-me...@chromium.org
Attention needed from Calder Kitagawa

Grace Cham added 1 comment

File chrome/android/java/src/org/chromium/chrome/browser/media/MediaCapturePickerInvoker.java
Line 41, Patchset 1 (Latest): if (action
== MediaCapturePickerHeadlessFragment.CaptureAction
.CAPTURE_WINDOW) {
org.chromium.content_public.browser.WebContents tabWebContents =
impl.getPickedWebContents();
if (tabWebContents != null) {
delegate.onPickTab(
tabWebContents,
impl.getPickedAudioSharePreference());
} else {
delegate.onPickWindow();
}
} else if (action
Grace Cham . unresolved

Note: this is the only meaningful action branch for now (other than cancel action), since action is always set to `CAPTURE_WINDOW`, see comment in [the launcher's callback](https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/java/src/org/chromium/chrome/browser/media/MediaCapturePickerHeadlessFragment.java;l=97?q=CaptureAction%20MediaCapture&ss=chromium). I keep the `CAPTURE_SCREEN` action branch like the current [MediaCapturePickerDialog](https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/java/src/org/chromium/chrome/browser/media/MediaCapturePickerDialog.java;l=195).
Please let me know if you think there's a better way to handle this!

Open in Gerrit

Related details

Attention is currently required from:
  • Calder Kitagawa
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: If51bf09ca5d4977aa6a14fe099d3e6a901b7ebc0
Gerrit-Change-Number: 7250408
Gerrit-PatchSet: 1
Gerrit-Owner: Grace Cham <hsc...@chromium.org>
Gerrit-Reviewer: Calder Kitagawa <ckit...@chromium.org>
Gerrit-Attention: Calder Kitagawa <ckit...@chromium.org>
Gerrit-Comment-Date: Fri, 12 Dec 2025 05:22:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Calder Kitagawa (Gerrit)

unread,
Dec 12, 2025, 11:07:17 AMDec 12
to Grace Cham, chromium...@chromium.org, feature-me...@chromium.org
Attention needed from Grace Cham

Calder Kitagawa added 9 comments

Patchset-level comments
File-level comment, Patchset 1 (Latest):
Calder Kitagawa . unresolved

Is it possible to add tests?

File chrome/android/java/src/org/chromium/chrome/browser/media/MediaCapturePickerHeadlessFragment.java
Line 109, Patchset 1 (Latest): startAndroidCapturePrompt(delegate, null);
Calder Kitagawa . unresolved
```suggestion
        startAndroidCapturePrompt(delegate, /* intent= */ null);
```
Line 115, Patchset 1 (Latest): if (intent != null) {
mLauncher.launch(intent);
} else {
mLauncher.launch(mMediaProjectionManager.createScreenCaptureIntent());
}
Calder Kitagawa . unresolved
```suggestion
        mLauncher.launch(intent != null ? intent :
            mMediaProjectionManager.createScreenCaptureIntent());
```
File chrome/android/java/src/org/chromium/chrome/browser/media/MediaCapturePickerInvoker.java
Line 35, Patchset 1 (Latest): != MediaCapturePickerHeadlessFragment.CaptureAction
.CAPTURE_CANCELLED) {
Calder Kitagawa . unresolved

statically import.

Line 37, Patchset 1 (Latest): org.chromium.content_public.browser.media.capture.ScreenCapture
Calder Kitagawa . unresolved

import this.

Line 42, Patchset 1 (Latest): == MediaCapturePickerHeadlessFragment.CaptureAction
.CAPTURE_WINDOW) {
Calder Kitagawa . unresolved

statically import.

Line 44, Patchset 1 (Latest): org.chromium.content_public.browser.WebContents tabWebContents =
Calder Kitagawa . unresolved

import this.

Line 33, Patchset 1 (Latest): (action, result) -> {
if (action
!= MediaCapturePickerHeadlessFragment.CaptureAction
.CAPTURE_CANCELLED) {
org.chromium.content_public.browser.media.capture.ScreenCapture
.onPick(params.webContents, result);

}

if (action
== MediaCapturePickerHeadlessFragment.CaptureAction
.CAPTURE_WINDOW) {
org.chromium.content_public.browser.WebContents tabWebContents =
impl.getPickedWebContents();
if (tabWebContents != null) {
delegate.onPickTab(
tabWebContents,
impl.getPickedAudioSharePreference());
} else {
delegate.onPickWindow();
}
} else if (action
== MediaCapturePickerHeadlessFragment.CaptureAction
.CAPTURE_SCREEN) {
delegate.onPickScreen();
} else {
delegate.onCancel();
}
Calder Kitagawa . unresolved

Can we pull this out into a helper method as well?

Line 27, Patchset 1 (Latest): if (impl != null) {
android.content.Intent intent = impl.createScreenCaptureIntent(context, params);
if (intent != null) {
var fragment = MediaCapturePickerHeadlessFragment.getInstanceForCurrentActivity();
if (fragment != null) {
fragment.startAndroidCapturePrompt(
(action, result) -> {
if (action
!= MediaCapturePickerHeadlessFragment.CaptureAction
.CAPTURE_CANCELLED) {
org.chromium.content_public.browser.media.capture.ScreenCapture
.onPick(params.webContents, result);

}

if (action
== MediaCapturePickerHeadlessFragment.CaptureAction
.CAPTURE_WINDOW) {
org.chromium.content_public.browser.WebContents tabWebContents =
impl.getPickedWebContents();
if (tabWebContents != null) {
delegate.onPickTab(
tabWebContents,
impl.getPickedAudioSharePreference());
} else {
delegate.onPickWindow();
}
} else if (action
== MediaCapturePickerHeadlessFragment.CaptureAction
.CAPTURE_SCREEN) {
delegate.onPickScreen();
} else {
delegate.onCancel();
}
},
intent);
return;
}
}
}
Calder Kitagawa . unresolved

Please invert some of the nesting so this is more readable. If you need another inner helper function to ensure `delegate.onCancel` is called in error paths please do so.

Open in Gerrit

Related details

Attention is currently required from:
  • Grace Cham
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: If51bf09ca5d4977aa6a14fe099d3e6a901b7ebc0
Gerrit-Change-Number: 7250408
Gerrit-PatchSet: 1
Gerrit-Owner: Grace Cham <hsc...@chromium.org>
Gerrit-Reviewer: Calder Kitagawa <ckit...@chromium.org>
Gerrit-Attention: Grace Cham <hsc...@chromium.org>
Gerrit-Comment-Date: Fri, 12 Dec 2025 16:07:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Grace Cham (Gerrit)

unread,
Dec 26, 2025, 12:54:22 AM (yesterday) Dec 26
to Rijubrata Bhaumik, AyeAye, Chromium LUCI CQ, Calder Kitagawa, chromium...@chromium.org, chfreme...@chromium.org, jophba...@chromium.org, mfoltz+wa...@chromium.org, feature-me...@chromium.org
Attention needed from Calder Kitagawa

Grace Cham added 10 comments

Patchset-level comments
Calder Kitagawa . unresolved

Is it possible to add tests?

Grace Cham

Added tests for MediaCapturePickerInvoker to check that the right function is called depending on the startAndroidCapturePrompt returned action and (fake) delegate state.
I think there's not much we could test with startAndroidCapturePrompt for now. After the UI is stable, we could add an integrated test that checks for UI elements / views and perform clicks on it. WDYT?

File-level comment, Patchset 3 (Latest):
Grace Cham . resolved

Thank you very much!

File chrome/android/java/src/org/chromium/chrome/browser/media/MediaCapturePickerHeadlessFragment.java
Line 109, Patchset 1: startAndroidCapturePrompt(delegate, null);
Calder Kitagawa . resolved
```suggestion
        startAndroidCapturePrompt(delegate, /* intent= */ null);
```
Grace Cham

Done

Line 115, Patchset 1: if (intent != null) {
mLauncher.launch(intent);
} else {
mLauncher.launch(mMediaProjectionManager.createScreenCaptureIntent());
}
Calder Kitagawa . resolved
```suggestion
        mLauncher.launch(intent != null ? intent :
            mMediaProjectionManager.createScreenCaptureIntent());
```
Grace Cham

Done

File chrome/android/java/src/org/chromium/chrome/browser/media/MediaCapturePickerInvoker.java
Line 35, Patchset 1: != MediaCapturePickerHeadlessFragment.CaptureAction
.CAPTURE_CANCELLED) {
Calder Kitagawa . resolved

statically import.

Grace Cham

Done

Line 37, Patchset 1: org.chromium.content_public.browser.media.capture.ScreenCapture
Calder Kitagawa . resolved

import this.

Grace Cham

Done

Line 42, Patchset 1: == MediaCapturePickerHeadlessFragment.CaptureAction
.CAPTURE_WINDOW) {
Calder Kitagawa . resolved

statically import.

Grace Cham

Done

Line 44, Patchset 1: org.chromium.content_public.browser.WebContents tabWebContents =
Calder Kitagawa . resolved

import this.

Grace Cham

Done

Line 33, Patchset 1: (action, result) -> {

if (action
!= MediaCapturePickerHeadlessFragment.CaptureAction
.CAPTURE_CANCELLED) {
org.chromium.content_public.browser.media.capture.ScreenCapture
.onPick(params.webContents, result);
}

if (action
== MediaCapturePickerHeadlessFragment.CaptureAction
.CAPTURE_WINDOW) {
org.chromium.content_public.browser.WebContents tabWebContents =
impl.getPickedWebContents();
if (tabWebContents != null) {
delegate.onPickTab(
tabWebContents,
impl.getPickedAudioSharePreference());
} else {
delegate.onPickWindow();
}
} else if (action
== MediaCapturePickerHeadlessFragment.CaptureAction
.CAPTURE_SCREEN) {
delegate.onPickScreen();
} else {
delegate.onCancel();
}
Calder Kitagawa . resolved

Can we pull this out into a helper method as well?

Grace Cham

Done

Line 27, Patchset 1: if (impl != null) {
Calder Kitagawa . resolved

Please invert some of the nesting so this is more readable. If you need another inner helper function to ensure `delegate.onCancel` is called in error paths please do so.

Grace Cham

Done - please reopen if there's other suggestion for refactoring for readbility!

Open in Gerrit

Related details

Attention is currently required from:
  • Calder Kitagawa
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: If51bf09ca5d4977aa6a14fe099d3e6a901b7ebc0
Gerrit-Change-Number: 7250408
Gerrit-PatchSet: 3
Gerrit-Owner: Grace Cham <hsc...@chromium.org>
Gerrit-Reviewer: Calder Kitagawa <ckit...@chromium.org>
Gerrit-Reviewer: Grace Cham <hsc...@chromium.org>
Gerrit-CC: Rijubrata Bhaumik <rijubrat...@intel.com>
Gerrit-Attention: Calder Kitagawa <ckit...@chromium.org>
Gerrit-Comment-Date: Fri, 26 Dec 2025 05:53:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Calder Kitagawa <ckit...@chromium.org>
satisfied_requirement
unsatisfied_requirement
open
diffy
Reply all
Reply to author
Forward
0 new messages