Separate VTTCue from VTTCueBox and LayoutVTTCue

Source/core/html/track/vtt/VTTCue.cpp

Index: Source/core/html/track/vtt/VTTCue.cpp
diff --git a/Source/core/html/track/vtt/VTTCue.cpp b/Source/core/html/track/vtt/VTTCue.cpp
index 0a273bc84e4205bb80cd4b0201837f74c97dfa15..3ff5ee22c8c979a2bd934b8b508ea1a66ed55354 100644
--- a/Source/core/html/track/vtt/VTTCue.cpp
+++ b/Source/core/html/track/vtt/VTTCue.cpp
@@ -145,22 +145,18 @@ static void setInlineStylePropertyIfNotEmpty(Element& element,
         element.setInlineStyleProperty(propertyID, value);
 }
 
-VTTCueBox::VTTCueBox(Document& document, VTTCue* cue)
+VTTCueBox::VTTCueBox(Document& document)
     : HTMLDivElement(document)
-    , m_cue(cue)
+    , m_snapToLinesPosition(std::numeric_limits<float>::quiet_NaN())
 {
     setShadowPseudoId(AtomicString("-webkit-media-text-track-display", AtomicString::ConstructFromLiteral));
 }
 
 void VTTCueBox::applyCSSProperties(const VTTDisplayParameters& displayParameters)
 {
-    // FIXME: Apply all the initial CSS positioning properties. http://wkb.ug/79916
-    if (!m_cue->regionId().isEmpty()) {
-        setInlineStyleProperty(CSSPropertyPosition, CSSValueRelative);
-        return;
-    }
+    // http://dev.w3.org/html5/webvtt/#applying-css-properties-to-webvtt-node-objects
 
-    // 3.5.1 On the (root) List of WebVTT Node Objects:
+    // Initialize the (root) list of WebVTT Node Objects with the following CSS settings:
 
     // the 'position' property must be set to 'absolute'
     setInlineStyleProperty(CSSPropertyPosition, CSSValueAbsolute);
@@ -183,7 +179,7 @@ void VTTCueBox::applyCSSProperties(const VTTDisplayParameters& displayParameters
     setInlineStyleProperty(CSSPropertyLeft, position.x(), CSSPrimitiveValue::CSS_PERCENTAGE);
 
     // the 'width' property must be set to width, and the 'height' property  must be set to height
-    if (m_cue->vertical() == horizontalKeyword()) {
+    if (displayParameters.writingMode == CSSValueHorizontalTb) {
         setInlineStyleProperty(CSSPropertyWidth, displayParameters.size, CSSPrimitiveValue::CSS_PERCENTAGE);
         setInlineStyleProperty(CSSPropertyHeight, CSSValueAuto);
     } else {
@@ -195,9 +191,12 @@ void VTTCueBox::applyCSSProperties(const VTTDisplayParameters& displayParameters
     // 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:
-    setInlineStyleProperty(CSSPropertyTextAlign, displayAlignmentMap[m_cue->cueAlignment()]);
+    setInlineStyleProperty(CSSPropertyTextAlign, displayParameters.textAlign);
 
-    if (!m_cue->snapToLines()) {
+    // TODO(philipj): The position adjustment for non-snap-to-lines cues has
+    // been removed from the spec:
+    // https://www.w3.org/Bugs/Public/show_bug.cgi?id=19178
+    if (std::isnan(displayParameters.snapToLinesPosition)) {
         // 10.13.1 Set up x and y:
         // Note: x and y are set through the CSS left and top above.
 
@@ -213,17 +212,21 @@ void VTTCueBox::applyCSSProperties(const VTTDisplayParameters& displayParameters
 
         setInlineStyleProperty(CSSPropertyWhiteSpace, CSSValuePre);
     }
-}
 
-LayoutObject* VTTCueBox::createLayoutObject(const ComputedStyle&)
-{
-    return new LayoutVTTCue(this);
+    // The snap-to-lines position is propagated to LayoutVTTCue.
+    m_snapToLinesPosition = displayParameters.snapToLinesPosition;
 }
 
-DEFINE_TRACE(VTTCueBox)
+LayoutObject* VTTCueBox::createLayoutObject(const ComputedStyle& style)
 {
-    visitor->trace(m_cue);
-    HTMLDivElement::trace(visitor);
+    // If WebVTT Regions are used, the regular WebVTT layout algorithm is no
+    // longer necessary, since cues having the region parameter set do not have
+    // any positioning parameters. Also, in this case, the regions themselves
+    // have positioning information.
+    if (style.position() == RelativePosition)
+        return HTMLDivElement::createLayoutObject(style);
+
+    return new LayoutVTTCue(this, m_snapToLinesPosition);
 }
 
 VTTCue::VTTCue(Document& document, double startTime, double endTime, const String& text)
@@ -243,19 +246,6 @@ VTTCue::VTTCue(Document& document, double startTime, double endTime, const Strin
     m_cueBackgroundBox->setShadowPseudoId(cueShadowPseudoId());
 }
 
-VTTCue::~VTTCue()

[sof, 2015/07/20 08:15:39]

It would be preferable to keep an empty destructor, to avoid synthesizing &
inlining a larger one.

[philipj_slow, 2015/07/20 08:34:16]

Done.

-{
-    // Using oilpan, if m_displayTree is in the document it will strongly keep
-    // the cue alive. Thus, if the cue is dead, either m_displayTree is not in
-    // the document or the entire document is dead too.
-#if !ENABLE(OILPAN)
-    // FIXME: This is scary, we should make the life cycle smarter so the destructor
-    // doesn't need to do DOM mutations.
-    if (m_displayTree)
-        m_displayTree->remove(ASSERT_NO_EXCEPTION);
-#endif
-}
-
 #ifndef NDEBUG
 String VTTCue::toString() const
 {
@@ -645,19 +635,26 @@ VTTCue::CueAlignment VTTCue::calculateComputedCueAlignment() const
 VTTDisplayParameters::VTTDisplayParameters()
     : size(std::numeric_limits<float>::quiet_NaN())
     , direction(CSSValueNone)
-    , writingMode(CSSValueNone) { }
+    , textAlign(CSSValueNone)
+    , writingMode(CSSValueNone)
+    , snapToLinesPosition(std::numeric_limits<float>::quiet_NaN()) { }
 
 VTTDisplayParameters VTTCue::calculateDisplayParameters() const
 {
     // http://dev.w3.org/html5/webvtt/#dfn-apply-webvtt-cue-settings
 
     VTTDisplayParameters displayParameters;
+
     // Steps 1 and 2.
     displayParameters.direction = determineTextDirection(m_vttNodeTree.get());
 
     if (displayParameters.direction == CSSValueRtl)
         UseCounter::count(document(), UseCounter::VTTCueRenderRtl);
 
+    // Note: The 'text-align' property is also determined here so that
+    // VTTCueBox::applyCSSProperties need not have access to a VTTCue.
+    displayParameters.textAlign = displayAlignmentMap[cueAlignment()];
+
     // 3. 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
@@ -747,6 +744,11 @@ VTTDisplayParameters VTTCue::calculateDisplayParameters() const
 
     // Step 9 not implemented (margin == 0).
 
+    // The snap-to-lines position is propagated to LayoutVTTCue.
+    displayParameters.snapToLinesPosition = m_snapToLines
+        ? computedLinePosition
+        : std::numeric_limits<float>::quiet_NaN();
+
     ASSERT(std::isfinite(displayParameters.size));
     ASSERT(displayParameters.direction != CSSValueNone);
     ASSERT(displayParameters.writingMode != CSSValueNone);
@@ -800,7 +802,7 @@ PassRefPtrWillBeRawPtr<VTTCueBox> VTTCue::getDisplayTree()
     ASSERT(track() && track()->isRendered() && isActive());
 
     if (!m_displayTree) {
-        m_displayTree = VTTCueBox::create(document(), this);
+        m_displayTree = VTTCueBox::create(document());
         m_displayTree->appendChild(m_cueBackgroundBox);
     }
 
@@ -819,8 +821,15 @@ PassRefPtrWillBeRawPtr<VTTCueBox> VTTCue::getDisplayTree()
     m_cueBackgroundBox->removeChildren();
     m_vttNodeTree->cloneChildNodes(m_cueBackgroundBox.get());
 
-    VTTDisplayParameters displayParameters = calculateDisplayParameters();
-    m_displayTree->applyCSSProperties(displayParameters);
+    // TODO(philipj): The region identifier may be non-empty without there being
+    // a corresponding region, in which case this VTTCueBox will be added
+    // directly to the text track container in updateDisplay().
+    if (regionId().isEmpty()) {

[fs, 2015/07/17 14:31:48]

Maybe you could add a "VTTRegion* region() { return track() &&
track()->regions() ? ... : nullptr; }" and use that (here and all the other
places that accesses the region. IIRC that's what the spec looks like now too
right?). Anyway, can be done later, since is equivalent to what's there already
IIUC.

[philipj_slow, 2015/07/20 07:33:30]

Yes, I changed the spec such that it should be possible to have an id->region
mapping while parsing and then after that simply let a VTTCue point to a
VTTRegion. I didn't want to change any behavior here, though.

+        VTTDisplayParameters displayParameters = calculateDisplayParameters();
+        m_displayTree->applyCSSProperties(displayParameters);
+    } else {
+        m_displayTree->setInlineStyleProperty(CSSPropertyPosition, CSSValueRelative);
+    }
 
     // Apply user override settings for text tracks
     applyUserOverrideCSSProperties();