Closed Bug 1299096 Opened 5 years ago Closed 5 years ago

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

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: kaku, Assigned: kaku)

References

Details

Attachments

(1 file)

Bug 1299096 - Remove the event-based play() method out from nsIDOMHTMLMediaElement.idl;
58 bytes, text/x-review-board-request
bzbarsky
: review+
Details 
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: nobody → kaku
Blocks: 1244768
Need a DOM peer to review that change.
Attachment #8818195 - Flags: review?(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...
Attachment #8818195 - Flags: review?(bzbarsky) → 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

> 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.
> 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)
(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)
Attachment #8818195 - Flags: review?(jwwang)
Try looks good, thanks for the reveiw!
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/3b46fcc012e5
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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.