Simon Hangl (Gerrit)

unread,

Jan 20, 2025, 7:05:12 AM1/20/25

to Colin Blundell, Chromium LUCI CQ, chromium...@chromium.org, Rijubrata Bhaumik, alexmo...@chromium.org, chfreme...@chromium.org, creis...@chromium.org, feature-me...@chromium.org, ffred...@chromium.org, jophba...@chromium.org, mfoltz...@chromium.org, navigation...@chromium.org, oshima...@chromium.org, tluk+...@chromium.org

Attention needed from Colin Blundell

Simon Hangl voted and added 1 comment

Votes added by Simon Hangl

Commit-Queue

+1

1 comment

Patchset-level comments

File-level comment, Patchset 3 (Latest):

Simon Hangl . resolved

blundell@ PTAL.

Open in Gerrit

Related details

Attention is currently required from:

Colin Blundell

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Colin Blundell (Gerrit)

unread,

Jan 20, 2025, 7:54:57 AM1/20/25

to Simon Hangl, Tricium, Colin Blundell, Chromium LUCI CQ, chromium...@chromium.org, Rijubrata Bhaumik, alexmo...@chromium.org, chfreme...@chromium.org, creis...@chromium.org, feature-me...@chromium.org, ffred...@chromium.org, jophba...@chromium.org, mfoltz...@chromium.org, navigation...@chromium.org, oshima...@chromium.org, tluk+...@chromium.org

Attention needed from Simon Hangl

Colin Blundell added 1 comment

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Colin Blundell . resolved

Thanks, Simon! Is there someone with context on the code/effort here who can do a technical review first? This looks like a non-trivial change, and I'm not familiar enough with the context to be the "main" reviewer.

Open in Gerrit

Related details

Attention is currently required from:

Simon Hangl

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Simon Hangl (Gerrit)

unread,

Jan 21, 2025, 8:47:42 AM1/21/25

to Elad Alon, Tricium, Colin Blundell, Chromium LUCI CQ, chromium...@chromium.org, Rijubrata Bhaumik, alexmo...@chromium.org, chfreme...@chromium.org, creis...@chromium.org, feature-me...@chromium.org, ffred...@chromium.org, jophba...@chromium.org, mfoltz...@chromium.org, navigation...@chromium.org, oshima...@chromium.org, tluk+...@chromium.org

Attention needed from Colin Blundell and Elad Alon

Simon Hangl added 2 comments

Patchset-level comments

File-level comment, Patchset 4:

Colin Blundell . resolved

Thanks, Simon! Is there someone with context on the code/effort here who can do a technical review first? This looks like a non-trivial change, and I'm not familiar enough with the context to be the "main" reviewer.

Simon Hangl

thanks colin. adding eladalon@ to the review.

File-level comment, Patchset 5 (Latest):

Simon Hangl . resolved

eladalon@ PTAL (see [here](https://chromium-review.googlesource.com/c/chromium/src/+/6172344/comments/847c20b6_b17dd937) for context).

Open in Gerrit

Related details

Attention is currently required from:

Colin Blundell
Elad Alon

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Elad Alon (Gerrit)

unread,

Jan 21, 2025, 2:41:41 PM1/21/25

to Simon Hangl, Tricium, Colin Blundell, Chromium LUCI CQ, chromium...@chromium.org, Rijubrata Bhaumik, alexmo...@chromium.org, chfreme...@chromium.org, creis...@chromium.org, feature-me...@chromium.org, ffred...@chromium.org, jophba...@chromium.org, mfoltz...@chromium.org, navigation...@chromium.org, oshima...@chromium.org, tluk+...@chromium.org

Attention needed from Colin Blundell and Simon Hangl

Elad Alon added 4 comments

File ash/multi_capture/multi_capture_service.h

Line 20, Patchset 5 (Latest): public:

Elad Alon . unresolved

Should these four methods be marked pure virtual?

Line 17, Patchset 5 (Latest):class ASH_EXPORT MultiCaptureService {

Elad Alon . unresolved

This class appears to wrap `ObserverList<Observer>` so tightly that it is not immediately evident to me what the add value of wrapper is. Could you please clarify?

File ash/shell.h

Line 1301, Patchset 5 (Latest): std::unique_ptr<MultiCaptureService> multi_capture_service_;

Elad Alon . unresolved

Would it make sense for this to be `base::ObserverList<MultiCaptureServiceObserver> observers_;`? That is, get rid of the wrapper around the ObserverList and have the observer class as unnested.

File chrome/browser/ash/multi_screen_capture/multi_capture_service.cc

File-level comment, Patchset 5 (Latest):

Elad Alon . unresolved

Please ensure a reviewer familiar with Ash.

Open in Gerrit

Related details

Attention is currently required from:

Colin Blundell
Simon Hangl

Submit Requirements:

Code-Coverage
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

Simon Hangl (Gerrit)

unread,

Jan 31, 2025, 9:10:21 AM1/31/25

to Elad Alon, Tricium, Colin Blundell, Chromium LUCI CQ, chromium...@chromium.org, Rijubrata Bhaumik, alexmo...@chromium.org, chfreme...@chromium.org, creis...@chromium.org, feature-me...@chromium.org, ffred...@chromium.org, jophba...@chromium.org, mfoltz...@chromium.org, navigation...@chromium.org, oshima...@chromium.org, tluk+...@chromium.org

Attention needed from Colin Blundell and Elad Alon

Simon Hangl added 4 comments

Patchset-level comments

File-level comment, Patchset 6 (Latest):

Simon Hangl . resolved

eladalon@, i implemented your suggestions. PTAL.
colinblundell@, are you willing to review the ash part, or should i include another reviewer?

File ash/multi_capture/multi_capture_service.h

Line 20, Patchset 5: public:

Elad Alon . resolved

Should these four methods be marked pure virtual?

Simon Hangl

Done

Line 17, Patchset 5:class ASH_EXPORT MultiCaptureService {

Elad Alon . resolved

This class appears to wrap `ObserverList<Observer>` so tightly that it is not immediately evident to me what the add value of wrapper is. Could you please clarify?

Simon Hangl

removed the wrapper class as you suggested.

File ash/shell.h

Line 1301, Patchset 5: std::unique_ptr<MultiCaptureService> multi_capture_service_;

Elad Alon . resolved

Would it make sense for this to be `base::ObserverList<MultiCaptureServiceObserver> observers_;`? That is, get rid of the wrapper around the ObserverList and have the observer class as unnested.

Simon Hangl

Done

Open in Gerrit

Related details

Attention is currently required from:

Colin Blundell
Elad Alon

Submit Requirements:

Code-Coverage
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

Simon Hangl (Gerrit)

unread,

Jan 31, 2025, 9:11:07 AM1/31/25

to Elad Alon, Tricium, Colin Blundell, Chromium LUCI CQ, chromium...@chromium.org, Rijubrata Bhaumik, alexmo...@chromium.org, chfreme...@chromium.org, creis...@chromium.org, feature-me...@chromium.org, ffred...@chromium.org, jophba...@chromium.org, mfoltz...@chromium.org, navigation...@chromium.org, oshima...@chromium.org, tluk+...@chromium.org

Attention needed from Colin Blundell and Elad Alon

Simon Hangl added 1 comment

File chrome/browser/ash/multi_screen_capture/multi_capture_service.cc

File-level comment, Patchset 5:

Elad Alon . unresolved

Please ensure a reviewer familiar with Ash.

Simon Hangl

blundell@, FYI.

satisfied_requirement

unsatisfied_requirement

open

diffy

Colin Blundell (Gerrit)

unread,

Jan 31, 2025, 9:12:15 AM1/31/25

to Simon Hangl, Mitsuru Oshima, Elad Alon, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Rijubrata Bhaumik, alexmo...@chromium.org, chfreme...@chromium.org, creis...@chromium.org, feature-me...@chromium.org, ffred...@chromium.org, jophba...@chromium.org, mfoltz...@chromium.org, navigation...@chromium.org, oshima...@chromium.org, tluk+...@chromium.org, Colin Blundell

Attention needed from Elad Alon, Mitsuru Oshima and Simon Hangl

Colin Blundell added 1 comment

Patchset-level comments

File-level comment, Patchset 6 (Latest):

Colin Blundell . resolved

Thanks!

blundell@ -> oshima@ for //ash

Open in Gerrit

Related details

Attention is currently required from:

Elad Alon
Mitsuru Oshima
Simon Hangl

Submit Requirements:

Code-Coverage
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

Elad Alon (Gerrit)

unread,

Jan 31, 2025, 9:49:22 AM1/31/25

to Simon Hangl, Mitsuru Oshima, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Rijubrata Bhaumik, alexmo...@chromium.org, chfreme...@chromium.org, creis...@chromium.org, feature-me...@chromium.org, ffred...@chromium.org, jophba...@chromium.org, mfoltz...@chromium.org, navigation...@chromium.org, oshima...@chromium.org, tluk+...@chromium.org

Attention needed from Mitsuru Oshima and Simon Hangl

Elad Alon voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Mitsuru Oshima
Simon Hangl

Submit Requirements:

Code-Coverage
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

Mitsuru Oshima (Gerrit)

unread,

Jan 31, 2025, 6:39:50 PM1/31/25

to Simon Hangl, Elad Alon, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Rijubrata Bhaumik, alexmo...@chromium.org, chfreme...@chromium.org, creis...@chromium.org, feature-me...@chromium.org, ffred...@chromium.org, jophba...@chromium.org, mfoltz...@chromium.org, navigation...@chromium.org, oshima...@chromium.org, tluk+...@chromium.org

Attention needed from Simon Hangl

Mitsuru Oshima added 3 comments

Commit Message

Line 15, Patchset 6 (Latest):The multi capture service is moved to `chrome/browser/ash/` in line

Mitsuru Oshima . unresolved

We should not add/move code in chrome/browser/ash unless there is a good reason to do so. (see Desired state section: https://chromium.googlesource.com/chromium/src/+/HEAD/docs/chromeos/code.md#desired-state)

Line 17, Patchset 6 (Latest):

Mitsuru Oshima . unresolved

Do you have a bug #?

If none, thne Bug: None

File chrome/browser/ash/crosapi/multi_capture_service_ash.cc

Line 55, Patchset 6 (Latest): }

Mitsuru Oshima . unresolved

+hidehiko@ for his input

This pattern is a bit odd. Given that

1) ash code has to be moved out from chrome/browser
2) crosai iml can be in //ash

Can you move the service impl in ash, add delegate for IsXxxAllowed methods, then inject?

Open in Gerrit

Related details

Attention is currently required from:

Simon Hangl

Submit Requirements:

Code-Coverage
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

Mitsuru Oshima (Gerrit)

unread,

Feb 3, 2025, 11:45:01 AM2/3/25

to Simon Hangl, Hidehiko Abe, Elad Alon, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Rijubrata Bhaumik, alexmo...@chromium.org, chfreme...@chromium.org, creis...@chromium.org, feature-me...@chromium.org, ffred...@chromium.org, jophba...@chromium.org, mfoltz...@chromium.org, navigation...@chromium.org, oshima...@chromium.org, tluk+...@chromium.org

Attention needed from Hidehiko Abe and Simon Hangl

Mitsuru Oshima added 1 comment

Patchset-level comments

File-level comment, Patchset 6 (Latest):

Mitsuru Oshima . resolved

sorry, looks like I didn't add hidehiko@ when I submitted my comments. +hidehiko@

Open in Gerrit

Related details

Attention is currently required from:

Hidehiko Abe
Simon Hangl

Submit Requirements:

Code-Coverage
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

Hidehiko Abe (Gerrit)

unread,

Feb 3, 2025, 11:56:38 AM2/3/25

to Simon Hangl, Elad Alon, Mitsuru Oshima, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Rijubrata Bhaumik, alexmo...@chromium.org, chfreme...@chromium.org, creis...@chromium.org, feature-me...@chromium.org, ffred...@chromium.org, jophba...@chromium.org, mfoltz...@chromium.org, navigation...@chromium.org, oshima...@chromium.org, tluk+...@chromium.org

Attention needed from Simon Hangl

Hidehiko Abe added 3 comments

Patchset-level comments

File-level comment, Patchset 6 (Latest):

Hidehiko Abe . resolved

I'll take a look whole code tomorrow my working hour. Just quick reply to the comment to move forward.

File-level comment, Patchset 6 (Latest):

Hidehiko Abe . resolved

I'll take a look whole code tomorrow my working hour. Just quick reply to the comment to move forward the discussion.

File chrome/browser/ash/crosapi/multi_capture_service_ash.cc

Line 55, Patchset 6 (Latest): }

Mitsuru Oshima . unresolved

+hidehiko@ for his input
This pattern is a bit odd. Given that
1) ash code has to be moved out from chrome/browser
2) crosai iml can be in //ash
Can you move the service impl in ash, add delegate for IsXxxAllowed methods, then inject?

Hidehiko Abe

chrome/browser/ash/policy/multi_screen_capture/multi_screen_capture_policy_service.cc

does not look like depending on //chrome actually. Can whole code move out from //chrome, then?

Open in Gerrit

Related details

Attention is currently required from:

Simon Hangl

Submit Requirements:

Code-Coverage
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

Simon Hangl (Gerrit)

unread,

Feb 18, 2025, 1:48:39 PM2/18/25

to Hidehiko Abe, Elad Alon, Mitsuru Oshima, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Rijubrata Bhaumik, alexmo...@chromium.org, chfreme...@chromium.org, creis...@chromium.org, feature-me...@chromium.org, ffred...@chromium.org, jophba...@chromium.org, mfoltz...@chromium.org, navigation...@chromium.org, oshima...@chromium.org, tluk+...@chromium.org

Attention needed from Elad Alon, Hidehiko Abe and Mitsuru Oshima

Simon Hangl added 2 comments

Patchset-level comments

File-level comment, Patchset 7 (Latest):

Simon Hangl . resolved

oshima@, hidehiko@ PTAL (see also the note below about what exactly I changed).

File chrome/browser/ash/crosapi/multi_capture_service_ash.cc

Line 55, Patchset 6: }

Mitsuru Oshima . unresolved

+hidehiko@ for his input
This pattern is a bit odd. Given that
1) ash code has to be moved out from chrome/browser
2) crosai iml can be in //ash
Can you move the service impl in ash, add delegate for IsXxxAllowed methods, then inject?

Hidehiko Abe

chrome/browser/ash/policy/multi_screen_capture/multi_screen_capture_policy_service.cc
does not look like depending on //chrome actually. Can whole code move out from //chrome, then?

Simon Hangl

Following up on our quick conversation I had with Hidehiko in chat, I implemented your suggestion by:

keeping `MultiCaptureServiceClient` but renaming it to `MultiCaptureService` and rename the notify functions to `NotifyMultiCapture*`.
calling into the `ash::Shell` directly from `ChromeContentBrowserClient`.
deleting the mojom parts.

Todos I inted to implement in follow-up CLs:

remove the crosapi implementation (I kept it for now as I think some tests might be affected).
move the `IsMultiCaptureAllowed*` oracles to the multi capture service.

Open in Gerrit

Related details

Attention is currently required from:

Elad Alon
Hidehiko Abe
Mitsuru Oshima

Submit Requirements:

Code-Coverage
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

Hidehiko Abe (Gerrit)

unread,

Feb 18, 2025, 11:29:10 PM2/18/25

to Simon Hangl, Elad Alon, Mitsuru Oshima, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Rijubrata Bhaumik, alexmo...@chromium.org, chfreme...@chromium.org, creis...@chromium.org, feature-me...@chromium.org, ffred...@chromium.org, jophba...@chromium.org, mfoltz...@chromium.org, navigation...@chromium.org, oshima...@chromium.org, tluk+...@chromium.org

Attention needed from Elad Alon, Mitsuru Oshima and Simon Hangl

Hidehiko Abe voted and added 4 comments

Votes added by Hidehiko Abe

Code-Review

+1

4 comments

Patchset-level comments

File-level comment, Patchset 7 (Latest):

Hidehiko Abe . resolved

LGTM with some comments and defer to other reviewers.

File ash/multi_capture/multi_capture_service.h

Line 13, Patchset 7 (Latest):

#include "mojo/public/cpp/bindings/receiver.h"
#include "mojo/public/cpp/bindings/remote.h"

Hidehiko Abe . unresolved

no longer needed.

File ash/multi_capture/multi_capture_service.cc

Line 14, Patchset 7 (Latest):

  for (Observer& observer : observers_) {
    observer.MultiCaptureServiceDestroyed();
  }

Hidehiko Abe . unresolved

nit/style/optional:
`observers_.Notify(&Observer::MultiCaptureServiceDestroyed);`
Ditto for all below cases.

File chrome/browser/ash/notifications/multi_capture_notifications.h

Line 21, Patchset 7 (Latest):}

Hidehiko Abe . unresolved

nit: `} // namespace base` for consistency.

Open in Gerrit

Related details

Attention is currently required from:

Elad Alon
Mitsuru Oshima
Simon Hangl

Submit Requirements:

Code-Coverage
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

Simon Hangl (Gerrit)

unread,

Feb 19, 2025, 8:24:59 AM2/19/25

to Hidehiko Abe, Elad Alon, Mitsuru Oshima, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Rijubrata Bhaumik, alexmo...@chromium.org, chfreme...@chromium.org, creis...@chromium.org, feature-me...@chromium.org, ffred...@chromium.org, jophba...@chromium.org, mfoltz...@chromium.org, navigation...@chromium.org, oshima...@chromium.org, tluk+...@chromium.org

Attention needed from Elad Alon and Mitsuru Oshima

Simon Hangl voted and added 5 comments

Votes added by Simon Hangl

Commit-Queue

+1

5 comments

Commit Message

Line 15, Patchset 6:The multi capture service is moved to `chrome/browser/ash/` in line

Mitsuru Oshima . resolved

We should not add/move code in chrome/browser/ash unless there is a good reason to do so. (see Desired state section: https://chromium.googlesource.com/chromium/src/+/HEAD/docs/chromeos/code.md#desired-state)

Simon Hangl

Done

Line 17, Patchset 6:

Mitsuru Oshima . resolved

Do you have a bug #?
If none, thne Bug: None

Simon Hangl

Done

File ash/multi_capture/multi_capture_service.h

Line 13, Patchset 7:

#include "mojo/public/cpp/bindings/receiver.h"
#include "mojo/public/cpp/bindings/remote.h"

Hidehiko Abe . resolved

no longer needed.

Simon Hangl

Done

File ash/multi_capture/multi_capture_service.cc

Line 14, Patchset 7:

  for (Observer& observer : observers_) {
    observer.MultiCaptureServiceDestroyed();
  }

Hidehiko Abe . resolved

nit/style/optional:
`observers_.Notify(&Observer::MultiCaptureServiceDestroyed);`
Ditto for all below cases.

Simon Hangl

Done

File chrome/browser/ash/notifications/multi_capture_notifications.h

Line 21, Patchset 7:}

Hidehiko Abe . resolved

nit: `} // namespace base` for consistency.

Simon Hangl

Done

Open in Gerrit

Related details

Attention is currently required from:

Elad Alon
Mitsuru Oshima

Submit Requirements:

Code-Coverage
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

Mitsuru Oshima (Gerrit)

unread,

Feb 19, 2025, 5:10:11 PM2/19/25

to Simon Hangl, Hidehiko Abe, Elad Alon, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Rijubrata Bhaumik, alexmo...@chromium.org, chfreme...@chromium.org, creis...@chromium.org, feature-me...@chromium.org, ffred...@chromium.org, jophba...@chromium.org, mfoltz...@chromium.org, navigation...@chromium.org, oshima...@chromium.org, tluk+...@chromium.org

Attention needed from Elad Alon and Simon Hangl

Mitsuru Oshima added 2 comments

Patchset-level comments

File-level comment, Patchset 10 (Latest):

Mitsuru Oshima . resolved

lg but one request before submitting this.

File ash/multi_capture/multi_capture_service.h

Mitsuru Oshima . unresolved

can you first create a small CL that just rename this file so that git can preserve the history?

Open in Gerrit

Related details

Attention is currently required from:

Elad Alon
Simon Hangl

Submit Requirements:

Code-Coverage
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

Simon Hangl (Gerrit)

unread,

Feb 20, 2025, 11:07:33 AM2/20/25

to Hidehiko Abe, Elad Alon, Mitsuru Oshima, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Rijubrata Bhaumik, alexmo...@chromium.org, chfreme...@chromium.org, creis...@chromium.org, feature-me...@chromium.org, ffred...@chromium.org, jophba...@chromium.org, mfoltz...@chromium.org, navigation...@chromium.org, oshima...@chromium.org, tluk+...@chromium.org

Attention needed from Elad Alon, Hidehiko Abe and Mitsuru Oshima

Simon Hangl added 2 comments

Patchset-level comments

File-level comment, Patchset 11 (Latest):

Simon Hangl . resolved

oshima@, hidehiko@, i implemented [this request](https://chromium-review.googlesource.com/c/chromium/src/+/6172344/comment/566eb968_3fbf8f00/), uploaded the [CL that renames the multi_capture_service_client](https://chromium-review.googlesource.com/c/chromium/src/+/6286046) and rebased on top of that. PTAL.

eladalon@, based on hidehiko's feedback i brought back the `MultiCaptureService` (in a next step, the policy specific ash code will move there) and from `ChromeContentBrowserClient` i call directly into `ash::Shell` instead of redirecting it via `chrome/browser/ash`. no other changes compared to your last approval. PTAL.

File ash/multi_capture/multi_capture_service.h

Mitsuru Oshima . resolved

can you first create a small CL that just rename this file so that git can preserve the history?

Simon Hangl

done and rebased on top of that CL.

Open in Gerrit

Related details

Attention is currently required from:

Elad Alon
Hidehiko Abe
Mitsuru Oshima

Submit Requirements:

Code-Coverage
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

Mitsuru Oshima (Gerrit)

unread,

Feb 20, 2025, 3:30:56 PM2/20/25

to Simon Hangl, Hidehiko Abe, Elad Alon, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Rijubrata Bhaumik, alexmo...@chromium.org, chfreme...@chromium.org, creis...@chromium.org, feature-me...@chromium.org, ffred...@chromium.org, jophba...@chromium.org, mfoltz...@chromium.org, navigation...@chromium.org, oshima...@chromium.org, tluk+...@chromium.org

Attention needed from Elad Alon, Hidehiko Abe and Simon Hangl

Mitsuru Oshima voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Elad Alon
Hidehiko Abe
Simon Hangl

Submit Requirements:

Code-Coverage
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

Hidehiko Abe (Gerrit)

unread,

Feb 20, 2025, 8:09:59 PM2/20/25

to Simon Hangl, Mitsuru Oshima, Elad Alon, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Rijubrata Bhaumik, alexmo...@chromium.org, chfreme...@chromium.org, creis...@chromium.org, feature-me...@chromium.org, ffred...@chromium.org, jophba...@chromium.org, mfoltz...@chromium.org, navigation...@chromium.org, oshima...@chromium.org, tluk+...@chromium.org

Attention needed from Elad Alon and Simon Hangl

Hidehiko Abe voted and added 2 comments

Votes added by Hidehiko Abe

Code-Review

+1

2 comments

Patchset-level comments

File-level comment, Patchset 12 (Latest):

Hidehiko Abe . resolved

slgtm

File ash/multi_capture/multi_capture_service.h

Line 47, Patchset 12 (Latest): explicit MultiCaptureService();

Hidehiko Abe . unresolved

nit: you do not need explicit, as you removed the param (sorry, overlooked in the prev iter).

Open in Gerrit

Related details

Attention is currently required from:

Elad Alon
Simon Hangl

Submit Requirements:

Code-Coverage
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

Simon Hangl (Gerrit)

unread,

Feb 21, 2025, 6:59:44 AM2/21/25

to Colin Blundell, Kinuko Yasuda, Hidehiko Abe, Mitsuru Oshima, Elad Alon, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Rijubrata Bhaumik, alexmo...@chromium.org, chfreme...@chromium.org, creis...@chromium.org, feature-me...@chromium.org, ffred...@chromium.org, jophba...@chromium.org, mfoltz...@chromium.org, navigation...@chromium.org, oshima...@chromium.org, tluk+...@chromium.org

Attention needed from Colin Blundell, Elad Alon and Kinuko Yasuda

Simon Hangl added 2 comments

Patchset-level comments

File-level comment, Patchset 13 (Latest):

Simon Hangl . resolved

dear reviewers, PTAL at

kinuko@: c/p/b/c/* (deleted files)
blundell@: s/v/p/m/*

File ash/multi_capture/multi_capture_service.h

Line 47, Patchset 12: explicit MultiCaptureService();

Hidehiko Abe . resolved

nit: you do not need explicit, as you removed the param (sorry, overlooked in the prev iter).

Simon Hangl

Done

Open in Gerrit

Related details

Attention is currently required from:

Colin Blundell
Elad Alon
Kinuko Yasuda

Submit Requirements:

Code-Coverage
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

Colin Blundell (Gerrit)

unread,

Feb 21, 2025, 7:32:09 AM2/21/25

to Simon Hangl, Colin Blundell, Kinuko Yasuda, Hidehiko Abe, Mitsuru Oshima, Elad Alon, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Rijubrata Bhaumik, alexmo...@chromium.org, chfreme...@chromium.org, creis...@chromium.org, feature-me...@chromium.org, ffred...@chromium.org, jophba...@chromium.org, mfoltz...@chromium.org, navigation...@chromium.org, oshima...@chromium.org, tluk+...@chromium.org

Attention needed from Elad Alon, Kinuko Yasuda and Simon Hangl

Colin Blundell voted and added 1 comment

Votes added by Colin Blundell

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 13 (Latest):

Colin Blundell . resolved

Thanks! Kinuko needs to review services/video_capture/public/mojom/multi_capture_service.mojom

Open in Gerrit

Related details

Attention is currently required from:

Elad Alon
Kinuko Yasuda
Simon Hangl

Submit Requirements:

Code-Coverage
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

Kinuko Yasuda (Gerrit)

unread,

Feb 25, 2025, 6:01:08 AM2/25/25

to Simon Hangl, Colin Blundell, Hidehiko Abe, Mitsuru Oshima, Elad Alon, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Rijubrata Bhaumik, alexmo...@chromium.org, chfreme...@chromium.org, creis...@chromium.org, feature-me...@chromium.org, ffred...@chromium.org, jophba...@chromium.org, mfoltz...@chromium.org, navigation...@chromium.org, oshima...@chromium.org, tluk+...@chromium.org

Attention needed from Elad Alon and Simon Hangl

Kinuko Yasuda voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Elad Alon
Simon Hangl

Submit Requirements:

Code-Coverage
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

Simon Hangl (Gerrit)

unread,

Feb 25, 2025, 6:18:44 AM2/25/25

to Kinuko Yasuda, Colin Blundell, Hidehiko Abe, Mitsuru Oshima, Elad Alon, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Rijubrata Bhaumik, alexmo...@chromium.org, chfreme...@chromium.org, creis...@chromium.org, feature-me...@chromium.org, ffred...@chromium.org, jophba...@chromium.org, mfoltz...@chromium.org, navigation...@chromium.org, oshima...@chromium.org, tluk+...@chromium.org

Attention needed from Elad Alon

Simon Hangl voted and added 1 comment

Votes added by Simon Hangl

Commit-Queue

+2

1 comment

File chrome/browser/ash/multi_screen_capture/multi_capture_service.cc

File-level comment, Patchset 5:

Elad Alon . resolved

Please ensure a reviewer familiar with Ash.

Simon Hangl

blundell@, FYI.

Simon Hangl

Done

Open in Gerrit

Related details

Attention is currently required from:

Elad Alon

Submit Requirements:

Code-Coverage
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

Simon Hangl (Gerrit)

unread,

Feb 25, 2025, 9:32:07 AM2/25/25

to Kinuko Yasuda, Colin Blundell, Hidehiko Abe, Mitsuru Oshima, Elad Alon, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Rijubrata Bhaumik, alexmo...@chromium.org, chfreme...@chromium.org, creis...@chromium.org, feature-me...@chromium.org, ffred...@chromium.org, jophba...@chromium.org, mfoltz...@chromium.org, navigation...@chromium.org, oshima...@chromium.org, tluk+...@chromium.org

Attention needed from Elad Alon

Simon Hangl voted and added 1 comment

Votes added by Simon Hangl

Commit-Queue

+2

1 comment

File chrome/browser/ash/crosapi/multi_capture_service_ash.cc

Line 55, Patchset 6: }

Mitsuru Oshima . resolved

+hidehiko@ for his input

This pattern is a bit odd. Given that
1) ash code has to be moved out from chrome/browser
2) crosai iml can be in //ash
Can you move the service impl in ash, add delegate for IsXxxAllowed methods, then inject?

Hidehiko Abe

chrome/browser/ash/policy/multi_screen_capture/multi_screen_capture_policy_service.cc
does not look like depending on //chrome actually. Can whole code move out from //chrome, then?

Simon Hangl

Following up on our quick conversation I had with Hidehiko in chat, I implemented your suggestion by:
keeping `MultiCaptureServiceClient` but renaming it to `MultiCaptureService` and rename the notify functions to `NotifyMultiCapture*`.
calling into the `ash::Shell` directly from `ChromeContentBrowserClient`.
deleting the mojom parts.
Todos I inted to implement in follow-up CLs:
remove the crosapi implementation (I kept it for now as I think some tests might be affected).
move the `IsMultiCaptureAllowed*` oracles to the multi capture service.

Simon Hangl

Done

Open in Gerrit

Related details

Attention is currently required from:

Elad Alon

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Feb 25, 2025, 10:12:25 AM2/25/25

to Simon Hangl, Kinuko Yasuda, Colin Blundell, Hidehiko Abe, Mitsuru Oshima, Elad Alon, Tricium, chromium...@chromium.org, Rijubrata Bhaumik, alexmo...@chromium.org, chfreme...@chromium.org, creis...@chromium.org, feature-me...@chromium.org, ffred...@chromium.org, jophba...@chromium.org, mfoltz...@chromium.org, navigation...@chromium.org, oshima...@chromium.org, tluk+...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

Remove `services/video_capture/multi_capture_service.mojom`

This CL refactors the notification stack for the `getAllScreensMedia`
API and removes unnecessary indirection by deleting the
`services/video_capture/multi_capture_service.mojom` interface. This
will also help to simplify removing the Lacros implementation of this
service. We do this by calling into `ash::Shell` from `ChromeContentBrowserClient`. The alternative of moving the code to `chrome/browser/ash` was not chosen due to the desired state described here [1].

Bug: 392852729

[1] https://chromium.googlesource.com/chromium/src/+/HEAD/docs/chromeos/code.md#desired-state

Change-Id: If529bbefa4a3b635b0fb489d9c1cd9466cff1938

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

Reviewed-by: Hidehiko Abe <hide...@chromium.org>

Commit-Queue: Simon Hangl <sim...@google.com>

Reviewed-by: Mitsuru Oshima <osh...@chromium.org>

Reviewed-by: Kinuko Yasuda <kin...@chromium.org>

Reviewed-by: Colin Blundell <blun...@chromium.org>

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

Files:

M ash/multi_capture/multi_capture_service.cc
M ash/multi_capture/multi_capture_service.h
M ash/shell.cc
M ash/shell.h
M ash/shell_delegate.h
M ash/system/unified/screen_capture_tray_item_view.cc
M ash/system/unified/screen_capture_tray_item_view.h
M ash/test_shell_delegate.cc
M ash/test_shell_delegate.h
M chrome/browser/DEPS
M chrome/browser/ash/crosapi/multi_capture_service_ash.cc
M chrome/browser/ash/crosapi/multi_capture_service_ash.h
M chrome/browser/ash/notifications/multi_capture_notifications.cc
M chrome/browser/ash/notifications/multi_capture_notifications.h
M chrome/browser/chrome_content_browser_client.cc
M chrome/browser/ui/ash/shell_delegate/chrome_shell_delegate.cc
M chrome/browser/ui/ash/shell_delegate/chrome_shell_delegate.h
M content/browser/renderer_host/media/media_stream_manager.cc
M content/public/browser/BUILD.gn
D content/public/browser/chromeos/multi_capture_service.cc
D content/public/browser/chromeos/multi_capture_service.h
M services/video_capture/public/mojom/BUILD.gn
D services/video_capture/public/mojom/multi_capture_service.mojom

Change size: L

Delta: 23 files changed, 78 insertions(+), 289 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Mitsuru Oshima, +1 by Hidehiko Abe, +1 by Kinuko Yasuda, +1 by Colin Blundell

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

Remove `services/video_capture/multi_capture_service.mojom` [chromium/src : main]

Simon Hangl (Gerrit)

Simon Hangl voted and added 1 comment

Votes added by Simon Hangl

1 comment

Related details

Colin Blundell (Gerrit)

Colin Blundell added 1 comment

Related details

Simon Hangl (Gerrit)

Simon Hangl added 2 comments

Related details

Elad Alon (Gerrit)

Elad Alon added 4 comments

Related details

Simon Hangl (Gerrit)

Simon Hangl added 4 comments

Related details

Simon Hangl (Gerrit)

Simon Hangl added 1 comment

Colin Blundell (Gerrit)

Colin Blundell added 1 comment

Related details

Elad Alon (Gerrit)

Elad Alon voted Code-Review+1

Related details

Mitsuru Oshima (Gerrit)

Mitsuru Oshima added 3 comments

Related details

Mitsuru Oshima (Gerrit)

Mitsuru Oshima added 1 comment

Related details

Hidehiko Abe (Gerrit)

Hidehiko Abe added 3 comments

Related details

Simon Hangl (Gerrit)

Simon Hangl added 2 comments

Related details

Hidehiko Abe (Gerrit)

Hidehiko Abe voted and added 4 comments

Votes added by Hidehiko Abe

4 comments

Related details

Simon Hangl (Gerrit)

Simon Hangl voted and added 5 comments

Votes added by Simon Hangl

5 comments

Related details

Mitsuru Oshima (Gerrit)

Mitsuru Oshima added 2 comments

Related details

Simon Hangl (Gerrit)

Simon Hangl added 2 comments

Related details

Mitsuru Oshima (Gerrit)

Mitsuru Oshima voted Code-Review+1

Related details

Hidehiko Abe (Gerrit)

Hidehiko Abe voted and added 2 comments

Votes added by Hidehiko Abe

2 comments

Related details

Simon Hangl (Gerrit)

Simon Hangl added 2 comments

Related details

Colin Blundell (Gerrit)

Colin Blundell voted and added 1 comment

Votes added by Colin Blundell

1 comment

Related details

Kinuko Yasuda (Gerrit)

Kinuko Yasuda voted Code-Review+1

Related details

Simon Hangl (Gerrit)

Simon Hangl voted and added 1 comment

Votes added by Simon Hangl

1 comment

Related details

Simon Hangl (Gerrit)

Simon Hangl voted and added 1 comment