Andrea Marchesini [:baku] Assignee
	Description • 12 years ago

Maybe we don't like 'moz-audiochanneltype' as attribute name...

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 1 • 12 years ago

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.

	Andrea Marchesini [:baku] Assignee
	Comment 2 • 12 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.

	Andrea Marchesini [:baku] Assignee
	Comment 5 • 12 years ago

This patch is based on Bug 817589 and enables the attribute only for B2G builds.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 7 • 12 years ago

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.

	Andrea Marchesini [:baku] Assignee
	Comment 9 • 12 years ago

Rebased on top of the patch number 4 of 805333

	Marco Chen [:mchen]
	Comment 10 • 12 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.

	Andrea Marchesini [:baku] Assignee
	Comment 11 • 12 years ago

Marco, give me a feedback about this patch :)

	Marco Chen [:mchen]
	Comment 12 • 12 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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 13 • 12 years ago

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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 15 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/83e55a58ca14

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/83e55a58ca14

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 17 • 12 years ago

Landed on beta branch:

https://hg.mozilla.org/releases/mozilla-beta/rev/f630f737647c

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 18 • 12 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/6538a8f915f1