Last Comment Bug 833388 - [webvtt] Add captions div to nsVideoFrame for webvtt subtitle display
: [webvtt] Add captions div to nsVideoFrame for webvtt subtitle display
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla22
Assigned To: Jesse Silver
:
: Jet Villegas (:jet)
Mentors:
Depends on: 875305
Blocks: webvtt
  Show dependency treegraph
 
Reported: 2013-01-22 08:52 PST by Jesse Silver
Modified: 2013-05-23 07:09 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Work in progress for the layout portion of the patch (13.78 KB, patch)
2013-01-31 08:52 PST, Jesse Silver
cpearce: review-
Details | Diff | Splinter Review
v2 of the Caption Div patch (8.53 KB, patch)
2013-02-21 05:37 PST, Jesse Silver
no flags Details | Diff | Splinter Review
v3 of the Caption Div patch (8.15 KB, patch)
2013-02-22 08:32 PST, Jesse Silver
roc: review+
Details | Diff | Splinter Review
Updating to the current m-c. (7.84 KB, patch)
2013-02-25 16:24 PST, Jesse Silver
giles: feedback+
Details | Diff | Splinter Review
Added in author/commit info (8.38 KB, patch)
2013-02-25 18:35 PST, Jesse Silver
no flags Details | Diff | Splinter Review
Removed trailing whitespace (7.90 KB, patch)
2013-02-26 13:10 PST, Jesse Silver
no flags Details | Diff | Splinter Review
Changed commit message slightly (7.90 KB, patch)
2013-02-26 13:17 PST, Jesse Silver
roc: review+
Details | Diff | Splinter Review

Description Jesse Silver 2013-01-22 08:52:23 PST
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/20100101 Firefox/17.0
Build ID: 20121128204232
Comment 1 David Humphrey (:humph) 2013-01-22 08:57:13 PST
Spin-off from bug 629350 to implement nsVideoFrame changes for webvtt captions.
Comment 2 Jesse Silver 2013-01-31 08:52:15 PST
Created attachment 708628 [details] [diff] [review]
Work in progress for the layout portion of the patch

This is simply the patch from #629350 split up into just the layout changes.
Comment 3 Jesse Silver 2013-02-07 12:02:27 PST
Comment on attachment 708628 [details] [diff] [review]
Work in progress for the layout portion of the patch

This is my initial patch for this that was simply from Ralph's original larger WebVTT patch. I would like to get this reviewed so I know how to move forward on this bug.
Comment 4 Chris Pearce (:cpearce) 2013-02-10 15:32:04 PST
Comment on attachment 708628 [details] [diff] [review]
Work in progress for the layout portion of the patch

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

Thanks for working on this! This patch is mostly there, I'm not granting review because we shouldn't mix unrelated build system changes in with behaviour changes, plus a few minor changes/questions.

I'd also like Roc to take a look at this, he's more qualified to review layout/generic/ changes than I.

::: configure.in
@@ +4245,5 @@
>  MOZ_OMX_PLUGIN=
>  MOZ_VP8=
>  MOZ_VP8_ERROR_CONCEALMENT=
>  MOZ_VP8_ENCODER=
> +MOZ_WEBVTT=1

This isn't used in this patch, this change, and the change in Makefile.in should be in another patch (possibly with other changes) and it should be reviewed by one of the build peers, like :khuey or :ted.

::: layout/generic/nsVideoFrame.cpp
@@ +114,5 @@
>    NS_TrustedNewXULElement(getter_AddRefs(mVideoControls), nodeInfo.forget());
>    if (!aElements.AppendElement(mVideoControls))
>      return NS_ERROR_OUT_OF_MEMORY;
>  
> +  // Set up the caption overlay div for showing any TextTrack data

Based on the definition of the <track> element's @kind attribute, it seems subtitles and captions should only be shown on <video> elements:
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#attr-track-kind

Indeed, the spec authors seem to be saying here that if you want subtitles on an audio element, you should use a video element instead:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=20898

So assuming we only need the caption div for subtitles and captions, I think we should only create it inside the if(HasVideoElement()) block.

(We use these video controls for audio too, the controls are written in JavaScript and figure out if they are controlling a video or audio mode and behave appropriately).

@@ +439,4 @@
>    for (nsIFrame *child = mFrames.FirstChild();
>         child;
>         child = child->GetNextSibling()) {
> +    if (child->GetContent() != mPosterImage || ShouldDisplayPoster()) {

Hmmm, why did you remove the boxFrame condition here?
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-02-10 18:54:37 PST
Comment on attachment 708628 [details] [diff] [review]
Work in progress for the layout portion of the patch

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

::: layout/generic/nsVideoFrame.cpp
@@ +120,5 @@
> +                                          nullptr,
> +                                          kNameSpaceID_XHTML,
> +                                          nsIDOMNode::ELEMENT_NODE);
> +  NS_ENSURE_TRUE(nodeInfo, NS_ERROR_OUT_OF_MEMORY);
> +  element = NS_NewHTMLDivElement(nodeInfo.forget());

Don't reuse the 'element' declaration. Use a new Element* declaratoin or just directly assign to mCaptionDiv.

@@ +135,5 @@
> +    return NS_ERROR_OUT_OF_MEMORY;
> +
> +  // add a test string so we can test the div
> +  nsCOMPtr<nsIDOMHTMLElement> div = do_QueryInterface(mCaptionDiv);
> +  rv = div->SetInnerHTML(NS_LITERAL_STRING(""));

Don't do this. This doesn't do anything anyway.

@@ +335,5 @@
> +                                       availableSize,
> +                                       aMetrics.width,
> +                                       aMetrics.height);
> +      kidReflowState.SetComputedWidth(aReflowState.ComputedWidth());
> +      kidReflowState.SetComputedHeight(aReflowState.ComputedHeight());

You need to subtract mCaptionDiv's computed border+padding here --- you need to pass in mCaptionDiv's content-box size to SetComputedWidth/SetComputedHeight.

@@ +439,4 @@
>    for (nsIFrame *child = mFrames.FirstChild();
>         child;
>         child = child->GetNextSibling()) {
> +    if (child->GetContent() != mPosterImage || ShouldDisplayPoster()) {

I think that's OK. Basically this code says that we display all child content unconditionally except for the poster image.
Comment 6 Jesse Silver 2013-02-21 05:37:47 PST
Created attachment 716496 [details] [diff] [review]
v2 of the Caption Div patch

Made changes to the patch based on reviews.

"You need to subtract mCaptionDiv's computed border+padding here --- you need to pass in mCaptionDiv's content-box size to SetComputedWidth/SetComputedHeight."

I'm a bit unsure about this one. I took a guess based on other places I saw SetComputedWidth() being used. I would like to know the proper way this should be done.
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-02-22 03:41:05 PST
Comment on attachment 716496 [details] [diff] [review]
v2 of the Caption Div patch

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

Otherwise looks good.

::: layout/generic/nsVideoFrame.cpp
@@ +113,5 @@
> +    // hang a class name on the div for default styling
> +    nsresult rv = mCaptionDiv->SetAttr(kNameSpaceID_None,
> +                                       nsGkAtoms::_class,
> +                                       NS_LITERAL_STRING("captionDiv"),
> +                                       true);

Actually I don't think we need this class. There's only one anonymous DIV in a <video> so you can just style video > div.
Comment 8 Jesse Silver 2013-02-22 08:32:15 PST
Created attachment 717161 [details] [diff] [review]
v3 of the Caption Div patch

Removed the .captionDiv class, changed the css to be for video > div instead.
Comment 9 Ralph Giles (:rillian) needinfo me 2013-02-25 10:11:15 PST
Jesse, please rebase this against current m-c. I'll do a try server push and then we can land.
Comment 10 Jesse Silver 2013-02-25 16:24:50 PST
Created attachment 718166 [details] [diff] [review]
Updating to the current m-c.
Comment 11 Ralph Giles (:rillian) needinfo me 2013-02-25 18:10:40 PST
Comment on attachment 718166 [details] [diff] [review]
Updating to the current m-c.

Thanks, the rebase looks good to me.

The next step is to generate a proper git diff with a commit message and author information suitable for check-in. This should include the name and email address you want in the commit history. Maintainers often do this for casual contributors, but it's good practice to learn to do so when submitting patches.

Create at git branch (or hg patch queue) with just this change on it, then do a 'git commit' or 'git rebase -i' and squash your changes, then write a commit message describing the bug, change, review approval and motivation. Standard style is something like:

  Bug 833388 - Add captions div to nsVideoFrame - r=roc
  [blank line]
  <description of what changed and why>

Please upload the result (output from git show or git format-patch) as a new attachment for final review.

Pushed this one to try as https://tbpl.mozilla.org/?tree=Try&rev=a3717d34b250
Comment 12 Jesse Silver 2013-02-25 18:35:51 PST
Created attachment 718209 [details] [diff] [review]
Added in author/commit info
Comment 13 Ralph Giles (:rillian) needinfo me 2013-02-26 10:28:35 PST
Comment on attachment 718209 [details] [diff] [review]
Added in author/commit info

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

Thanks! Commit message looks good. You should probably strip the changed-files section to avoid hg importing that as part of the commit message.

Please remove the trailing whitespace. You can use 'git diff --check' to catch problems like this. You also have inconsistent line endings in your patch, with windows eol on all the new additions, but unix on the context. You should fix that, probably by configuring your editor to use whatever line ending convention msysgit expects. Sorry for not noticing earlier.

::: layout/generic/nsVideoFrame.cpp
@@ +113,5 @@
> +
> +    if (!aElements.AppendElement(mCaptionDiv))
> +      return NS_ERROR_OUT_OF_MEMORY;
> +  }
> +  

Please remove this trailing whitespace.

@@ +325,5 @@
> +                                       aMetrics.height);
> +      nsSize size(aReflowState.ComputedWidth(), aReflowState.ComputedHeight());
> +      size.width -= kidReflowState.mComputedBorderPadding.LeftRight();
> +      size.height -= kidReflowState.mComputedBorderPadding.TopBottom();
> +      

Here too.

@@ +328,5 @@
> +      size.height -= kidReflowState.mComputedBorderPadding.TopBottom();
> +      
> +      kidReflowState.SetComputedWidth(std::max(size.width, 0));
> +      kidReflowState.SetComputedHeight(std::max(size.height, 0));
> +      

And here.
Comment 14 Jesse Silver 2013-02-26 13:10:11 PST
Created attachment 718622 [details] [diff] [review]
Removed trailing whitespace
Comment 15 Jesse Silver 2013-02-26 13:17:47 PST
Created attachment 718627 [details] [diff] [review]
Changed commit message slightly
Comment 16 Ralph Giles (:rillian) needinfo me 2013-02-26 13:26:35 PST
https://tbpl.mozilla.org/?tree=Try&rev=4e3ade8ac100
Comment 17 Jesse Silver 2013-02-26 13:28:01 PST
Comment on attachment 718627 [details] [diff] [review]
Changed commit message slightly

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

There were changes made because of the rebase, so it needs to be looked over one more time.
Comment 18 Ryan VanderMeulen [:RyanVM] 2013-02-27 06:26:34 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/41458ff35ced

Thanks for the patch!
Comment 19 Ryan VanderMeulen [:RyanVM] 2013-02-27 17:55:37 PST
https://hg.mozilla.org/mozilla-central/rev/41458ff35ced
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2013-05-23 07:06:26 PDT
>+video > div {
>+video > div p {

Why are we adding slow rules like this (which will require nontrivial extra work for every div and p) to html.css?  See https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Writing_efficient_CSS

Please get this fixed!

Note You need to log in before you can comment on or make changes to this bug.