WebIDL enum for AudioChannel in HTMLMediaElement

RESOLVED FIXED in mozilla28

Status

()

defect
RESOLVED FIXED
6 years ago
4 months 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)

No description provided.
Posted patch attribute2.patch (obsolete) — Splinter Review
Attachment #815733 - Flags: review?(ehsan)
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+
> ::: 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.
(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
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
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 → ---
(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)
(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
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Flags: in-testsuite?
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.