Closed Bug 830672 Opened 8 years ago Closed 8 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21
blocking-b2g tef+
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
Blocks: 828200
Assignee: nobody → amarchesini
Attached patch patch (obsolete) — Splinter Review
Attachment #702193 - Flags: review?(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.
Attached patch patch (obsolete) — Splinter Review
Attachment #702193 - Attachment is obsolete: true
Attachment #702193 - Flags: review?(mchen)
Attachment #702212 - Flags: review?(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.
Attachment #702212 - Flags: review?(mchen) → review+
Attached patch patchSplinter Review
Attachment #702212 - Attachment is obsolete: true
Attachment #702781 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/48461c81811e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
blocking-b2g: --- → tef?
Blocks: 828283
tef+ because bug 828283 needs this patch. AudioChannelService is used in b2g only so this patch should land on b2g18 too.
(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?
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.
blocking-b2g: tef? → tef+
Summary: Follow up 828200 - the audio-channel-changed should follow comment 6 → Follow up 828200 - the audio-channel-changed should follow comment 6 from bug 828200
https://hg.mozilla.org/releases/mozilla-b2g18/rev/c99fb2843bdc
Patch for 830648 was not landed yet. Pushed again to b2g18.
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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.