1299096 - Remove the event-based play() method out from nsIDOMHTMLMediaElement.idl

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P3)

Description

[Tzuhao Kuo [:kaku]]

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.

Comment 1

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1299096 - Remove the event-based play() method out from nsIDOMHTMLMediaElement.idl;

Review commit: https://reviewboard.mozilla.org/r/98328/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98328/

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Need a DOM peer to review that change.

Comment 3

[Tzuhao Kuo [:kaku]]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=94f5581d7086f273d9761e7906a66d1b26dca00e
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba6379f13e115fd6a314c6b40d691c3fe7c2cfc6

Comment 4

[Boris Zbarsky [:bzbarsky]]

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...

Comment 5

[Tzuhao Kuo [:kaku]]

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 6

[Tzuhao Kuo [:kaku]]

Comment on attachment 8818195 [details]
Bug 1299096 - Remove the event-based play() method out from nsIDOMHTMLMediaElement.idl;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98328/diff/1-2/

Comment 7

[Tzuhao Kuo [:kaku]]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=279ec209d5f4077b95ce5fd45449477282cf9456
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9367a38cdb3700cd4d470993f211f7cfb556c5e7

Comment 8

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 9

[Tzuhao Kuo [:kaku]]

(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.

Comment 10

[Tzuhao Kuo [:kaku]]

Comment on attachment 8818195 [details]
Bug 1299096 - Remove the event-based play() method out from nsIDOMHTMLMediaElement.idl;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98328/diff/2-3/

Comment 11

[Tzuhao Kuo [:kaku]]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e931e3449b48b2ce05765079d84fc74b99bba9f
https://treeherder.mozilla.org/#/jobs?repo=try&revision=47fda667e94c454ba89708779e4cd6a04f515658

Comment 12

[Tzuhao Kuo [:kaku]]

Comment on attachment 8818195 [details]
Bug 1299096 - Remove the event-based play() method out from nsIDOMHTMLMediaElement.idl;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98328/diff/3-4/

Comment 13

[Tzuhao Kuo [:kaku]]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9340947773d44de775335ffafb519696d1893445
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb3d6133774c39addf911a30feb408e405d18640

Comment 14

[Tzuhao Kuo [:kaku]]

Try looks good, thanks for the reveiw!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b46fcc012e5
Remove the event-based play() method out from nsIDOMHTMLMediaElement.idl; r=bz

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3b46fcc012e5

Comment 17

[Kohei Yoshino]

I guess this change won't affect site-compat.