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

Source/core/html/track/TextTrackCue.cpp

Index: Source/core/html/track/TextTrackCue.cpp
diff --git a/Source/core/html/track/TextTrackCue.cpp b/Source/core/html/track/TextTrackCue.cpp
index f80d78cbc5f8fc8bff2c3be130ea49516817f4c0..b1c908787384c5642727799feb1506ed47528e76 100644
--- a/Source/core/html/track/TextTrackCue.cpp
+++ b/Source/core/html/track/TextTrackCue.cpp
@@ -73,6 +73,18 @@ static const String& endKeyword()
     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();
@@ -153,8 +165,12 @@ void TextTrackCueBox::applyCSSProperties(const IntSize&)
         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:
@@ -435,6 +451,10 @@ const String& TextTrackCue::align() const
         return middleKeyword();
     case End:
         return endKeyword();
+    case Left:
+        return leftKeyword();
+    case Right:
+        return rightKeyword();
     default:
         ASSERT_NOT_REACHED();
         return emptyString();
@@ -456,6 +476,10 @@ void TextTrackCue::setAlign(const String& value, ExceptionState& es)
         alignment = Middle;
     else if (value == endKeyword())
         alignment = End;
+    else if (value == leftKeyword())
+        alignment = Left;
+    else if (value == rightKeyword())
+        alignment = Right;
     else
         es.throwUninformativeAndGenericDOMException(SyntaxError);
 
@@ -650,6 +674,10 @@ void TextTrackCue::determineTextDirection()
 
 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();
 
@@ -665,13 +693,15 @@ void TextTrackCue::calculateDisplayParameters()
     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);
@@ -695,28 +725,32 @@ void TextTrackCue::calculateDisplayParameters()
                 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:
@@ -732,10 +766,6 @@ void TextTrackCue::calculateDisplayParameters()
 
     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)
@@ -1094,6 +1124,14 @@ void TextTrackCue::setCueSettings(const String& input)
             // 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)