[CoCE] Integrate MediaStream into the <usermedia> element [chromium/src : main]
Ravjit Uppal (Gerrit)
Ravjit Uppal voted Commit-Queue+0![]( "Open in Gerrit")
| Commit-Queue | +0 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ravjit Uppal (Gerrit)
Ravjit Uppal added 1 comment![]( "Open in Gerrit")
if (event.type() == event_type_names::kDOMActivate && PermissionsGranted()) {Related details
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Nguyen (Gerrit)
Thomas Nguyen added 1 comment![]( "Open in Gerrit")
if (event.type() == event_type_names::kDOMActivate && PermissionsGranted()) {So && !IsLegacyMode() here for now right? @tun...@chromium.org
- Ravjit Uppal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ravjit Uppal (Gerrit)
Ravjit Uppal added 1 comment![]( "Open in Gerrit")
if (event.type() == event_type_names::kDOMActivate && PermissionsGranted()) {N/A. Discussed offline: We will update it later.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guido Urdaneta (Gerrit)
Guido Urdaneta added 4 comments![]( "Open in Gerrit")
Member<MediaStreamDescriptor> stream_descriptor_;You should cache a MediaStream, not a descriptor (see comment below).
It should be possible to delegate this to HTMLUserMediaElementMediaStream and let the code in modules/mediastream handle it.
return stream_descriptor_.Get();Can you delegate this to HTMLUserMediaElementMediaStream?
After all, you're using GetDescriptor and SetDescriptor only from HTMLUserMediaElementMediaStream, so you don't really need these methods here.
if (stream_descriptor_ && stream_descriptor_->Active()) {You can delegate this check to provider->StartRequest(), so that you don't need the descriptor here.
return MediaStream::Create(element.GetExecutionContext(), descriptor);You should cache the MediaStream once created and return it, not create a new one every time.
If you ever need the descriptor, you can get it from the MediaStream.
Related details
- Ravjit Uppal
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ravjit Uppal (Gerrit)
Ravjit Uppal voted and added 4 comments![]( "Open in Gerrit")
Votes added by Ravjit Uppal
| Commit-Queue | +1 |
4 comments
You should cache a MediaStream, not a descriptor (see comment below).
It should be possible to delegate this to HTMLUserMediaElementMediaStream and let the code in modules/mediastream handle it.
Done
Can you delegate this to HTMLUserMediaElementMediaStream?
After all, you're using GetDescriptor and SetDescriptor only from HTMLUserMediaElementMediaStream, so you don't really need these methods here.
Done
if (stream_descriptor_ && stream_descriptor_->Active()) {You can delegate this check to provider->StartRequest(), so that you don't need the descriptor here.
Done
return MediaStream::Create(element.GetExecutionContext(), descriptor);You should cache the MediaStream once created and return it, not create a new one every time.
If you ever need the descriptor, you can get it from the MediaStream.
Added caching logic in third_party/blink/renderer/modules/mediastream/user_media_request_provider_impl.cc @line 78
Related details
- Guido Urdaneta
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guido Urdaneta (Gerrit)
Guido Urdaneta added 3 comments![]( "Open in Gerrit")
mediastream code lgtm, but can you add unit tests?
return MediaStream::Create(element.GetExecutionContext(), descriptor);Ravjit UppalYou should cache the MediaStream once created and return it, not create a new one every time.
If you ever need the descriptor, you can get it from the MediaStream.
Added caching logic in third_party/blink/renderer/modules/mediastream/user_media_request_provider_impl.cc @line 78
Acknowledged
MediaStream* existing_stream = HTMLUserMediaElementMediaStream::stream(*element);80 columns?
Related details
- Ravjit Uppal
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Nguyen (Gerrit)
Thomas Nguyen added 8 comments![]( "Open in Gerrit")
"//third_party/blink/renderer/modules/mediastream/html_user_media_element_media_stream.idl",I think we should rename to `user_media_element_stream` to match with naming in other partial interface `user_media_element_constraints`, in the same folder.
Or change the `user_media_element_constraints` to `html_user_media_element_constraints`
Can do it in a follow-up
if (PermissionsGranted()) {At this point, we will not trigger stream-acquisition without user interaction. We only consider the logic in the next phase.
if (event.type() == event_type_names::kDOMActivate && PermissionsGranted()) {Please add a TODO here regarding the migration path (the intercorrelation between type attribute and stored constraints)
if (GetDocument().domWindow()) {See comment bellow, we can move to use ExecutionContext
: public Supplement<LocalDOMWindow> {It's better to be a supplement of `ExecutionContext`. In the past, we had some crashes regarding null `LocalDOMWindow` and we moved to `ExecutionContext` instead.
void UserMediaRequestProviderImpl::ProvideTo(LocalDOMWindow& window) {`ExecutionContext`
LocalDOMWindow* window = element->GetDocument().domWindow();Ditto, move to ExecutionContext
MediaConstraints audio_constraints;Please add a TODO to use the stored constraints from the element. Note we also need to revise the constraints based on the type attribute (can be in a follow-up)
Related details
- Ravjit Uppal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ravjit Uppal (Gerrit)
Ravjit Uppal added 7 comments![]( "Open in Gerrit")
mediastream code lgtm, but can you add unit tests?
Added some tests to check for null pointers, caching and stream setting.
"//third_party/blink/renderer/modules/mediastream/html_user_media_element_media_stream.idl",I think we should rename to `user_media_element_stream` to match with naming in other partial interface `user_media_element_constraints`, in the same folder.
Or change the `user_media_element_constraints` to `html_user_media_element_constraints`
Can do it in a follow-up
ack
At this point, we will not trigger stream-acquisition without user interaction. We only consider the logic in the next phase.
Done
if (event.type() == event_type_names::kDOMActivate && PermissionsGranted()) {Please add a TODO here regarding the migration path (the intercorrelation between type attribute and stored constraints)
Done
: public Supplement<LocalDOMWindow> {It's better to be a supplement of `ExecutionContext`. In the past, we had some crashes regarding null `LocalDOMWindow` and we moved to `ExecutionContext` instead.
Wondering what could cause it as <usermedia> only exist in the DOM, meaning they will never exist inside Service Workers or Dedicated Workers(where execution context is non null, but LocalDOMWindow is null)
Also LocalDOMWindow is needed to fetch UserMediaClient
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/mediastream/user_media_client.h;l=65;drc=926943cad55d11056856e35487b1a0d19af4d579
MediaStream* existing_stream = HTMLUserMediaElementMediaStream::stream(*element);Ravjit Uppal80 columns?
Done
Please add a TODO to use the stored constraints from the element. Note we also need to revise the constraints based on the type attribute (can be in a follow-up)
- Guido Urdaneta
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Nguyen (Gerrit)
Thomas Nguyen added 1 comment![]( "Open in Gerrit")
: public Supplement<LocalDOMWindow> {Ravjit UppalIt's better to be a supplement of `ExecutionContext`. In the past, we had some crashes regarding null `LocalDOMWindow` and we moved to `ExecutionContext` instead.
Wondering what could cause it as <usermedia> only exist in the DOM, meaning they will never exist inside Service Workers or Dedicated Workers(where execution context is non null, but LocalDOMWindow is null)
Also LocalDOMWindow is needed to fetch UserMediaClient
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/mediastream/user_media_client.h;l=65;drc=926943cad55d11056856e35487b1a0d19af4d579
I think that does not break the existing rule of Supplement, as way to "attach" new dependency to existing objects at runtime. It means UserMediaRequestProvider can be a able to be created at runtime as a field of ExecutionContext but no way to use it.
Related details
- Guido Urdaneta
- Ravjit Uppal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Nguyen (Gerrit)
Thomas Nguyen added 4 comments![]( "Open in Gerrit")
See comment bellow, we can move to use ExecutionContext
Done
: public Supplement<LocalDOMWindow> {Ravjit UppalIt's better to be a supplement of `ExecutionContext`. In the past, we had some crashes regarding null `LocalDOMWindow` and we moved to `ExecutionContext` instead.
Thomas NguyenWondering what could cause it as <usermedia> only exist in the DOM, meaning they will never exist inside Service Workers or Dedicated Workers(where execution context is non null, but LocalDOMWindow is null)
Also LocalDOMWindow is needed to fetch UserMediaClient
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/mediastream/user_media_client.h;l=65;drc=926943cad55d11056856e35487b1a0d19af4d579
I think that does not break the existing rule of Supplement, as way to "attach" new dependency to existing objects at runtime. It means UserMediaRequestProvider can be a able to be created at runtime as a field of ExecutionContext but no way to use it.
After reconsidering it a bit and likely, the UserMediaRequesatProvider::From() is called only under click event when the window is available. So it will be fine here
(and we must make sure that, this `From` should be only called in context where the window is available)
void UserMediaRequestProviderImpl::ProvideTo(LocalDOMWindow& window) {Thomas Nguyen`ExecutionContext`
Done
LocalDOMWindow* window = element->GetDocument().domWindow();Thomas NguyenDitto, move to ExecutionContext
Done
Thomas Nguyen (Gerrit)
Thomas Nguyen added 1 comment![]( "Open in Gerrit")
: public Supplement<LocalDOMWindow> {Ravjit UppalIt's better to be a supplement of `ExecutionContext`. In the past, we had some crashes regarding null `LocalDOMWindow` and we moved to `ExecutionContext` instead.
Thomas NguyenWondering what could cause it as <usermedia> only exist in the DOM, meaning they will never exist inside Service Workers or Dedicated Workers(where execution context is non null, but LocalDOMWindow is null)
Also LocalDOMWindow is needed to fetch UserMediaClient
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/mediastream/user_media_client.h;l=65;drc=926943cad55d11056856e35487b1a0d19af4d579
Thomas NguyenI think that does not break the existing rule of Supplement, as way to "attach" new dependency to existing objects at runtime. It means UserMediaRequestProvider can be a able to be created at runtime as a field of ExecutionContext but no way to use it.
After reconsidering it a bit and likely, the UserMediaRequesatProvider::From() is called only under click event when the window is available. So it will be fine here
(and we must make sure that, this `From` should be only called in context where the window is available)
Done
Related details
- Guido Urdaneta
- Ravjit Uppal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Nguyen (Gerrit)
Thomas Nguyen voted and added 1 comment![]( "Open in Gerrit")
Related details
- Guido Urdaneta
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guido Urdaneta (Gerrit)
Guido Urdaneta voted and added 2 comments![]( "Open in Gerrit")
Votes added by Guido Urdaneta
| Code-Review | +1 |
2 comments
lgtm % comments
class MODULES_EXPORT UserMediaRequestCallbacks finalRename to UserMediaRequestProviderCallbacks to better differentiate from UserMediaRequest::Callbacks
Related details
- Ravjit Uppal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ravjit Uppal (Gerrit)
Ravjit Uppal voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ravjit Uppal
| Commit-Queue | +1 |
1 comment
Rename to UserMediaRequestProviderCallbacks to better differentiate from UserMediaRequest::Callbacks
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 6 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
6 comments
third_party/blink/renderer/* LGTM, with some nits.
if (event.type() == event_type_names::kDOMActivate && PermissionsGranted()) {There are a few such call sites, and I think I do the same thing every time I review them, which is to check that the code in `HTMLCapabilityElementBase::HandleActivation` checks that the event is *trusted* before allowing the permission request to be initiated. Would you mind adding a comment about that here? (And also in the other places like this, if you wouldn't mind?)
Event* event = Event::Create(event_type_names::kDOMActivate);
element->DefaultEventHandler(*event);Rather than explicitly sending a DOMActivate, do you want to try clicking the element instead?
static MediaStream* stream(HTMLUserMediaElement&);This should likely be `Stream`
class MODULES_EXPORT UserMediaRequestProivderCallbacks final`Provider`
if (permissions.Contains(AtomicString("camera"))) {These are sprinkled everywhere in the codebase. Would you mind consolidating all of those into third_party/blink/renderer/core/html/keywords.json5 ?
Related details
- Ravjit Uppal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ravjit Uppal (Gerrit)
Ravjit Uppal added 5 comments![]( "Open in Gerrit")
if (event.type() == event_type_names::kDOMActivate && PermissionsGranted()) {There are a few such call sites, and I think I do the same thing every time I review them, which is to check that the code in `HTMLCapabilityElementBase::HandleActivation` checks that the event is *trusted* before allowing the permission request to be initiated. Would you mind adding a comment about that here? (And also in the other places like this, if you wouldn't mind?)
Done
Event* event = Event::Create(event_type_names::kDOMActivate);
element->DefaultEventHandler(*event);Rather than explicitly sending a DOMActivate, do you want to try clicking the element instead?
Done
static MediaStream* stream(HTMLUserMediaElement&);This should likely be `Stream`
It is implemented as partial interface therefore it doesn't map to Pascal case.
Shall I leave it as is or update the IDL to include `ImplementedAs Stream` ?
class MODULES_EXPORT UserMediaRequestProivderCallbacks finalRavjit Uppal`Provider`
Done
These are sprinkled everywhere in the codebase. Would you mind consolidating all of those into third_party/blink/renderer/core/html/keywords.json5 ?
- Guido Urdaneta
- Mason Freed
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guido Urdaneta (Gerrit)
Guido Urdaneta voted Code-Review+1![]( "Open in Gerrit")
- Mason Freed
- Ravjit Uppal
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Nguyen (Gerrit)
Thomas Nguyen voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Mason Freed
- Ravjit Uppal
Mason Freed (Gerrit)
Mason Freed voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
2 comments
static MediaStream* stream(HTMLUserMediaElement&);Ravjit UppalThis should likely be `Stream`
It is implemented as partial interface therefore it doesn't map to Pascal case.
Shall I leave it as is or update the IDL to include `ImplementedAs Stream` ?
- Ravjit Uppal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ravjit Uppal (Gerrit)
Ravjit Uppal voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[CoCE] Integrate MediaStream into the <usermedia> element
MediaStream is now directly available in the <usermedia> element when it
is clicked and the required permissions are there.
- M chrome/browser/ui/views/permissions/embedded_permission_prompt_interactive_uitest.cc
- M third_party/blink/renderer/bindings/generated_in_modules.gni
- M third_party/blink/renderer/bindings/idl_in_modules.gni
- M third_party/blink/renderer/core/html/build.gni
- M third_party/blink/renderer/core/html/html_geolocation_element.cc
- M third_party/blink/renderer/core/html/html_user_media_element.cc
- M third_party/blink/renderer/core/html/html_user_media_element.h
- M third_party/blink/renderer/core/html/html_user_media_element_test.cc
- M third_party/blink/renderer/core/html/keywords.json5
- A third_party/blink/renderer/core/html/user_media_request_provider.cc
- A third_party/blink/renderer/core/html/user_media_request_provider.h
- M third_party/blink/renderer/modules/BUILD.gn
- M third_party/blink/renderer/modules/mediastream/BUILD.gn
- A third_party/blink/renderer/modules/mediastream/html_user_media_element_media_stream.cc
- A third_party/blink/renderer/modules/mediastream/html_user_media_element_media_stream.h
- A third_party/blink/renderer/modules/mediastream/html_user_media_element_media_stream_test.cc
- A third_party/blink/renderer/modules/mediastream/user_media_request_provider_impl.cc
- A third_party/blink/renderer/modules/mediastream/user_media_request_provider_impl.h
- A third_party/blink/renderer/modules/mediastream/user_media_request_provider_impl_test.cc
- M third_party/blink/renderer/modules/modules_initializer.cc
- M third_party/blink/web_tests/webexposed/element-instance-property-listing-expected.txt
- M third_party/blink/web_tests/webexposed/global-interface-listing-expected.txt
Code-Review: +1 by Guido Urdaneta, +1 by Thomas Nguyen, +1 by Mason Freed
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ravjit Uppal (Gerrit)
Ravjit Uppal has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |