1311901 - Add comments to the internal states of MDSM

Status: RESOLVED FIXED

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

Comment 1

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

Attachment: Bug 1311901 - document the internal states of MDSM.

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

Comment 2

[Tzuhao Kuo [:kaku]]

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:
```

Comment 3

[Kilik Kuo [:kikuo] (inactive)]

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.

Comment 4

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

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!

Comment 5

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

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 6

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

Comment on attachment 8803213 [details]
Bug 1311901 - document the internal states of MDSM.

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6e892f4718b0
document the internal states of MDSM. r=kaku,kikuo

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6e892f4718b0