WebIDL enum for AudioChannel in HTMLMediaElement

RESOLVED FIXED in mozilla28

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: baku, Assigned: baku)

Tracking

Trunk
mozilla28
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 815733 [details] [diff] [review]
attribute2.patch
Attachment #815733 - Flags: review?(ehsan)

Comment 2

5 years ago
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?
Attachment #815733 - Flags: review?(khuey)
Attachment #815733 - Flags: review?(ehsan)
Attachment #815733 - Flags: feedback+
(Assignee)

Comment 3

5 years ago
> ::: 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

5 years ago
(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 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.
Attachment #815733 - Flags: review?(khuey) → review+
Where's the spec for this? We don't generally use WebIDL enums for enumerated reflecting attributes.
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
https://hg.mozilla.org/mozilla-central/rev/5506e7033c85
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
(Assignee)

Comment 11

5 years ago
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.
(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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla28 → ---
(Assignee)

Comment 13

5 years ago
(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?
Flags: needinfo?(cbook)
(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 :)
Flags: needinfo?(cbook)
(Assignee)

Comment 15

5 years ago
(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?
https://hg.mozilla.org/mozilla-central/rev/cd68cdbe8c88
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.