Closed
Bug 1299096
Opened 6 years ago
Closed 6 years ago
Remove the event-based play() method out from nsIDOMHTMLMediaElement.idl
Categories
(Core :: Audio/Video: Playback, defect, P3)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: kaku, Assigned: kaku)
References
Details
Attachments
(1 file)
We are going to implement the promise-based play() method in the HTMLMediaElement.webidl (bug 1244768). Follow-up bug 1244768 comment 50, we want to remove the event-based play() method out from nsIDOMHTMLMediaElement.idl here.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → kaku
Updated•6 years ago
|
Priority: -- → P3
Comment hidden (mozreview-request) |
Comment 2•6 years ago
|
||
Need a DOM peer to review that change.
Assignee | ||
Comment 3•6 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=94f5581d7086f273d9761e7906a66d1b26dca00e https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba6379f13e115fd6a314c6b40d691c3fe7c2cfc6
Assignee | ||
Updated•6 years ago
|
Attachment #8818195 -
Flags: review?(bzbarsky)
![]() |
||
Comment 4•6 years ago
|
||
mozreview-review |
Comment on attachment 8818195 [details] Bug 1299096 - Remove the event-based play() method out from nsIDOMHTMLMediaElement.idl; https://reviewboard.mozilla.org/r/98328/#review99578 r=me with the comments below fixed. I wish mozreview still had footers... ::: dom/html/HTMLMediaElement.cpp:893 (Diff revision 1) > - nsresult rv = mOwner->Play(); > - if (NS_WARN_IF(NS_FAILED(rv))) { > + ErrorResult rv; > + RefPtr<Promise> toBeIgnored = mOwner->Play(rv); > + MOZ_ASSERT_IF(toBeIgnored && toBeIgnored->State() == Promise::PromiseState::Rejected, > + rv.Failed()); > + if (NS_WARN_IF(rv.Failed())) { > + rv.StealNSResult(); This is a slightly odd pattern. Is the goal to have that warning from the NS_WARN_IF? Because if the idea is to ignore the ErorResult, just make it an IgnoredErrorResult so it will suppress the exception automatically in its destructor. If you want to warn if rv.Failed(), I'd do it like so: if (rv.Failed()) { NS_WARNING(some-useful-text-here); } and drop the explicit return in trailing position in a void function...
Attachment #8818195 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 5•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8818195 [details] Bug 1299096 - Remove the event-based play() method out from nsIDOMHTMLMediaElement.idl; https://reviewboard.mozilla.org/r/98328/#review99578 > This is a slightly odd pattern. Is the goal to have that warning from the NS_WARN_IF? > > Because if the idea is to ignore the ErorResult, just make it an IgnoredErrorResult so it will suppress the exception automatically in its destructor. If you want to warn if rv.Failed(), I'd do it like so: > > if (rv.Failed()) { > NS_WARNING(some-useful-text-here); > } > > and drop the explicit return in trailing position in a void function... Thanks for the review. I will modify the code to keep the warning.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•6 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=279ec209d5f4077b95ce5fd45449477282cf9456 https://treeherder.mozilla.org/#/jobs?repo=try&revision=9367a38cdb3700cd4d470993f211f7cfb556c5e7
![]() |
||
Comment 8•6 years ago
|
||
> Thanks for the review. I will modify the code to keep the warning.
Is there a reason you decided to use a manual SuppressException call instead of using IgnoredErrorResult like I suggested?
Flags: needinfo?(kaku)
Assignee | ||
Comment 9•6 years ago
|
||
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #8) > > Thanks for the review. I will modify the code to keep the warning. > > Is there a reason you decided to use a manual SuppressException call instead > of using IgnoredErrorResult like I suggested? No, sorry, I was confused somehow, and I think the right way is using IgnoredErrorResult, warning if it is failed and then suppress the exception automatically in the destructor. Will update the patch again. Thanks for reminding.
Flags: needinfo?(kaku)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•6 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e931e3449b48b2ce05765079d84fc74b99bba9f https://treeherder.mozilla.org/#/jobs?repo=try&revision=47fda667e94c454ba89708779e4cd6a04f515658
Updated•6 years ago
|
Attachment #8818195 -
Flags: review?(jwwang)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•6 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9340947773d44de775335ffafb519696d1893445 https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb3d6133774c39addf911a30feb408e405d18640
Comment 15•6 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3b46fcc012e5 Remove the event-based play() method out from nsIDOMHTMLMediaElement.idl; r=bz
Keywords: checkin-needed
Comment 16•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3b46fcc012e5
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Updated•6 years ago
|
Keywords: dev-doc-needed,
site-compat
Comment 17•6 years ago
|
||
I guess this change won't affect site-compat.
Keywords: dev-doc-needed,
site-compat
You need to log in
before you can comment on or make changes to this bug.
Description
•