[CE] Correct logic of DefaultEventHandler in HTMLUserMediaElement [chromium/src : main]
Thomas Nguyen (Gerrit)
Thomas Nguyen added 1 comment![]( "Open in Gerrit")
Hi Joey, Ravjit, could you please take a look at this?
Related details
- Joey Arhar
- 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 added 1 comment![]( "Open in Gerrit")
Need one more reviewer for third_party/blink/web_tests/VirtualTestSuites, Anders could you please take a looK?
Related details
- Anders Hartvoll Ruud
- Joey Arhar
- Ravjit Uppal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 2 comments![]( "Open in Gerrit")
HTMLUserMediaElementMediaStream::From(*element_).SetMediaStream(stream);
element_->DispatchEvent(*Event::Create(event_type_names::kStream));
}These lines should have less indentation
It isn't obvious to me why this patch adds a virtual test suite. Want to add a paragraph to the commit message explaining why it is added in this patch as opposed to an earlier one which actually added these flags?
Related details
- Anders Hartvoll Ruud
- 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 2 comments![]( "Open in Gerrit")
HTMLUserMediaElementMediaStream::From(*element_).SetMediaStream(stream);
element_->DispatchEvent(*Event::Create(event_type_names::kStream));
}These lines should have less indentation
Oh, thanks for catching this. I don't know why `git cl format` did not work and the presubmit did not warn about this.
It isn't obvious to me why this patch adds a virtual test suite. Want to add a paragraph to the commit message explaining why it is added in this patch as opposed to an earlier one which actually added these flags?
This is to add `BypassPepcSecurityForTesting` args to the tests. Basically we are mirroring the logic of <geolocation> element (HTMLGeolocationElement), however, the test in <usermedia> contains a "click test" which is disabled by security measures.
I will add a comment about that.
Related details
- Anders Hartvoll Ruud
- Joey Arhar
- 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 removed Anders Hartvoll Ruud from this change![]( "Open in Gerrit")
- Joey Arhar
- 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 1 comment![]( "Open in Gerrit")
I need to fix the test again. So, remove reviewers and I will add you back
Related details
- Joey Arhar
- 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 2 comments![]( "Open in Gerrit")
HTMLUserMediaElementMediaStream::From(*element_).SetMediaStream(stream);
element_->DispatchEvent(*Event::Create(event_type_names::kStream));
}Thomas NguyenThese lines should have less indentation
Oh, thanks for catching this. I don't know why `git cl format` did not work and the presubmit did not warn about this.
Done
Thomas NguyenIt isn't obvious to me why this patch adds a virtual test suite. Want to add a paragraph to the commit message explaining why it is added in this patch as opposed to an earlier one which actually added these flags?
This is to add `BypassPepcSecurityForTesting` args to the tests. Basically we are mirroring the logic of <geolocation> element (HTMLGeolocationElement), however, the test in <usermedia> contains a "click test" which is disabled by security measures.
I will add a comment about that.
Added comment and moved to the right place.
Thomas Nguyen (Gerrit)
Thomas Nguyen added 1 comment![]( "Open in Gerrit")
Thomas NguyenIt isn't obvious to me why this patch adds a virtual test suite. Want to add a paragraph to the commit message explaining why it is added in this patch as opposed to an earlier one which actually added these flags?
Thomas NguyenThis is to add `BypassPepcSecurityForTesting` args to the tests. Basically we are mirroring the logic of <geolocation> element (HTMLGeolocationElement), however, the test in <usermedia> contains a "click test" which is disabled by security measures.
I will add a comment about that.
Added comment and moved to the right place.
Done
Related details
- Joey Arhar
- Ravjit Uppal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 1 comment![]( "Open in Gerrit")
Thomas NguyenIt isn't obvious to me why this patch adds a virtual test suite. Want to add a paragraph to the commit message explaining why it is added in this patch as opposed to an earlier one which actually added these flags?
Thomas NguyenThis is to add `BypassPepcSecurityForTesting` args to the tests. Basically we are mirroring the logic of <geolocation> element (HTMLGeolocationElement), however, the test in <usermedia> contains a "click test" which is disabled by security measures.
I will add a comment about that.
Thomas NguyenAdded comment and moved to the right place.
Done
So is the virtual test suite supposed to be there forever? Could we instead check whether we are running in a web test to bypass the security check?
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 1 comment![]( "Open in Gerrit")
Thomas NguyenIt isn't obvious to me why this patch adds a virtual test suite. Want to add a paragraph to the commit message explaining why it is added in this patch as opposed to an earlier one which actually added these flags?
Thomas NguyenThis is to add `BypassPepcSecurityForTesting` args to the tests. Basically we are mirroring the logic of <geolocation> element (HTMLGeolocationElement), however, the test in <usermedia> contains a "click test" which is disabled by security measures.
I will add a comment about that.
Thomas NguyenAdded comment and moved to the right place.
Joey ArharDone
So is the virtual test suite supposed to be there forever? Could we instead check whether we are running in a web test to bypass the security check?
We’ll keep the click tests as they are.
and yes, we can check in the native code by allowing WPT-driven clicks (`IsRunningWebTest`. I’m slightly worried about future tests that might need to block synthetic clicks, but since there’s nothing like that in the WPT right now, let's keep it simple. If we need to, we can always bring back the virtual test suites later.
Updated.
Related details
- Joey Arhar
- Ravjit Uppal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 1 comment![]( "Open in Gerrit")
await new Promise(resolve => t.step_timeout(resolve, 600));What exactly is this waiting for? Does it not work if you put 0 instead of 600 in the timeout? Or maybe 0 with a rAF?
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 1 comment![]( "Open in Gerrit")
await new Promise(resolve => t.step_timeout(resolve, 600));What exactly is this waiting for? Does it not work if you put 0 instead of 600 in the timeout? Or maybe 0 with a rAF?
In the first 500ms after being added to DOM and rendered on a page, the element is unclickable. So at least it should be 501ms.
Related details
- Joey Arhar
- 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 Code-Review+1![]( "Open in Gerrit")
- Joey Arhar
- 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. |
Joey Arhar (Gerrit)
Joey Arhar voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joey Arhar
| Code-Review | +1 |
1 comment
await new Promise(resolve => t.step_timeout(resolve, 600));Thomas NguyenWhat exactly is this waiting for? Does it not work if you put 0 instead of 600 in the timeout? Or maybe 0 with a rAF?
In the first 500ms after being added to DOM and rendered on a page, the element is unclickable. So at least it should be 501ms.
- Ravjit Uppal
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/59276.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
- Ravjit Uppal
- Thomas Nguyen
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 Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Ravjit Uppal
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 with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
15 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/web_tests/external/wpt/html/semantics/permission-element/usermedia/set-constraints-combinations.tentative.html
Insertions: 2, Deletions: 2.
@@ -82,10 +82,10 @@
await new Promise(r => t.step_timeout(r, 600));
if (should_trigger) {
- await test_driver.bless("click", () => usermedia.click());
+ await test_driver.click(usermedia);
await stream_promise;
} else {
- await test_driver.bless("click", () => usermedia.click());
+ await test_driver.click(usermedia);
// Wait a bit to ensure no stream event is fired
await new Promise(r => t.step_timeout(r, 200));
}
```
Change information
[CE] Correct logic of DefaultEventHandler in HTMLUserMediaElement
Ensure that HTMLUserMediaElement does not initiate multiple concurrent
media stream requests. Previously, rapid user activations could trigger
overlapping requests before the initial one had completed.
Additionally, the activation logic was updated to use the base
capability element's activation handler, ensuring that permission
checks and media requests are correctly synchronized with user events.
Fixed: 500303904
- M chrome/browser/permissions/permission_element_browsertest.cc
- M chrome/browser/ui/views/permissions/embedded_permission_prompt_interactive_uitest.cc
- M third_party/blink/renderer/core/html/html_capability_element_base.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/modules/mediastream/user_media_request_provider_impl.cc
- M third_party/blink/web_tests/external/wpt/html/semantics/permission-element/usermedia/set-constraints-combinations.tentative.html
- M third_party/blink/web_tests/external/wpt/html/semantics/permission-element/usermedia/usermedia-constraints-descriptors.tentative.html
- M third_party/blink/web_tests/external/wpt/html/semantics/permission-element/usermedia/usermedia-no-constraints.tentative.html
Code-Review: +1 by Joey Arhar
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/59276
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |