Separate the text track container from the media controls

Source/core/layout/LayoutMedia.cpp

 /*
  * Copyright (C) 2007, 2008, 2009, 2010 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:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 35 matching lines @@
 {
     return toHTMLMediaElement(node());
 }
 
 void LayoutMedia::layout()
 {
     LayoutSize oldSize = contentBoxRect().size();
 
     LayoutImage::layout();
 
-    LayoutBox* controlsRenderer = toLayoutBox(m_children.firstChild());
-    if (!controlsRenderer)
-        return;
-
-    bool controlsNeedLayout = controlsRenderer->needsLayout();
     LayoutSize newSize = contentBoxRect().size();
-    if (newSize == oldSize && !controlsNeedLayout)
-        return;
 
     LayoutState state(*this, locationOffset());
 
-    controlsRenderer->setLocation(LayoutPoint(borderLeft(), borderTop()) + LayoutSize(paddingLeft(), paddingTop()));
-    controlsRenderer->style()->setHeight(Length(newSize.height(), Fixed));
-    controlsRenderer->style()->setWidth(Length(newSize.width(), Fixed));
-    controlsRenderer->forceLayout();
+    for (LayoutObject* child = m_children.firstChild(); child; child = child->nextSibling()) {

[philipj_slow, 2015/02/26 09:51:52]

Rune, does this make any sense? I couldn't find any other LayoutObjects
iterating its children like this which I have very low confidence that it's
correct.

[fs, 2015/02/26 12:30:51]

I believe RenderBlockFlow uses a while-loop (see
RenderBlockFlow::layoutBlockChildren). (Now, using a RenderBlockFlow in place of
LayoutMedia and a shadow "video layer" element might be nice, but feels like
that might be making it too difficult at this time...)

[rune, 2015/02/26 13:17:15]

I'm not too familiar with layout, but this looks like a custom layout method
that makes its children fill the content area of the replaced parent. It would
possibly be cleaner to have a single block child of the LayoutMedia in which the
media controls and text track container renderers would live, but handling
anonymous renderers adds to the complexity as well. I don't feel confident to
give you a definite answer about how it should work. I think having
Rendering/Layout children of replaced content is messy to begin with.

[philipj_slow, 2015/02/27 07:13:52]

Yeah, I'll leave this alone if it's not too terrible.

+        ASSERT(child->node()->isMediaControls() || child->node()->isTextTrackContainer());
+
+        if (newSize == oldSize && !child->needsLayout())
+            continue;
+
+        LayoutBox* layoutBox = toLayoutBox(child);
+        layoutBox->setLocation(LayoutPoint(borderLeft(), borderTop()) + LayoutSize(paddingLeft(), paddingTop()));

[philipj_slow, 2015/02/26 09:51:52]

Not sure if this could be done with width/height: inherit instead. Would that be
more elegant if it worked?

+        layoutBox->style()->setHeight(Length(newSize.height(), Fixed));
+        layoutBox->style()->setWidth(Length(newSize.width(), Fixed));
+        layoutBox->forceLayout();
+    }
+
     clearNeedsLayout();
 }
 
 bool LayoutMedia::isChildAllowed(LayoutObject* child, const LayoutStyle&) const
 {
-    // The only allowed child is the media controls. The user agent stylesheet
-    // (mediaControls.css) has ::-webkit-media-controls { display: flex; }. If
-    // author style sets display: inline we would get an inline renderer as a
-    // child of replaced content, which is not supposed to be possible. This
-    // check can be removed if ::-webkit-media-controls is made internal.
-    return child->isFlexibleBox();
+    // Two types of child layout objects are allowed: media controls
+    // and the text track container.
+
+    // The user agent stylesheet (mediaControls.css) has
+    // ::-webkit-media-controls { display: flex; }. If author style
+    // sets display: inline we would get an inline renderer as a child
+    // of replaced content, which is not supposed to be possible. This
+    // check can be removed if ::-webkit-media-controls is made
+    // internal.
+    if (child->node()->isMediaControls())

[fs, 2015/02/26 12:30:51]

Maybe assert child->node() here (in this method) as well.

[philipj_slow, 2015/02/27 07:13:52]

Done.

+        return child->isFlexibleBox();
+
+    // The text track container should only be added for video elements.
+    if (child->node()->isTextTrackContainer()) {
+        ASSERT(isVideo());
+        return true;
+    }
+
+    return false;
 }
 
 void LayoutMedia::paintReplaced(const PaintInfo&, const LayoutPoint&)
 {
 }
 
 } // namespace blink