Disallow non-flexbox child renderers for RenderMedia

Source/core/rendering/RenderMedia.cpp

Index: Source/core/rendering/RenderMedia.cpp
diff --git a/Source/core/rendering/RenderMedia.cpp b/Source/core/rendering/RenderMedia.cpp
index 08994661a9b5a82362dc2eb6190a23eae6250b35..9be96225cded5b44e413657fd4a37d8fe58c7c1e 100644
--- a/Source/core/rendering/RenderMedia.cpp
+++ b/Source/core/rendering/RenderMedia.cpp
@@ -77,6 +77,16 @@ void RenderMedia::layout()
     clearNeedsLayout();
 }
 
+bool RenderMedia::isChildAllowed(RenderObject* child, RenderStyle*) 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.

[Julien - ping for review, 2014/10/29 18:42:15]

I really think this is peppering over the fact that we shouldn't allow people to
use this pseudo. I understand the need for a short-term solution in the wake of
its (potential) popularity but we should deprecate / use-count them as this is
engineering effort wasted on a feature that shouldn't be exposed anyway :(

[philipj_slow, 2014/10/29 18:54:04]

I agree, but failed to convince the API OWNERS. I can work on a CL to add
UseCounters for non-standard pseudo-classes and pseudo-elements if you'll review
them :)

[philipj_slow, 2014/10/29 21:24:24]

To be clear, that cannot block fixing the bug itself.

[Julien - ping for review, 2014/10/29 21:27:12]

I am definitely supportive of more data on these! Sunsetting those proprietary
pseudos needs to happen and this is the best way forward for now.

+    return child->isFlexibleBox();

[Julien - ping for review, 2014/10/29 18:42:15]

How about display: inline-flex? I think Elliott's suggestion is a lot better
here:

::webkit-media-controls { display: flex !important; }

Authors can still style other properties differently but we ensure that we get
the proper child.

[philipj_slow, 2014/10/29 18:54:04]

That would break pages that do ::webkit-media-controls { display: none; } to
hide the controls in fullscreen (they get added even if there's no controls
attribute) which is exactly what
http://css-tricks.com/custom-controls-in-html5-video-full-screen/ suggests. This
C++ fix only came about due to conservatism about stuff like that, so...

About inline-flex, that isn't what mediaControls.css uses, is it plausible that
pages could use it in some meaningful way? It seems far fetched to me, but if
you disagree I can expand the list of allowed child renders.

[Julien - ping for review, 2014/10/29 21:27:12]

OK, I thought we could get away with forcing display but you convinced me it
isn't good enough.
Your comment seems to imply that having an inline descendant (isInline()) is the
issue and inline-flex is definitely inline. If your issue is with having
something that's not a RenderFlexibleBox then you can discard my comment.

I don't think inline-flex would give us much too and this is security sensitive
so I wouldn't mind being on the conservative side.

Either way, we should probably document through testing what's allowed or not.

[philipj_slow, 2014/10/29 21:40:37]

Truthfully, I'm not 100% confident that I've correctly diagnosed the root cause.
In discussion with Rune and Morten the fact that we have an inline as a child of
replaced content is what stood out as unusual, and they guessed that some
special handling was probably in place to make block content work that just
isn't there for inlines.
Would a test case for every supported display value be sufficient, or are you
mostly worried about inline-flex? I just assumed that inline-flex would result
in something other than a RenderFlexibleBox, but have done no testing.

+}

[Julien - ping for review, 2014/10/29 21:27:12]

I also forgot: RenderButton is a flexible box so it would pass this check. Do we
want to allow -webkit-appearance: button to work too?

[philipj_slow, 2014/10/29 21:40:37]

Ugh, as long as it doesn't crash I don't think it matters. I guess you want a
test case to prove it doesn't crash :)

+
 void RenderMedia::paintReplaced(PaintInfo&, const LayoutPoint&)
 {
 }