Closed Bug 865407 Opened 7 years ago Closed 6 years ago

Implement the text track processing model

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: reyre, Assigned: reyre)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-needed)

Attachments

(9 files, 27 obsolete files)

1.78 KB, patch
rillian
: checkin+
Details | Diff | Splinter Review
9.34 KB, patch
rillian
: checkin+
Details | Diff | Splinter Review
6.63 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
4.15 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
1.16 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
53.44 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
21.37 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
26.16 KB, patch
rillian
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
896 bytes, patch
rillian
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64) AppleWebKit/537.31 (KHTML, like Gecko) Chrome/26.0.1410.64 Safari/537.31
Currently we're not attaching the TextTracks correctly to the MediaElement's caption overlay. We are only using one div and have no special rules for the positioning of the cues in light of their respective alignments, positioning, etc. 

What we need to do is implement the specified TextTrack processing model that takes into account all these things as well as whether or not there are multiple tracks that need  to display at the same time.

See:
http://dev.w3.org/html5/webvtt/#processing-model
OS: Windows 8 → All
Hardware: x86_64 → All
Depends on: webvtt
Blocks: webvtt
No longer depends on: webvtt
Depends on: 881976
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
QA Contact: rick.eyre
I'm thinking we can put this in either the TextTrackList class or the HTMLMediaElement. I think putting it in the HTMLMediaElement makes more sense. Thoughts anyone?
Assignee: nobody → rick.eyre
QA Contact: rick.eyre
Depends on: 883173
(In reply to Rick Eyre (:reyre) from comment #2)
> I'm thinking we can put this in either the TextTrackList class or the
> HTMLMediaElement. I think putting it in the HTMLMediaElement makes more
> sense. Thoughts anyone?

HTMLMediaElement has a lot of stuff in it already. Rather than adding a whole lot of new functionality to this class, it would be good if we could put it in a client class, either the TextTrackList, or a new class that specifically manages this.

Ideally, we want to reduce the amount of stuff you need to keep in your head when you're reading and working on various parts of the code. We've not always been good at that in the past. ;)
Depends on: 882714
Sounds good Chris. This algorithm interacts with the HTMLMediaElement a lot so I think the TextTrackList will have to have a reference to it, but other than that it can live in TextTrackList no problem.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
It will be easy enough to have the JS WebVTT parser do this in its ConvertCueToDOMTree() function. I've opened an issue on the GitHub repo to track that: https://github.com/andreasgal/vtt.js/issues/86
So the JS parser is close to having the processing model implemented within it. We still, however, need Gecko to run the JS processing model. We can use this bug to track that. I'll start working on it within the next few days.
Would it be fine to have a class within HTMLMediaElement.cpp that manages this or do you want a separate file Chris? We need to be able to run the 'time marches on' algorithm here http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#time-marches-on and on step 18 we will call the code in the JS parser.

Just to give some context as well, we'll have to be putting the code for sourcing out-of-band text tracks somewhere in the future too: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#sourcing-out-of-band-text-tracks
Flags: needinfo?(cpearce)
If possible please put this in its own class that take whatever input it needs from the media element (or wherever) to do what needs to be done. There's already too much stuff in the media element.
Flags: needinfo?(cpearce)
So I think to do this we can create a TextTrackManager class and have it manage everything to do with TextTracks for the MediaElement. Then the code to deal with the processing model could live in it as well. The MediaElement can just call it where appropriate if it needs to.
How about something like this to get started Chris? We can start to build everything the TextTrack spec needs into this class.
Attachment #807294 - Flags: review?(cpearce)
Comment on attachment 807294 [details] [diff] [review]
Part 1 v1: Add TextTrackManager class

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

Is there any logic that you need that isn't in TextTrackList? It seems that you're just adding a wrapper around a single TextTrackList, and calling the TextTrackList's functions.

I was under the impression there would be more logic folded into this class, as is, it's just a thin wrapper around TextTrackList that doesn't do much.

::: content/html/content/src/TextTrackManager.cpp
@@ +13,5 @@
> +
> +TextTrackManager::TextTrackManager()
> +  : mMediaElement(nullptr)
> +  , mTextTracks(nullptr)
> +{

Use MOZ_COUNT_CTOR(TextTrackManager) in all constructors, and MOZ_COUNT_DTOR(TextTrackManager) in the destructor, so that you can track whether this leaks (by setting the XPCOM_MEM_LEAK_LOG=2 environment variable when you run Firefox).

::: content/html/content/src/TextTrackManager.h
@@ +32,5 @@
> +  void AddTextTrack(TextTrack* aTextTrack) {
> +    mTextTracks->AddTextTrack(aTextTrack);
> +  }
> +
> +  void Update(double aTime)

Braces riding high on inline declarations, i.e.

void Update(double aTime) {
Comment on attachment 807294 [details] [diff] [review]
Part 1 v1: Add TextTrackManager class

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

Ah, your comment when you uploaded your patch got lost in my bugmail. This is fine as a starting point, assuming that you're going to move more logic related to text tracks into this class.

::: content/html/content/src/TextTrackManager.h
@@ +32,5 @@
> +  void AddTextTrack(TextTrack* aTextTrack) {
> +    mTextTracks->AddTextTrack(aTextTrack);
> +  }
> +
> +  void Update(double aTime)

It would be good to have a comment here explaining what it means to "update" a TextTrackManager with a time.

@@ +39,5 @@
> +  }
> +
> +private:
> +  // The HTMLMediaElement that this TextTrackManager manages the TextTracks of.
> +  nsRefPtr<HTMLMediaElement> mMediaElement;

Don't you need to make the cycle collector aware of these two refptrs too? Or is it enough to unlink the media element's reference to the manager?
(In reply to Chris Pearce (:cpearce) from comment #12)

> Ah, your comment when you uploaded your patch got lost in my bugmail. This
> is fine as a starting point, assuming that you're going to move more logic
> related to text tracks into this class.

Okay, great. We still have a number of algorithms to add into the TextTrackManager class to do
with the sequence of events of updating TextTracks and rendering them, etc.

> Don't you need to make the cycle collector aware of these two refptrs too?
> Or is it enough to unlink the media element's reference to the manager?

I assumed that since TextTrackManager is being allocated on the stack that it will automatically
break the cycle when TextTrackManager goes out of scope. Now that I'm thinking about this though
I'm not too sure about that.I wanted to keep cycle collection out of this since it's going to be
a small utility class that only the HTMLMediaElement uses. If we can manually break it ourselves
then I would go for that option. However, I think I remember hearing that that's frowned upon and
we really should use the macros if we need to participate in CC.
Attachment #807294 - Flags: review?(cpearce)
Comment on attachment 807294 [details] [diff] [review]
Part 1 v1: Add TextTrackManager class

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

As per our conversation, a few changes.

::: content/html/content/src/HTMLMediaElement.cpp
@@ +1968,4 @@
>    RegisterFreezableElement();
>    NotifyOwnerDocumentActivityChanged();
>  
> +  mTextTrackManager = TextTrackManager(this);

You can do this in the media element constructor.

::: content/html/content/src/TextTrackManager.h
@@ +38,5 @@
> +    mTextTracks->Update(aTime);
> +  }
> +
> +private:
> +  // The HTMLMediaElement that this TextTrackManager manages the TextTracks of.

Actually, I think this can be a weak reference (a HTMLMediaElement*) since the lifetime of the TextTrackManager is bound by the lifetime of the media element, since it's a member of the media element.

I think the TextTrackList should still be a refptr, and cycle collector aware, for safety.
Thanks for updating the bug with that info Chris. They're much better then the quick notes I took during our convo.

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

> You can do this in the media element constructor.

I've changed mTextTrackManager to an nsRefPtr in order to make the CC aware of HTMLMediaElement's TextTrackManager as UNLINK and TRAVERSE only accept smart pointers. Also, my original intention of keeping this simple and having mTextTrackManager allocated on the stack is not possible, as you mentioned ;), due to HTMLMediaElement being allocated on the heap so we might as well hold it in an nsRefPtr.

> Actually, I think this can be a weak reference (a HTMLMediaElement*) since
> the lifetime of the TextTrackManager is bound by the lifetime of the media
> element, since it's a member of the media element.
> 
> I think the TextTrackList should still be a refptr, and cycle collector
> aware, for safety.

Done.

I was talking to Andrew McCreight and he was saying that we can form a cycle in this situation since HTMLMediaElement can be held alive by JS and in turn hold the TextTrackList alive via TextTrackManager which can be held alive by JS as well, etc.

I'll be adding some more patches soon that make use of this and we can land this with those--just in case this path ends up not working.
Attachment #807294 - Attachment is obsolete: true
Attachment #809873 - Flags: review?(cpearce)
Rebased to current.
Attachment #809873 - Attachment is obsolete: true
Attachment #809873 - Flags: review?(cpearce)
Attachment #810012 - Flags: review?(cpearce)
Comment on attachment 810012 [details] [diff] [review]
Part 1 v2: Add TextTrackManager class

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

::: content/html/content/src/TextTrackManager.h
@@ +32,5 @@
> +  {
> +    return mTextTracks;
> +  }
> +
> +  already_AddRefed<TextTrack> AddTextTrack(TextTrackKind aKind,

I personally favour putting the implementations of methods in the .cpp file, rather than inline in the header file, as it keeps the implementation away from the interface. The interface is what callers of the API use and should be interested in. It also means that you can predeclare classes instead of including their .h files, so that leads to faster builds. (You don't have to make this change to land your patch, though you can if you like, your patch is fine as is).
Attachment #810012 - Flags: review?(cpearce) → review+
(In reply to Chris Pearce (:cpearce) from comment #17)

> I personally favour putting the implementations of methods in the .cpp file,
> rather than inline in the header file, as it keeps the implementation away
> from the interface. The interface is what callers of the API use and should
> be interested in. It also means that you can predeclare classes instead of
> including their .h files, so that leads to faster builds. (You don't have to
> make this change to land your patch, though you can if you like, your patch
> is fine as is).

I've been wondering about that too recently. Especially with the move to reduce the amount of header includes in each header file. I'll move them to the .cpp file like you suggest.

Thanks Chris.
Depends on: 921484
Moved function definitions to the .cpp file.
Attachment #810012 - Attachment is obsolete: true
Attachment #811322 - Flags: review?(cpearce)
Had to remove the top line generated by git format-patch.
Attachment #811322 - Attachment is obsolete: true
Attachment #811322 - Flags: review?(cpearce)
Attachment #811324 - Flags: review?(cpearce)
Attachment #811329 - Flags: review?(giles)
Attachment #811331 - Flags: review?(giles)
Attachment #811331 - Flags: review?(bzbarsky)
Attachment #811333 - Flags: review?(giles)
Attachment #811333 - Flags: review?(bzbarsky)
I still need to add in the vtt.js code which will make this all come together. I'll be adding that sometime next week.

Please note that this doesn't account for TextTrackCues that are in regions. They'll end up being displayed as if they are not in a region. To work regions into this it's going to take a little more work so I'd like to land that in a separate bug.
Comment on attachment 811331 [details] [diff] [review]
Part 4 v1: Expose TextTrackCue::DisplayState to chrome JS r=rillian,bz

>+  NS_ENSURE_TRUE_VOID(mDisplayState);

Can't be null; NS_NewHTMLDivElement is infallible.

>-  mDisplayState->AppendChild(*frag, rv);
>+  nsCOMPtr<nsIDOMNode> throwAway;
>+  mDisplayState->AppendChild(frag, getter_AddRefs(throwAway));

Why this change?  If it's just C++ hiding stuff, then:

  mDisplayState->nsINode::AppendChild(*frag, rv);

or add the relevant "using" to nsGenericHTMLElement.

>+  attribute HTMLDivElement displayState;

Can this getter not get called before CreateCueOverlay?  Seems like this should be a nullable attribute...

Also, is this meant to be a writable attribute?

Finally, can you please put this on a separate partial interface that's clearly documented as Mozilla-specific extensions?

r=me with those bits fixed.
Attachment #811331 - Flags: review?(bzbarsky) → review+
Comment on attachment 811333 [details] [diff] [review]
Part 5 v1: Move update display of cues code to TextTrackManager r=rillian,bz

>+++ b/content/html/content/src/TextTrackManager.cpp
> TextTrackManager::TextTrackManager(HTMLMediaElement *aMediaElement)

>+ nsresult rv;

This is unused; why have it?

Is there only one TextTrackManager, ever?  If not, what keeps us from repeatedly setting the static bit?

>+TextTrackManager::ArrayToJSArray(const nsTArray<T>& aArray)
>+  JSObject* jsArray;

This needs to be rooted.

>+TextTrackManager::UpdateCueDisplay()
>+    JS::Value jsCues = ArrayToJSArray<nsRefPtr<TextTrackCue> >(activeCues);
>+    JS::Value jsRegions = ArrayToJSArray<nsRefPtr<TextTrackRegion> >(regions);

Those both need to be rooted as well.

I think it would be better, actually, to just use an array-type variant here and rely on XPConnect to do the conversion to JS.  For one thing, it'll do it in the right compartment.

>+  nsContentUtils::SetNodeTextContent(overlay, EmptyString(), true);

Generally speaking after this point "overlay" might be dead.  I think you want to be holding a strong reference to it.

>+  nsCOMPtr<nsIDOMNode> throwAway;

Why not use the version of AppendChild that doesn't force you into an outparam?

>+++ b/content/media/TextTrackCue.h
>+  bool Reset();

Can we call this HasBeenReset() or something?  "Reset" sounds like a command to it to reset itself.
Attachment #811333 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky [:bz] from comment #26)

> Why this change?  If it's just C++ hiding stuff, then:
> 
>   mDisplayState->nsINode::AppendChild(*frag, rv);

Yeah, just C++ hiding stuff. I've updated to use nsINode::AppendChild().

> Also, is this meant to be a writable attribute?

Yeah, it's meant to be writeable. It's exposed to JS so that the JS WebVTT parser can directly store the computed subtitle div on the VTTCue so we don't have to constantly rebuild it.
Attachment #812189 - Flags: review?(giles)
Attachment #812189 - Flags: review?(bzbarsky)
Attachment #811331 - Attachment is obsolete: true
Attachment #811331 - Flags: review?(giles)
Comment on attachment 812189 [details] [diff] [review]
Part 4 v2: Expose TextTrackCue::DisplayState to chrome JS r=rillian,bz

r=me
Attachment #812189 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #27)

Addressed all comments.

> Is there only one TextTrackManager, ever?  If not, what keeps us from
> repeatedly setting the static bit?

Ah yeah, good catch!
Attachment #811333 - Attachment is obsolete: true
Attachment #811333 - Flags: review?(giles)
Attachment #812192 - Flags: review?(giles)
Attachment #812192 - Flags: review?(bzbarsky)
Comment on attachment 812192 [details] [diff] [review]
Part 5 v2: Move update display of cues code to TextTrackManager r=rillian,bz

I would appreciated an interdiff here.
Attached patch Part 5 v1->v2 interdiff (obsolete) — Splinter Review
Attachment #812201 - Flags: review?(bzbarsky)
Comment on attachment 812201 [details] [diff] [review]
Part 5 v1->v2 interdiff

r=me
Attachment #812201 - Flags: review?(bzbarsky) → review+
Attachment #812192 - Flags: review?(bzbarsky) → review+
Attachment #811324 - Flags: review?(cpearce) → review+
Attachment #811327 - Flags: review?(giles) → review+
Comment on attachment 811329 [details] [diff] [review]
Part 3 v1: Add TextTrackList::GetAllActiveCues() r=rillian

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

::: content/media/TextTrackCueList.cpp
@@ +118,5 @@
>  
> +void
> +TextTrackCueList::GetArray(nsTArray<nsRefPtr<TextTrackCue> >& aCues)
> +{
> +  aCues = nsTArray<nsRefPtr<TextTrackCue> >(mList);

This makes a copy of the pointer array, right? That's fine for the active cue list, but I'm curious if it's necessary. It could bloat if anyone called this on the full cue list.
Attachment #811329 - Flags: review?(giles) → review+
Attachment #812189 - Flags: review?(giles) → review+
Thanks for review Ralph.

(In reply to Ralph Giles (:rillian) from comment #34)

> ::: content/media/TextTrackCueList.cpp
> @@ +118,5 @@
> >  
> > +void
> > +TextTrackCueList::GetArray(nsTArray<nsRefPtr<TextTrackCue> >& aCues)
> > +{
> > +  aCues = nsTArray<nsRefPtr<TextTrackCue> >(mList);
> 
> This makes a copy of the pointer array, right? That's fine for the active
> cue list, but I'm curious if it's necessary. It could bloat if anyone called
> this on the full cue list.

I was struggling with what was the best thing to do here. Originally I wanted to pass it around just with pointers, but to be safe we would need to use nsRefPtr<nsTArray<nsRefPtr<TextTrackCue> > > which doesn't look the greatest. That doesn't outweigh the performance issues though. I also asked around a bit and was told that using references for nsTArrays was a good thing.

Do you have a preference Ralph?
Comment on attachment 812192 [details] [diff] [review]
Part 5 v2: Move update display of cues code to TextTrackManager r=rillian,bz

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

I assume you're removing the .caption-text style because the processing model will set those in a subsequent patch. Doesn't that still break display with the current patch? Either way, you should probably describe why you removed them in the commit message.
Attachment #812192 - Flags: review?(giles) → review+
(In reply to Ralph Giles (:rillian) from comment #36)
-----------------------------------------------------------
 
> I assume you're removing the .caption-text style because the processing
> model will set those in a subsequent patch. Doesn't that still break display
> with the current patch? Either way, you should probably describe why you
> removed them in the commit message.

Yeah, that's true. I can move the removal of the .caption-text styling in the patch that actually introduces the new styles applied by the processing model.
(In reply to Rick Eyre (:reyre) from comment #37)

> Yeah, that's true. I can move the removal of the .caption-text styling in
> the patch that actually introduces the new styles applied by the processing
> model.

That would be cleaner.
(In reply to Rick Eyre (:reyre) from comment #35)

> Do you have a preference Ralph?

I can't find any literal 'nsRefPtr<nsTArray<...> > instances in the code, so we must always be doing this with a typedef, which hides to complexity.

I think passing a reference is better, but I don't feel strongly and I'm hardly a expert in our smart pointers.
(In reply to Ralph Giles (:rillian) from comment #39)

> I can't find any literal 'nsRefPtr<nsTArray<...> > instances in the code, so
> we must always be doing this with a typedef, which hides to complexity.
> 
> I think passing a reference is better, but I don't feel strongly and I'm
> hardly a expert in our smart pointers.

I think using a nsRefPtr<nsTArray< could be good. I'll ask around about it and figure it out.
Just rebasing to current m-c. Carrying forward r=rillian.
Attachment #811327 - Attachment is obsolete: true
In the interest of not blocking people who are working on other parts of the Track spec I'd like to land patches part 1 and part 2 so that code can start being added to the TextTrackManager class. It's currently blocking some of Andrew's work, most notably in bug 882665.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=f1d60d869513
Rebasing against current m-c. Carrying forward r=cpearce.
Attachment #811324 - Attachment is obsolete: true
Whiteboard: [leave-open]
Just an update on this: I've been busy at work trying to implement the final part of the processing model in the JS parser, but have been running into trouble with finding an implementation of a CSS layout engine that we can use in NodeJS as this is what our test suite runs on. We've settled on using PhantomJS NodeJS bridge so it shouldn't be too long before I have a final patch for this. I'm hoping for the end of the month.
Status: REOPENED → NEW
Attachment #812192 - Attachment is obsolete: true
Attachment #812201 - Attachment is obsolete: true
Just updated the CSS styles on the caption-div box and removed the code that deals with 'reset' since that's moved to vtt.js now. Carrying forward r=bz.

Can you review the CSS style changes Ralph?
Attachment #8346776 - Attachment is obsolete: true
Attachment #8347421 - Flags: review?(giles)
Not sure if I'm doing this right. I'm trying to expose the WebIDL property as _reset and call it 'reset' internally in Gecko. Is this possible Boris?
Attachment #8346777 - Attachment is obsolete: true
Attachment #8347423 - Flags: feedback?(bzbarsky)
This still needs to land in vtt.js so there might be a few last changes to it. I'll wait to ask for review until it lands.
Attachment #8346778 - Attachment is obsolete: true
Comment on attachment 8347423 [details] [diff] [review]
Part 6 v1: Expose VTTCue::Reset to Chrome JS r=bz

> I'm trying to expose the WebIDL property as _reset

WebIDL reserves identifiers starting with '_'.  It also maps strings starting with '_' to the identifier with that '_' stripped off (so you can escape reserved words that way).

So if your webidl says "_reset" it will be "reset" in JS.

Why do you need a property named _reset in JS, exactly?
Attachment #8347423 - Flags: feedback?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #53)

> WebIDL reserves identifiers starting with '_'.  It also maps strings
> starting with '_' to the identifier with that '_' stripped off (so you can
> escape reserved words that way).

Ah, okay I see.

> Why do you need a property named _reset in JS, exactly?

We wanted '_reset' as that's what we named it in the vtt.js polyfill (prefaced with '_' as it's implementation specific and not in the spec). It's not a big deal if we can't use it in Gecko. We can just rename it to 'reset' in the polyfill.

Thanks for the info Boris.
Updated a few of the style changes.

Carrying forward r=bz.
Attachment #8347421 - Attachment is obsolete: true
Attachment #8347421 - Flags: review?(giles)
Attachment #8348831 - Flags: review?(giles)
Attachment #8347423 - Attachment is obsolete: true
Attachment #8348833 - Flags: review?(bzbarsky)
Attachment #8347426 - Attachment is obsolete: true
Attachment #8348835 - Flags: review?(giles)
Comment on attachment 8348831 [details] [diff] [review]
Part 5 v4: Move update display of cues code to TextTrackManager r=rillian,bz

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

Am I still just reviewing the CSS changes here? See questions:

::: layout/style/html.css
@@ +728,5 @@
>  
>  video > .caption-box {
>    position: relative;
> +  overflow: hidden;
> +  padding: 10%;

This is our 'user-agent-defined number to define a margin' from the spec?

@@ +729,5 @@
>  video > .caption-box {
>    position: relative;
> +  overflow: hidden;
> +  padding: 10%;
> +  box-sizing: border-box;

Do we really need quirks mode here? I guess this is to get our padding into the width and height for the subsequent layout calculations? It would be nice if we could do this with normal rendering.

@@ +730,5 @@
>    position: relative;
> +  overflow: hidden;
> +  padding: 10%;
> +  box-sizing: border-box;
> +  -moz-box-sizing: border-box;

You don't need to provide two different versions of the same property. This is baked in to gecko, so if we support the non-prefixed version there's no reason to
Attachment #8348831 - Flags: review?(giles)
Comment on attachment 8348835 [details] [diff] [review]
Part 7 v2: Update vtt.js to latest version r=rillan

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

Please mention that they're now calling processCues in the commit message.
Attachment #8348835 - Flags: review?(giles) → review+
(In reply to Ralph Giles (:rillian) from comment #58)
----------------------------------------------------------
 
> Am I still just reviewing the CSS changes here? See questions:

Yep, I think that should be fine. Feel free to review TextTrackManager.cpp as well if you'd like as that's where the only changes are. I got rid of finding the first cues in TextTrackManager as vtt.js will do that now.

> ::: layout/style/html.css
> @@ +728,5 @@
> >  
> >  video > .caption-box {
> >    position: relative;
> > +  overflow: hidden;
> > +  padding: 10%;
> 
> This is our 'user-agent-defined number to define a margin' from the spec?

In the old spec it would have been, but now it's defined as 1.5vh/vw, so it should be 1.5%... I just went though a messy rebase, sorry about that. I've gone with using percentage as it accomplishes the same thing that the spec wants us to do. The spec redefines the 'viewport', in the case of WebVTT, as the area taken up by the video. Which is equivalent to just using percentage, if I understand correctly. I've filed a bug to just change it to percentage as I feel that would be easier to understand and less convoluted.

> @@ +729,5 @@
> >  video > .caption-box {
> >    position: relative;
> > +  overflow: hidden;
> > +  padding: 10%;
> > +  box-sizing: border-box;
> 
> Do we really need quirks mode here? I guess this is to get our padding into
> the width and height for the subsequent layout calculations? It would be
> nice if we could do this with normal rendering.

I think box-sizing, IIUC, is supposed to allow our padding setting to work even though we're using absolutely positioned cues within the overlay. However, I'm having trouble getting any padding at all actually, using a number of different settings. I was going to do a follow up bug for that.

> @@ +730,5 @@
> >    position: relative;
> > +  overflow: hidden;
> > +  padding: 10%;
> > +  box-sizing: border-box;
> > +  -moz-box-sizing: border-box;
> 
> You don't need to provide two different versions of the same property. This
> is baked in to gecko, so if we support the non-prefixed version there's no
> reason to

I'll update.
(In reply to Rick Eyre (:reyre) from comment #60)

> Yep, I think that should be fine. Feel free to review TextTrackManager.cpp
> as well if you'd like as that's where the only changes are. I got rid of
> finding the first cues in TextTrackManager as vtt.js will do that now.

Should be *dirty* cues.
Removed -moz-box-sizing and changed padding to 1.5%.
Attachment #8348831 - Attachment is obsolete: true
Attachment #8348963 - Flags: review?(giles)
Comment on attachment 8348833 [details] [diff] [review]
Part 6 v2: Expose VTTCue::Reset to Chrome JS r=bz

r=me I guess, though hasBeenReset seems like a more reasonable name for a property returning a boolean here....
Attachment #8348833 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] (Vacation Dec 19 to Jan 1) from comment #63)

> r=me I guess, though hasBeenReset seems like a more reasonable name for a
> property returning a boolean here....

Woops! I did change it to hasBeenReset in the previous version when you suggested it, but forgot to carry that change over to this version (wasn't thinking). I agree and can change it to hasBeenReset.
Comment on attachment 8348963 [details] [diff] [review]
Part 5 v5: Move update display of cues code to TextTrackManager r=rillian,bz

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

Ok. I'm not clear on the deeper implications of border-box, but we can fix it later if it causes problems. Other changes look good.
Attachment #8348963 - Flags: review?(giles) → review+
Renamed 'reset' property to 'hasBeenReset'.

Carrying forward r=bz
Attachment #8348833 - Attachment is obsolete: true
Rebasing to current.

Carrying forward r=rillian.
Attachment #811329 - Attachment is obsolete: true
Rebasing to current.

Carrying forward r=rillian,bz
Attachment #812189 - Attachment is obsolete: true
Mentioned that we're calling processCues() now in the commit message.

Carrying forward r=rillian.

Try: https://tbpl.mozilla.org/?tree=Try&rev=efc922811fc8
Attachment #8348835 - Attachment is obsolete: true
Comment on attachment 8350110 [details] [diff] [review]
Part 6 v3: Expose VTTCue::Reset to Chrome JS r=bz

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

> Bug 867407 - Part 6: Expose VTTCue::Reset to Chrome JS.

Watch out - that's not this bug.
(In reply to :Ms2ger from comment #70)
> Comment on attachment 8350110 [details] [diff] [review]
> Part 6 v3: Expose VTTCue::Reset to Chrome JS r=bz
> 
> Review of attachment 8350110 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > Bug 867407 - Part 6: Expose VTTCue::Reset to Chrome JS.
> 
> Watch out - that's not this bug.

Good catch. Thanks!
Rebasing to current. Carrying forward r=rillian.
Attachment #8350124 - Attachment is obsolete: true
Rebasing to current. Carrying forward r=rillian,bz
Attachment #8350125 - Attachment is obsolete: true
Rebasing to current. Carrying forward r=rillian,bz
Attachment #8348963 - Attachment is obsolete: true
Rebasing to current and fixing bug number in commit message. Carrying forward r=bz.
Attachment #8350110 - Attachment is obsolete: true
Rebasing to current. Carrying forward r=rillian.
Attachment #8350127 - Attachment is obsolete: true
Try looks green: https://tbpl.mozilla.org/?tree=Try&rev=c7f9ea019cb5

There will be one more patch after this most likely.
Keywords: checkin-needed
Attachment #8358397 - Flags: checkin+
Attachment #8358399 - Flags: checkin+
Attachment #8358401 - Flags: checkin+
Attachment #8358402 - Flags: checkin+
Attachment #8358403 - Flags: checkin+
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #78)

> Tests? :)

Woops! I can add them as a follow up? :-). Most of it will be easy to test, but the actual WebVTT positioning stuff would need reftests so that might take time.
Attachment #8358397 - Flags: checkin+
Attachment #8358399 - Flags: checkin+
Attachment #8358401 - Flags: checkin+
Attachment #8358402 - Flags: checkin+
Attachment #8358403 - Flags: checkin+
I _think_ I got it. Switched 'overlay->nsINode::Length()' to 'overlay->Length()'. This was the line it was failing on because XPCOM couldn't find the method at some point so I've made it less explicit so that it will be more flexible as to where to find the definition of Length.

Carrying forward r=rillian,bz.
Attachment #8358401 - Attachment is obsolete: true
Try: https://tbpl.mozilla.org/?tree=Try&rev=3f3b99e51871

I'm having a hard time reproducing it locally and on the try server. So apologies if this breaks inbound again.
Keywords: checkin-needed
(In reply to Rick Eyre (:reyre) from comment #79)
> Woops! I can add them as a follow up? :-). Most of it will be easy to test,
> but the actual WebVTT positioning stuff would need reftests so that might
> take time.

Whatever floats your boat, I'm just asking the question :)
Comment on attachment 8358397 [details] [diff] [review]
Part 3 v3: Add TextTrackList::GetAllActiveCues() r=rillian

https://hg.mozilla.org/integration/mozilla-inbound/rev/705f43739079
Attachment #8358397 - Flags: checkin+
Comment on attachment 8358399 [details] [diff] [review]
Part 4 v4: Expose TextTrackCue::DisplayState to chrome JS r=rillian,bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/401b8593a881
Attachment #8358399 - Flags: checkin+
Comment on attachment 8359808 [details] [diff] [review]
Part 5 v7: Move update display of cues code to TextTrackManager r=rillian,bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/9264d50055ea
Attachment #8359808 - Flags: checkin+
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #90)
> http://www.livememe.com/ibmq0xt
> 
> Bumped.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b7c25fa0543c

Follow-up fix for a couple places that were expected the old IID.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8056d3f9c002
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #90)
> http://www.livememe.com/ibmq0xt
> 
> Bumped.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b7c25fa0543c

Sorry about that Ryan! I had no idea that you needed to change the IID when changing the interface :S. Nice meme too ;). Thanks for the fixes!
Depends on: 967157
These last two patches complete the initial implementation of the processing algorithm. It doesn't fully support the vertical text bits of the algorithm yet, but I don't think that's a big deal though since Firefox doesn't support it yet either.
It doesn't support region processing yet either, but the region processing part of the spec is still very much in flux, Chrome has only implemented it behind a pref I believe. I think it'll be fine if we open another bug to add in region processing when the spec gets a bit more stable.
Attachment #8380122 - Flags: review?(giles) → review+
Attachment #8380123 - Flags: review?(giles) → review+
Thanks for review Ralph. Try push seems to be green: https://tbpl.mozilla.org/?tree=Try&rev=15d5cbbcb038
Keywords: checkin-needed
Whiteboard: [leave-open]
Attachment #8380122 - Flags: checkin+
Attachment #8380123 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/60bbb4d3ed09
https://hg.mozilla.org/mozilla-central/rev/e2cf0a2fa737
Status: NEW → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Depends on: CVE-2014-1525
You need to log in before you can comment on or make changes to this bug.