833385 - [webvtt] Implement Track element and TextTrack* DOM classes

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Dale Karp (:daleee)]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.17 (KHTML, like Gecko) Chrome/24.0.1312.52 Safari/537.17

Comment 1

[David Humphrey (:humph)]

Spin-off from bug 629350 to implement DOM bindings for webvtt and track element specs.

Comment 2

[Dale Karp (:daleee)]

Attachment: Track DOM implementation (work in progress) 1

Sorry for letting this bug become stale for so long.

This included David Humphrey and I's work on the track DOM element up until now. A few notes:

- Missing HTMLMediaElement's addTextTrack method
- Missing some validation

I'm sure there is a bunch of stuff I am not doing correctly but I am looking forward to being pointed in the right direction!

Comment 3

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

Comment on attachment 713898 [details] [diff] [review]
Track DOM implementation (work in progress) 1

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

Thanks for your patch; it looks generally good to me. There's a few memory management issues, though, quite a few nits, and a number of places where you appear to have reverted recent work.

::: w/content/html/content/src/HTMLTrackElement.cpp
@@ +59,5 @@
> +  NS_DECL_NSIOBSERVER
> +  NS_DECL_NSIINTERFACEREQUESTOR
> +
> +public:
> +  LoadListener(HTMLTrackElement *aElement)

* to the left. Please check the entire patch.

@@ +61,5 @@
> +
> +public:
> +  LoadListener(HTMLTrackElement *aElement)
> +    : mElement(aElement),
> +      mLoadID(aElement->GetCurrentLoadID())

, on this line, below the :

@@ +64,5 @@
> +    : mElement(aElement),
> +      mLoadID(aElement->GetCurrentLoadID())
> +    {
> +      NS_ABORT_IF_FALSE(mElement, "Must pass an element to the callback");
> +    }

Indent this block two spaces instead of four.

@@ +92,5 @@
> +NS_IMETHODIMP
> +HTMLTrackElement::LoadListener::OnStartRequest(nsIRequest* aRequest,
> +					       nsISupports* aContext)
> +{
> +  printf("track got start request\n");

All printfs will need to be removed before landing; no hurry, though.

@@ +117,5 @@
> +  printf("Track got data! %u bytes at offset %llu\n", aCount, aOffset);
> +
> +  nsresult rv;
> +  uint64_t available;
> +  bool blocking;

Declare these as close as possible to their first use.

@@ +131,5 @@
> +  rv = aStream->Available(&available);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  printf("Track has %llu bytes available\n", available);
> +
> +  char *buf = (char *)malloc(aCount);

Use moz_malloc if this has OOM potential, otherwise moz_xmalloc; and use static_cast.

@@ +186,5 @@
> +
> +
> +/** HTMLTrackElement */
> +HTMLTrackElement::HTMLTrackElement(already_AddRefed<nsINodeInfo> aNodeInfo)
> +    : nsGenericHTMLElement(aNodeInfo)

: should be indented no more than two spaces.

@@ +214,5 @@
> +
> +NS_IMPL_ELEMENT_CLONE(HTMLTrackElement)
> +
> +JSObject*
> +HTMLTrackElement::WrapNode(JSContext* cx, JSObject* scope, bool* triedToWrap)

aFoo for arguments, here too.

@@ +225,5 @@
> +{
> +#ifdef MOZ_WEBVTT
> +  nsCAutoString value(
> +      "text/webvtt"
> +      );

NS_(NAMED_)LITERAL_CSTRING (if I'm not mistaken; the _s could be different.)

@@ +235,5 @@
> +  return NS_ERROR_NOT_IMPLEMENTED;
> +#endif
> +}
> +
> +/** copied from nsHTMLMediaElement::NewURIFromString */

Please find a common superclass to put this in?

@@ +257,5 @@
> +      doc->GetDocumentURI() &&
> +      NS_SUCCEEDED(doc->GetDocumentURI()->Equals(*aURI, &equal)) &&
> +      equal) {
> +    // give up
> +    NS_RELEASE(*aURI);

Hmm, no, I don't think so. Please store the result of nsContentUtils::NewURIWithDocumentCharset in a local nsCOMPtr<nsIURI> uri, and use uri.forget(aURI) at the end of the function, before returning NS_OK.

@@ +267,5 @@
> +
> +nsresult
> +HTMLTrackElement::LoadResource(nsIURI* aURI)
> +{
> +  nsresult rv;

Again, move down.

@@ +284,5 @@
> +				 nullptr, // extra
> +				 &shouldLoad,
> +				 nsContentUtils::GetContentPolicy(),
> +				 nsContentUtils::GetSecurityManager());
> +  NS_ENSURE_SUCCESS(rv,rv);

Space after comma. (Also in the cases below.)

@@ +286,5 @@
> +				 nsContentUtils::GetContentPolicy(),
> +				 nsContentUtils::GetSecurityManager());
> +  NS_ENSURE_SUCCESS(rv,rv);
> +  if (NS_CP_REJECTED(shouldLoad))
> +    return NS_ERROR_FAILURE;

{}

@@ +297,5 @@
> +  nsCOMPtr<nsIContentSecurityPolicy> csp;
> +  rv = NodePrincipal()->GetCsp(getter_AddRefs(csp));
> +  NS_ENSURE_SUCCESS(rv,rv);
> +  if (csp) {
> +    channelPolicy = do_CreateInstance("@mozilla.org/nschannelpolicy;1");

You probably need to check if this is null.

@@ +336,5 @@
> +						 aParent,
> +						 aBindingParent,
> +						 aCompileEventHandlers);
> +  if(NS_FAILED(rv))
> +    return rv;

NS_ENSURE_SUCCESS(rv, rv);

@@ +341,5 @@
> +
> +  LOG(PR_LOG_DEBUG, ("Track Element bound to tree."));
> +  fprintf(stderr, "Track element bound to tree.\n");
> +  if (!aParent || !aParent->IsNodeOfType(nsINode::eMEDIA))
> +    return NS_OK;

{}

@@ +344,5 @@
> +  if (!aParent || !aParent->IsNodeOfType(nsINode::eMEDIA))
> +    return NS_OK;
> +
> +  // Store our parent so we can look up its frame for display
> +  mMediaParent = getter_AddRefs(aParent);

Uh, no getter_AddRefs here

@@ +353,5 @@
> +  LOG(PR_LOG_DEBUG, ("Track element sent notification to parent."));
> +
> +  // Find our 'src' url
> +  nsAutoString src;
> +  nsCOMPtr<nsIURI> uri;

Declare this inside the if

@@ +356,5 @@
> +  nsAutoString src;
> +  nsCOMPtr<nsIURI> uri;
> +
> +  if (GetAttr(kNameSpaceID_None, nsGkAtoms::src, src)) {
> +    nsresult rv = NewURIFromString(src, getter_AddRefs(uri));

You're shadowing rv here

::: w/content/html/content/src/HTMLTrackElement.h
@@ +45,5 @@
> +    GetHTMLAttr(nsGkAtoms::kind, aKind);
> +  }
> +  void SetKind(const nsAString& aKind, ErrorResult& aError)
> +  {
> +    SetHTMLAttr(nsGkAtoms::kind, aKind);

..., aError

@@ +84,5 @@
> +  {
> +    SetHTMLBoolAttr(nsGkAtoms::_default, aDefault, aError);
> +  }
> +
> +  uint16_t ReadyState()

All these getters should probably be const.

@@ +97,5 @@
> +  }
> +
> +  virtual nsresult Clone(nsINodeInfo* aNodeInfo, nsINode** aResult) const;
> +  virtual nsresult SetAcceptHeader(nsIHttpChannel* aChannel);
> +  virtual nsIDOMNode* AsDOMNode() { return this; }

Please add MOZ_OVERRIDE to all these, and add a comment about which superclass they come from (like <http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLLinkElement.h>).

@@ +104,5 @@
> +  // child track element.
> +  virtual nsresult BindToTree(nsIDocument* aDocument, nsIContent* aParent,
> +                              nsIContent* aBindingParent, bool aCompileEventHandlers);
> +
> +  PRUint32 GetCurrentLoadID() { return mCurrentLoadID; }

uint32_t

@@ +111,5 @@
> +  virtual JSObject* WrapNode(JSContext* aCx, JSObject* aScope,
> +                             bool* aTriedToWrap) MOZ_OVERRIDE;
> +
> +  class LoadListener;
> +  PRUint32 mCurrentLoadID;

Again.

::: c/content/html/content/src/Makefile.in
@@ -31,4 @@
>  
>  EXPORTS_mozilla/dom = \
>  		HTMLAnchorElement.h \
> -		HTMLAreaElement.h \

Uh, revert all this!

::: c/content/html/content/src/nsGenericHTMLElement.h
@@ +13,4 @@
>  #include "nsGkAtoms.h"
>  #include "nsContentCreatorFunctions.h"
>  #include "mozilla/ErrorResult.h"
> +#include "nsContentUtils.h"

Don't add this.

@@ +359,5 @@
>     * in aIsFocusable.
>     */
>    virtual bool IsHTMLFocusable(bool aWithMouse,
> +                                 bool *aIsFocusable,
> +                                 int32_t *aTabIndex);

Revert this. And all other changes in this file, except the addition of

NS_DECLARE_NS_NEW_HTML_ELEMENT(Track)

::: c/dom/media/Makefile.in
@@ +47,5 @@
>  
> +ifdef MOZ_WEBVTT
> +EXPORTS_NAMESPACES += \
> +	mozilla/dom \
> +	$(NULL)

Indent by two spaces instead of a tab and revert unrelated changes in this file.

::: w/dom/media/TextTrack.cpp
@@ +21,5 @@
> +
> +NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(TextTrack)
> +  NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER
> +NS_IMPL_CYCLE_COLLECTION_TRACE_END
> +

These three blocks should be

NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_1(TextTrack, mParent)

That would also fix the bug where you don't unlink mParent.

@@ +28,5 @@
> +
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(TextTrack)
> +  NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> +NS_INTERFACE_MAP_END

You need to forward to nsDOMEventTargetHelper; look at one of its existing subclasses for guidance.

@@ +33,5 @@
> +
> +TextTrack::TextTrack(nsISupports *aParent,
> +    const nsAString& aKind,
> +    const nsAString& aLabel,
> +    const nsAString& aLanguage) :

Indentation is off here.

@@ +38,5 @@
> +  mParent(aParent),
> +  mKind(aKind),
> +  mLabel(aLabel),
> +  mLanguage(aLanguage),
> +  mType(EmptyString()),

Initializing to the empty string isn't necessary.

@@ +51,5 @@
> +TextTrack::~TextTrack()
> +{
> +  mParent = nullptr;
> +  delete mCueList;
> +  delete mActiveCueList;

Never delete smart pointers. This destructor should be empty; everything is handled by the smart pointers themselves.

@@ +95,5 @@
> +TextTrackMode
> +TextTrack::Mode()
> +{
> +  return mMode;
> +  //XXX: how to return a string here?

You don't need to. The binding will return a string from your enum value.

@@ +108,5 @@
> +  };*/
> +}
> +
> +void
> +TextTrack::SetMode(TextTrackMode value)

aValue

@@ +111,5 @@
> +void
> +TextTrack::SetMode(TextTrackMode value)
> +{
> +  //XXX: spec seems to want strings, how to
> +  //accept strings here?

Again, this is fine.

@@ +118,5 @@
> +
> +TextTrackCueList*
> +TextTrack::GetCues()
> +{
> +  if(mMode == TextTrackMode::Disabled) {

'if (', with space

@@ +127,5 @@
> +
> +TextTrackCueList*
> +TextTrack::GetActiveCues()
> +{
> +  if(mMode == TextTrackMode::Disabled) {

Again.

@@ +134,5 @@
> +  return mActiveCueList;
> +}
> +
> +void
> +TextTrack::AddCue(TextTrackCue& cue)

aCue

::: w/dom/media/TextTrack.h
@@ +26,5 @@
> +{
> +public:
> +  NS_DECL_ISUPPORTS_INHERITED
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(TextTrack)
> +  NS_FORWARD_NSIDOMEVENTTARGET(nsDOMEventTargetHelper::)

Check if you need this. If it compiles without, remove it.

@@ +31,5 @@
> +
> +  TextTrack(nsISupports *aParent,
> +      const nsAString& aKind,
> +      const nsAString& aLabel,
> +      const nsAString& aLanguage);

Indentation

@@ +45,5 @@
> +  void GetLanguage(nsAString& aLanguage);
> +  void GetInBandMetadataTrackDispatchType(nsAString& aType);
> +
> +  TextTrackMode Mode();
> +  void SetMode(TextTrackMode value);

aValue

@@ +48,5 @@
> +  TextTrackMode Mode();
> +  void SetMode(TextTrackMode value);
> +
> +  TextTrackCueList* GetCues();
> +  TextTrackCueList* GetActiveCues();

All these getters should be const, and can probably be inlined.

@@ +50,5 @@
> +
> +  TextTrackCueList* GetCues();
> +  TextTrackCueList* GetActiveCues();
> +
> +  void AddCue(TextTrackCue& cue);

aCue

@@ +68,5 @@
> +
> +  nsRefPtr<TextTrackCueList> mCueList;
> +  nsRefPtr<TextTrackCueList> mActiveCueList;
> +
> +};

No need for that empty line.

::: w/dom/media/TextTrackCue.cpp
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "TextTrackCue.h"

Similar comments as above apply here too.

@@ +32,5 @@
> +
> +TextTrackCue::TextTrackCue(nsISupports *aGlobal,
> +                           const double aStartTime,
> +	                         const double aEndTime,
> +	                         const nsAString& aText)

Indentation: you've got tabs here.

@@ +34,5 @@
> +                           const double aStartTime,
> +	                         const double aEndTime,
> +	                         const nsAString& aText)
> +  : mGlobal(aGlobal),
> +    mText(aText),

, to the start of the line

@@ +81,5 @@
> +void
> +TextTrackCue::SetId(const nsAString& aId)
> +{
> +  if (mId == aId)
> +    return;

{}

@@ +195,5 @@
> +
> +void
> +TextTrackCue::SetSize(int32_t aSize)
> +{
> +  // XXXhumph: validate?

"On setting, if the new value is negative or greater than 100, then an IndexSizeError exception must be thrown."

@@ +212,5 @@
> +
> +void
> +TextTrackCue::SetAlign(const nsAString& aAlign)
> +{
> +  // XXXhumph: validate?

This should be an enum, IMO; then you get error handling for free.

I filed <https://www.w3.org/Bugs/Public/show_bug.cgi?id=20996>.

@@ +214,5 @@
> +TextTrackCue::SetAlign(const nsAString& aAlign)
> +{
> +  // XXXhumph: validate?
> +  if (mAlign == aAlign)
> +    return;

{}

@@ +238,5 @@
> +  CueChanged();
> +}
> +
> +DocumentFragment*
> +TextTrackCue::GetCueAsHTML()

This will create a new object, so it should return already_AddRefed<DocumentFragment>.

@@ +247,5 @@
> +
> +bool
> +TextTrackCue::operator==(const TextTrackCue& rhs) const {
> +  //XXX: todo
> +  return false;

Hmm? Why do you need this?

::: w/dom/media/TextTrackCue.h
@@ +16,5 @@
> +namespace dom {
> +
> +class TextTrack;
> +
> +class TextTrackCue MOZ_FINAL : public nsDOMEventTargetHelper

As well as here

@@ +32,5 @@
> +  Constructor(nsISupports* aGlobal,  const double aStartTime,
> +              const double aEndTime, const nsAString& aText,
> +              ErrorResult& aRv)
> +  {
> +    return new TextTrackCue(aGlobal, aStartTime, aEndTime, aText);

This leads to use-after-free, I'm afraid. Use

nsRefPtr<TextTrackCue> textTrackCue = new TextTrackCue(aGlobal, aStartTime, aEndTime, aText);
return textTrackCue.forget();

::: w/dom/media/TextTrackCueList.cpp
@@ +47,5 @@
> +  return TextTrackCueListBinding::Wrap(aCx, aScope, this, aTriedToWrap);
> +}
> +
> +TextTrackCue*
> +TextTrackCueList::IndexedGetter(int32_t aIndex, bool& aFound)

uint32_t

@@ +50,5 @@
> +TextTrackCue*
> +TextTrackCueList::IndexedGetter(int32_t aIndex, bool& aFound)
> +{
> +  aFound = aIndex < mLength;
> +  return aFound ? &mList[aIndex] : nullptr;

You're returning a pointer to an object which is going to be deleted as soon as the TextTrackCueList dies... That sounds dangerous.

@@ +56,5 @@
> +
> +TextTrackCue*
> +TextTrackCueList::GetCueById(const nsAString& id)
> +{
> +  if(id.EqualsLiteral("")) return nullptr;

if (aId.IsEmpty()) {
  return nullptr;
}

@@ +57,5 @@
> +TextTrackCue*
> +TextTrackCueList::GetCueById(const nsAString& id)
> +{
> +  if(id.EqualsLiteral("")) return nullptr;
> +  for (PRUint32 i = 0; i < mList.Length(); i++) {

uint32_t

::: w/dom/media/TextTrackCueList.h
@@ +16,5 @@
> +namespace dom {
> +
> +class TextTrackCue;
> +
> +// XXXhumph: should this follow what MediaStreamList does, and use NonRefcountedDOMObject?

No, I don't think so. C++ needs to hold on to this object after it's been returned.

@@ +25,5 @@
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(TextTrackCueList)
> +
> +  // TextTrackCueList WebIDL
> +  TextTrackCueList(nsISupports *aParent);

* to the left

@@ +51,5 @@
> +  nsCOMPtr<nsISupports> mParent;
> +
> +  nsTArray<TextTrackCue> mList;
> +
> +  uint32_t mLength;

Instead of using mLength (which you're using uninitialized!), use mList.Length() everywhere.

::: w/dom/webidl/HTMLTrackElement.webidl
@@ +7,5 @@
> + * http://www.whatwg.org/specs/web-apps/current-work/#the-track-element
> + */
> +
> +interface HTMLTrackElement : HTMLElement {
> +     [SetterThrows]

Indentation looks strange here; please indent [SetterThrows] by no more than two spaces.

::: c/dom/webidl/WebIDL.mk
@@ -17,4 @@
>    AudioNode.webidl \
>    AudioParam.webidl \
>    AudioSourceNode.webidl \
> -  BatteryManager.webidl \

Again, revert the spurious changes.

@@ +204,5 @@
> +  HTMLTrackElement.webidl \
> +  TextTrack.webidl \
> +	TextTrackCue.webidl \
> +	TextTrackCueList.webidl \
> +	$(NULL)

And don't use tabs

Comment 4

[Dale Karp (:daleee)]

Attachment: Track DOM implementation (work in progress) 2

Comment 5

[Dale Karp (:daleee)]

Ms2ger, thanks a lot for the feedback! I've been making my way through your review and while I believe I've gotten a lot of it fixed, I did get stuck on a few points.

1) In HTMLTrackElement.cpp:
@@ +344,5 @@
> +  if (!aParent || !aParent->IsNodeOfType(nsINode::eMEDIA))
> +    return NS_OK;
> +
> +  // Store our parent so we can look up its frame for display
> +  mMediaParent = getter_AddRefs(aParent);

> Uh, no getter_AddRefs here

What should I be using here if not a getter_AddRefs? Is there a reason I can't have one here in particular? I've seen it used other places within the class.

2) Some of the reverts you've asked me to make, such as re-adding HTMLAreaElement.h to the exports in content/html/content/src/Makefile.in, causes build errors (Target HTMLAreaElement.h not found). When I actually search for the files in question in my tree, they don't exist.

Also, reverting most of the changes in nsGenericHTMLElement.h except for the NS_DECLARE_NS_NEW_HTML_ELEMENT(Track) causes build errors. I feel like the changes made to this file were required to get the build working with the Track element (but I could be completely wrong here).

3) Thanks a lot for filing that W3 bug regarding SetAlign in TextTrackCue being an enum! How should this be handled until the W3 decides whether to change the spec or not?

4) 
::: w/dom/media/TextTrackCue.h
@@ +16,5 @@
> +namespace dom {
> +
> +class TextTrack;
> +
> +class TextTrackCue MOZ_FINAL : public nsDOMEventTargetHelper

>As well as here

What does this comment refer to?

I realize you are probably busy so I really do appreciate you taking the time to review my patch :).

Comment 6

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

(In reply to Dale Karp from comment #5)
> Ms2ger, thanks a lot for the feedback! I've been making my way through your
> review and while I believe I've gotten a lot of it fixed, I did get stuck on
> a few points.

No problem, I'm happy to help out :)

> 1) In HTMLTrackElement.cpp:
> @@ +344,5 @@
> > +  if (!aParent || !aParent->IsNodeOfType(nsINode::eMEDIA))
> > +    return NS_OK;
> > +
> > +  // Store our parent so we can look up its frame for display
> > +  mMediaParent = getter_AddRefs(aParent);
> 
> > Uh, no getter_AddRefs here
> 
> What should I be using here if not a getter_AddRefs? Is there a reason I
> can't have one here in particular? I've seen it used other places within the
> class.

So, getter_AddRefs exists to support the following pattern:

GetFoo(nsIFoo** aFoo)
{
  Foo* foo = new Foo();
  foo->AddRef(); // (or more typically, NS_ADDREF(foo))
  *aFoo = foo;
}

UseFoo()
{
  nsCOMPtr<nsIFoo> foo;
  GetFoo(getter_AddRefs(foo));
  ...
}

In this case, the object has a refcount of one when GetFoo() returns. To avoid leaks, we need to tell the nsCOMPtr it has to decrease the refcount when it goes out of scope. Usually, when you assign a value to an nsCOMPtr, it does an AddRef itself, but as one already happened in GetFoo(), that would leave us with a refcount of two, and it will only be decreased by one when the nsCOMPtr dies, so we leak. The way this is solved is by using getter_AddRefs, which tells the nsCOMPtr that it shouldn't do the second AddRef.

This case is another story, though: there's no equivalent of the AddRef in GetFoo() above. If mMediaParent doesn't AddRef when you assign aParent to it, the refcount will be one too low. This means that, when the refcount is decreased to zero, the object will be destroyed, but there still is one pointer holding on to the object, which leads to quite nasty bugs.

... I hope that made some sense. Our memory management is quite complex.

> 2) Some of the reverts you've asked me to make, such as re-adding
> HTMLAreaElement.h to the exports in content/html/content/src/Makefile.in,
> causes build errors (Target HTMLAreaElement.h not found). When I actually
> search for the files in question in my tree, they don't exist.
> 
> Also, reverting most of the changes in nsGenericHTMLElement.h except for the
> NS_DECLARE_NS_NEW_HTML_ELEMENT(Track) causes build errors. I feel like the
> changes made to this file were required to get the build working with the
> Track element (but I could be completely wrong here).

That's really strange; your tree seems to have gotten in a weird state. It may be easiest to 

> 3) Thanks a lot for filing that W3 bug regarding SetAlign in TextTrackCue
> being an enum! How should this be handled until the W3 decides whether to
> change the spec or not?

Implement them as enums, I think, but add a pointer to the bug.

> 4) 
> ::: w/dom/media/TextTrackCue.h
> @@ +16,5 @@
> > +namespace dom {
> > +
> > +class TextTrack;
> > +
> > +class TextTrackCue MOZ_FINAL : public nsDOMEventTargetHelper
> 
> >As well as here
> 
> What does this comment refer to?

I don't remember, and I don't see anything in particular looking back at it. Sounds like you can ignore that comment.

> I realize you are probably busy so I really do appreciate you taking the
> time to review my patch :).

I live to serve :)

Comment 7

[Dale Karp (:daleee)]

Attachment: Track DOM implementation (work in progress) 3

Before continuing on with my work on this, I thought it would be best to rebase my work against an up-to-date revision of mozilla-central. This patch should apply cleanly without conflicts, as well as build.

Comment 8

[Dale Karp (:daleee)]

I only have two items left in the review to address but I'm having a little trouble figuring out how to solve these two issues. I was wondering if I could get pointed in the right direction?

1)
@@ +50,5 @@
>> +TextTrackCue*
>> +TextTrackCueList::IndexedGetter(int32_t aIndex, bool& aFound)
>> +{
>> +  aFound = aIndex < mLength;
>> +  return aFound ? &mList[aIndex] : nullptr;
>
> You're returning a pointer to an object which is going to be deleted as soon as > the TextTrackCueList dies... That sounds dangerous.

How can I return a TextTrackCue that won't go out of scope? Create a new TTC, assign it the value of mList[aIndex] and return that?

2)
::: w/dom/media/TextTrackCueList.h
>@@ +16,5 @@
>> +namespace dom {
>> +
>> +class TextTrackCue;
>> +
>> +// XXXhumph: should this follow what MediaStreamList does, and use >NonRefcountedDOMObject?
>
>No, I don't think so. C++ needs to hold on to this object after it's been >returned.

In that case, is there anything that needs to be done in this case in order for C++ to be able to hold on to object after it's been returned or do the RefCount macros at the beginning take care of that?

:D Thanks in advance!

Comment 9

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

(In reply to Dale Karp from comment #8)
> I only have two items left in the review to address but I'm having a little
> trouble figuring out how to solve these two issues. I was wondering if I
> could get pointed in the right direction?
> 
> 1)
> @@ +50,5 @@
> >> +TextTrackCue*
> >> +TextTrackCueList::IndexedGetter(int32_t aIndex, bool& aFound)
> >> +{
> >> +  aFound = aIndex < mLength;
> >> +  return aFound ? &mList[aIndex] : nullptr;
> >
> > You're returning a pointer to an object which is going to be deleted as soon as > the TextTrackCueList dies... That sounds dangerous.
> 
> How can I return a TextTrackCue that won't go out of scope? Create a new
> TTC, assign it the value of mList[aIndex] and return that?

mList should be an nsTArray<nsRefPtr<TextTrackCue>>, and then you can return mList[aIndex] instead of &mList[aIndex].

> 2)
> ::: w/dom/media/TextTrackCueList.h
> >@@ +16,5 @@
> >> +namespace dom {
> >> +
> >> +class TextTrackCue;
> >> +
> >> +// XXXhumph: should this follow what MediaStreamList does, and use >NonRefcountedDOMObject?
> >
> >No, I don't think so. C++ needs to hold on to this object after it's been >returned.
> 
> In that case, is there anything that needs to be done in this case in order
> for C++ to be able to hold on to object after it's been returned or do the
> RefCount macros at the beginning take care of that?

You can just remove the comment.

> :D Thanks in advance!

No problem

Comment 10

[Dale Karp (:daleee)]

Attachment: Track DOM implementation (work in progress) 4

I'm *pretty* sure I've addressed all of Ms2ger's concerns at this point. 

Things still on the table:
- Validation (throwing JS exceptions or returning nullptrs)
- Finishing TextTrackList
- TextTrack::CueChange()

I believe that's all, although I could be missing something.

Comment 11

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

Note <http://html5.org/tools/web-apps-tracker?from=7741&to=7742>.

Comment 12

[Ralph Giles (:rillian)]

Comment on attachment 721996 [details] [diff] [review]
Track DOM implementation (work in progress) 4

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

::: w/content/html/content/src/HTMLTrackElement.h
@@ +20,5 @@
> +
> +class TextTrack;
> +
> +class HTMLTrackElement MOZ_FINAL : public nsGenericHTMLElement,
> +                                   public nsIDOMHTMLElement

If you put the comma under the colon on the next line, subsequent changes to the inheritance list don't have to touch the previous line. See :peterv's example in HTMLUnknownElement.h.

Please do this in all the new classes.

Comment 13

[Ralph Giles (:rillian)]

Attachment: pref off the track element

Comment 14

[Ralph Giles (:rillian)]

The second patch, together with the patch from bug 852350, pref's off the <track> element implementation. Doesn't seem to disable the TextTrackCue constructor though. More to do.

Now that bug 841493 has landed, it will be easy to extend this mechanism to media.textTracks, media.addTextTrack, etc.

Comment 15

[Ralph Giles (:rillian)]

The TextTrackCue issue from comment #14 is resolved by the updated patch on bug 852350.

Comment 16

[Ralph Giles (:rillian)]

The webaudio implementation is in content/media rather than dom/media, also MediaManager is there. Does content/media make more sense for the DOM class implementations?

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

I think it makes more sense to put these in with HTMLMediaElement in content/html/content/.

If we're going to put them anywhere else, media element should be moved there too at least.

Comment 18

[Ralph Giles (:rillian)]

Comment on attachment 721996 [details] [diff] [review]
Track DOM implementation (work in progress) 4

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

reyre's integration branch as a TextTrackList.webidl which is missing in this patch.

Comment 19

[Ralph Giles (:rillian)]

(In reply to Chris Pearce (:cpearce) from comment #17)

> I think it makes more sense to put these in with HTMLMediaElement in
> content/html/content/.

TimeRanges and MediaError are in content/html/content/src/ so there's certainly precedent.

On IRC roc suggested content/media, reserving content/html/content/ for things which are elements (including HTMLTrackElement). I guess I like that better only because content/html/content/src is ridiculously long.

Comment 20

[Ralph Giles (:rillian)]

Attachment: updated dom implementation

Updated and rebased version of Dale's patch.

- :humph's rebase to current gc code
- related clean-up from reyre's integration branch
- move dom implementation to content/media as per comment #19
- Segfault fix from 855100
- remove remaining spurious changes

Comment 21

[Ralph Giles (:rillian)]

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

Comment 22

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 733100 [details] [diff] [review]
updated dom implementation

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

::: content/media/Makefile.in
@@ +69,4 @@
>    VideoUtils.cpp \
>    $(NULL)
>  
> +ifdef MOZ_WEBVTT

We should remove the MOZ_WEBVTT once we've enabled this feature.

::: content/media/MediaDecoder.h
@@ +747,5 @@
>    static bool IsWebMEnabled();
>  #endif
>  
> +#ifdef MOZ_WEBVTT
> +  static bool IsWebVTTEnabled();

It doesn't look like this function is called anywhere? Is it used in a patch based on top of this one?

If we do need it, I don't think we should store this on the decoder, it could be a static function on the media element or on whatever object is going to need to call it.

Comment 23

[Ralph Giles (:rillian)]

(In reply to Chris Pearce (:cpearce) from comment #22)
 
> We should remove the MOZ_WEBVTT once we've enabled this feature.

I'm happy to remove it now, then. The interfaces are all controlled by the about:config pref, we don't really test these build-time switches, and the disabled code isn't large.

> > +#ifdef MOZ_WEBVTT
> > +  static bool IsWebVTTEnabled();
> 
> It doesn't look like this function is called anywhere? Is it used in a patch
> based on top of this one?

Yes, it's used by the LoadListenter code from bug 833382, which we want to land on top of this patch. I'm happy to put it wherever, I only chose MediaDecoder because off the other isFooEnabled() methods are already there. I'll remove it for now and attach it to WebVTTLoadListener or HTMLMediaElement in that patch.

Thanks for the review!

Comment 24

[Ralph Giles (:rillian)]

Attachment: updated dom implementation

Updated patch in response to cpearce's comments:

- Moved IsWebVTTEnabled() from MediaDecoder to HTMLTrackElement.
- Removed MOZ_WEBVTT build-time switch in and just rely on the runtime pref.

The early try push failed to build with a warning in nsHTMLFormElement,cpp, which we didn't touch. Still investigating.

Comment 25

[Ralph Giles (:rillian)]

Try failures were from an unrelated change, and should have been resolved by https://hg.mozilla.org/mozilla-central/rev/c4cec4613cac.

Pushed the new patch as https://tbpl.mozilla.org/?tree=Try&rev=e0accd31c661

Comment 26

[Ralph Giles (:rillian)]

Attachment: updated dom implementation

Correct rebase error which was causing the try build failures.

Comment 27

[Ralph Giles (:rillian)]

https://tbpl.mozilla.org/?tree=Try&rev=1e71451f48fa

One orange on try:
17:02:30 INFO - 5551 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/imptests/editing/conformancetest/test_runtest.html | [["removeformat",""]] "[foo<tt>bar</tt>baz]" compare innerHTML; assert_equals: Unexpected innerHTML (after normalizing inline style) expected "foobarbaz" but got "foo<tt>bar</tt>baz"
17:02:30 INFO - 5553 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/imptests/editing/conformancetest/test_runtest.html | [["removeformat",""]] "foo<tt>b[a]r</tt>baz" compare innerHTML; assert_equals: Unexpected innerHTML (after normalizing inline style) expected "foo<tt>b</tt>a<tt>r</tt>baz" but got "foo<tt>bar</tt>baz"

Comment 28

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

The tables in parser/htmlparser/src/nsElementTable.cpp and editor/libeditor/html/nsHTMLEditUtils.cpp should be in the same order.

Comment 29

[Ralph Giles (:rillian)]

Attachment: updated dom implementation v8

Updated patch:

- Fix tag order in the old parser/editor tree to address R2 orange
- Remove MOZ_MEDIA per bug 857022

Comment 30

[Ralph Giles (:rillian)]

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

Comment 31

[Ralph Giles (:rillian)]

Green on try.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 734897 [details] [diff] [review]
updated dom implementation v8

Stealing review from Ms2ger.  Would appreciate links to the relevant spec bits!

Comment 33

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

Comment on attachment 734897 [details] [diff] [review]
updated dom implementation v8

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

Just issues on first sight; bz will take over for a better review.

::: configure.in
@@ -4219,4 @@
>  MOZ_VP8=
>  MOZ_VP8_ERROR_CONCEALMENT=
>  MOZ_VP8_ENCODER=
> -MOZ_WEBVTT=1

I'd prefer if you put this into a separate patch and got review from a build peer.

::: content/base/src/nsGkAtomList.h
@@ +65,4 @@
>  GK_ATOM(active, "active")
>  GK_ATOM(activetitlebarcolor, "activetitlebarcolor")
>  GK_ATOM(actuate, "actuate")
> +GK_ATOM(onaddtrack, "onaddtrack")

I'm not sure how this file is sorted, but I don't think this belongs here.

@@ +892,4 @@
>  GK_ATOM(rel, "rel")
>  GK_ATOM(rem, "rem")
>  GK_ATOM(removeelement, "removeelement")
> +GK_ATOM(onremovetrack, "onremovetrack")

Or this here.

::: content/html/content/public/HTMLMediaElement.h
@@ +29,5 @@
>  #include "AudioChannelAgent.h"
>  #include "mozilla/Attributes.h"
>  #include "mozilla/ErrorResult.h"
> +#include "mozilla/dom/TextTrack.h"
> +#include "mozilla/dom/TextTrackList.h"

dom before ErrorResult

@@ +1087,5 @@
>    // An agent used to join audio channel service.
>    nsCOMPtr<nsIAudioChannelAgent> mAudioChannelAgent;
> +
> +  // List of our attached text track objects.
> +  nsRefPtr<mozilla::dom::TextTrackList> mTextTracks;

Drop all the mozilla::dom:: qualifications, you shouldn't need them (anymore).

@@ +1091,5 @@
> +  nsRefPtr<mozilla::dom::TextTrackList> mTextTracks;
> +
> +public:
> +
> +  already_AddRefed<mozilla::dom::TextTrackList> TextTracks() const;

Why are these at the end of the class? Put them with the rest of the WebIDL API.

@@ +1095,5 @@
> +  already_AddRefed<mozilla::dom::TextTrackList> TextTracks() const;
> +
> +  already_AddRefed<mozilla::dom::TextTrack> AddTextTrack(const nsAString& aKind,
> +                                                         const NonNull<nsAString>& aLabel,
> +                                                         const NonNull<nsAString>& aLanguage);

These should take const nsAString&; in general, you should never see NonNull outside dom/bindings.

::: content/html/content/src/HTMLMediaElement.cpp
@@ +3726,5 @@
> +/* readonly attribute nsISupports textTracks; */
> +already_AddRefed<mozilla::dom::TextTrackList>
> +HTMLMediaElement::TextTracks() const
> +{
> +  return mTextTracks.get();

This is a use-after-free. You should probably just return a raw pointer here

::: content/html/content/src/HTMLTrackElement.cpp
@@ +33,5 @@
> +#include "nsThreadUtils.h"
> +#include "nsIFrame.h"
> +#include "nsVideoFrame.h"
> +#include "nsISupportsImpl.h"
> +#include "webvtt/parser.h"

Sort this list, except that HTMLTrackElement.h should remain first.

@@ +61,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +//NS_IMPL_ISUPPORTS5(HTMLTrackElement, nsIRequestObserver, nsIStreamListener,
> +//    nsIChannelEventSink, nsIInterfaceRequestor, nsIObserver)

Drop this

@@ +240,5 @@
> +  }
> +
> +  // Store our parent so we can look up its frame for display.
> +  if (!mMediaParent) {
> +    mMediaParent = do_QueryInterface(aParent);

I don't understand the need for mMediaParent; you should be able to pass it through LoadResource to CreateTextTrack. Then again, I don't know what part of the spec this is trying to implement. Pointers would be welcome.

@@ +251,5 @@
> +    // Find our 'src' url
> +    nsAutoString src;
> +
> +    // TODO: we might want to instead call LoadResource() in a
> +    // SetAttr override, like we do in media element.

Don't override SetAttr, please. Maybe AfterSetAttr... Does the spec actually say something about when the track should be loaded and what should happen when you change src?

::: content/html/content/src/HTMLTrackElement.h
@@ +6,5 @@
> +#ifndef mozilla_dom_HTMLTrackElement_h
> +#define mozilla_dom_HTMLTrackElement_h
> +
> +#define WEBVTT_NO_CONFIG_H 1
> +#define WEBVTT_STATIC 1

These should probably be gone.

@@ +45,5 @@
> +  // nsIDOMHTMLElement
> +  NS_FORWARD_NSIDOMHTMLELEMENT_TO_GENERIC
> +
> +  // HTMLTrackElement WebIDL
> +  void GetKind(nsAString& aKind) const

Make these all return DOMString&

@@ +97,5 @@
> +  }
> +
> +  TextTrack* Track();
> +
> +  virtual nsresult SetAcceptHeader(nsIHttpChannel* aChannel);

Who calls this?

@@ +108,5 @@
> +  virtual void GetItemValueText(nsAString& aText)
> +  {
> +    GetSrc(aText);
> +  }
> +  virtual void SetItemValueText(const nsAString& aText) {

{ on the next line

@@ +118,5 @@
> +  // child track element.
> +  virtual nsresult BindToTree(nsIDocument* aDocument, nsIContent* aParent,
> +                              nsIContent* aBindingParent, bool aCompileEventHandlers);
> +
> +  uint32_t GetCurrentLoadID() { return mCurrentLoadID; }

const, and four lines

@@ +132,5 @@
> +  uint32_t mCurrentLoadID;
> +  nsRefPtr<TextTrack> mTrack;
> +  nsCOMPtr<nsIChannel> mChannel;
> +  nsCOMPtr<nsIContent> mMediaParent;
> +  uint16_t mReadyState;

For better memory use, put these from largest to smallest size, i.e., first the pointers, then the uint32_t, then the uint16_t.

::: content/html/content/src/Makefile.in
@@ +13,5 @@
>  LIBRARY_NAME	= gkconhtmlcon_s
>  LIBXUL_LIBRARY	= 1
>  
> +DEFINES += \
> +  -DWEBVTT_NO_CONFIG_H=1 \

Why do we need this? That's quite brittle...

::: content/html/content/src/nsGenericHTMLElement.h
@@ +1973,4 @@
>  NS_DECLARE_NS_NEW_HTML_ELEMENT(Thead)
>  NS_DECLARE_NS_NEW_HTML_ELEMENT(Time)
>  NS_DECLARE_NS_NEW_HTML_ELEMENT(Title)
> +#if defined(MOZ_MEDIA)

No ifdef

::: content/media/TextTrack.cpp
@@ +14,5 @@
> +
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(TextTrack)
> +  NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> +NS_INTERFACE_MAP_END_INHERITING(nsDOMEventTargetHelper)

nsDOMEventTargetHelper should handle wrappercache and nsISupports. Unless you're adding something to CC, this can all go. But maybe you should add something to CC; haven't checked.

@@ +40,5 @@
> +TextTrack::TextTrack(nsISupports* aParent)
> +  : mParent(aParent)
> +  , mKind(NS_LITERAL_STRING("subtitles"))
> +  , mLabel(EmptyString())
> +  , mLanguage(EmptyString())

Don't need to initialize to an empty string

@@ +45,5 @@
> +  , mType()
> +  , mMode(TextTrackMode::Disabled)
> +{
> +  mCueList = new TextTrackCueList(aParent);
> +  mActiveCueList = new TextTrackCueList(aParent);

These can be initializers too

::: content/media/TextTrackCue.cpp
@@ +12,5 @@
> +
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(TextTrackCue)
> +  NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> +NS_INTERFACE_MAP_END_INHERITING(nsDOMEventTargetHelper)

Same as earlier

::: content/media/TextTrackCue.h
@@ +157,5 @@
> +  {
> +    return mLine;
> +  }
> +
> +  void SetLine(double value)

aArg

@@ +163,5 @@
> +    //XXX: validate?
> +    mLine = value;
> +  }
> +
> +  int32_t Position()

const

@@ +178,5 @@
> +    mPosition = aPosition;
> +    CueChanged();
> +  }
> +
> +  int32_t Size()

const

::: content/media/TextTrackCueList.cpp
@@ +26,5 @@
> +}
> +
> +TextTrackCueList::~TextTrackCueList()
> +{
> +  mParent = nullptr;

Shouldn't need this

@@ +32,5 @@
> +void
> +TextTrackCueList::Update(double time)
> +{
> +  uint32_t i, length = mList.Length();
> +  for (i = 0; i < length; i++) {

Declare i in the loop header

@@ +55,5 @@
> +
> +TextTrackCue*
> +TextTrackCueList::GetCueById(const nsAString& id)
> +{
> +  if(id.EqualsLiteral("")) {

if (aId.IsEmpty()) {

::: content/media/TextTrackCueList.h
@@ +1,4 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim:set ts=2 sw=2 et tw=78: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,

Copy the boilerplate that has "file," on the next line

@@ +29,5 @@
> +  ~TextTrackCueList();
> +
> +  virtual JSObject* WrapObject(JSContext* aCx, JSObject* aScope) MOZ_OVERRIDE;
> +
> +  nsISupports* GetParentObject()

const

@@ +35,5 @@
> +    return mParent;
> +  }
> +
> +  uint32_t
> +  Length()

const, one line

::: content/media/TextTrackList.h
@@ +29,5 @@
> +  ~TextTrackList();
> +
> +  virtual JSObject* WrapObject(JSContext* aCx, JSObject* aScope) MOZ_OVERRIDE;
> +
> +  nsISupports* GetParentObject()

const

@@ +35,5 @@
> +    return mGlobal;
> +  }
> +
> +  uint32_t
> +  Length()

const, and "uint32_t Length() const" on one line

@@ +62,5 @@
> +  IMPL_EVENT_HANDLER(removetrack)
> +
> +private:
> +  nsCOMPtr<nsISupports> mGlobal;
> +  nsTArray<nsRefPtr<TextTrack>> mTextTracks;

>> still breaks in some compilers.

::: dom/interfaces/html/nsIDOMHTMLMediaElement.idl
@@ +80,5 @@
>             attribute boolean defaultMuted;
>  
> +  // tracks
> +  //readonly attribute TextTrackList textTracks;
> +  //TextTrack addTextTrack(in DOMString kind, [optional] in DOMString label, [optional] in DOMString language);

This file is dead; don't bother adding anything to it.

::: dom/webidl/HTMLMediaElement.webidl
@@ +87,5 @@
>    // tracks
>    //readonly attribute AudioTrackList audioTracks;
>    //readonly attribute VideoTrackList videoTracks;
> +  readonly attribute TextTrackList textTracks;
> +  TextTrack addTextTrack(         DOMString kind,

Weird indentation, and kind should be an enum (<https://www.w3.org/Bugs/Public/show_bug.cgi?id=21656>).

::: dom/webidl/TextTrack.webidl
@@ +28,5 @@
> +  void addCue(TextTrackCue cue);
> +  void removeCue(TextTrackCue cue);
> +
> +	         [SetterThrows]
> +	         attribute EventHandler oncuechange;

You've got hard tabs here. Check the rest of the patch too?

::: dom/webidl/TextTrackCue.webidl
@@ +40,5 @@
> +  attribute DOMString text;
> +  DocumentFragment getCueAsHTML();
> +
> +  [SetterThrows]
> +    attribute EventHandler onenter;

Spacing is weird

::: parser/htmlparser/src/nsElementTable.cpp
@@ -90,4 @@
>  DECL_TAG_LIST(gTRKids,{eHTMLTag_td COMMA eHTMLTag_th COMMA eHTMLTag_form COMMA eHTMLTag_script})// Removed INPUT - Ref. Bug 20087, 25382 |  Removed MAP to fix 58942
>  DECL_TAG_LIST(gTBodyKids,{eHTMLTag_tr COMMA eHTMLTag_form}) // Removed INPUT - Ref. Bug 20087, 25382
>  DECL_TAG_LIST(gULKids,{eHTMLTag_li COMMA eHTMLTag_p})
> -#ifdef MOZ_MEDIA

This ifdef doesn't exist on MXR. Please rebase on a current tree?

::: parser/htmlparser/src/nsHTMLTags.cpp
@@ +265,4 @@
>    {'t', 'i', 't', 'l', 'e', '\0'};
>  static const PRUnichar sHTMLTagUnicodeName_tr[] =
>    {'t', 'r', '\0'};
> +#if defined(MOZ_MEDIA)

Drop the ifdef

Comment 34

[Ralph Giles (:rillian)]

Thanks for the review. I'll update the patch momentarily. Responses inline.

(In reply to :Ms2ger from comment #33)
> Comment on attachment 734897 [details] [diff] [review]
> updated dom implementation v8

> > -MOZ_WEBVTT=1
> 
> I'd prefer if you put this into a separate patch and got review from a build
> peer.

I opened bug 860338.

> ::: content/html/content/public/HTMLMediaElement.h
>
> dom before ErrorResult

Done.

> Drop all the mozilla::dom:: qualifications, you shouldn't need them
> (anymore).

Good point.

> > +  already_AddRefed<mozilla::dom::TextTrackList> TextTracks() const;
> 
> Why are these at the end of the class? Put them with the rest of the WebIDL
> API.

Moved to the initial public section.

> These should take const nsAString&; in general, you should never see NonNull
> outside dom/bindings.

Done.

> ::: content/html/content/src/HTMLMediaElement.cpp
> @@ +3726,5 @@
> > +/* readonly attribute nsISupports textTracks; */
> > +already_AddRefed<mozilla::dom::TextTrackList>
> > +HTMLMediaElement::TextTracks() const
> > +{
> > +  return mTextTracks.get();
> 
> This is a use-after-free. You should probably just return a raw pointer here

Good catch. I have attempted to use a raw pointer.

> ::: content/html/content/src/HTMLTrackElement.cpp
>
> Sort [the include] list, except that HTMLTrackElement.h should remain first.

Ok.

> > +//NS_IMPL_ISUPPORTS5(HTMLTrackElement, nsIRequestObserver, nsIStreamListener,
> > +//    nsIChannelEventSink, nsIInterfaceRequestor, nsIObserver)
> 
> Drop this

Done.

> @@ +240,5 @@
> > +  }
> > +
> > +  // Store our parent so we can look up its frame for display.
> > +  if (!mMediaParent) {
> > +    mMediaParent = do_QueryInterface(aParent);
> 
> I don't understand the need for mMediaParent; you should be able to pass it
> through LoadResource to CreateTextTrack. Then again, I don't know what part
> of the spec this is trying to implement. Pointers would be welcome.

That would work if we moved caption update to the TextTrack. The branch this patch comes from does the update from HTMLTrackElement. I believe this is just about being able to find the parent to look up its nsVideoFrame so we can stuff the correct TextTrackCue(s) into it during playback.

> @@ +251,5 @@
> > +    // Find our 'src' url
> > +    nsAutoString src;
> > +
> > +    // TODO: we might want to instead call LoadResource() in a
> > +    // SetAttr override, like we do in media element.
> 
> Don't override SetAttr, please. Maybe AfterSetAttr... Does the spec actually
> say something about when the track should be loaded and what should happen
> when you change src?

Good to know about AfterSetAttr.

The idea here is that we have to handle updating everything when the 'src' attr is modified, just like in a Media element. The spec now almost gives us an out, indicating that the a new resource is loaded only when the track element is parented to a new media element or _or_ when the mode is changed. Seems more convenient if any src mutation reloads the resource though.

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#start-the-track-processing-model

> ::: content/html/content/src/HTMLTrackElement.h
> @@ +6,5 @@
> > +#ifndef mozilla_dom_HTMLTrackElement_h
> > +#define mozilla_dom_HTMLTrackElement_h
> > +
> > +#define WEBVTT_NO_CONFIG_H 1
> > +#define WEBVTT_STATIC 1
> 
> These should probably be gone.

For now they're necessary. Please open a bug against the webvtt parser library if you want this to go away.

> @@ +45,5 @@
> > +  // nsIDOMHTMLElement
> > +  NS_FORWARD_NSIDOMHTMLELEMENT_TO_GENERIC
> > +
> > +  // HTMLTrackElement WebIDL
> > +  void GetKind(nsAString& aKind) const
> 
> Make these all return DOMString&
> 
> @@ +97,5 @@
> > +  }
> > +
> > +  TextTrack* Track();
> > +
> > +  virtual nsresult SetAcceptHeader(nsIHttpChannel* aChannel);
> 
> Who calls this?

Our load listener calls it in bug 833382.

> > +  virtual void SetItemValueText(const nsAString& aText) {

Fixed.

> > +  uint32_t GetCurrentLoadID() { return mCurrentLoadID; }
> 
> const, and four lines

Ok.

> @@ +132,5 @@
> > +  uint32_t mCurrentLoadID;
> > +  nsRefPtr<TextTrack> mTrack;
> > +  nsCOMPtr<nsIChannel> mChannel;
> > +  nsCOMPtr<nsIContent> mMediaParent;
> > +  uint16_t mReadyState;
> 
> For better memory use, put these from largest to smallest size, i.e., first
> the pointers, then the uint32_t, then the uint16_t.

Fixed.

> ::: content/html/content/src/Makefile.in
> @@ +13,5 @@
> >  LIBRARY_NAME	= gkconhtmlcon_s
> >  LIBXUL_LIBRARY	= 1
> >  
> > +DEFINES += \
> > +  -DWEBVTT_NO_CONFIG_H=1 \
> 
> Why do we need this? That's quite brittle...

Yes. Please file a bug.

> ::: content/html/content/src/nsGenericHTMLElement.h
> @@ +1973,4 @@
> >  NS_DECLARE_NS_NEW_HTML_ELEMENT(Thead)
> >  NS_DECLARE_NS_NEW_HTML_ELEMENT(Time)
> >  NS_DECLARE_NS_NEW_HTML_ELEMENT(Title)
> > +#if defined(MOZ_MEDIA)
> 
> No ifdef
> 
> ::: content/media/TextTrack.cpp
> @@ +14,5 @@
> > +
> > +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(TextTrack)
> > +  NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
> > +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> > +NS_INTERFACE_MAP_END_INHERITING(nsDOMEventTargetHelper)
> 
> nsDOMEventTargetHelper should handle wrappercache and nsISupports. Unless
> you're adding something to CC, this can all go. But maybe you should add
> something to CC; haven't checked.
> 
> @@ +40,5 @@
> > +TextTrack::TextTrack(nsISupports* aParent)
> > +  : mParent(aParent)
> > +  , mKind(NS_LITERAL_STRING("subtitles"))
> > +  , mLabel(EmptyString())
> > +  , mLanguage(EmptyString())
> 
> Don't need to initialize to an empty string

Fixed.

> 
> @@ +45,5 @@
> > +  , mType()
> > +  , mMode(TextTrackMode::Disabled)
> > +{
> > +  mCueList = new TextTrackCueList(aParent);
> > +  mActiveCueList = new TextTrackCueList(aParent);
> 
> These can be initializers too

Moved.

> ::: content/media/TextTrackCue.cpp
> @@ +12,5 @@
> > +
> > +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(TextTrackCue)
> > +  NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
> > +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> > +NS_INTERFACE_MAP_END_INHERITING(nsDOMEventTargetHelper)
> 
> Same as earlier
> 
> ::: content/media/TextTrackCue.h
> @@ +157,5 @@
> > +  {
> > +    return mLine;
> > +  }
> > +
> > +  void SetLine(double value)
> 
> aArg

aLine?


> @@ +163,5 @@
> > +    //XXX: validate?
> > +    mLine = value;
> > +  }
> > +
> > +  int32_t Position()
> 
> const

Added.

> @@ +178,5 @@
> > +    mPosition = aPosition;
> > +    CueChanged();
> > +  }
> > +
> > +  int32_t Size()
> 
> const

Added.

> ::: content/media/TextTrackCueList.cpp
> @@ +26,5 @@
> > +}
> > +
> > +TextTrackCueList::~TextTrackCueList()
> > +{
> > +  mParent = nullptr;
> 
> Shouldn't need this

I thought this was good for safety, but removed.

> @@ +32,5 @@
> > +void
> > +TextTrackCueList::Update(double time)
> > +{
> > +  uint32_t i, length = mList.Length();
> > +  for (i = 0; i < length; i++) {
> 
> Declare i in the loop header

Ok.

> @@ +55,5 @@
> > +
> > +TextTrackCue*
> > +TextTrackCueList::GetCueById(const nsAString& id)
> > +{
> > +  if(id.EqualsLiteral("")) {
> 
> if (aId.IsEmpty()) {

Better, thanks.

> ::: content/media/TextTrackCueList.h
> @@ +1,4 @@
> > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> > +/* vim:set ts=2 sw=2 et tw=78: */
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> 
> Copy the boilerplate that has "file," on the next line

Fixed.

> @@ +29,5 @@
> > +  ~TextTrackCueList();
> > +
> > +  virtual JSObject* WrapObject(JSContext* aCx, JSObject* aScope) MOZ_OVERRIDE;
> > +
> > +  nsISupports* GetParentObject()
> 
> const
> 
> @@ +35,5 @@
> > +    return mParent;
> > +  }
> > +
> > +  uint32_t
> > +  Length()
> 
> const, one line
> 
> ::: content/media/TextTrackList.h
> @@ +29,5 @@
> > +  ~TextTrackList();
> > +
> > +  virtual JSObject* WrapObject(JSContext* aCx, JSObject* aScope) MOZ_OVERRIDE;
> > +
> > +  nsISupports* GetParentObject()
> 
> const

Added. 

> @@ +35,5 @@
> > +    return mGlobal;
> > +  }
> > +
> > +  uint32_t
> > +  Length()
> 
> const, and "uint32_t Length() const" on one line

Fixed.

> > +  nsTArray<nsRefPtr<TextTrack>> mTextTracks;
> 
> >> still breaks in some compilers.

Builds on try. What do you suggest instead?

> ::: dom/interfaces/html/nsIDOMHTMLMediaElement.idl
>
> This file is dead; don't bother adding anything to it.

*Sigh* Removed.

> ::: dom/webidl/HTMLMediaElement.webidl
> @@ +87,5 @@
> >    // tracks
> >    //readonly attribute AudioTrackList audioTracks;
> >    //readonly attribute VideoTrackList videoTracks;
> > +  readonly attribute TextTrackList textTracks;
> > +  TextTrack addTextTrack(         DOMString kind,
> 
> Weird indentation, and kind should be an enum
> (<https://www.w3.org/Bugs/Public/show_bug.cgi?id=21656>).

The indentation is to align with the type column on the subsequent arguments which carry the 'optional' qualifier, and is standard formatting in webidl from the spec.

I'd like to address the kind change separately.

> ::: dom/webidl/TextTrack.webidl
> @@ +28,5 @@
> > +  void addCue(TextTrackCue cue);
> > +  void removeCue(TextTrackCue cue);
> > +
> > +	         [SetterThrows]
> > +	         attribute EventHandler oncuechange;
> 
> You've got hard tabs here. Check the rest of the patch too?

So I do. Thanks for catching that.

Comment 35

[Ralph Giles (:rillian)]

Attachment: updated dom implementation v9

Rebased and updated patch in response to review comments.

Comment 36

[Ralph Giles (:rillian)]

Still green on try. https://tbpl.mozilla.org/?tree=Try&rev=5c324cf7438a

Comment 38

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

Note <http://html5.org/tools/web-apps-tracker?from=7807&to=7808> (but use double quotes for the default value).

Comment 39

[Boris Zbarsky [:bzbarsky]]

>> >> still breaks in some compilers.

>Builds on try. What do you suggest instead?

Try uses newer compiler versions than we nominally support.

I suggest "nsTArray<nsRefPtr<TextTrack> >" or "nsTArray< nsRefPtr<TextTrack> >" (I personally prefer the latter, but some people prefer the former).

I'm sorry I haven't gotten to this yet; it's the #1 thing I plan to work on Monday morning.

Comment 40

[Andrew McCreight [:mccr8]]

Please confirm that these fuzz bugs are all fixed before landing this patch.  A number of them were filed before you addressed issues found by Ms2ger, so they may not reproduce any more.

Comment 41

[Ralph Giles (:rillian)]

(In reply to Andrew McCreight [:mccr8] from comment #40)
> Please confirm that these fuzz bugs are all fixed before landing this patch.
> A number of them were filed before you addressed issues found by Ms2ger, so
> they may not reproduce any more.

Please don't insist on this. The functionality in question is hidden behind a pref. I really want to reduce the patch load we're carrying on the integration so we have more time to work on things like fixing the fuzz issues.

Comment 42

[Andrew McCreight [:mccr8]]

Ah, yeah, if it is prefed off that should be fine.

Comment 43

[Ralph Giles (:rillian)]

Thanks for clarifying.

Comment 44

[Ralph Giles (:rillian)]

(In reply to Boris Zbarsky (:bz) from comment #39)

> I suggest "nsTArray<nsRefPtr<TextTrack> >" or "nsTArray< nsRefPtr<TextTrack>
> >" (I personally prefer the latter, but some people prefer the former).

Just to clarify, it's actually the extra whitespace which fixes the build issue?

Comment 45

[Boris Zbarsky [:bzbarsky]]

What fixes it is the space between '>' and '>'.  Without that, '>>' is treated as operator>> per older versions of the C++ spec, which causes a compile error.

Comment 46

[Ralph Giles (:rillian)]

Right, thanks for clarifying.

../../../dist/include/mozilla/dom/TextTrackList.h:68:30: error: a space is
      required between consecutive right angle brackets (use '> >')

clang version 3.2 (trunk 163716)

I guess so!

Comment 47

[Ralph Giles (:rillian)]

Attachment: updated dom implementation v10

Updated patch:

- Rebase on today's m-c, porting exports to moz.build per bug 846634.
- Use nsTArray< nsRefPtr<Foo> > for compiler compatibility.

I did not address Ms2ger's comments about the gc macros. I'd like further review feedback about whether they're correct or not.

I also didn't implement the DOMString& request for the getter arguments. smaug suggested the intent was to improve performance (and that it wasn't worthwhile). Dale came up with https://github.com/RickEyre/mozilla-central/pull/10/files which is really messy and I think doesn't refcount the the stringbuffer properly. That can be improved by making the members themselves DOMStrings, but without adding new DOMString ctors initialization from C++ remains painful.

Comment 48

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 738814 [details] [diff] [review]
updated dom implementation v10

>+++ b/content/html/content/public/HTMLMediaElement.h
>+  already_AddRefed<TextTrack> AddTextTrack(const nsAString& aKind,

Why aren't we using an enum for aKind like the spec says?

>+++ b/content/html/content/src/HTMLMediaElement.cpp

You need to add mTextTracks to cycle collection for this class; otherwise you'll get leaks.

>+HTMLMediaElement::GetTextTracks(nsISupports** aTextTracks)
>+  *aTextTracks = this->TextTracks();

If anyone ever calls this, it will crash.

That said, I'd prefer you just didn't add this at all.

>+HTMLMediaElement::AddTextTrack(const nsAString& aKind,
>+  NS_ADDREF(*_retval = mTextTracks->AddTextTrack(aKind, aLabel, aLanguage).get());

And if anyone ever calls _this_ it will leak.  Again, just don't add it.

>+++ b/content/html/content/src/HTMLTrackElement.cpp
>+#include "HTMLTrackElement.h"

#include "mozilla/dom/HTMLTrackElement.h"

>+#include "HTMLUnknownElement.h"

#include "mozilla/dom/HTMLUnknownElement.h"

>+NS_INTERFACE_TABLE_HEAD(HTMLTrackElement)

This class needs to implement cycle collection and traverse/unlink at least the element members it's holding.  Otherwise you will get leaks.

>+HTMLTrackElement::SetAcceptHeader(nsIHttpChannel* aChannel)

This seems to be unused.  Is it used by later patches or something?

>+  } else {

Please don't do else after return.  Just remove the else bit and outdent the return NS_ERROR_NOT_IMPLEMENTED.

>+HTMLTrackElement::LoadResource(nsIURI* aURI)
>+  if (mChannel) {

This relies on you having a pointer to the actual channel that's still in flight, right?  But you're not observing redirects and updating the pointer, so this pointer can end up pointing to something that's not active anymore even though the load is still proceeding.  You probably want to update the pointer on redirect.

I guess that can happen in whatever bug you actually start calling AsyncOpen on the channel on, as long as it happens.

>+  rv = NS_CheckContentLoadPolicy(nsIContentPolicy::TYPE_MEDIA,

Hmm.  I guess this is sorta MEDIA-ish...

You need a CheckLoadURIWithPrincipal check here too.  It needs to come before the content policy check.

>+                                 static_cast<nsGenericHTMLElement*>(this),

ToSupports(this)

>+  CreateTextTrack();

What if we already have one?  Do we still want to create a new one?  If so, what should we do with the old one?  Right now we just keep adding more and more of them, looks like.

>+HTMLTrackElement::CreateTextTrack()
>+  nsCOMPtr<nsIDOMHTMLMediaElement> domMediaElem(do_QueryInterface(mMediaParent));
>+  if (domMediaElem) {
>+    HTMLMediaElement* mediaElem = static_cast<HTMLMediaElement*>(mMediaParent.get());

If we're going to cache mMediaParent at all, we should make it an nsRefPtr<HTMLMediaElement> and be done with it.  Then this whole thing will become:

  if (mMediaParent) {
    mMediaParent->AddTextTrack(mTrack);
  }

>+HTMLTrackElement::BindToTree(nsIDocument* aDocument,
>+  if (!aDocument) {
>+    return NS_OK;

Why?  I don't see anything in the spec that says to do this....

>+    mMediaParent = do_QueryInterface(aParent);

This should just be a static_cast<nsHTMLMediaElement*>.

>+    media->NotifyAddedSource();

Should this really happen synchronously under BindToTree?

>+    // Find our 'src' url
>+    nsAutoString src;

Afaict from the spec this is supposed to happen somewhat async.... but more on this below.

>+        LOG(PR_LOG_ALWAYS, ("%p Trying to load from src=%s", this,
>+        NS_ConvertUTF16toUTF8(src).get()));

Please fix the indent.

>+        LoadResource(uri);

This is not OK to do from BindToTree.  You need to do it off a script runner or an event loop runnable, if you don't want exploitable-crash bugs.

The spec says this too; see the "stable state" bits.

Note that we have some infrastructure for the "stable state" stuff over in nsHTMLMediaElement.  We should consider hoisting it up to somehere more shared and using it here.  A followup is OK, as long as you stop doing things completely sync before landing.

This class presumably needs an UnbindFromTree that resets mMediaParent and perhaps does other cleanup, right?

Per spec we should be responding to changes in text track mode too, right?  I assume there's a followup on that, but it might be good to document it here somewhere, with bug#.

>+++ b/content/html/content/src/HTMLTrackElement.h
>+  void GetKind(nsAString& aKind) const

This is not following the spec.  This is supposed to only return the known values, defaulting to "subtitles" if the value is not one of the known ones.  Please do make this an enum in the IDL, parse this as an enumerate attribute in your class's ParseAttribute (which you should add), etc.

Also, shouldn't changes to this attribute be propagated to mTrack?  As far as I can tell from the spec they should.

>+  void GetSrc(nsAString& aSrc) const

This is supposed to reflect as a URL attribute per spec.  Please fix that.

>+  void GetSrclang(nsAString& aSrclang) const

DOMString.

>+  void GetLabel(nsAString& aLabel) const

DOMString.

>+  // Superclass for Clone() and AsDOMNode() is nsINode.

Not sure why that comment is needed...

>+  uint16_t ReadyState() const

This returns a value that's never initialized.  Please initialize it in the constructor.

>+  uint32_t GetCurrentLoadID() const

This is unused, and the value it returns is never initialized.

>+  static bool IsWebVTTEnabled();

Should NS_NewHTMLTrackElement be calling this instead of directly calling the binding PrefEnabled?

>+++ b/content/html/content/src/Makefile.in
>+		-DWEBVTT_NO_CONFIG_H=1 \

I assume you're getting a build system peer to cover this part of the review, right?

>+++ b/content/html/content/src/nsGenericHTMLElement.cpp
>+/** copied from nsHTMLMediaElement::NewURIFromString */

Why copied instead of moved?  Please kill off the old version if they're identical, and remove the comment.

>+++ b/content/html/content/src/nsGenericHTMLElement.h
>+  nsresult NewURIFromString(const nsAutoString& aURISpec, nsIURI** aURI);

Please document (in particular the special-cased behavior for empty strings)?

That said, the empty string behavior here does not match the behavior in the spec (specifically for cases when the base URI is not equal to the document URI).  Is that a bug in the spec or in the patch?

>+++ b/content/media/TextTrack.cpp
>+#include "TextTrack.h"

mozilla/dom/TextTrack.h

>+NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_1(TextTrack, mParent)

NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_3(TextTrack, mParent, mCueList, mActiveCueList), no?

>+  NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
>+  NS_INTERFACE_MAP_ENTRY(nsISupports)

Neither of these lines should be here; nsDOMEventTargetHelper handles them for you.

>+TextTrack::TextTrack(nsISupports* aParent,
>+                     const nsAString& aKind,

Again, that should be an enum.

>+TextTrack::AddCue(TextTrackCue& aCue)
>+  //XXX: if cue exists, remove

Uh... yes.  And update its mTrack, and it seems to have an mTrackElement that's not being updated either.  Please at least get a followup filed on this being all broken, blocking this stuff being turned on, and put the bug number in the comment.

>+TextTrack::RemoveCue(TextTrackCue& aCue)
>+  //XXX: if cue does not exists throw

Likewise: need a bug tracking this that blocks turn-on.

>diff --git a/content/media/TextTrack.h b/content/media/TextTrack.h
>+#include "TextTrackCue.h"
>+#include "TextTrackCueList.h"

Both of those should be mozilla/dom

>+  void CueChanged(TextTrackCue& aCue);

May be worth not intermixing that with the parts that implement the spec API....

>+++ b/content/media/TextTrackCue.cpp
>+#include "TextTrackCue.h"

mozilla/dom

>+NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_1(TextTrackCue, mGlobal)

NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_2(TextTrackCue, mGlobal, mTrack)

but see below about mTrackElement and mHead

>+  NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
>+  NS_INTERFACE_MAP_ENTRY(nsISupports)

Again, neither of these should be here.

>+TextTrackCue::TextTrackCue(nsISupports* aGlobal,
>+                           const double aStartTime,
>+                           const double aEndTime,
>+                           const nsAString& aText)

This constructor never sets mTrackElement.  Since that's a raw pointer (which on the face of it seems very odd to me), that means it will have some random value.  I doubt that's intended.

This constructor also never sets mHead.  That can't be good either.

>+  , mPosition(50)
>+  , mSize(100)

Might be worth documenting where these magic numbers are coming from.  The spec doesn't have these things, so I can't tell whether they're right or not.

>+TextTrackCue::TextTrackCue(nsISupports* aGlobal,

Neither of these two constructors ever sets mLine or mAlign.  That needs to be fixed.

>+  , mTrackElement(aTrackElement)
>+  , mHead(head)

What's the ownership model for these?  What makes sure those don't become dangling pointers?

This needs at the very least documentation.

>+TextTrackCue::DisplayCue()
>+  mTrackElement->DisplayCueText(mHead);

mTrackElement can be null, no?

>+++ b/content/media/TextTrackCue.h
>+#include "TextTrack.h"

mozilla/dom

>+  // TextTrackCue WebIDL
>+  static already_AddRefed<TextTrackCue>
>+  Constructor(GlobalObject& aGlobal,

There is no constructor on this interface.

It looks like this code is admixing the WebVTTCue interface onto the TextTrackCue interface.  Please don't do that, unless you think the spec is wrong and there are spec bugs filed.  Just follow the spec.

>+              const double aStartTime,
>+              const double aEndTime,

Drop those const; they're pointless.

>+  void SetStartTime(const double aStartTime)
>+    //XXXhumph: validate?

Needs bugs filed.  Also, no need for the const.

>+  void SetEndTime(const double aEndTime)
>+    //XXXhumph: validate?

Likewise.

>+  void SetPauseOnExit(const bool aPauseOnExit)

Again, no need for the const.

>+  void SetLine(double aLine)
>+    //XXX: validate?

Need bug.

>+  void SetPosition(int32_t aPosition)
>+    // XXXhumph: validate?

Needs bug.

>+  void SetSize(int32_t aSize)
>+    if (aSize < 0 || aSize > 100) {
>+      //XXX:throw IndexSizeError;

Just add that.  Mark this as SetterThrows and throw the error.

>+  void SetAlign(TextTrackCueAlign& aAlign)

Why is this method, unlike the others, not checking whether the value actually changed?

>+  void SetText(const nsAString& aText)
>+    // XXXhumph: validate?

You know the drill. ;)

>+  operator==(const TextTrackCue& rhs) const
>+    return (mId.Equals(rhs.mId));

Remove the extra parens that aren't needed, please.

>+++ b/content/media/TextTrackCueList.cpp
>+#include "TextTrackCueList.h"

mozilla/dom

>+NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_1(TextTrackCueList, mParent)

NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_2(TextTrackCueList, mParent, mList), right?

>+TextTrackCueList::GetCueById(const nsAString& aId)
>+  if(aId.IsEmpty()) {

Space after "if", please.

>+    mList[i]->GetId(tid);

You may want to expose a

  const nsAString& Id() const { return mId; }

on TextTrackCue, to make this code simpler.

>+++ b/content/media/TextTrackCueList.h
>+#include "TextTrackCue.h"

mozilla/dom

>+++ b/content/media/TextTrackList.cpp
>+#include "TextTrackList.h"

mozilla/dom

>+NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_1(TextTrackList, mGlobal)

NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_2(TextTrackList, mGlobal, mTracks)

>+  NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
>+  NS_INTERFACE_MAP_ENTRY(nsISupports)

Remove those lines.

>+++ b/content/media/TextTrackList.h
>+#include "TextTrack.h"

mozilla/dom

>+class TextTrack;

You already included the header....

>+++ b/dom/interfaces/html/nsIDOMHTMLMediaElement.idl

Please just don't add stuff to this file.  It's dead code.

>+++ b/dom/webidl/HTMLMediaElement.webidl
>+  TextTrack addTextTrack(         DOMString kind,
>+                         optional DOMString label = "",
>+                         optional DOMString language = "");

Please just live the whitespace the way it is in the spec.

Shouldn't textTracks and addTextTrack be conditioned on the media.webvtt.enabled pref?

>+++ b/dom/webidl/HTMLTrackElement.webidl
>+  [SetterThrows]

I think these can all be [Pure] too.

>+  attribute DOMString kind;

Please fix this to match the spec or at least get a bug on file to do it and mention it here.

>+++ b/dom/webidl/TextTrack.webidl
>+  readonly attribute DOMString kind;

Again, need to fix or get bug filed.

>+++ b/dom/webidl/TextTrackCue.webidl
>+ * The origin of this IDL file is
>+ * http://www.whatwg.org/specs/web-apps/current-work/#texttrackcue

And <http://dev.w3.org/html5/webvtt/#webvtt-api>, yes?

Again, this should be two separate interfaces unless the spec is buggy; then please link to spec bugs.

>+interface TextTrack;

Please remove that.  It's wrong to do that for webidl interfaces.

>+++ b/dom/webidl/TextTrackList.webidl
>+ * http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#texttracklist

Please link to the single-page version.

>+++ b/dom/webidl/WebIDL.mk
>+  HTMLTrackElement.webidl \
>   HTMLTimeElement.webidl \

>+  TextTrackList.webidl \
>+  TextTrackCue.webidl \
>+  TextTrackCueList.webidl \

Please fix the alphabetical ordering.

I'd really appreciate an interdiff when the comments are addressed...

Comment 49

[Daniel Veditz [:dveditz]]

(In reply to Boris Zbarsky (:bz) from comment #48)
> >+  rv = NS_CheckContentLoadPolicy(nsIContentPolicy::TYPE_MEDIA,
> 
> You need a CheckLoadURIWithPrincipal check here too.  It needs to come
> before the content policy check.

I recently stumbled on nsContentUtils::CheckSecurityBeforeLoad() which does both. Unfortunately it also throws in a same-origin check which limits its use (it's only called from one place). Wonder if it's worth making the same-origin check optional and then promoting this utility more broadly -- fewer things to remember when people add content-initiated network loads.

Comment 50

[Boris Zbarsky [:bzbarsky]]

CheckSecurityBeforeLoad was basically added for XBL; it has various weirdness in terms of what it does...

But yes, having a one-stop API for CheckLoadURI+conpol would be nice.

Comment 51

[Ralph Giles (:rillian)]

Attachment: dom implementation v11

Sorry turning this around took so long. Hopefully we're closer now.

Interdiff to follow, but briefly I:

- Tried to fix the cycle collection. I think the bare pointer mTrackElement may have been intended as a weak reference. It's now an nsRefPtr.

- Kind being an enum in the spec post-dates the patch, but I've changed it.

- Many things postponed to other bugs. This has taken long enough as it is.

- I'd like to leave the WebVTTCue split, which is also new, until after this basic implementation lands.

- Ted approved the crazy -DWEBVTT_NO_CONFIG_H elsewhere in the tree. I've filed bug 868629 to remove it, but for now it's necessary.

Comment 52

[Ralph Giles (:rillian)]

Attachment: diff against v10

Changes relative to the previous patch, for reference.

Comment 53

[Ralph Giles (:rillian)]

I forgot to address mHead ownership. I believe the idea is that the LoadListener in bug 833382 runs the resource through the parser, and then creates a TextTrackCue for each parsed queue, which takes ownership of the webvtt_node tree produced by the parser. The bare pointer is thus ok, except we'd need a destructor to call webvtt_node_destroy() on it.

I'm happy to hear ideas on how to make that safer though.

Comment 54

[Boris Zbarsky [:bzbarsky]]

Ralph, it'll be a few days before I get to this, unfortunately.  Definitely not on Monday, and probably not on Tuesday....  Thank you in advance for the interdiff!

Comment 55

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 745404 [details] [diff] [review]
dom implementation v11

Sorry for the review lag here.  :(

>+++ b/content/html/content/src/HTMLTrackElement.cpp
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(HTMLTrackElement, nsGenericHTMLElement)

This and the unlink bits can be replaced by:

NS_IMPL_CYCLE_COLLECTION_INHERITED_3(HTMLTrackElement, nsGenericHTMLElement,
                                     mTrack, mChannel, mMediaParent)

>+HTMLTrackElement::CreateTextTrack()
>+  nsCOMPtr<nsIDOMHTMLMediaElement> domMediaElem(do_QueryInterface(mMediaParent));

What happened to just making mMediaParent an nsRefPtr<HTMLMediaElement> and removing the redundant code?

>+HTMLTrackElement::SetKind(TextTrackKind aKind, ErrorResult& aError)
>+  const EnumEntry& string = TextTrackKindValues::strings[aKind];

This will need merging to the changes from bug 869073, as will the code in ParseAttribute.  Just as a heads-up.

>+HTMLTrackElement::BindToTree(nsIDocument* aDocument,
>+  if (!aDocument) {

Again, why?

>+    // Find our 'src' url
>+    nsAutoString src;

I'm OK with this for now because now it's an (expensive) no-op, in the interests of getting this patch landed, but before this code starts doing anything it really needs to move to running async.  Please make sure there's a bug tracking that and add it to the comments here.  If that's bug 833382, make sure the comment says clearly that the code needs to become async in the process.

Again, you need an UnbindFromTree that, if needed, nulls out mMediaParent at the very least.

>+++ b/content/html/content/src/HTMLTrackElement.h
>+  void GetSrc(nsAString& aSrc) const
>+    GetHTMLAttr(nsGkAtoms::src, aSrc);

Again, this is wrong.  Do we not have test coverage for the attribute-reflecting properties on this interface?  This needs to reflect as a url, not just a string.  See GetHTMLURIAttr.

>+  void GetSrclang(nsAString& aSrclang) const

Again, DOMString.

>+  void GetLabel(nsAString& aLabel) const

Again, DOMString.

>+++ b/content/html/content/src/nsGenericHTMLElement.cpp
>+/** copied from nsHTMLMediaElement::NewURIFromString */
>+nsGenericHTMLElement::NewURIFromString(const nsAutoString& aURISpec,

Again, why copied instead of moved?  Please fix that.

>+++ b/content/html/content/src/nsGenericHTMLElement.h
>+  nsresult NewURIFromString(const nsAutoString& aURISpec, nsIURI** aURI);

Again, please document, especially the empty-string special-casing.

r=me me with the above addressed and an interdiff showing the changes posted...

Comment 56

[Boris Zbarsky [:bzbarsky]]

> except we'd need a destructor to call webvtt_node_destroy() on it.

Oh, and this bit.  Please do add that destructor, if the ownership model is that simple.  If not, we can consider adding some sort of RAII container for webvtt nodes later.

Comment 57

[Boris Zbarsky [:bzbarsky]]

And thank you again for the interdiff: that made the second review much faster; again sorry it was so slow to happen. :(

Comment 58

[Ralph Giles (:rillian)]

Attachment: dom implementation v12

Updated in response to comments. Interdiff to follow.

Comment 59

[Ralph Giles (:rillian)]

Attachment: diff against v11

Diff over the version 11 of the patch.

(In reply to Boris Zbarsky (:bz) from comment #55)

> This and the unlink bits can be replaced by:
> 
> NS_IMPL_CYCLE_COLLECTION_INHERITED_3(HTMLTrackElement, nsGenericHTMLElement,
>                                      mTrack, mChannel, mMediaParent)

Much cleaner, thanks.

> What happened to just making mMediaParent an nsRefPtr<HTMLMediaElement> and
> removing the redundant code?

Missed this, sorry. Should be ok now.

> This will need merging to the changes from bug 869073, as will the code in
> ParseAttribute.  Just as a heads-up.

Thanks, updated.

> >+HTMLTrackElement::BindToTree(nsIDocument* aDocument,
> >+  if (!aDocument) {
> 
> Again, why?

This is bug 871747 now.
 
> >+    // Find our 'src' url
> >+    nsAutoString src;
> 
> I'm OK with this for now because now it's an (expensive) no-op, in the
> interests of getting this patch landed, but before this code starts doing
> anything it really needs to move to running async.  Please make sure there's
> a bug tracking that and add it to the comments here.  If that's bug 833382,
> make sure the comment says clearly that the code needs to become async in
> the process.

Thanks. Punted to 833382, except I said 833385 in the commit instead. Oops.

 
> Again, you need an UnbindFromTree that, if needed, nulls out mMediaParent at
> the very least.

Added.

> Again, this is wrong.  Do we not have test coverage for the
> attribute-reflecting properties on this interface?  This needs to reflect as
> a url, not just a string.  See GetHTMLURIAttr.

Calling GetHTMLURIAttr now.

> Again, DOMString.

Fixed.

> >+/** copied from nsHTMLMediaElement::NewURIFromString */
> >+nsGenericHTMLElement::NewURIFromString(const nsAutoString& aURISpec,
> 
> Again, why copied instead of moved?  Please fix that.

I've removed the HTMLMediaElement version. Please double-check the addition of the NS_RELEASE on the argument in the empty spec string case. It's called with getter_AddRefs is both locations, but I'm not sure if that's correct.

Comment 60

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 749574 [details] [diff] [review]
dom implementation v12

> Thanks. Punted to 833382, except I said 833385 in the commit instead. Oops.

I assume you'll fix that before landing.

>+HTMLTrackElement::UnbindFromTree(bool aDeep, bool aNullParent)
>+{
>+  if (mMediaParent) {
>+    mMediaParent = nullptr;

Should only do that if aNullParent, I think.  UnbindFromTree is always called all the way down the tree, so if our parent media element is removed from the DOM our UnbindFromTree will be called (but with aNullParent set to false).

>+  virtual void UnbindFromTree(bool aDeep, bool aNullParent);

MOZ_OVERRIDE, please.  And I guess same for the BindToTree.

>+   * Create a URI for the given aURISpec string.

Please do document the empty-string behavior, so people who don't want it know not to use this function.

r=me with those nits.  Thank you again for the interdiff!

Comment 61

[Ralph Giles (:rillian)]

Attachment: dom implementation v13

Update to fix final nits. Carrying forward previous r+.

Comment 62

[Ralph Giles (:rillian)]

Attachment: diff against v12

Comment 63

[Ralph Giles (:rillian)]

Pushed to try as https://tbpl.mozilla.org/?tree=Try&rev=076d9777c623

Comment 64

[Boris Zbarsky [:bzbarsky]]

>+   * Returns INVALID_STATE_ERR and releases *aURI if aURISpec is empty.

How about:

  Returns INVALID_STATE_ERR and nulls out *aURI if aURISpec is empty and the document's URI
  matches the element's base URI.

?

Comment 65

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

(In reply to Boris Zbarsky (:bz) from comment #64)
> >+   * Returns INVALID_STATE_ERR and releases *aURI if aURISpec is empty.
> 
> How about:
> 
>   Returns INVALID_STATE_ERR and nulls out *aURI if aURISpec is empty and the
> document's URI
>   matches the element's base URI.
> 
> ?

s/Returns/Throws/

Comment 66

[Ralph Giles (:rillian)]

Try failed with ../../../../dist/include/nsAttrValue.h:178:18: error: inline function 'int16_t nsAttrValue::GetEnumValue() const' used but never defined and/or a link error. Amazingly this built on my linux64 box.

Need to add nsAttrValueInlines.h. Will push to try again when it reopens.

Comment 67

[Ralph Giles (:rillian)]

How about:

/**
 * Create a URI for the given aURISpec string.
 * Returns INVALID_STATE_ERR and releases *aURI if aURISpec is empty
 * and the document's URI matches the element's base URI.
 */

I like 'release' better than null, although I suppose NS_RELEASE does both.

I like 'return' better than 'throw' because it's a return value, not an exception.

Comment 68

[David Zbarsky (:dzbarsky)]

This should fix https://mxr.mozilla.org/mozilla-central/source/dom/imptests/failures/html/old-tests/submission/Opera/microdata/test_001.html.json?force=1

Comment 69

[Boris Zbarsky [:bzbarsky]]

> I like 'release' better than null, although I suppose NS_RELEASE does both.

The key part from the caller's point of view is that the pointer they passed in becomes null.  The fact that there was a release involved is an implementation detail that's transparent to the caller...

Comment 70

[Ralph Giles (:rillian)]

Attachment: dom implementation v14

More fixes.

- Include header for GetEnumValue.
- Add new webvtt symbols to the gkmedias export list.
- Add MOZ_OVERRIDE to BindTo/UnbindFromTree.
- Improve NewURIFromString documentation.

Comment 71

[Ralph Giles (:rillian)]

Got some suspicious mochitest results on try, but they don't seem to be valid locally.

Trying again with the new rebase. https://tbpl.mozilla.org/?tree=Try&rev=efb32b81620b

Comment 72

[Ralph Giles (:rillian)]

Failures seem repeatable. Comparing the same revision with and without the patch on try, I get three segfaults. Looks like I have some kind of memory error.

Compare https://tbpl.mozilla.org/?tree=Try&rev=e3e8ab4c6548
and https://tbpl.mozilla.org/?tree=Try&rev=ed2cc47c379b

 /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | Exited with code 11 during test run

 /tests/dom/media/tests/mochitest/test_peerConnection_bug835370.html | Exited with code 11 during test run

 chrome://mochitests/content/a11y/accessible/tests/mochitest/editabletext/test_1.html | Exited with code 11 during test run

 file:///builds/slave/test/build/tests/reftest/tests/dom/base/crashtests/275912-1.html | Exited with code 11 during test run
 file:///builds/slave/test/build/tests/reftest/tests/dom/base/crashtests/275912-1.html | application crashed [@ mozilla::dom::HTMLMediaElement::FireTimeUpdate(bool)]

 file:///builds/slave/test/build/tests/reftest/tests/parser/htmlparser/tests/reftest/bug566280-1.html | Exited with code 11 during test run
 file:///builds/slave/test/build/tests/reftest/tests/parser/htmlparser/tests/reftest/bug566280-1.html | application crashed [@ mozilla::dom::HTMLMediaElement::FireTimeUpdate(bool)]

Comment 73

[Andrew McCreight [:mccr8]]

Running the mochitests with ASAN should help figure out what the problem is.

Comment 74

[Boris Zbarsky [:bzbarsky]]

Looking at those try logs, we have:

21:54:20     INFO -  Crash reason:  SIGSEGV
21:54:20     INFO -  Crash address: 0x48

Looks like a null deref.  Top stack frame is:

libxul.so!mozilla::dom::HTMLMediaElement::FireTimeUpdate(bool) [nsTArray.h:ed2cc47c379b : 363 + 0x2]

Looks like FireTimeUpdate calls mTextTracks->Update().  But mTextTracks gets nulled out during unlink, and if you look at the stack it looks like so:

mozilla::dom::HTMLMediaElement::FireTimeUpdate(bool) [nsTArray.h:ed2cc47c379b : 363 + 0x2]
mozilla::dom::HTMLMediaElement::Pause(mozilla::ErrorResult&) [HTMLMediaElement.cpp:ed2cc47c379b : 1480 + 0x9]
...
mozilla::dom::HTMLSharedElement::UnbindFromTree(bool, bool) [HTMLSharedElement.cpp:ed2cc47c379b : 305 + 0xb]
nsDocument::cycleCollection::UnlinkImpl(void*) [nsDocument.cpp:ed2cc47c379b : 1818 + 0x20]

All of which is to say, we probably need a null-check of mTextTracks there and some comments about when it can and cannot be assumed non-null or something...

Comment 75

[Ralph Giles (:rillian)]

Attachment: dom implementation v15

Null-check mTextTracks in FireTimeUpdate.

Fixes a segfault when HTMLMediaElement is unlinked by the cycle collector before its parent. Thanks to reyre and bz for debugging assistance. Green on try now.

Comment 76

[Ralph Giles (:rillian)]

Attachment: diff against v13

Interdiff with changes since v13 of the patch.

Comment 77

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 752569 [details] [diff] [review]
dom implementation v15

r=me

Comment 78

[Ralph Giles (:rillian)]

Thanks. Green-ish full try https://tbpl.mozilla.org/?tree=Try&rev=fd3b9f821344

Comment 79

[Ralph Giles (:rillian)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/65d9aab57df9

Comment 80

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/65d9aab57df9