830672 - Follow up 828200 - the audio-channel-changed should follow comment 6 from bug 828200

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 2

[Marco Chen [:mchen]]

Comment on attachment 702193 [details] [diff] [review]
patch

Review of attachment 702193 [details] [diff] [review]:
-----------------------------------------------------------------

The logic for finding higher audio channel type is good.
And please correct the right comments for unit test.

Thanks.

::: dom/audiochannel/tests/TestAudioChannelService.cpp
@@ +282,5 @@
> +  // Settings visible the normal channel.
> +  rv = normalAgent.mAgent->SetVisibilityState(true);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // Normal should not be playing because not visible.

// Normal should be playing because it is visible.

And the following comments for each channels are wrong too.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 4

[Marco Chen [:mchen]]

Comment on attachment 702212 [details] [diff] [review]
patch

Review of attachment 702212 [details] [diff] [review]:
-----------------------------------------------------------------

Review+ after fixing the wrong comment.

Thanks.

::: dom/audiochannel/tests/TestAudioChannelService.cpp
@@ +336,5 @@
> +  // Settings visible the pNotification channel.
> +  rv = pNotificationAgent.mAgent->SetVisibilityState(true);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // pNotification should not be playing because not visible.

// pNotification should be playing because visible.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 6

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/48461c81811e

https://tbpl.mozilla.org/?tree=Try&rev=d5e0f1b78f54

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/48461c81811e

Comment 8

[Andrea Marchesini [:baku]]

tef+ because bug 828283 needs this patch. AudioChannelService is used in b2g only so this patch should land on b2g18 too.

Comment 9

[Alex Keybl [:akeybl]]

(In reply to Andrea Marchesini (:baku) from comment #8)
> tef+ because bug 828283 needs this patch. AudioChannelService is used in b2g
> only so this patch should land on b2g18 too.

Can you explain why this is a requirement for bug 828283?

Comment 10

[Andrea Marchesini [:baku]]

The patch for bug 828283 is based on the visibility of the media elements. In this patch we implement a visibility enum for each audio channel type.
Without this patch the visible-audio-channel-changed event will be emitted wrongly.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/ca138e4376f1

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Backed out for test failures in bug 829561, which this depends on:
https://hg.mozilla.org/releases/mozilla-b2g18/rev/d139dca8c3b4

Comment 13

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/c99fb2843bdc
Patch for 830648 was not landed yet. Pushed again to b2g18.

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

Landed on mozilla-b2g18/gaia master prior to the 1/25 branching to mozilla-b2g18_v1_0_0/v1.0.0, updating status-b2g-v1.0.0 to fixed.