880064 - Convert HTMLTrackELement::kind to an appropriate type

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Rick Eyre (:reyre)]

Recently Ms2ger brought up a point that HTMLTrackElement::kind should probably not be an enumeration due to some issues that this causes with reflecting attributes.

See:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=22221  

We'll want to keep track of the changes in the spec and change when the type of kind if it ever happens.

Comment 1

[Rick Eyre (:reyre)]

This has been fixed in the WHATWG spec now so I think we can move forward and change HTMLTrackElement::Kind to a DOMString.

Comment 2

[Caitlin Potter (:caitp)]

There's already a bug open for this assigned to me

Comment 3

[Rick Eyre (:reyre)]

Attachment: v1: Change HTMLTrackElement::Kind to a DOMString

Comment 4

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

Sorry for the delay, it's been a while since I had to think about this code. I'll try to get to it early next week at the latest.

Comment 5

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

Comment on attachment 771444 [details] [diff] [review]
v1: Change HTMLTrackElement::Kind to a DOMString

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

Sorry this took so long, and thanks for the patch. r=me with the comments below addressed.

::: content/html/content/src/HTMLTrackElement.cpp
@@ +135,5 @@
>    GetSrclang(srcLang);
>    GetLabel(label);
> +  const nsAttrValue* value = GetParsedAttr(nsGkAtoms::kind);
> +  TextTrackKind kind = value ? static_cast<TextTrackKind>(value->GetEnumValue())
> +                        : TextTrackKind::Subtitles;

the ? and : should be aligned with the start of the condition:

  TextTrackKind kind = value
                       ? static_cast<TextTrackKind>(value->GetEnumValue())
                       : TextTrackKind::Subtitles;

::: content/html/content/src/HTMLTrackElement.h
@@ +33,5 @@
> +  { "metadata", static_cast<int16_t>(TextTrackKind::Metadata) },
> +  { 0 }
> +};
> +// The default value for kKindTable is "subtitles"
> +static const char* kKindTableDefaultString = kKindTable->tag;

I'd prefer putting this in the .cpp

@@ +62,5 @@
>    // HTMLTrackElement WebIDL
> +  void GetKind(DOMString& aKind) const
> +  {
> +    GetEnumAttr(nsGkAtoms::kind, kKindTableDefaultString,
> +                kKindTableDefaultString, aKind);

And then move this out of line too. Please use the 3-arg variant, though.

Comment 6

[Rick Eyre (:reyre)]

Attachment: v2: Change HTMLTrackElement::Kind to a DOMString r=Ms2ger

Thanks for review Ms2ger. Addressed all comments.

Carrying forward r=Ms2ger

Try push: https://tbpl.mozilla.org/?tree=Try&rev=596049349903

Comment 7

[Rick Eyre (:reyre)]

Attachment: v3: Change HTMLTrackElement::Kind to a DOMString r=Ms2ger

There was a small issue with the typing in a ternary statement on B2G. See here: https://tbpl.mozilla.org/?tree=Try&rev=596049349903

It seems like the problem is due to Enums being implemented differently on the compiler that B2G uses then on the one m-c uses. See here: http://mxr.mozilla.org/mozilla-central/source/mfbt/TypedEnum.h#122

Solution was to just use an if statement. Can you quickly review this Ms2ger? Relevant changes are in HTMLTrackElement::CreateTextTrack().

Try push: https://tbpl.mozilla.org/?tree=Try&rev=5d6bc19ce47e

Comment 8

[Rick Eyre (:reyre)]

Comment on attachment 790855 [details] [diff] [review]
v3: Change HTMLTrackElement::Kind to a DOMString r=Ms2ger

>From: Rick Eyre <[e-mail]>
>Date: Thu, 4 Jul 2013 14:16:05 -0400
>Subject: Bug 880064 - Change HTMLTrackElement::Kind to a DOMString value r=Ms2ger
>
>This was originally in the spec as an enumerated value, but due to the current
>problems with reflecting enumerated values it had to be changed to a DOMString.
>This has been changed in the HTML5 spec as well.
>
>diff --git a/content/html/content/src/HTMLTrackElement.cpp b/content/html/content/src/HTMLTrackElement.cpp
>index d0ff3b8..f1c6c8d 100644
>--- a/content/html/content/src/HTMLTrackElement.cpp
>+++ b/content/html/content/src/HTMLTrackElement.cpp
>@@ -62,6 +62,9 @@ NS_NewHTMLTrackElement(already_AddRefed<nsINodeInfo> aNodeInfo,
> namespace mozilla {
> namespace dom {
> 
>+// The default value for kKindTable is "subtitles"
>+static const char* kKindTableDefaultString = kKindTable->tag;
>+
> /** HTMLTrackElement */
> HTMLTrackElement::HTMLTrackElement(already_AddRefed<nsINodeInfo> aNodeInfo)
>   : nsGenericHTMLElement(aNodeInfo)
>@@ -91,6 +94,12 @@ NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(HTMLTrackElement)
> NS_INTERFACE_MAP_END_INHERITING(nsGenericHTMLElement)
> 
> void
>+HTMLTrackElement::GetKind(DOMString& aKind) const
>+{
>+  GetEnumAttr(nsGkAtoms::kind, kKindTableDefaultString, aKind);
>+}
>+
>+void
> HTMLTrackElement::OnChannelRedirect(nsIChannel* aChannel,
>                                     nsIChannel* aNewChannel,
>                                     uint32_t aFlags)
>@@ -130,37 +139,20 @@ HTMLTrackElement::CreateTextTrack()
>   nsString label, srcLang;
>   GetSrclang(srcLang);
>   GetLabel(label);
>-  mTrack = new TextTrack(OwnerDoc()->GetParentObject(), Kind(), label, srcLang);
>-
>-  if (mMediaParent) {
>-    mMediaParent->AddTextTrack(mTrack);
>-  }
>-}
>-
>-TextTrackKind
>-HTMLTrackElement::Kind() const
>-{
>   const nsAttrValue* value = GetParsedAttr(nsGkAtoms::kind);
>-  if (!value) {
>-    return TextTrackKind::Subtitles;
>+
>+  TextTrackKind kind;
>+  if (value) {
>+    kind = static_cast<TextTrackKind>(value->GetEnumValue());
>+  } else {
>+    kind = TextTrackKind::Subtitles;
>   }
>-  return static_cast<TextTrackKind>(value->GetEnumValue());
>-}
> 
>-static EnumEntry
>-StringFromKind(TextTrackKind aKind)
>-{
>-  return TextTrackKindValues::strings[static_cast<int>(aKind)];
>-}
>+  mTrack = new TextTrack(OwnerDoc()->GetParentObject(), kind, label, srcLang);
> 
>-void
>-HTMLTrackElement::SetKind(TextTrackKind aKind, ErrorResult& aError)
>-{
>-  const EnumEntry& string = StringFromKind(aKind);
>-  nsAutoString kind;
>-
>-  kind.AssignASCII(string.value, string.length);
>-  SetHTMLAttr(nsGkAtoms::kind, kind, aError);
>+  if (mMediaParent) {
>+    mMediaParent->AddTextTrack(mTrack);
>+  }
> }
> 
> bool
>@@ -169,16 +161,6 @@ HTMLTrackElement::ParseAttribute(int32_t aNamespaceID,
>                                  const nsAString& aValue,
>                                  nsAttrValue& aResult)
> {
>-  // Map html attribute string values to TextTrackKind enums.
>-  static const nsAttrValue::EnumTable kKindTable[] = {
>-    { "subtitles", static_cast<int16_t>(TextTrackKind::Subtitles) },
>-    { "captions", static_cast<int16_t>(TextTrackKind::Captions) },
>-    { "descriptions", static_cast<int16_t>(TextTrackKind::Descriptions) },
>-    { "chapters", static_cast<int16_t>(TextTrackKind::Chapters) },
>-    { "metadata", static_cast<int16_t>(TextTrackKind::Metadata) },
>-    { 0 }
>-  };
>-
>   if (aNamespaceID == kNameSpaceID_None && aAttribute == nsGkAtoms::kind) {
>     // Case-insensitive lookup, with the first element as the default.
>     return aResult.ParseEnumValue(aValue, kKindTable, false, kKindTable);
>diff --git a/content/html/content/src/HTMLTrackElement.h b/content/html/content/src/HTMLTrackElement.h
>index 60b73cb..754f7a5 100644
>--- a/content/html/content/src/HTMLTrackElement.h
>+++ b/content/html/content/src/HTMLTrackElement.h
>@@ -21,6 +21,16 @@
> namespace mozilla {
> namespace dom {
> 
>+// Map html attribute string values to TextTrackKind enums.
>+static const nsAttrValue::EnumTable kKindTable[] = {
>+  { "subtitles", static_cast<int16_t>(TextTrackKind::Subtitles) },
>+  { "captions", static_cast<int16_t>(TextTrackKind::Captions) },
>+  { "descriptions", static_cast<int16_t>(TextTrackKind::Descriptions) },
>+  { "chapters", static_cast<int16_t>(TextTrackKind::Chapters) },
>+  { "metadata", static_cast<int16_t>(TextTrackKind::Metadata) },
>+  { 0 }
>+};
>+
> class WebVTTLoadListener;
> 
> class HTMLTrackElement MOZ_FINAL : public nsGenericHTMLElement
>@@ -35,8 +45,11 @@ public:
>                                            nsGenericHTMLElement)
> 
>   // HTMLTrackElement WebIDL
>-  TextTrackKind Kind() const;
>-  void SetKind(TextTrackKind aKind, ErrorResult& aError);
>+  void GetKind(DOMString& aKind) const;
>+  void SetKind(const nsAString& aKind, ErrorResult& aError)
>+  {
>+    SetHTMLAttr(nsGkAtoms::kind, aKind, aError);
>+  }
> 
>   void GetSrc(DOMString& aSrc) const
>   {
>diff --git a/content/html/content/test/test_track.html b/content/html/content/test/test_track.html
>index ee393d9..1b24a51 100644
>--- a/content/html/content/test/test_track.html
>+++ b/content/html/content/test/test_track.html
>@@ -20,14 +20,13 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=833386
> SimpleTest.waitForExplicitFinish();
> SpecialPowers.pushPrefEnv({"set": [["media.webvtt.enabled", true]]},
>   function() {
>-    /* TODO:: See https://bugzilla.mozilla.org/show_bug.cgi?id=880064 and https://www.w3.org/Bugs/Public/show_bug.cgi?id=22221
>     reflectLimitedEnumerated({
>       element: document.createElement("track"),
>       attribute: "kind",
>       validValues: ["subtitles", "captions", "descriptions", "chapters", "metadata"],
>       invalidValues: ["foo", "bar", "\u0000", "null", "", "subtitle", "caption", "description", "chapter", "meta"],
>       defaultValue: "subtitles"
>-    });*/
>+    });
>     // Default attribute
>     reflectBoolean({
>       element: document.createElement("track"),
>@@ -55,26 +54,6 @@ SpecialPowers.pushPrefEnv({"set": [["media.webvtt.enabled", true]]},
>     var track = document.createElement("track");
>     is(track.readyState, 0, "Default ready state should be 0 (NONE).");
> 
>-    // Following are manual track.kind tests until the reflect.js problems get
>-    // cleared up above.
>-    // See: https://bugzilla.mozilla.org/show_bug.cgi?id=880064 and
>-    //      https://www.w3.org/Bugs/Public/show_bug.cgi?id=22221
>-    is(track.kind, "subtitles", "Default track kind should be subtitles.");
>-
>-    // Kind should not be able to be set to bogus value.
>-    track.kind = "bogus";
>-    is(track.kind, "subtitles", "Track kind should not be able to be set to a bogus value.");
>-
>-    checkKind("captions", "Kind should be set to captions.");
>-    checkKind("descriptions", "Kind should be set to descriptions.");
>-    checkKind("chapters", "Kind should be set to chapters.");
>-    checkKind("metadata", "Kind should be set to metadata.");
>-
>-    function checkKind(kind, message) {
>-      track.kind = kind;
>-      is(track.kind, kind, message);
>-    }
>-
>     SimpleTest.finish();
>   }
> );
>diff --git a/dom/webidl/HTMLTrackElement.webidl b/dom/webidl/HTMLTrackElement.webidl
>index d72f70c..fa31b69 100644
>--- a/dom/webidl/HTMLTrackElement.webidl
>+++ b/dom/webidl/HTMLTrackElement.webidl
>@@ -10,7 +10,7 @@
> [Pref="media.webvtt.enabled"]
> interface HTMLTrackElement : HTMLElement {
>   [SetterThrows, Pure]
>-  attribute TextTrackKind kind;
>+  attribute DOMString kind;
>   [SetterThrows, Pure]
>   attribute DOMString src;
>   [SetterThrows, Pure]
>-- 
>1.7.12.4 (Apple Git-37)
>

Comment 9

[Rick Eyre (:reyre)]

Disregard that last comment... :/. Just renamed the patch to v3: "".

Comment 10

[Rick Eyre (:reyre)]

Attachment: v2 and v3 interdiff

Comment 11

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

Comment on attachment 791276 [details] [diff] [review]
v2 and v3 interdiff

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

::: content/html/content/src/HTMLTrackElement.cpp
@@ +141,5 @@
>    GetLabel(label);
>    const nsAttrValue* value = GetParsedAttr(nsGkAtoms::kind);
> +
> +  TextTrackKind kind;
> +  if (value) {

Nit: I would prefer moving the value declaration here, like:

  TextTrackKind kind;
  if (const nsAttrValue* value = GetParsedAttr(nsGkAtoms::kind)) {
    kind = static_cast<TextTrackKind>(value->GetEnumValue());

Comment 12

[Rick Eyre (:reyre)]

Attachment: v4: Change HTMLTrackElement::Kind to a DOMString r=Ms2ger

Thanks for review.

(In reply to :Ms2ger from comment #11)

> Nit: I would prefer moving the value declaration here, like:
> 
>   TextTrackKind kind;
>   if (const nsAttrValue* value = GetParsedAttr(nsGkAtoms::kind)) {
>     kind = static_cast<TextTrackKind>(value->GetEnumValue());

Done.

Carrying forward r=Ms2ger

Try push: https://tbpl.mozilla.org/?tree=Try&rev=2e0382262a99

Comment 13

[Rick Eyre (:reyre)]

Try looks green.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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