Bug 79747 - Determine parameters for rendering a TextTrackCue
Summary: Determine parameters for rendering a TextTrackCue
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Victor Carbune
URL:
Keywords:
Depends on:
Blocks: 79347 79750
  Show dependency treegraph
 
Reported: 2012-02-27 23:12 PST by Victor Carbune
Modified: 2012-04-19 12:24 PDT (History)
3 users (show)

See Also:


Attachments
Preliminary patch (7.73 KB, patch)
2012-03-15 03:35 PDT, Victor Carbune
eric.carlson: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Victor Carbune 2012-02-27 23:12:19 PST
This bug represents inner substeps 10.1, 10.4 - 10.9 of section 3.5 of WebVTT rendering rules:
http://dev.w3.org/html5/webvtt/#webvtt-cue-text-rendering-rules

"10. For each text track cue cue in cues that has not yet had corresponding CSS boxes added to output, in text track cue order, run the following substeps:
10.1 Let nodes be the list of WebVTT Node Objects obtained by applying the WebVTT cue text parsing rules to the cue's text track cue text.
[...]"
Comment 1 Victor Carbune 2012-03-15 03:35:31 PDT
Created attachment 132011 [details]
Preliminary patch

Implemented the computational steps described in the spec. The patch could be submitted stand-alone - as it is - or in the same time with the first implemented rendering case that is blocked by it.
Comment 2 WebKit Review Bot 2012-03-15 04:02:13 PDT
Comment on attachment 132011 [details]
Preliminary patch

Attachment 132011 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11954572
Comment 3 Eric Carlson 2012-03-15 07:59:50 PDT
Comment on attachment 132011 [details]
Preliminary patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132011&action=review

This is close, but it needs some clean-up and tests.

> Source/WebCore/html/track/TextTrackCue.cpp:128
> +    m_displayWritingModeMap[Horizontal] = CSSValueHorizontalTb;
> +    m_displayWritingModeMap[VerticalGrowingLeft] = CSSValueVerticalLr;
> +    m_displayWritingModeMap[VerticalGrowingRight] = CSSValueVerticalRl;

Nit: it would be good to have a comment here about why these are correct defaults.

> Source/WebCore/html/track/TextTrackCue.cpp:438
> +    int maximumSize;

This will remain uninitialized if none of the conditions below evaluate to true. Even if this can't logically happen, the compiler will complain.

> Source/WebCore/html/track/TextTrackCue.cpp:456
> +    m_displaySize = m_cueSize < maximumSize ? m_cueSize : maximumSize;

std::min() ?

> Source/WebCore/html/track/TextTrackCue.cpp:460
> +    // width be 'sizeâvw' and height be 'auto'. Otherwise, let width be 'auto'
> +    // and height be 'sizeâvh'. (These are CSS values used by the next section

Nit: "sizeâvw" and "sizeâvh" ?

> Source/WebCore/html/track/TextTrackCue.cpp:504
> +    // 10.10 Let left be 'x-positionâvw' and top be 'y-positionâvh'.

Nit: copy pasta here as well

> Source/WebCore/html/track/TextTrackCue.h:161
> +    int m_displayWritingModeMap[3];

WritingDirection is a private enum, so I think we should add a sentinel terminal value and use it here instead of hard coding the size of this array.
Comment 4 Victor Carbune 2012-04-19 12:24:40 PDT
Submitted as part of https://bugs.webkit.org/show_bug.cgi?id=79750
Changeset 114640 (http://trac.webkit.org/changeset/114640)