925594 - WebIDL enum for AudioChannel in HTMLMediaElement

Status: RESOLVED FIXED

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

Comment 1

[Andrea Marchesini [:baku]]

Attachment: attribute2.patch

Comment 2

[(no longer active)]

Comment on attachment 815733 [details] [diff] [review]
attribute2.patch

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

::: content/html/content/src/HTMLMediaElement.cpp
@@ +3904,5 @@
> +HTMLMediaElement::SetMozAudioChannelType(AudioChannel aValue, ErrorResult& aRv)
> +{
> +  nsCString channel;
> +  channel.AssignASCII(AudioChannelValues::strings[uint32_t(aValue)].value,
> +                      AudioChannelValues::strings[uint32_t(aValue)].length);

Hmm, I'm not sure if it's safe to rely on AudioChannelValues::strings like this.  Kyle, is this kosher?

::: content/html/content/test/test_mozaudiochannel.html
@@ +42,5 @@
>  ok(audio3.mozAudioChannelType == "content", "Default audio3 channel == 'content'");
>  try {
>  audio3.mozAudioChannelType = "foo";
>  } catch(e) {}
> +ok(audio3.mozAudioChannelType == "content", "audio3 channel == 'content'");

Hmm, why this change of behavior?

Comment 3

[Andrea Marchesini [:baku]]

> ::: content/html/content/test/test_mozaudiochannel.html
> @@ +42,5 @@
> >  ok(audio3.mozAudioChannelType == "content", "Default audio3 channel == 'content'");
> >  try {
> >  audio3.mozAudioChannelType = "foo";
> >  } catch(e) {}
> > +ok(audio3.mozAudioChannelType == "content", "audio3 channel == 'content'");
> 
> Hmm, why this change of behavior?

The previous code had the check of the audioChannelType in HTMLMediaElement and if the setter failed, the new audioChannel was set to normal. Now the check is done by the bindings and the previous audiochannel is untouched if the new one is wrong. I think this is much better.

Comment 4

[(no longer active)]

(In reply to comment #3)
> > ::: content/html/content/test/test_mozaudiochannel.html
> > @@ +42,5 @@
> > >  ok(audio3.mozAudioChannelType == "content", "Default audio3 channel == 'content'");
> > >  try {
> > >  audio3.mozAudioChannelType = "foo";
> > >  } catch(e) {}
> > > +ok(audio3.mozAudioChannelType == "content", "audio3 channel == 'content'");
> > 
> > Hmm, why this change of behavior?
> 
> The previous code had the check of the audioChannelType in HTMLMediaElement and
> if the setter failed, the new audioChannel was set to normal. Now the check is
> done by the bindings and the previous audiochannel is untouched if the new one
> is wrong. I think this is much better.

Sounds good!

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 815733 [details] [diff] [review]
attribute2.patch

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

If ehsan is happy with the behavior change, r=me with the comment addressed.

::: content/html/content/public/HTMLMediaElement.h
@@ +514,5 @@
>  
>    double MozFragmentEnd();
>  
> +  AudioChannel MozAudioChannelType() const;
> +  void SetMozAudioChannelType(AudioChannel aValue, ErrorResult& aRv);

Ugh, really sucks that we can't forward declare these.

::: content/html/content/src/HTMLMediaElement.cpp
@@ +3905,5 @@
> +{
> +  nsCString channel;
> +  channel.AssignASCII(AudioChannelValues::strings[uint32_t(aValue)].value,
> +                      AudioChannelValues::strings[uint32_t(aValue)].length);
> +  SetHTMLAttr(nsGkAtoms::mozaudiochannel, NS_ConvertUTF8toUTF16(channel), aRv);

There's no need to go through a narrow string here.  You can declare an nsString and still AssignASCII to it.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: attribute2.patch

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

Comment 7

[Andrea Marchesini [:baku]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/5506e7033c85

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Where's the spec for this? We don't generally use WebIDL enums for enumerated reflecting attributes.

Comment 9

[Carsten Book [:Tomcat]]

had to back out this change because of perma orange test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=29773348&tree=Mozilla-Inbound

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5506e7033c85

Comment 11

[Andrea Marchesini [:baku]]

I didn't see this issue. This patch seems already landed to m-c. Let me fix this orange issue in a follow up today.

Comment 12

[Ed Morley [:emorley]]

(In reply to Andrea Marchesini (:baku) from comment #11)
> I didn't see this issue. This patch seems already landed to m-c. Let me fix
> this orange issue in a follow up today.

My fault, missed the backout, needs merging too.

Transplant:
https://hg.mozilla.org/mozilla-central/rev/d4a27d8eda28

Comment 13

[Andrea Marchesini [:baku]]

(In reply to Carsten Book [:Tomcat] from comment #9)
> had to back out this change because of perma orange test failures like
> https://tbpl.mozilla.org/php/getParsedLog.php?id=29773348&tree=Mozilla-
> Inbound

I don't see any permanent orange tests. It was green on mozilla-inbound and in mozilla-central.  Tomcat, where does that log come from?

Comment 14

[Carsten Book [:Tomcat]]

(In reply to Andrea Marchesini (:baku) from comment #13)
> (In reply to Carsten Book [:Tomcat] from comment #9)
> > had to back out this change because of perma orange test failures like
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=29773348&tree=Mozilla-
> > Inbound
> 
> I don't see any permanent orange tests. It was green on mozilla-inbound and
> in mozilla-central.  Tomcat, where does that log come from?

oh its from mozilla-inbound its on the B2G ICS Emulator Opt build. Also its in your try server push https://tbpl.mozilla.org/?tree=Try&rev=435a68a47053 for B2G ICS Emulator Opt, the "2" in mochitest opt :)

Comment 15

[Andrea Marchesini [:baku]]

(In reply to :Ms2ger from comment #8)
> Where's the spec for this? We don't generally use WebIDL enums for
> enumerated reflecting attributes.

This is not part of the spec. it's a moz only feature. The reason why I wrote this is to have 1 single implementation/definition for webaudio and HTMLMediaElement of AudioChannelTypes.

Do you have any proposal instead having an enum?

Comment 16

[Andrea Marchesini [:baku]]

Attachment: attribute2.patch

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

Comment 17

[Andrea Marchesini [:baku]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/cd68cdbe8c88

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cd68cdbe8c88