Closed Bug 817043 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(1 file, 7 obsolete files)

Attached patch patch (obsolete) — Splinter Review
Maybe we don't like 'moz-audiochanneltype' as attribute name...
Attachment #687175 - Flags: review?(jonas)
Assignee: nobody → amarchesini
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-
> 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.
Attached patch patch b (obsolete) — Splinter Review
Attachment #687175 - Attachment is obsolete: true
Attachment #687527 - Flags: feedback?(jonas)
Duplicate of this bug: 816872
Attached patch patch b (obsolete) — Splinter Review
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)
Attached patch patch c (obsolete) — Splinter Review
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-
Attached patch patch d (obsolete) — Splinter Review
Attachment #687863 - Attachment is obsolete: true
Attachment #688212 - Flags: review?(jonas)
Attached patch patch d (obsolete) — Splinter Review
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)
Blocks: 805333
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.
Attached patch patch e (obsolete) — Splinter Review
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 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+
Attached patch patch fSplinter Review
Attachment #688276 - Attachment is obsolete: true
Attachment #688276 - Flags: feedback?(mchen)
Attachment #688351 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/83e55a58ca14
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Depends on: 855615
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.