Add comments to the internal states of MDSM

RESOLVED FIXED in Firefox 52

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
Assignee: nobody → jwwang
Blocks: 1295892
Priority: -- → P3
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8803213 - Flags: review?(kikuo)
Attachment #8803213 - Flags: review?(kaku)

Comment 2

2 years ago
mozreview-review
Comment on attachment 8803213 [details]
Bug 1311901 - document the internal states of MDSM.

https://reviewboard.mozilla.org/r/87500/#review86504

::: dom/media/MediaDecoderStateMachine.cpp:951
(Diff revision 1)
>    const uint32_t mBufferingWait = 15;
>  };
>  
> +/**
> + * Purpose: play all the decoded data and fire the 'ended' event.
> + *   DORMANT if any dormant request.

Maybe follow the comment pattern of other states, add the following two lines:

```
 *
 * Transition to:
```
Attachment #8803213 - Flags: review?(kaku) → review+

Comment 3

2 years ago
mozreview-review
Comment on attachment 8803213 [details]
Bug 1311901 - document the internal states of MDSM.

https://reviewboard.mozilla.org/r/87500/#review86516

::: dom/media/MediaDecoderStateMachine.cpp:361
(Diff revision 1)
> + * Transition to other states when CDM is ready:
> + *   DORMANT if any pending dormant request.
> + *   DECODING_FIRSTFRAME otherwise.

Just my personal suggestion, I would like to see all kinds of possible state-transition for each states, i.e. SHUTDONW here and elsewhere also.  Though it's defined in the base one.  It would be easier to understand what each state may transit-to without having a look around first.
Attachment #8803213 - Flags: review?(kikuo) → review+
(Assignee)

Comment 4

2 years ago
mozreview-review-reply
Comment on attachment 8803213 [details]
Bug 1311901 - document the internal states of MDSM.

https://reviewboard.mozilla.org/r/87500/#review86504

> Maybe follow the comment pattern of other states, add the following two lines:
> 
> ```
>  *
>  * Transition to:
> ```

Good catch!
(Assignee)

Comment 5

2 years ago
mozreview-review-reply
Comment on attachment 8803213 [details]
Bug 1311901 - document the internal states of MDSM.

https://reviewboard.mozilla.org/r/87500/#review86516

> Just my personal suggestion, I would like to see all kinds of possible state-transition for each states, i.e. SHUTDONW here and elsewhere also.  Though it's defined in the base one.  It would be easier to understand what each state may transit-to without having a look around first.

The transition list should be complete for each state, except SHUTDOWN is omitted for any other state could transition to SHUTDOWN. Is this what you are looking for?
Comment hidden (mozreview-request)

Comment 7

2 years ago
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6e892f4718b0
document the internal states of MDSM. r=kaku,kikuo

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6e892f4718b0
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.