Fix cue rendering test and include support for left/right alignment.

Source/core/html/track/TextTrackCue.cpp

 /*
  * Copyright (C) 2011 Google Inc.  All rights reserved.
  * Copyright (C) 2011, 2012, 2013 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
@@ skipping 55 matching lines @@
     DEFINE_STATIC_LOCAL(const String, middle, ("middle"));
     return middle;
 }
 
 static const String& endKeyword()
 {
     DEFINE_STATIC_LOCAL(const String, end, ("end"));
     return end;
 }
 
+static const String& leftKeyword()
+{
+    DEFINE_STATIC_LOCAL(const String, left, ("left"));
+    return left;
+}
+
+static const String& rightKeyword()
+{
+    DEFINE_STATIC_LOCAL(const String, right, ("right"));
+    return right;
+}
+
 static const String& horizontalKeyword()
 {
     return emptyString();
 }
 
 static const String& verticalGrowingLeftKeyword()
 {
     DEFINE_STATIC_LOCAL(const String, verticalrl, ("rl"));
     return verticalrl;
 }
@@ skipping 60 matching lines @@
     }
 
     // The 'text-align' property on the (root) List of WebVTT Node Objects must
     // be set to the value in the second cell of the row of the table below
     // whose first cell is the value of the corresponding cue's text track cue
     // alignment:
     if (m_cue->align() == startKeyword())
         setInlineStyleProperty(CSSPropertyTextAlign, CSSValueStart);
     else if (m_cue->align() == endKeyword())
         setInlineStyleProperty(CSSPropertyTextAlign, CSSValueEnd);
-    else
+    else if (m_cue->align() == middleKeyword())

[philipj_slow, 2013/10/03 09:04:43]

The implementation of TextTrackCue::align() already asserts if the align keywork
wasn't one of the known ones, so dropping the default-to-middle here should be
safe, but it's not really obvious when looking at this code.

The spec expresses this bit of the algorithm as a mapping of the alignment
concepts and not the strings themselves, so perhaps this could be converted to a
switch over m_cue->getAlignment() to get rid of the unnecessary string
conversion/comparison, and also make it clear to the compiler and humans that
all cases are handled?

One could even go one step further and just convert this to an mapping array
from CueAlignment to CSSPropertyTextAlign values, but that might require some
static_assert to make the compile fail if things get out of sync.

[vcarbune.chromium, 2013/10/04 13:30:26]

I added the map and a getCSSAlignment method on TextTrackCue.

         setInlineStyleProperty(CSSPropertyTextAlign, CSSValueCenter);
+    else if (m_cue->align() == leftKeyword())
+        setInlineStyleProperty(CSSPropertyTextAlign, CSSValueLeft);
+    else if (m_cue->align() == rightKeyword())
+        setInlineStyleProperty(CSSPropertyTextAlign, CSSValueRight);
 
     if (!m_cue->snapToLines()) {
         // 10.13.1 Set up x and y:
         // Note: x and y are set through the CSS left and top above.
 
         // 10.13.2 Position the boxes in boxes such that the point x% along the
         // width of the bounding box of the boxes in boxes is x% of the way
         // across the width of the video's rendering area, and the point y%
         // along the height of the bounding box of the boxes in boxes is y%
         // of the way across the height of the video's rendering area, while
@@ skipping 260 matching lines @@
 
 const String& TextTrackCue::align() const
 {
     switch (m_cueAlignment) {
     case Start:
         return startKeyword();
     case Middle:
         return middleKeyword();
     case End:
         return endKeyword();
+    case Left:
+        return leftKeyword();
+    case Right:
+        return rightKeyword();
     default:
         ASSERT_NOT_REACHED();
         return emptyString();
     }
 }
 
 void TextTrackCue::setAlign(const String& value, ExceptionState& es)
 {
     // http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#dom-texttrackcue-align
     // On setting, the text track cue alignment must be set to the value given in the
     // first cell of the row in the table above whose second cell is a case-sensitive
     // match for the new value, if any. If none of the values match, then the user
     // agent must instead throw a SyntaxError exception.
 
     CueAlignment alignment = m_cueAlignment;
     if (value == startKeyword())
         alignment = Start;
     else if (value == middleKeyword())
         alignment = Middle;
     else if (value == endKeyword())
         alignment = End;
+    else if (value == leftKeyword())
+        alignment = Left;
+    else if (value == rightKeyword())
+        alignment = Right;
     else
         es.throwUninformativeAndGenericDOMException(SyntaxError);
 
     if (alignment == m_cueAlignment)
         return;
 
     cueWillChange();
     m_cueAlignment = alignment;
     cueDidChange();
 }
@@ skipping 174 matching lines @@
                 || charDirection == WTF::Unicode::RightToLeftArabic) {
                 m_displayDirection = CSSValueRtl;
                 return;
             }
         }
     }
 }
 
 void TextTrackCue::calculateDisplayParameters()
 {
+    // A text track cue has a text track cue computed line position whose value
+    // is defined in terms of the other aspects of the cue.
+    m_computedLinePosition = calculateComputedLinePosition();

[acolwell GONE FROM CHROMIUM, 2013/10/02 16:19:37]

Does this really need to be moved to the top of the function or is it sufficient
to just move it before the places it is used below. If it doesn't need to be up
here then I'd prefer it to be closer to where it is used so that people don't
think it is accessed in one of the method calls between here and there.

[philipj_slow, 2013/10/03 09:04:43]

In <http://dev.w3.org/html5/webvtt/> it's used in step 10.8, which is step 10.9
in this source file, putting it there would be nice. Or, to never compute it
when not needed, one could add a updateComputedLinePosition which both sets
m_computedLinePosition and returns it.

Victor, was it just a bug in the original implementation that the line position
was updated *after* being used in this algorithm, or is it in some way related
to the align change?

[vcarbune.chromium, 2013/10/04 13:30:26]

I moved it right before the point where it's used. 
This is probably a bug in the original implementation, but it's use case is not
covered by tests (since moving it around doesn't break anything). However, it
needs to be computed here as other code paths depend on it (and removing it
breaks the test, but doesn't have anything to do with align).

Probably I should file a separate bug for proper test coverage of this case and
we can leave it before where it's used, or as in the original case. 

+
     // Steps 10.2, 10.3
     determineTextDirection();
 
     // 10.4 If the text track cue writing direction is horizontal, then let
     // block-flow be 'tb'. Otherwise, if the text track cue writing direction is
     // vertical growing left, then let block-flow be 'lr'. Otherwise, the text
     // track cue writing direction is vertical growing right; let block-flow be
     // 'rl'.
     m_displayWritingMode = m_displayWritingModeMap[m_writingDirection];
 
     // 10.5 Determine the value of maximum size for cue as per the appropriate
     // rules from the following list:
     int maximumSize = m_textPosition;
     if ((m_writingDirection == Horizontal && m_cueAlignment == Start && m_displayDirection == CSSValueLtr)
             || (m_writingDirection == Horizontal && m_cueAlignment == End && m_displayDirection == CSSValueRtl)
-            || (m_writingDirection == VerticalGrowingLeft && m_cueAlignment == Start)
-            || (m_writingDirection == VerticalGrowingRight && m_cueAlignment == Start)) {
+            || (m_writingDirection == Horizontal && m_cueAlignment == Left)
+            || (m_writingDirection == VerticalGrowingLeft && (m_cueAlignment == Start || m_cueAlignment == Left))
+            || (m_writingDirection == VerticalGrowingRight && (m_cueAlignment == Start || m_cueAlignment == Left))) {
         maximumSize = 100 - m_textPosition;
     } else if ((m_writingDirection == Horizontal && m_cueAlignment == End && m_displayDirection == CSSValueLtr)
             || (m_writingDirection == Horizontal && m_cueAlignment == Start && m_displayDirection == CSSValueRtl)
-            || (m_writingDirection == VerticalGrowingLeft && m_cueAlignment == End)
-            || (m_writingDirection == VerticalGrowingRight && m_cueAlignment == End)) {
+            || (m_writingDirection == Horizontal && m_cueAlignment == Right)
+            || (m_writingDirection == VerticalGrowingLeft && (m_cueAlignment == End || m_cueAlignment == Right))
+            || (m_writingDirection == VerticalGrowingRight && (m_cueAlignment == End || m_cueAlignment == Right))) {
         maximumSize = m_textPosition;
     } else if (m_cueAlignment == Middle) {
         maximumSize = m_textPosition <= 50 ? m_textPosition : (100 - m_textPosition);
         maximumSize = maximumSize * 2;
     }

[acolwell GONE FROM CHROMIUM, 2013/10/02 16:19:37]

nit: Should we have an } else { ASSERT_NOT_REACHED(); } here to make sure we
have all the combinations covered? 

[vcarbune.chromium, 2013/10/04 13:30:26]

Done.

 
     // 10.6 If the text track cue size is less than maximum size, then let size
     // be text track cue size. Otherwise, let size be maximum size.
     m_displaySize = std::min(m_cueSize, maximumSize);
 
     // 10.8 Determine the value of x-position or y-position for cue as per the
     // appropriate rules from the following list:
     if (m_writingDirection == Horizontal) {
         if (m_cueAlignment == Start) {
             if (m_displayDirection == CSSValueLtr)
                 m_displayPosition.first = m_textPosition;
             else
                 m_displayPosition.first = 100 - m_textPosition - m_displaySize;
         } else if (m_cueAlignment == End) {
             if (m_displayDirection == CSSValueRtl)
                 m_displayPosition.first = 100 - m_textPosition;
             else
                 m_displayPosition.first = m_textPosition - m_displaySize;
+        } else if (m_cueAlignment == Left) {
+            if (m_displayDirection == CSSValueLtr)
+                m_displayPosition.first = m_textPosition;
+            else
+                m_displayPosition.first = 100 - m_textPosition;
+        } else if (m_cueAlignment == Right) {
+            if (m_displayDirection == CSSValueLtr)
+                m_displayPosition.first = m_textPosition - m_displaySize;
+            else
+                m_displayPosition.first = 100 - m_textPosition - m_displaySize;
+        } else if (m_cueAlignment == Middle) {
+            if (m_displayDirection == CSSValueLtr)
+                m_displayPosition.first = m_textPosition - m_displaySize / 2;
+            else
+                m_displayPosition.first = 100 - m_textPosition - m_displaySize / 2;
         }
+    } else {
+        // Cases for m_writingDirection being VerticalGrowing{Left|Right}

[acolwell GONE FROM CHROMIUM, 2013/10/02 16:19:37]

nit: Please use switch(m_cueAlignment) here and above so the compiler can help
us ensure we handle all possible values. 

[vcarbune.chromium, 2013/10/04 13:30:26]

Done. I had to add a default case though because of the map I added; I
understand the idea would be to not have it so the compiler shouts if not
covered; I can revert this or remove the map, if you prefer, otherwise we have
this debug runtime assert.

+        if (m_cueAlignment == Start || m_cueAlignment == Left)
+            m_displayPosition.second = m_textPosition;
+        else if (m_cueAlignment == End || m_cueAlignment == Right)
+            m_displayPosition.second = m_textPosition - m_displaySize;
+        else if (m_cueAlignment == Middle)
+            m_displayPosition.second = m_textPosition - m_displaySize / 2;
     }
 
-    if ((m_writingDirection == VerticalGrowingLeft && m_cueAlignment == Start)
-            || (m_writingDirection == VerticalGrowingRight && m_cueAlignment == Start)) {
-        m_displayPosition.second = m_textPosition;
-    } else if ((m_writingDirection == VerticalGrowingLeft && m_cueAlignment == End)
-            || (m_writingDirection == VerticalGrowingRight && m_cueAlignment == End)) {
-        m_displayPosition.second = 100 - m_textPosition;
-    }
-
-    if (m_writingDirection == Horizontal && m_cueAlignment == Middle) {
-        if (m_displayDirection == CSSValueLtr)
-            m_displayPosition.first = m_textPosition - m_displaySize / 2;
-        else
-           m_displayPosition.first = 100 - m_textPosition - m_displaySize / 2;
-    }
-
-    if ((m_writingDirection == VerticalGrowingLeft && m_cueAlignment == Middle)
-        || (m_writingDirection == VerticalGrowingRight && m_cueAlignment == Middle))
-        m_displayPosition.second = m_textPosition - m_displaySize / 2;
-
     // 10.9 Determine the value of whichever of x-position or y-position is not

[philipj_slow, 2013/10/03 09:04:43]

This is step 10.8 in <http://dev.w3.org/html5/webvtt/>, so something is out of
sync. Can you add a link somewhere to the exact spec commit that this is now
supposed to be in sync with? That's really helpful when checking if there are
new spec changes to implement.

[vcarbune.chromium, 2013/10/04 13:30:26]

Looking at the dates when this was submitted, looks like r7059. Is there an easy
way to see the whole spec at a given revision number?

I'm tempted to say it might be a manual numbering error, step 10.7 is missing
since the moment these lines were submitted.

[philipj_slow, 2013/10/07 08:52:31]

For HTML there's <https://github.com/whatwg/html-mirror>, which you can check
out and bisect locally to find stuff.

For WebVTT you can browse <https://dvcs.w3.org/hg/text-tracks/> and then take
the hash of a commit to get to a URL like
<https://dvcs.w3.org/hg/text-tracks/raw-file/6803fbcb723d/webvtt/webvtt.html>.

However, it would be a lot of work to verify that the implementation is in sync
with any particular version, so if you're not already sure then it can wait
until someone can do a full spec-syncing pass.

[vcarbune.chromium, 2013/10/07 09:37:28]

Thanks for the explanation, I think this can be easily done in a different
(dedicated) pass, so I just added a FIXME in the code for now.

     // yet calculated for cue as per the appropriate rules from the following
     // list:
     if (m_snapToLines && m_displayPosition.second == undefinedPosition && m_writingDirection == Horizontal)
         m_displayPosition.second = 0;
 
     if (!m_snapToLines && m_displayPosition.second == undefinedPosition && m_writingDirection == Horizontal)
         m_displayPosition.second = m_computedLinePosition;
 
     if (m_snapToLines && m_displayPosition.first == undefinedPosition
             && (m_writingDirection == VerticalGrowingLeft || m_writingDirection == VerticalGrowingRight))
         m_displayPosition.first = 0;
 
     if (!m_snapToLines && (m_writingDirection == VerticalGrowingLeft || m_writingDirection == VerticalGrowingRight))
         m_displayPosition.first = m_computedLinePosition;
-
-    // A text track cue has a text track cue computed line position whose value
-    // is defined in terms of the other aspects of the cue.
-    m_computedLinePosition = calculateComputedLinePosition();
 }
 
 void TextTrackCue::markFutureAndPastNodes(ContainerNode* root, double previousTimestamp, double movieTime)
 {
     DEFINE_STATIC_LOCAL(const String, timestampTag, ("timestamp"));
 
     bool isPastNode = true;
     double currentTimestamp = previousTimestamp;
     if (currentTimestamp > movieTime)
         isPastNode = false;
@@ skipping 338 matching lines @@
             if (cueAlignment == startKeyword())
                 m_cueAlignment = Start;
 
             // 2. If value is a case-sensitive match for the string "middle", then let cue's text track cue alignment be middle alignment.
             else if (cueAlignment == middleKeyword())
                 m_cueAlignment = Middle;
 
             // 3. If value is a case-sensitive match for the string "end", then let cue's text track cue alignment be end alignment.
             else if (cueAlignment == endKeyword())
                 m_cueAlignment = End;
+
+            // 4. If value is a case-sensitive match for the string "left", then let cue's text track cue alignment be left alignment.
+            else if (cueAlignment == leftKeyword())
+                m_cueAlignment = Left;
+
+            // 5. If value is a case-sensitive match for the string "right", then let cue's text track cue alignment be right alignment.
+            else if (cueAlignment == rightKeyword())
+                m_cueAlignment = Right;
             }
             break;
 #if ENABLE(WEBVTT_REGIONS)
         case RegionId:
             m_regionId = WebVTTParser::collectWord(input, &position);
             break;
 #endif
         case None:
             break;
         }
@@ skipping 77 matching lines @@
         return false;
     if (m_cueSize != cue.size())
         return false;
     if (align() != cue.align())
         return false;
 
     return true;
 }
 
 } // namespace WebCore