[MSC] Screen auto selection for getDisplayMediaSet. [chromium/src : main]
Simon Hangl (Gerrit)
Attention is currently required from: Elad Alon.
Simon Hangl would like Elad Alon to review this change.
[MSC] Screen auto selection for getDisplayMediaSet.
This change enables auto selection for getDisplayMediaSet.
It propagates the autoSelectAllScreens property through the
call stack, loads a list of all attached screens and forwards
it for capturing.
Change-Id: I4cc00e9dd6807c5a992367a31f10e261c372b89e
---
M content/browser/renderer_host/media/media_stream_manager.cc
M content/browser/renderer_host/media/media_stream_manager.h
M content/browser/renderer_host/media/media_stream_manager_unittest.cc
M third_party/blink/common/mediastream/media_stream_mojom_traits.cc
M third_party/blink/public/common/mediastream/media_stream_controls.h
M third_party/blink/public/common/mediastream/media_stream_mojom_traits.h
M third_party/blink/public/mojom/mediastream/media_stream.mojom
M third_party/blink/renderer/modules/mediastream/user_media_processor.cc
M third_party/blink/renderer/modules/mediastream/user_media_request.cc
M third_party/blink/renderer/modules/mediastream/user_media_request.h
10 files changed, 139 insertions(+), 22 deletions(-)
To view, visit change 3605769. To unsubscribe, or for help writing mail filters, visit settings.
Simon Hangl (Gerrit)
Attention is currently required from: Elad Alon.
1 comment:
Patchset:
eladalon@: this change enables "select all screens". PTAL.
To view, visit change 3605769. To unsubscribe, or for help writing mail filters, visit settings.
Elad Alon (Gerrit)
Attention is currently required from: Simon Hangl.
28 comments:
File content/browser/renderer_host/media/media_stream_manager.h:
URI or URL? (Same question for `fake_origin_uri`.)
Patch Set #21, Line 534:
ChecksHow does it handle this check? It does not seem to return a value.
Patch Set #21, Line 536:
const int&Change to an int.
Patch Set #21, Line 541:
const boolChange to a simple bool, it does not hurt if the function modifies primitive parameters passed by value.
Patch Set #21, Line 568:
RetrievesThe method is called "HandleAttachedDevices". Does it handle or retrieve? I don't see a return value, so presumably handle?
Patch Set #21, Line 758:
fake_origin_uri_for_testing_Same question about URL vs. URI.
File content/browser/renderer_host/media/media_stream_manager.cc:
nit: Undo.
MediaDeviceSaltAndOrigin{/*device_id_salt=*/std::string(),
/*group_id_salt=*/std::string(), security_origin,
/*has_focus=*/true, /*is_background=*/false});Could you use this syntax?
```
MediaDeviceSaltAndOrigin{
.device_id_salt = std::string(),
.group_id_salt = std::string,
...
};
```// Cache the |label| in the device name field, for unit test purpose
// only.Could you undo the change?
Patch Set #21, Line 2002:
base::Unretained(this)Annotate Unretained with rationale.
Patch Set #21, Line 2016:
if (fake_origin_uri_for_testing_.is_valid()) {Is it intentional that `browser_context` is left nullptr if `fake_origin_uri_for_testing_` is not set? (It would make things simpler if you always read it.)
Patch Set #21, Line 2019:
render_process_hostCheck for nullness? Is it guaranteed it's still alive when you're processing this?
const ProcessLock process_lock = render_process_host->GetProcessLock();
origin = process_lock.lock_url();Note to self: Scrutinize this part in following iterations of the review. (ProcessLock is not used anywhere else in this file.)
Patch Set #21, Line 2030:
base::Unretained(this)Please add a comment explaining why Unretained is safe whenever you use it.
GetContentClient()
->browser()
->IsGetDisplayMediaSetSelectAllScreensAllowed(browser_context,
origin))Could you use an intermediary so as to make this easier to read?
Patch Set #21, Line 2042:
if (!select_all_screens_allowed) {1. If `select_all_screens` was specified even though `!select_all_screens_allowed`, we should kill the render process using a BadMessage.
2. Can you do it in the dispatcher?Patch Set #21, Line 2051:
DCHECK(surface_enumerator);Btw, I recall there was one implementation of `SurfaceEnumerator` that returned `nullptr`. Is it guaranteed it's not the one used?
// If forced screen selection is requested, there is no need to post the
// request to the UI.Could you please explain why not?
Patch Set #21, Line 2056:
UnretainedSame.
Patch Set #21, Line 2059:
HandleAttachedDevicesThe implementation assumes (1) screens and (2) video, but the name does not reflect that.
Patch Set #21, Line 2063:
DCHECK(request);Is this DCHECK truly guaranteed? Can the request not get asynchronously ejected from the container where you Find() it?
Patch Set #21, Line 2067:
media_id.ToString()Could you cache this value in an intermediary variable so as to avoid computing it twice?
Patch Set #21, Line 2072:
? true : falseThe `? true : false` part can be dropped, as the result of `!=` is already a bool.
Patch Set #21, Line 2083:
emplace_backI think push_back would be equivalent in this case. Or am I wrong?
File content/browser/renderer_host/media/media_stream_manager.cc:
// If using the fake UI, it will just auto-select from the available
// devices. The fake UI doesn't work for desktop sharing requests since we
// can't see its devices from here; always use the real UI for such
// requests. The processing below for MEDIA_GUM_DESKTOP_VIDEO_CAPTURE is forPlease undo this change if it's only a reshuffling caused by formatting double-commented text during development.
Patch Set #24, Line 2907:
void MediaStreamManager::UseFakeOriginUriForTesting(DCHECK thread.
File content/browser/renderer_host/media/media_stream_manager_unittest.cc:
Patch Set #21, Line 192:
FakeContentBrowserClient(const GURL& allowed_url)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: building this file or its dependencies failed; this diagnostic might be incorrect as a result.)
Please fix.
File content/public/browser/content_browser_client.h:
Patch Set #21, Line 637:
// Checks whether screen auto selection is allowed for capturing.nit: Would be good to explain what screen auto-selection is.
To view, visit change 3605769. To unsubscribe, or for help writing mail filters, visit settings.
Simon Hangl (Gerrit)
Attention is currently required from: Elad Alon.
30 comments:
Patchset:
eladalon@: i revised the CL. PTAL.
File content/browser/renderer_host/media/media_stream_manager.h:
URI or URL? (Same question for `fake_origin_uri`. […]
url.
Patch Set #21, Line 534:
ChecksHow does it handle this check? It does not seem to return a value.
the implementation now uses base::BindPostTask. the result is returned through the passed callback (explicit now in the function signature).
Patch Set #21, Line 536:
const int&Change to an int.
Done
Patch Set #21, Line 541:
const bool
Change to a simple bool, it does not hurt if the function modifies primitive parameters passed by va […]
Done
Patch Set #21, Line 568:
Retrieves
The method is called "HandleAttachedDevices". […]
Done
Patch Set #21, Line 758:
fake_origin_uri_for_testing_Same question about URL vs. URI.
Done
File content/browser/renderer_host/media/media_stream_manager.cc:
nit: Undo.
Done
MediaDeviceSaltAndOrigin{/*device_id_salt=*/std::string(),
/*group_id_salt=*/std::string(), security_origin,
/*has_focus=*/true, /*is_background=*/false});
Could you use this syntax? […]
did not compile (only available from c++20 [1]).
[1] https://en.cppreference.com/w/cpp/language/aggregate_initialization
// Cache the |label| in the device name field, for unit test purpose
// only.Could you undo the change?
Done
Patch Set #21, Line 2002:
base::Unretained(this)Annotate Unretained with rationale.
Done
Patch Set #21, Line 2016:
if (fake_origin_uri_for_testing_.is_valid()) {
Is it intentional that `browser_context` is left nullptr if `fake_origin_uri_for_testing_` is not se […]
yes, in my (unit) tests the renderer_process_host was null.
Patch Set #21, Line 2019:
render_process_hostCheck for nullness? Is it guaranteed it's still alive when you're processing this?
indeed. now i exit early in this case.
Patch Set #21, Line 2030:
base::Unretained(this)Please add a comment explaining why Unretained is safe whenever you use it.
Done
GetContentClient()
->browser()
->IsGetDisplayMediaSetSelectAllScreensAllowed(browser_context,
origin))Could you use an intermediary so as to make this easier to read?
Done
Patch Set #21, Line 2042:
if (!select_all_screens_allowed) {
1. […]
1) i moved it over to the dispatcher.
2) a BadMessage is emitted on error.
3) adapted tests for both (positive and negative) cases.
Patch Set #21, Line 2051:
DCHECK(surface_enumerator);
Btw, I recall there was one implementation of `SurfaceEnumerator` that returned `nullptr`. […]
Done
// If forced screen selection is requested, there is no need to post the
// request to the UI.Could you please explain why not?
Done
Patch Set #21, Line 2056:
UnretainedSame.
this one actually in principle could outlive the `MediaStreamManager`. i also moved over ownership of the `ScreenEnumerator` object to the callback to make sure it does not get destructed.
Patch Set #21, Line 2059:
HandleAttachedDevicesThe implementation assumes (1) screens and (2) video, but the name does not reflect that.
Done
Patch Set #21, Line 2063:
DCHECK(request);
Is this DCHECK truly guaranteed? Can the request not get asynchronously ejected from the container w […]
true. now rejected with permission denied.
Patch Set #21, Line 2067:
media_id.ToString()Could you cache this value in an intermediary variable so as to avoid computing it twice?
Done
Patch Set #21, Line 2072:
? true : falseThe `? true : false` part can be dropped, as the result of `!=` is already a bool.
Done
Patch Set #21, Line 2083:
emplace_back
I think push_back would be equivalent in this case. […]
Done
File content/browser/renderer_host/media/media_stream_manager.cc:
Patch Set #22, Line 30:
#include "base/test/bind.h"Remove.
Patch Set #22, Line 57:
#include "content/public/browser/desktop_capture.h"Remove.
File content/browser/renderer_host/media/media_stream_manager.cc:
// If using the fake UI, it will just auto-select from the available
// devices. The fake UI doesn't work for desktop sharing requests since we
// can't see its devices from here; always use the real UI for such
// requests. The processing below for MEDIA_GUM_DESKTOP_VIDEO_CAPTURE is for
Please undo this change if it's only a reshuffling caused by formatting double-commented text during […]
Done
Patch Set #24, Line 2907:
void MediaStreamManager::UseFakeOriginUriForTesting(DCHECK thread.
Done
File content/browser/renderer_host/media/media_stream_manager_unittest.cc:
Patch Set #21, Line 192:
FakeContentBrowserClient(const GURL& allowed_url)
> single-argument constructors must be marked explicit to avoid unintentional implicit conversions ( […]
Done
File content/public/browser/content_browser_client.h:
Patch Set #21, Line 637:
// Checks whether screen auto selection is allowed for capturing.nit: Would be good to explain what screen auto-selection is.
Done
To view, visit change 3605769. To unsubscribe, or for help writing mail filters, visit settings.
Elad Alon (Gerrit)
Attention is currently required from: Simon Hangl.
13 comments:
File content/browser/renderer_host/media/media_stream_dispatcher_host.h:
// Sets a fake origin url for the select all screens check.
void UseFakeOriginUrlForTesting(const GURL& fake_origin_url);Can we remove this and `fake_origin_url_for_testing_`? We don't have to use a unit test to check this if it requires too much test-code in prod-code. We can use a browser test instead, and then we won't need to fake. Does that make sense, or am I overlooking something?
File content/browser/renderer_host/media/media_stream_dispatcher_host.cc:
Patch Set #28, Line 385:
const ProcessLock process_lockCould you please explain why this is necessary? I've not seen this used before. Is it the new recommended way for obtaining origins?
See here for example:
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/media/media_devices_util.cc;l=159-162;drc=c673ac8876330a42eed34e298ff585636af113bcreturn;
}It would be nice to make a single hop to the UI thread.
Wdyt of this?```
return;
}
base::PostTask(GetUIThreadTaskRunner({}).get(), FROM_HERE,
base::BindOnce(&GenerateStreamsChecksOnUIthread...);
} // <-- Finish GenerateStreams.void MediaStreamDispatcherHost::GenerateStreamsChecksOnUIthread(...) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
// 1. Validate all-screens and early-exit if necessary.
// 2. Run salt_and_origin_callback_ synchronously.
// 3. Post result to DoGenerateStreams on the UI thread.
}void MediaStreamDispatcherHost::DoGenerateStreams(
int32_t page_request_id,
const blink::StreamControls& controls,
bool user_gesture,
blink::mojom::StreamSelectionInfoPtr audio_stream_selection_info_ptr,
GenerateStreamsCallback callback,
MediaDeviceSaltAndOrigin salt_and_origin) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
```
File content/browser/renderer_host/media/media_stream_manager.h:
Patch Set #28, Line 561:
// Handles all attached screens.This comment is unclear. (Note that HandleAccessRequestResponse is clear because it's handling the response. What's handled here - I am not sure.) But please see my next comments before you find a new name; you might not need one.
Patch Set #28, Line 766:
base::WeakPtrFactory<MediaStreamManager> weak_factory_{this};Undo this. MSM is owned by BrowserLoop. See comments inside of the .cc file for why `Unretained` is safe. Like here:
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/media/media_stream_manager.cc;l=1100-1104
File content/browser/renderer_host/media/media_stream_manager.cc:
MediaDeviceSaltAndOrigin{/*device_id_salt=*/std::string(),
/*group_id_salt=*/std::string(), security_origin,
/*has_focus=*/true, /*is_background=*/false});
did not compile (only available from c++20 [1]). […]
It works here:
https://source.chromium.org/search?q=%22%7B.%22%20f:content%2Fbrowser%2F&sq=&ss=chromium
(But possibly something else is different?)
File content/browser/renderer_host/media/media_stream_manager.cc:
Patch Set #28, Line 786:
IsSelectAllScreensnit: Move next to `IsGetDisplayMediaSet()`. (Or maybe just drop it altogether? It's public, right? I don't think the getter adds value...?)
MediaDeviceSaltAndOrigin(/*device_id_salt=*/std::string(),
/*group_id_salt=*/std::string(),
/*origin=*/security_origin,
/*has_focus=*/true,
/*is_background=*/false));This is great, but does it really need to be part of this CL? If you're not really changing anything, or compelled to do this because of a change I am overlooking, then please do this in a separate CL. (Or not at all if you're too busy.)
Patch Set #28, Line 1535:
/*user gesture=*/falseSimilarly.
// If forced screen selection is requested, there is no need to post the
// request to the UI as all screens are automatically selected and no user
// input is required.Potentially misleading because Ash still enumerates on the UI thread. Consider just dropping the comment.
Patch Set #28, Line 2042:
PERMISSION_DENIEDMaybe INVALID_STATE? I think this would happen if the request is cancelled while the other task is in flight?
Patch Set #28, Line 2046:
blink::mojom::StreamDevicesSet stream_devices_set;Would it make sense for EnumerateScreens() to return this set instead of the list of IDs? Other than the `video_type`, it does not look like MSM is contributing anything here, does it?
Note that if this advice makes sense, and if you take it, then you might be able to just wrap the piping to HandleAccessRequestResponse in a OnceCallback and not have to worry about adding and naming HandleAttachedVideoScreens.
File third_party/blink/public/common/mediastream/media_stream_controls.h:
Patch Set #28, Line 57:
select_all_screensIf you s/select_all_screens/request_all_screens, that would put security-minded folk more at ease, and you'll have an easier time. See one line above you for prior art. (I'd *not* add _permission here.) Please also consider consistency if doing this.
To view, visit change 3605769. To unsubscribe, or for help writing mail filters, visit settings.
Simon Hangl (Gerrit)
Attention is currently required from: Elad Alon.
13 comments:
Patchset:
eladalon@: i revised the cl. PTAL.
File content/browser/renderer_host/media/media_stream_dispatcher_host.h:
// Sets a fake origin url for the select all screens check.
void UseFakeOriginUrlForTesting(const GURL& fake_origin_url);
Can we remove this and `fake_origin_url_for_testing_`? We don't have to use a unit test to check thi […]
i can, but then i need to remove the unit test (which i think currently adds some value). a browser test i can only introduce when the whole loop is closed. i suggest:
- we keep it for now and tag it with a todo.
- when the loop is closed i replace the unit test with a browser test.
File content/browser/renderer_host/media/media_stream_dispatcher_host.cc:
Patch Set #28, Line 385:
const ProcessLock process_lock
Could you please explain why this is necessary? I've not seen this used before. […]
switcher over to the method you suggested.
return;
}
It would be nice to make a single hop to the UI thread. […]
i changed it to something very similar:
- i kept PostTaskAndReply as i received [1] for GenerateStreamsCallback when i tried to move the callback.
- i return a std::pair<bool, MediaDeviceSaltAndOrigin>. the bool is used to determine if the check was successful.
[1] Check failed: !connected. ... was destroyed without first either being run or its corresponding binding being closed. It is an error to drop response callbacks which still correspond to an open interface pipe.
File content/browser/renderer_host/media/media_stream_manager.h:
Patch Set #28, Line 766:
base::WeakPtrFactory<MediaStreamManager> weak_factory_{this};Undo this. MSM is owned by BrowserLoop. See comments inside of the . […]
Done
File content/browser/renderer_host/media/media_stream_manager.cc:
MediaDeviceSaltAndOrigin{/*device_id_salt=*/std::string(),
/*group_id_salt=*/std::string(), security_origin,
/*has_focus=*/true, /*is_background=*/false});
It works here: […]
Done
File content/browser/renderer_host/media/media_stream_manager.cc:
Patch Set #28, Line 786:
IsSelectAllScreens
nit: Move next to `IsGetDisplayMediaSet()`. […]
Done
MediaDeviceSaltAndOrigin(/*device_id_salt=*/std::string(),
/*group_id_salt=*/std::string(),
/*origin=*/security_origin,
/*has_focus=*/true,
/*is_background=*/false));
This is great, but does it really need to be part of this CL? If you're not really changing anything […]
Done
Patch Set #28, Line 1535:
/*user gesture=*/falseSimilarly.
Done
// If forced screen selection is requested, there is no need to post the
// request to the UI as all screens are automatically selected and no user
// input is required.
Potentially misleading because Ash still enumerates on the UI thread. […]
Done
Patch Set #28, Line 2042:
PERMISSION_DENIED
Maybe INVALID_STATE? I think this would happen if the request is cancelled while the other task is i […]
Done
Patch Set #28, Line 2046:
blink::mojom::StreamDevicesSet stream_devices_set;
Would it make sense for EnumerateScreens() to return this set instead of the list of IDs? Other than […]
it does make sense. i will revise the base CL and fix this in the next iteration.
File third_party/blink/public/common/mediastream/media_stream_controls.h:
Patch Set #28, Line 57:
select_all_screens
If you s/select_all_screens/request_all_screens, that would put security-minded folk more at ease, a […]
Done
To view, visit change 3605769. To unsubscribe, or for help writing mail filters, visit settings.
Simon Hangl (Gerrit)
Attention is currently required from: Elad Alon.
2 comments:
File content/browser/renderer_host/media/media_stream_manager.h:
Patch Set #28, Line 561:
// Handles all attached screens.This comment is unclear. […]
this will go away in the next iteration.
File content/browser/renderer_host/media/media_stream_manager.cc:
const ProcessLock process_lock = render_process_host->GetProcessLock();
origin = process_lock.lock_url();
Note to self: Scrutinize this part in following iterations of the review. […]
ProcessLock is gone now.
To view, visit change 3605769. To unsubscribe, or for help writing mail filters, visit settings.
Simon Hangl (Gerrit)
Attention is currently required from: Elad Alon.
1 comment:
File content/browser/renderer_host/media/media_stream_manager.h:
Patch Set #28, Line 561:
// Handles all attached screens.
this will go away in the next iteration.
gone now.
To view, visit change 3605769. To unsubscribe, or for help writing mail filters, visit settings.
Elad Alon (Gerrit)
Attention is currently required from: Simon Hangl.
23 comments:
File content/browser/renderer_host/media/media_stream_dispatcher_host.h:
I think you meant "e.g." rather than "i.e." here. But maybe it would be better to list both tests rather than give an example?
Patch Set #31, Line 84:
const GURL& fake_origin_url_for_testing`fake_origin_url_for_testing` as a parameter has "code-smell". (https://en.wikipedia.org/wiki/Code_smell)
Maybe you could just let the other member access it directly?
File content/browser/renderer_host/media/media_stream_dispatcher_host.h:
// Sets a fake origin url for the select all screens check.
void UseFakeOriginUrlForTesting(const GURL& fake_origin_url);
i can, but then i need to remove the unit test (which i think currently adds some value). […]
I am worried about adding complexity in production code that runs for all video-conferencing calls, to support unit tests for a feature that has not yet been completed, and letting it land while a lot of the people are on summer vacation. But please see my other comments about ideas for minimizing risks.
File content/browser/renderer_host/media/media_stream_dispatcher_host.cc:
Patch Set #31, Line 16:
#include "content/browser/renderer_host/frame_tree_node.h"rm
Patch Set #31, Line 327:
std::pair<bool, MediaDeviceSaltAndOrigin>nit: Make a private struct for this.
Patch Set #31, Line 337:
if (request_all_screens) {A helper function that only checks the state related to this, would reduce indentation and simplify things by a lot.
Patch Set #31, Line 339:
content::BrowserContext* browser_context = nullptr;You could declare this in a tighter context.
Patch Set #31, Line 342:
url = fake_origin_url_for_testing;It does not look like you actually read `url` if you enter this branch. Am I missing something due to the late hour?
RenderProcessHost* render_process_host =
RenderProcessHost::FromID(render_process_id);Do you need both RPH and RFHI?
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.h;l=453;drc=e41678a49a5eed60e29b0a313ad4f495ecd4d0c9Patch Set #31, Line 353:
url = render_frame_host->frame_tree_node()Do you need to go through the tree? Isn't this good enough?
https://source.chromium.org/chromium/chromium/src/+/main:content/public/browser/render_frame_host.h;l=296?q=GetMainFrame&ss=chromiumPatch Set #31, Line 357:
GetURL()Should the policy really be by URL? Not by origin? Is this an intentional part of the design?
Patch Set #31, Line 365:
return {false, MediaDeviceSaltAndOrigin()};Once you use a struct, can you use `{.a = false, .b = MediaDeviceSaltAndOrigin()}` here? (With something more specific than a/b, of course.)
Patch Set #31, Line 443:
fake_origin_url_for_testing_Can `GenerateStreamsChecksOnUIThread` access this directly?
if (!ui_check_result.first) {
ReceivedBadMessage(
render_process_id_,
bad_message::MSDH_REQUEST_ALL_SCREENS_NOT_ALLOWED_FOR_ORIGIN);
return;
}Do we have a browser test that proves that this is not hit, by checking that this request is blocked by the render process? If not, we should add such a test. (I've been bitten by this type of problem before. I was missing the keyword `return` in the render process and ended up soft-failing in the render process and then immediately killing the render process using a BadMessage.)
Patch Set #31, Line 497:
std::move(salt_and_origin)It's odd that you std::move away from a const-ref. Specifically, the const part. Does this compile? (Asking for my own education.)
At any rate, even if we remove const, I am worried that aliasing might hide that we've std::move-ed this if we access once through the alias and once directly. Wdyt of using the following instead?
```
MediaDeviceSaltAndOrigin salt_and_origin = std::move(ui_check_result.second);
ui_check_result = {};
```
File content/browser/renderer_host/media/media_stream_manager.cc:
const ProcessLock process_lock = render_process_host->GetProcessLock();
origin = process_lock.lock_url();
ProcessLock is gone now.
Ack
File content/browser/renderer_host/media/media_stream_manager.cc:
Patch Set #31, Line 1946:
nullptr/*param=*/
Patch Set #31, Line 2013:
/*screen_enumerator=*/nullptrnit: `screen_enumerator.get()` also works. Your choice, but note that comments are less authoritative than code. (For example, if `A*` is changed to `B*` in the signature, `nullptr` would still be accepted with an outdated comment, but the `.get()` version would not.)
Patch Set #31, Line 2013:
media::AudioParameters()media::AudioParameters() - see similar question below.
// Using base::Unretained is safe since MediaStreamManager is deleted on
// the UI thread, after the IO thread has been stopped.I find this one more compelling:
// It is safe to bind base::Unretained(this) because MediaStreamManager is
// owned by BrowserMainLoop.Patch Set #31, Line 2022:
screen_enumeratorThis looks very scary: `x->Func(..., std::move(x))`
Patch Set #31, Line 2025:
media::AudioParameters()Is this code correct? Always empty audio parameters? Don't you want to go through `ReadOutputParamsAndPostRequestToUI` here too?
File third_party/blink/renderer/modules/mediastream/user_media_processor.cc:
Patch Set #31, Line 847:
request_all_screensPlease note that you've renamed to request_ in some places, but kept auto_select_ in others. Do you plan to clean this up later? Or do you want to maintain this discrepancy? (I don't recommend the latter.)
To view, visit change 3605769. To unsubscribe, or for help writing mail filters, visit settings.
Simon Hangl (Gerrit)
Attention is currently required from: Simon Hangl.
Simon Hangl uploaded patch set #35 to this change.
[MSC] Screen auto selection for getDisplayMediaSet.
This change enables auto selection for getDisplayMediaSet.
It propagates the autoSelectAllScreens property through the
call stack, loads a list of all attached screens and forwards
it for capturing.
Bug: 1300883
Change-Id: I4cc00e9dd6807c5a992367a31f10e261c372b89e
---
M chrome/browser/chrome_content_browser_client.cc
M chrome/browser/chrome_content_browser_client.h
M content/browser/bad_message.h
M content/browser/renderer_host/media/media_stream_dispatcher_host.cc
M content/browser/renderer_host/media/media_stream_dispatcher_host.h
M content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
M content/browser/renderer_host/media/media_stream_manager.cc
M content/browser/renderer_host/media/media_stream_manager.h
M content/public/browser/content_browser_client.cc
M content/public/browser/content_browser_client.h
M third_party/blink/common/mediastream/media_stream_mojom_traits.cc
M third_party/blink/public/common/mediastream/media_stream_controls.h
M third_party/blink/public/common/mediastream/media_stream_mojom_traits.h
M third_party/blink/public/mojom/mediastream/media_stream.mojom
M third_party/blink/renderer/modules/mediastream/user_media_processor.cc
M third_party/blink/renderer/modules/mediastream/user_media_request.cc
M third_party/blink/renderer/modules/mediastream/user_media_request.h
M tools/metrics/histograms/enums.xml
18 files changed, 320 insertions(+), 35 deletions(-)
To view, visit change 3605769. To unsubscribe, or for help writing mail filters, visit settings.
Simon Hangl (Gerrit)
Attention is currently required from: Elad Alon.
25 comments:
Patchset:
eladalon@: i revised the CL. PTAL.
File content/browser/renderer_host/media/media_stream_dispatcher_host.h:
I think you meant "e.g." rather than "i.e." here. […]
actually i meant "i.e." == "that is", as the test is the only check that is performed. though i omitted the salt computation.
Patch Set #31, Line 84:
const GURL& fake_origin_url_for_testing
`fake_origin_url_for_testing` as a parameter has "code-smell". (https://en.wikipedia. […]
to my knowledge this is the only way i could get access to the member in this static function (has to be static to not get in trouble with the sequence checker or using unretained (which i belive cannot be used with the MediaStreamDispatcherHost).
based on the comment below [1], i removed the test now and added a todo for a future browser test when the getDisplayMediaSet loop is closed (crbug.com/1337042).
[1] https://chromium-review.googlesource.com/c/chromium/src/+/3605769/comments/9b449115_b6a05eb1
File content/browser/renderer_host/media/media_stream_dispatcher_host.h:
// Sets a fake origin url for the select all screens check.
void UseFakeOriginUrlForTesting(const GURL& fake_origin_url);
I am worried about adding complexity in production code that runs for all video-conferencing calls, […]
test is removed now.
File content/browser/renderer_host/media/media_stream_dispatcher_host.cc:
Patch Set #31, Line 16:
#include "content/browser/renderer_host/frame_tree_node.h"rm
Done
Patch Set #31, Line 327:
std::pair<bool, MediaDeviceSaltAndOrigin>nit: Make a private struct for this.
Done
Patch Set #31, Line 337:
if (request_all_screens) {
A helper function that only checks the state related to this, would reduce indentation and simplify […]
Done
Patch Set #31, Line 339:
content::BrowserContext* browser_context = nullptr;You could declare this in a tighter context.
i revised the whole block.
Patch Set #31, Line 342:
url = fake_origin_url_for_testing;
It does not look like you actually read `url` if you enter this branch. […]
url is passed in `browser_client->IsGetDisplayMediaSetSelectAllScreensAllowed`, but i removed the fake url now anyways.
RenderProcessHost* render_process_host =
RenderProcessHost::FromID(render_process_id);
Do you need both RPH and RFHI? […]
Done
Patch Set #31, Line 353:
url = render_frame_host->frame_tree_node()
Do you need to go through the tree? Isn't this good enough? […]
Done
Patch Set #31, Line 357:
GetURL()Should the policy really be by URL? Not by origin? Is this an intentional part of the design?
yes, the interface accepts the url and passes it over to library functions that do the origin check [1].
Patch Set #31, Line 365:
return {false, MediaDeviceSaltAndOrigin()};
Once you use a struct, can you use `{.a = false, . […]
Done
Patch Set #31, Line 443:
fake_origin_url_for_testing_Can `GenerateStreamsChecksOnUIThread` access this directly?
no, i cannot bind the callback with a WeakPtr as the sequence checker would complain then.
if (!ui_check_result.first) {
ReceivedBadMessage(
render_process_id_,
bad_message::MSDH_REQUEST_ALL_SCREENS_NOT_ALLOWED_FOR_ORIGIN);
return;
}
Do we have a browser test that proves that this is not hit, by checking that this request is blocked […]
no, unless i am missing something, i cannot write a browser test until the basic functionality is landed for getDisplayMediaSet (as i will not be able to enter this path before). i created a bug and added a todo.
Patch Set #31, Line 497:
std::move(salt_and_origin)
It's odd that you std::move away from a const-ref. Specifically, the const part. […]
indeed. it did compile, yes. replaced with your suggestion.
File content/browser/renderer_host/media/media_stream_manager.cc:
Patch Set #21, Line 2016:
if (fake_origin_uri_for_testing_.is_valid()) {
yes, in my (unit) tests the renderer_process_host was null.
removed.
File content/browser/renderer_host/media/media_stream_manager.cc:
Patch Set #28, Line 2046:
blink::mojom::StreamDevicesSet stream_devices_set;it does make sense. i will revise the base CL and fix this in the next iteration.
Done
File content/browser/renderer_host/media/media_stream_manager.cc:
Patch Set #31, Line 1946:
nullptr/*param=*/
i removed this parameter due to [1].
[1] https://chromium-review.googlesource.com/c/chromium/src/+/3605769/comments/fbf5280e_9e685c5c
Patch Set #31, Line 2013:
/*screen_enumerator=*/nullptr
nit: `screen_enumerator.get()` also works. […]
i removed this due to [1].
[1] https://chromium-review.googlesource.com/c/chromium/src/+/3605769/comments/fbf5280e_9e685c5c
Patch Set #31, Line 2013:
media::AudioParameters()media::AudioParameters() - see similar question below.
see answer to question below.
// Using base::Unretained is safe since MediaStreamManager is deleted on
// the UI thread, after the IO thread has been stopped.
I find this one more compelling: […]
Done
Patch Set #31, Line 2022:
screen_enumerator
This looks very scary: `x->Func(... […]
true. the idea was to keep the screen_enumerator alive. but i can achieve the same by holding the ScreenEnumerator as a member function and lazy loading it on the first time needed. the move is gone now.
Patch Set #31, Line 2025:
media::AudioParameters()
Is this code correct? Always empty audio parameters? Don't you want to go through `ReadOutputParamsA […]
audio is not supported with multi screen capture (explicitly outscoped).
File third_party/blink/renderer/modules/mediastream/user_media_processor.cc:
Patch Set #31, Line 847:
request_all_screens
Please note that you've renamed to request_ in some places, but kept auto_select_ in others. […]
i kept it for api exposed interfaces as this was the name decided in the design doc and there is already a policy delivered that refers to auto_select_all_screens. i guess then it's best to roll it all back to auto_select_all_screens?
To view, visit change 3605769. To unsubscribe, or for help writing mail filters, visit settings.
Elad Alon (Gerrit)
Attention is currently required from: Simon Hangl.
13 comments:
File content/browser/renderer_host/media/media_stream_dispatcher_host.h:
FRIEND_TEST_ALL_PREFIXES(MediaStreamDispatcherHostTest,
SelectAllScreensPolicyAllowed);
FRIEND_TEST_ALL_PREFIXES(MediaStreamDispatcherHostTest,
SelectAllScreensPolicyForbidden);I couldn't find these in this CL.
GenerateStreamsUIThreadCheckResult() = default;
bool request_allowed;IIANM the default ctor leaves `request_allowed` unassigned - so a garbage value. That'd be a bug. Please do this:
```
bool request_allowed = false;
```Patch Set #38, Line 145:
GenerateStreamsUIThreadCheckResultWould making this const-ref help avoid a copy? It contains strings and origins, and these can be sizeable.
File content/browser/renderer_host/media/media_stream_dispatcher_host.cc:
if (!ui_check_result.first) {
ReceivedBadMessage(
render_process_id_,
bad_message::MSDH_REQUEST_ALL_SCREENS_NOT_ALLOWED_FOR_ORIGIN);
return;
}
no, unless i am missing something, i cannot write a browser test until the basic functionality is la […]
Ack
File content/browser/renderer_host/media/media_stream_dispatcher_host.cc:
Patch Set #38, Line 326:
bool MediaStreamDispatcherHost::CheckRequestAllScreensAllowed(DCHECK thread.
RenderProcessHost* render_process_host =
RenderProcessHost::FromID(render_process_id);
if (!render_process_host)
return false;rm
Patch Set #38, Line 333:
render_frame_hostCheck this for nullness.
GURL url =
render_frame_host->GetMainFrame()->GetLastCommittedOrigin().GetURL();
content::BrowserContext* browser_context =
render_frame_host->GetBrowserContext(I'd have inlined these two, but it's up to you.
File content/browser/renderer_host/media/media_stream_manager.h:
Patch Set #38, Line 759:
= nullptrThis is not necessary because unique_ptr's default ctor takes care of this. (Unlike raw pointers.)
File content/browser/renderer_host/media/media_stream_manager.cc:
Patch Set #31, Line 2025:
media::AudioParameters()
audio is not supported with multi screen capture (explicitly outscoped).
Ack
File content/browser/renderer_host/media/media_stream_manager.cc:
if (!screen_enumerator_) {
screen_enumerator_ =This raises a question - should it really be a member? Do we want to keep a screen enumerator alive for the runtime of the browser? Isn't it enough to only construct and use one when necessary?
screen_enumerator_->EnumerateScreens(
request->video_type(),
base::BindOnce(&MediaStreamManager::HandleAccessRequestResponse,
base::Unretained(this), label,
media::AudioParameters()));
} else {
ReadOutputParamsAndPostRequestToUI(label, request,
MediaDeviceEnumeration());```
screen_enumerator_->EnumerateScreens(
request->video_type(),
base::BindOnce(&MediaStreamManager::HandleAccessRequestResponse,
base::Unretained(this), label,
media::AudioParameters()));
return;
}ReadOutputParamsAndPostRequestToUI(label, request, MediaDeviceEnumeration());
}
```
File third_party/blink/renderer/modules/mediastream/user_media_processor.cc:
Patch Set #31, Line 847:
request_all_screens
i kept it for api exposed interfaces as this was the name decided in the design doc and there is alr […]
Okay, let's file an issue for clean-up and make the decision then.
To view, visit change 3605769. To unsubscribe, or for help writing mail filters, visit settings.
Simon Hangl (Gerrit)
Attention is currently required from: Elad Alon.
11 comments:
Patchset:
eladalon@: i revised the cl. PTAL.
File content/browser/renderer_host/media/media_stream_dispatcher_host.h:
FRIEND_TEST_ALL_PREFIXES(MediaStreamDispatcherHostTest,
SelectAllScreensPolicyAllowed);
FRIEND_TEST_ALL_PREFIXES(MediaStreamDispatcherHostTest,
SelectAllScreensPolicyForbidden);I couldn't find these in this CL.
Done
GenerateStreamsUIThreadCheckResult() = default;
bool request_allowed;
IIANM the default ctor leaves `request_allowed` unassigned - so a garbage value. That'd be a bug. […]
Done
Patch Set #38, Line 145:
GenerateStreamsUIThreadCheckResult
Would making this const-ref help avoid a copy? It contains strings and origins, and these can be siz […]
no, as it would clash with the subsequent
```
MediaDeviceSaltAndOrigin salt_and_origin =
std::move(ui_check_result.salt_and_origin);
ui_check_result = {};
```
and
```
media_stream_manager_->GenerateStreams(
render_process_id_, render_frame_id_, requester_id_, page_request_id,
controls, std::move(salt_and_origin),
```i also checked how the initial implementation solved this problem [1], but there it was a copy as well.
[1] https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/media/media_stream_dispatcher_host.cc;l=402
File content/browser/renderer_host/media/media_stream_dispatcher_host.cc:
Patch Set #38, Line 326:
bool MediaStreamDispatcherHost::CheckRequestAllScreensAllowed(DCHECK thread.
Done
RenderProcessHost* render_process_host =
RenderProcessHost::FromID(render_process_id);
if (!render_process_host)
return false;rm
Done
Patch Set #38, Line 333:
render_frame_hostCheck this for nullness.
Done
GURL url =
render_frame_host->GetMainFrame()->GetLastCommittedOrigin().GetURL();
content::BrowserContext* browser_context =
render_frame_host->GetBrowserContext(I'd have inlined these two, but it's up to you.
Done
File content/browser/renderer_host/media/media_stream_manager.h:
Patch Set #38, Line 759:
= nullptr
This is not necessary because unique_ptr's default ctor takes care of this. (Unlike raw pointers. […]
Done
File content/browser/renderer_host/media/media_stream_manager.cc:
if (!screen_enumerator_) {
screen_enumerator_ =
This raises a question - should it really be a member? Do we want to keep a screen enumerator alive […]
see question in chat:
" so initially i had the screen enumerator moved over to the callback so that it is guaranteed to be alive, but you (rightfully so) pointed out that this is pretty scary [1]. the only alternative i see is to keep it as a member of MediaStreamManager. is there any other alternative?[1] https://chromium-review.googlesource.com/c/chromium/src/+/3605769/comments/fbf5280e_9e685c5c"
screen_enumerator_->EnumerateScreens(
request->video_type(),
base::BindOnce(&MediaStreamManager::HandleAccessRequestResponse,
base::Unretained(this), label,
media::AudioParameters()));
} else {
ReadOutputParamsAndPostRequestToUI(label, request,
MediaDeviceEnumeration());
``` […]
Done
To view, visit change 3605769. To unsubscribe, or for help writing mail filters, visit settings.
Elad Alon (Gerrit)
Attention is currently required from: Simon Hangl.
1 comment:
File content/browser/renderer_host/media/media_stream_manager.cc:
if (!screen_enumerator_) {
screen_enumerator_ =
see question in chat: […]
Would this work?
```
std::unique_ptr<media::ScreenEnumerator> enumerator =
GetContentClient()->browser()->CreateScreenEnumerator();
...
enumerator->EnumeratreScreens(...);
```
Even if the enumerator posts a task, so long as the task does not need to respond directly to the enumerator, we can safely destroy it, right? IIANM, you'd have even made it static, if not for your need to expose it as an overridden virtual method? So the actual object is not truly necessary. Or am I wrong?
To view, visit change 3605769. To unsubscribe, or for help writing mail filters, visit settings.
Simon Hangl (Gerrit)
Patchset:
eladalon@: i fixed your remaining comment. PTAL.
File content/browser/renderer_host/media/media_stream_manager.cc:
if (!screen_enumerator_) {
screen_enumerator_ =
Would this work? […]
hm, you know my code better than i do 👍 . just checked it, you are absolutely right, no need to keep the object alive. the posted task is not a static member function but a regular function.
To view, visit change 3605769. To unsubscribe, or for help writing mail filters, visit settings.
Elad Alon (Gerrit)
Attention is currently required from: Simon Hangl.
Patch set 42:Code-Review +1
5 comments:
File content/browser/renderer_host/media/media_stream_dispatcher_host.h:
Patch Set #42, Line 67:
GenerateStreamsUIThreadCheckResult() = default;Is this line strictly required?
int render_process_id,
int render_frame_id,It's more idiomatic in this codebase to pass these two ints first.
File content/browser/renderer_host/media/media_stream_manager.cc:
if (!screen_enumerator_) {
screen_enumerator_ =
hm, you know my code better than i do 👍 . […]
Ack
File content/browser/renderer_host/media/media_stream_manager.cc:
Patch Set #42, Line 2013:
PERMISSION_DENIED`NOT_SUPPORTED` seems more appropriate given the current implementations of CreateScreenEnumerator, where there's one impl that always returns nullptr and one that never returns nullptr. Or wdyt?
File content/public/browser/content_browser_client.h:
Patch Set #42, Line 637:
Screen auto selection allows// Check if applications whose URL is |url| are allowed to perform all-screens-auto-selection, which...
To view, visit change 3605769. To unsubscribe, or for help writing mail filters, visit settings.
Simon Hangl (Gerrit)
Attention is currently required from: Avi Drissman, Tom Sepez.
Simon Hangl would like Avi Drissman and Tom Sepez to review this change.
[MSC] Screen auto selection for getDisplayMediaSet.
This change enables auto selection for getDisplayMediaSet.
It propagates the autoSelectAllScreens property through the
call stack, loads a list of all attached screens and forwards
it for capturing.
Bug: 1300883
Change-Id: I4cc00e9dd6807c5a992367a31f10e261c372b89e
---
M chrome/browser/chrome_content_browser_client.cc
M chrome/browser/chrome_content_browser_client.h
M content/browser/bad_message.h
M content/browser/renderer_host/media/media_stream_dispatcher_host.cc
M content/browser/renderer_host/media/media_stream_dispatcher_host.h
M content/browser/renderer_host/media/media_stream_manager.cc
M content/public/browser/content_browser_client.cc
M content/public/browser/content_browser_client.h
M third_party/blink/common/mediastream/media_stream_mojom_traits.cc
M third_party/blink/public/common/mediastream/media_stream_controls.h
M third_party/blink/public/common/mediastream/media_stream_mojom_traits.h
M third_party/blink/public/mojom/mediastream/media_stream.mojom
M third_party/blink/renderer/modules/mediastream/user_media_processor.cc
M third_party/blink/renderer/modules/mediastream/user_media_request.cc
M third_party/blink/renderer/modules/mediastream/user_media_request.h
M tools/metrics/histograms/enums.xml
16 files changed, 160 insertions(+), 6 deletions(-)
Simon Hangl (Gerrit)
Attention is currently required from: Avi Drissman, Tom Sepez.
1 comment:
Patchset:
dear reviewers, PTAL at the following files:
avi@: content/public/browser/content_browser_client.h,
content/public/browser/content_browser_client.cc, content/browser/bad_message.h
tsepez@: third_party/blink/public/common/mediastream/media_stream_mojom_traits.h, third_party/blink/common/mediastream/media_stream_mojom_traits.cc, third_party/blink/public/mojom/mediastream/media_stream.mojom
To view, visit change 3605769. To unsubscribe, or for help writing mail filters, visit settings.
Simon Hangl (Gerrit)
Attention is currently required from: Alex Gough, Avi Drissman.
Simon Hangl would like Alex Gough to review this change.
Simon Hangl removed Tom Sepez from this change.
To view, visit change 3605769. To unsubscribe, or for help writing mail filters, visit settings.
Simon Hangl (Gerrit)
Attention is currently required from: Alex Gough, Avi Drissman.
1 comment:
Patchset:
ajgo@, i added a new field in StreamControls. PTAL: third_party/blink/common/mediastream/media_stream_mojom_traits.cc, third_party/blink/public/common/mediastream/media_stream_mojom_traits.h, third_party/blink/public/mojom/mediastream/media_stream.mojom
To view, visit change 3605769. To unsubscribe, or for help writing mail filters, visit settings.
Simon Hangl (Gerrit)
Attention is currently required from: Alex Gough, Arthur Sonzogni.
Simon Hangl would like Arthur Sonzogni to review this change.
Simon Hangl removed Avi Drissman from this change.
16 files changed, 179 insertions(+), 6 deletions(-)
Simon Hangl (Gerrit)
Attention is currently required from: Alex Gough, Arthur Sonzogni.
Patch set 46:Commit-Queue +1
1 comment:
Patchset:
arthursonzogni@: for continuation i moved you from the previous cl to this as reviewer (see comment [1]). PTAL at
content/public/browser/content_browser_client.cc, content/public/browser/content_browser_client.h, content/browser/bad_message.h
[1] https://chromium-review.googlesource.com/c/chromium/src/+/3650509/comments/368f00c7_06ebd144
To view, visit change 3605769. To unsubscribe, or for help writing mail filters, visit settings.
Alex Gough (Gerrit)
Attention is currently required from: Arthur Sonzogni, Simon Hangl.
Patch set 47:Code-Review +1
1 comment:
Patchset:
lgtm mojom - thanks for the comment about ownership!
To view, visit change 3605769. To unsubscribe, or for help writing mail filters, visit settings.
Arthur Sonzogni (Gerrit)
File content/public/browser/content_browser_client.h:
virtual std::unique_ptr<media::ScreenEnumerator> CreateScreenEnumerator()
const;This new API returning an object, the object implements a single function.
Did you considered exposing directly a virtual method, and not adding this object in the middle?
Maybe you are going to add additional methods and this will deserve its own interface?
Or maybe you think it will need to hold same state. I see there is already "get_all_windows_delegate_", but it is always equal to ash::Shell::GetAllRootWindow(), except for one test, where it is replaced by a different function. Potentially, this could be replaced by a global and a ReplaceXXXForTesting() like we do often:
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/loader/navigation_url_loader.cc;l=76-80;drc=b443215a203fbe4bfac2e839e47987971d21a218;bpv=1;bpt=1
To view, visit change 3605769. To unsubscribe, or for help writing mail filters, visit settings.
Arthur Sonzogni (Gerrit)
Attention is currently required from: Simon Hangl.
2 comments:
File content/browser/renderer_host/media/media_stream_dispatcher_host.cc:
Patch Set #47, Line 326:
CheckRequestAllScreensAllowed|request_all_screens| is always false. Are you going to add tests in follow-up so that all this code is covered?
Patch Set #47, Line 338:
.GetURL()If the data we pass is the origin, it would be better to pass the url::Origin directly.
I understand you are trying to reuse "IsGetDisplayMediaSetSelectAllScreensAllowed" later, which is taking a GURL.
Still, it would be better to plumb the origin instead, to the deepest level you can. Then proceed with the conversion when calling IsGetDisplayMediaSetSelectAllScreensAllowed and add a TODO about converting this function in the future for url::Origin.Note: In the near future, on top of the origin, this might need to take the storage key instead of the simple origin. See:
https://privacycg.github.io/storage-partitioning/
To view, visit change 3605769. To unsubscribe, or for help writing mail filters, visit settings.
Simon Hangl (Gerrit)
Attention is currently required from: Arthur Sonzogni.
8 comments:
Patchset:
arthursonzogni@: i revised the cl and replied to your comments. PTAL.
File content/browser/renderer_host/media/media_stream_dispatcher_host.h:
Patch Set #42, Line 67:
GenerateStreamsUIThreadCheckResult() = default;Is this line strictly required?
Done
int render_process_id,
int render_frame_id,It's more idiomatic in this codebase to pass these two ints first.
Done
File content/browser/renderer_host/media/media_stream_dispatcher_host.cc:
Patch Set #47, Line 326:
CheckRequestAllScreensAllowed
|request_all_screens| is always false. […]
yes, i plan to add a set of browser tests when the loop for the getDisplayMediaSet API is closed. i added an explicit todo here too.
Patch Set #47, Line 338:
.GetURL()
If the data we pass is the origin, it would be better to pass the url::Origin directly. […]
pushed origin a bit further. added a todo.
File content/browser/renderer_host/media/media_stream_manager.cc:
Patch Set #42, Line 2013:
PERMISSION_DENIED
`NOT_SUPPORTED` seems more appropriate given the current implementations of CreateScreenEnumerator, […]
Done
File content/public/browser/content_browser_client.h:
virtual std::unique_ptr<media::ScreenEnumerator> CreateScreenEnumerator()
const;
This new API returning an object, the object implements a single function. […]
i did consider this, though i suggest to keep the object for the following reasons:
- additional methods may indeed be added (e.g. different surfaces, adding filters for screen selection). time frame: mid-term
- the implementation will grow significantly in complexity as it will also have to handle lacros. time frame: short-term
- we will move from a sorting based matching to id based matching. this may also add some complexity and may indeed require holding a state (e.g. to generate sequential ids or to dispatch asynchronous requests to fetch these ids).
File content/public/browser/content_browser_client.h:
Patch Set #42, Line 637:
Screen auto selection allows
// Check if applications whose URL is |url| are allowed to perform all-screens-auto-selection, which […]
Done
To view, visit change 3605769. To unsubscribe, or for help writing mail filters, visit settings.
Arthur Sonzogni (Gerrit)
Attention is currently required from: Simon Hangl.
Patch set 49:Code-Review +1
4 comments:
Patchset:
LGTM for:
```
content/public/browser/content_browser_client.cc
content/public/browser/content_browser_client.h
content/browser/bad_message.h
```
File content/browser/renderer_host/media/media_stream_dispatcher_host.cc:
Patch Set #47, Line 326:
CheckRequestAllScreensAllowed
yes, i plan to add a set of browser tests when the loop for the getDisplayMediaSet API is closed. […]
Ack
Patch Set #47, Line 338:
.GetURL()
pushed origin a bit further. added a todo.
Thanks!
File content/public/browser/content_browser_client.h:
virtual std::unique_ptr<media::ScreenEnumerator> CreateScreenEnumerator()
const;
i did consider this, though i suggest to keep the object for the following reasons: […]
Ack
To view, visit change 3605769. To unsubscribe, or for help writing mail filters, visit settings.
Simon Hangl (Gerrit)
Attention is currently required from: Simon Hangl.
Patch set 52:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
49 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
[MSC] Screen auto selection for getDisplayMediaSet.
This change enables auto selection for getDisplayMediaSet.
It propagates the autoSelectAllScreens property through the
call stack, loads a list of all attached screens and forwards
it for capturing.
Bug: 1300883
Change-Id: I4cc00e9dd6807c5a992367a31f10e261c372b89e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3605769
Reviewed-by: Elad Alon <elad...@chromium.org>
Reviewed-by: Alex Gough <aj...@chromium.org>
Commit-Queue: Simon Hangl <sim...@google.com>
Reviewed-by: Arthur Sonzogni <arthurs...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1022849}
---
M chrome/browser/chrome_content_browser_client.cc
M chrome/browser/chrome_content_browser_client.h
M chrome/browser/media/webrtc/capture_policy_utils.h
M content/browser/bad_message.h
M content/browser/renderer_host/media/media_stream_dispatcher_host.cc
M content/browser/renderer_host/media/media_stream_dispatcher_host.h
M content/browser/renderer_host/media/media_stream_manager.cc
M content/public/browser/content_browser_client.cc
M content/public/browser/content_browser_client.h
M third_party/blink/common/mediastream/media_stream_mojom_traits.cc
M third_party/blink/public/common/mediastream/media_stream_controls.h
M third_party/blink/public/common/mediastream/media_stream_mojom_traits.h
M third_party/blink/public/mojom/mediastream/media_stream.mojom
M third_party/blink/renderer/modules/mediastream/user_media_processor.cc
M third_party/blink/renderer/modules/mediastream/user_media_request.cc
M third_party/blink/renderer/modules/mediastream/user_media_request.h
M tools/metrics/histograms/enums.xml
17 files changed, 188 insertions(+), 6 deletions(-)
Silvia Medina (Gerrit)
Silvia Medina added 1 comment![]( "Open in Gerrit")
Related details
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |