<audio moz-audiochanneltype="content"> doesn't work.

RESOLVED FIXED in Firefox 18

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: baku, Assigned: baku)

Tracking

Trunk
mozilla20
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox18 fixed, firefox19 fixed)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 687175 [details] [diff] [review]
patch

Maybe we don't like 'moz-audiochanneltype' as attribute name...
Attachment #687175 - Flags: review?(jonas)
(Assignee)

Updated

5 years ago
Assignee: nobody → amarchesini
(Assignee)

Updated

5 years ago
OS: Linux → All
Hardware: x86_64 → All
Comment on attachment 687175 [details] [diff] [review]
patch

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

::: content/base/src/nsGkAtomList.h
@@ +568,5 @@
>  GK_ATOM(mouseout, "mouseout")
>  GK_ATOM(mouseover, "mouseover")
>  GK_ATOM(mousethrough, "mousethrough")
>  GK_ATOM(mouseup, "mouseup")
> +GK_ATOM(mozaudiochanneltype, "moz-audiochanneltype")

Let's just call the attribute "mozaudiochannel". So change this to

GK_ATOM(mozaudiochannel, "mozaudiochannel")

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +3502,5 @@
> +void nsHTMLMediaElement::ParseMozAudioChannelTypeValue(const nsAString& aValue,
> +                                                       nsAttrValue& aResult)
> +{
> +  if (NS_SUCCEEDED(SetMozAudioChannelType(aValue))) {
> +    aResult.SetTo(aValue);

This is inverse from how we do almost all other mapped attributes which I think will be confusing.

It also introduces risk if we start doing things from the SetMozAudioChannelType function since we're generally in a pretty inconsistent state when ParseAttribute is called.

It'll also not store the right value in the DOM. Generally we always allow any value to be set in the DOM, so something like:

<audio mozaudiochannel="aBc123">

should return "aBc123" if someone calls element.getAttribute("mozaudiochannel").

Instead use the same pattern as the preload attribute and use an enum table. Then replace the Get/SetMozAudioChannelType functions with the same type of macro used to implement Get/SetPreload.

We can still keep the mAudioChannelType member. Just set mAudioChannelType = aResult.GetEnumValue() if parsing the enum succeeds.

That way you can also insert a security check before setting mAudioChannelType while allowing the contents in the DOM to always be what the webpage set.
Attachment #687175 - Flags: review?(jonas) → review-
(Assignee)

Comment 2

5 years ago
> That way you can also insert a security check before setting
> mAudioChannelType while allowing the contents in the DOM to always be what
> the webpage set.


What about the permission? We don't want to allow: <audio mozaudiochannel="alarm"> if the app doesn't have permission to do this. Is it?
I'm going to submit a new patch, but just give me a feedback if this is not the right approach.
(Assignee)

Comment 3

5 years ago
Created attachment 687527 [details] [diff] [review]
patch b
Attachment #687175 - Attachment is obsolete: true
Attachment #687527 - Flags: feedback?(jonas)

Updated

5 years ago
Duplicate of this bug: 816872
(Assignee)

Comment 5

5 years ago
Created attachment 687784 [details] [diff] [review]
patch b

This patch is based on Bug 817589 and enables the attribute only for B2G builds.
Attachment #687527 - Attachment is obsolete: true
Attachment #687527 - Flags: feedback?(jonas)
Attachment #687784 - Flags: review?(jonas)
(Assignee)

Comment 6

5 years ago
Created attachment 687863 [details] [diff] [review]
patch c
Attachment #687784 - Attachment is obsolete: true
Attachment #687784 - Flags: review?(jonas)
Attachment #687863 - Flags: review?(jonas)
Comment on attachment 687863 [details] [diff] [review]
patch c

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

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +1966,5 @@
>      }
> +
> +    if (aAttribute == nsGkAtoms::mozaudiochannel) {
> +      return aResult.ParseEnumValue(aValue, kMozAudioChannelAttributeTable, false,
> +                                    &kMozAudioChannelAttributeTable[0]);

What you should do here is something like:

bool parsed = aResult.ParseEnumValue(...);
if (parsed && !currentlyPlayingMedia &&
    HasPermissionToUseChannel(aResult.GetEnumValue())) {
  mAudioChannelType = aResult.GetEnumValue();
}


Then you can remove the existing Get/SetMozAudioChannelType functions and use a macro.

Ideally we should do something like switching channels once a audio stops playing if the attribute was changed while we were playing, but lets do that as a followup.
Attachment #687863 - Flags: review?(jonas) → review-
(Assignee)

Comment 8

5 years ago
Created attachment 688212 [details] [diff] [review]
patch d
Attachment #687863 - Attachment is obsolete: true
Attachment #688212 - Flags: review?(jonas)
(Assignee)

Comment 9

5 years ago
Created attachment 688244 [details] [diff] [review]
patch d

Rebased on top of the patch number 4 of 805333
Attachment #688212 - Attachment is obsolete: true
Attachment #688212 - Flags: review?(jonas)
Attachment #688244 - Flags: review?(jonas)
(Assignee)

Updated

5 years ago
Blocks: 805333

Comment 10

5 years ago
Hi Andrea,

There was a regression from mozAudioChannelType and the problem is listed as below.

Some apps will try to assign mozAudioChannelType with the same value many times.
Ex: The music.js from music app will assign it each time of playing new song.

From the patch now, once mDecoder is not null then the assignment of mozAudioChannelType will return error even the type is the same. So suggest to take this case into consideration or the regression will be happened again.
(Assignee)

Comment 11

5 years ago
Created attachment 688276 [details] [diff] [review]
patch e

Marco, give me a feedback about this patch :)
Attachment #688244 - Attachment is obsolete: true
Attachment #688244 - Flags: review?(jonas)
Attachment #688276 - Flags: review?(jonas)
Attachment #688276 - Flags: feedback?(mchen)

Comment 12

5 years ago
Comment on attachment 688276 [details] [diff] [review]
patch e

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

Hi Andrea,

Thanks for changing the code for regression I mentioned.
It is ok for me to nsHTMLMediaElement.cpp.

On the other way, from the web developer point of view, I am wondering that the one of attribute from audio tag is called mozaudiochannel but we should use mozAudioChannelType in JS code. Note that the current Gaia uses mozAudioChannelType in JS code.
Comment on attachment 688276 [details] [diff] [review]
patch e

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

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +1976,5 @@
> +        return false;
> +      }
> +
> +      // If the audioChannelType is not changed, let's return immediately.
> +      AudioChannelType audioChannelType = (AudioChannelType)aResult.GetEnumValue();

Use static_cast<AudioChannelType>(aResult.GetEnumValue())

@@ +1982,5 @@
> +        return true;
> +      }
> +
> +      if (mDecoder) {
> +        return false;

You should return "true" here so that we still store the value as parsed.

Returning "true" simply means that we successfully parsed the value and set aResult. It doesn't mean that we are taking any particular action. Returning false just means that the caller will overwrite what's in aResult and set it to a string value.

@@ +1986,5 @@
> +        return false;
> +      }
> +
> +      if (!CheckAudioChannelPermissions(aValue)) {
> +        return false;

Same here.
Attachment #688276 - Flags: review?(jonas) → review+
(Assignee)

Comment 14

5 years ago
Created attachment 688351 [details] [diff] [review]
patch f
Attachment #688276 - Attachment is obsolete: true
Attachment #688276 - Flags: feedback?(mchen)
Attachment #688351 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/83e55a58ca14
https://hg.mozilla.org/mozilla-central/rev/83e55a58ca14
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Landed on beta branch:

https://hg.mozilla.org/releases/mozilla-beta/rev/f630f737647c
status-firefox18: --- → fixed
https://hg.mozilla.org/releases/mozilla-aurora/rev/6538a8f915f1
status-firefox19: --- → fixed

Updated

5 years ago
Depends on: 855615
You need to log in before you can comment on or make changes to this bug.