1187763 - Refactor AudioSink::AudioLoop into a series of events.

Status: RESOLVED FIXED

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

Description

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

See bug 1186801 comment 5 and bug 1186801 comment 6.

Comment 1

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

Try is green. https://treeherder.mozilla.org/#/jobs?repo=try&revision=2548132b1f92

Comment 2

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

Attachment: MozReview Request: Bug 1187763. Part 1 - move while loop out of WaitingForAudioToPlay().

Bug 1187763. Part 1 - move while loop out of WaitingForAudioToPlay().

Comment 3

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

Attachment: MozReview Request: Bug 1187763. Part 2 - extract some code of AudioLoop() into its own function.

Bug 1187763. Part 2 - extract some code of AudioLoop() into its own function.

Comment 4

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

Attachment: MozReview Request: Bug 1187763. Part 3 - refactor AudioSink::AudioLoop into a series of events.

Bug 1187763. Part 3 - refactor AudioSink::AudioLoop into a series of events.

Comment 5

[Matthew Gregan [:kinetik]]

Comment on attachment 8639128 [details]
MozReview Request: Bug 1187763. Part 1 - move while loop out of WaitingForAudioToPlay().

https://reviewboard.mozilla.org/r/14175/#review12761

Ship It!

Comment 6

[Matthew Gregan [:kinetik]]

Comment on attachment 8639129 [details]
MozReview Request: Bug 1187763. Part 2 - extract some code of AudioLoop() into its own function.

https://reviewboard.mozilla.org/r/14177/#review12841

Ship It!

Comment 7

[Matthew Gregan [:kinetik]]

Comment on attachment 8639130 [details]
MozReview Request: Bug 1187763. Part 3 - refactor AudioSink::AudioLoop into a series of events.

https://reviewboard.mozilla.org/r/14179/#review12845

::: dom/media/AudioSink.cpp:60
(Diff revision 1)
> +  nsresult rv =

I think you can make this DebugOnly<nsresult>?

::: dom/media/AudioSink.cpp:333
(Diff revision 1)
> +    case AUDIOSINK_STATE_ERROR: {

No need for braces here, and considering there are only two cases, just add a break for the former rather than letting it fall through (which I'd add a // FALLTHROUGH comment for otherwise).

Comment 8

[Matthew Gregan [:kinetik]]

Comment on attachment 8639130 [details]
MozReview Request: Bug 1187763. Part 3 - refactor AudioSink::AudioLoop into a series of events.

https://reviewboard.mozilla.org/r/14179/#review12847

Ship It!

Comment 9

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

https://reviewboard.mozilla.org/r/14179/#review12845

> I think you can make this DebugOnly<nsresult>?

Sure.

> No need for braces here, and considering there are only two cases, just add a break for the former rather than letting it fall through (which I'd add a // FALLTHROUGH comment for otherwise).

Not sure if I understand how this format should be. Is it like

case AUDIOSINK_STATE_COMPLETE: {
  // blah
  break;
}

case AUDIOSINK_STATE_SHUTDOWN:
  break;
case AUDIOSINK_STATE_ERROR:
  break;
} // end of switch

Comment 10

[Matthew Gregan [:kinetik]]

(In reply to JW Wang [:jwwang] from comment #9)
> case AUDIOSINK_STATE_SHUTDOWN:
>   break;
> case AUDIOSINK_STATE_ERROR:
>   break;

Yeah, this one. :-)

Comment 11

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

Thanks! :)

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0fe041a5500
https://hg.mozilla.org/integration/mozilla-inbound/rev/2360777e82ad
https://hg.mozilla.org/integration/mozilla-inbound/rev/82c871e225ab

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d0fe041a5500
https://hg.mozilla.org/mozilla-central/rev/2360777e82ad
https://hg.mozilla.org/mozilla-central/rev/82c871e225ab