Implement <video controls> dodging for text track layout

Implement <video controls> dodging for text track layout

This is a follow-up to "Separate the text track container from the media
controls": https://codereview.chromium.org/949203002/

Previously, we got this behavior for free because the text track
container was part of the media controls.

BUG=448795
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192281

[philipj_slow, 2015-03-17 09:10:32]

philipj@opera.com changed reviewers:
+ fs@opera.com

[philipj_slow, 2015-03-17 09:10:32]

This kind of works, but look how naughty I had to be, using MediaControls inside
layout and such.

Another thing I realized after testing this is that we can only really implement
the behavior where the cues move back when the controls are hidden, which I'm
not sure is really the best thing. It also isn't what the spec suggests,
although I can of course change that if we think it's stupid.

WDYT?

[philipj_slow, 2015-03-17 09:12:34]

After putting it all together I think one might just as well have LayoutVTTCue
go look for the MediaControls and get the bounding rect,
LayoutTextTrackContainer seems like a pointless optimization in hindsight.

[philipj_slow, 2015-03-17 09:18:44]

Spec: "If the last time these rules were run, the user agent was not exposing a
user interface for video, but now it is, optionally let reset be true.
Otherwise, let reset be false."

There's no provision for moving cues around when controls are hidden. We did
move them in Presto, however. It's not terribly important, but not moving them
feels slightly nicer.

[fs, 2015-03-17 10:27:56]

On 2015/03/17 09:12:34, philipj_UTC7 wrote:
> After putting it all together I think one might just as well have LayoutVTTCue
> go look for the MediaControls and get the bounding rect,
> LayoutTextTrackContainer seems like a pointless optimization in hindsight.

I've considered various ways of eliminating one or the other (to reduce the
amount of "special" code). Given this code, it seems like LayoutVTTCue might be
the preferred one to eliminate. (I've not been sure about how that scheme would
work together with the positioned boxes.)

Regardless though, the dance in LayoutMedia::layout will still be required, and
maybe just accessing the controls from LayoutVTTCue could be a fine solution for
now.

https://codereview.chromium.org/1018593004/diff/1/Source/core/layout/LayoutMe...
File Source/core/layout/LayoutMedia.cpp (right):

https://codereview.chromium.org/1018593004/diff/1/Source/core/layout/LayoutMe...
Source/core/layout/LayoutMedia.cpp:70: for (LayoutObject* child =
m_children.lastChild(); child; child = child->previousSibling()) {
I think you could also just drop the for-loop entirely and do something like:

RELEASE_ASSERT(onlyTwoChildren(m_children));
LayoutObject* controls = m_children.lastChild();
LayoutObject* textTrackContainer = nullptr;
if (m_children.lastChild() != m_children.firstChild())
    textTrackContainer = m_children.firstChild();

// Layout one and then the other.
...

Or some variant thereof.

[philipj_slow, 2015-03-17 15:43:49]

take TextTrackContainer out of the loop

[philipj_slow, 2015-03-17 16:15:43]

OK, with TextTrackContainer out of the way this is less unsightly. Testing this
I notice that it's actually the cueTimeline().updateActiveCues(currentTime())
that's triggering the cue movement not the setInlineStyleProperty in
MediaControlElement::hide(). This isn't really strange, as only parents are
invalidated.

This makes it not in sync, where no animation frame with visible overlap is the
goal. Needs more tinkering.

[fs, 2015-03-17 16:46:48]

https://codereview.chromium.org/1018593004/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutVTTCue.cpp (right):

https://codereview.chromium.org/1018593004/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutVTTCue.cpp:33: #include
"core/layout/LayoutTextTrackContainer.h"
No longer needed?

https://codereview.chromium.org/1018593004/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutVTTCue.cpp:46: SnapToLinesLayouter(LayoutVTTCue&
cueBox, IntRect& controlsRect, float linePosition)
const IntRect& ?

https://codereview.chromium.org/1018593004/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutVTTCue.cpp:131: ASSERT(!m_cueBox.nextSibling());
What is being asserted here?

[philipj_slow, 2015-03-17 17:44:11]

cleanup

[philipj_slow, 2015-03-17 17:44:55]

https://codereview.chromium.org/1018593004/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutVTTCue.cpp (right):

https://codereview.chromium.org/1018593004/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutVTTCue.cpp:33: #include
"core/layout/LayoutTextTrackContainer.h"
On 2015/03/17 16:46:48, fs wrote:
> No longer needed?

Done.

https://codereview.chromium.org/1018593004/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutVTTCue.cpp:46: SnapToLinesLayouter(LayoutVTTCue&
cueBox, IntRect& controlsRect, float linePosition)
On 2015/03/17 16:46:48, fs wrote:
> const IntRect& ?

Done.

https://codereview.chromium.org/1018593004/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutVTTCue.cpp:131: ASSERT(!m_cueBox.nextSibling());
On 2015/03/17 16:46:48, fs wrote:
> What is being asserted here?

Sorry, too eager and untested, this actually fails on
media/track/track-cue-rendering-horizontal.html

I imagined that the reason the above loop only considers the previous siblings
is that it has no next sibling, but that isn't true. Do you know why it makes
sense to only consider the previous siblings?

Reverted in any case.

[fs, 2015-03-17 17:53:30]

https://codereview.chromium.org/1018593004/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutVTTCue.cpp (right):

https://codereview.chromium.org/1018593004/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutVTTCue.cpp:131: ASSERT(!m_cueBox.nextSibling());
On 2015/03/17 17:44:54, philipj_UTC7 wrote:
> On 2015/03/17 16:46:48, fs wrote:
> > What is being asserted here?
> 
> Sorry, too eager and untested, this actually fails on
> media/track/track-cue-rendering-horizontal.html
> 
> I imagined that the reason the above loop only considers the previous siblings
> is that it has no next sibling, but that isn't true. Do you know why it makes
> sense to only consider the previous siblings?

Well, the cues should dodge each other in certain order - this happens to be
(or, "should be", rather) the order in which the cues were inserted into the
container (== the "DOM order"/logical order). So when the next cue becomes
active, it's first layouted, and then has its position adjusted based on all its
predecessors (which will not be relayouted.)

[philipj_slow, 2015-03-18 08:19:45]

https://codereview.chromium.org/1018593004/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutVTTCue.cpp (right):

https://codereview.chromium.org/1018593004/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutVTTCue.cpp:131: ASSERT(!m_cueBox.nextSibling());
On 2015/03/17 17:53:30, fs wrote:
> On 2015/03/17 17:44:54, philipj_UTC7 wrote:
> > On 2015/03/17 16:46:48, fs wrote:
> > > What is being asserted here?
> > 
> > Sorry, too eager and untested, this actually fails on
> > media/track/track-cue-rendering-horizontal.html
> > 
> > I imagined that the reason the above loop only considers the previous
siblings
> > is that it has no next sibling, but that isn't true. Do you know why it
makes
> > sense to only consider the previous siblings?
> 
> Well, the cues should dodge each other in certain order - this happens to be
> (or, "should be", rather) the order in which the cues were inserted into the
> container (== the "DOM order"/logical order). So when the next cue becomes
> active, it's first layouted, and then has its position adjusted based on all
its
> predecessors (which will not be relayouted.)

Hmm, right. In my imagination we would reach this point before the following cue
is added, but that isn't so. Order aside, if no cue overlaps any previous cue,
then no cues can overlap.

[philipj_slow, 2015-03-18 09:59:52]

I make an un-delightful discovery about how marking past and future nodes also
causes the layout to be invalidated continuously:
https://code.google.com/p/chromium/issues/detail?id=322434#c6

[fs, 2015-03-18 10:05:01]

On 2015/03/18 09:59:52, philipj_UTC7 wrote:
> I make an un-delightful discovery about how marking past and future nodes also
> causes the layout to be invalidated continuously:
> https://code.google.com/p/chromium/issues/detail?id=322434#c6

Yeah, I've noticed that as well, and was intending to make that better. It's
ended up beside the plate at the moment though...

[philipj_slow, 2015-03-20 05:50:38]

ready for review

[philipj_slow, 2015-03-20 05:58:19]

PTAL, with the constant invalidation gone it seems to just work.

https://codereview.chromium.org/1018593004/diff/60001/Source/core/html/shadow...
File Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/1018593004/diff/60001/Source/core/html/shadow...
Source/core/html/shadow/MediaControls.cpp:204: m_panel->show();
This change of order isn't really needed, I just thought it cleaner if the
callback would always happen after the inline style change.

https://codereview.chromium.org/1018593004/diff/60001/Source/core/layout/Layo...
File Source/core/layout/LayoutMedia.cpp (right):

https://codereview.chromium.org/1018593004/diff/60001/Source/core/layout/Layo...
Source/core/layout/LayoutMedia.cpp:65: bool seenTextTrackContainer = false;
What do you think, way overkill? If you prefer I could take your advice and
first find the children, then layout them in the wanted order. It'd probably
still be a for-loop though, when I tried just peeking at first firstChild and
lastChild it wasn't really make it clearer.

[fs, 2015-03-20 09:58:54]

lgtm

https://codereview.chromium.org/1018593004/diff/60001/Source/core/html/shadow...
File Source/core/html/shadow/MediaControlElements.cpp (right):

https://codereview.chromium.org/1018593004/diff/60001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:171: if (m_isDisplayed &&
m_opaque) {
Nit: Unneeded {}

https://codereview.chromium.org/1018593004/diff/60001/Source/core/layout/Layo...
File Source/core/layout/LayoutMedia.cpp (right):

https://codereview.chromium.org/1018593004/diff/60001/Source/core/layout/Layo...
Source/core/layout/LayoutMedia.cpp:65: bool seenTextTrackContainer = false;
On 2015/03/20 05:58:19, philipj_UTC7 wrote:
> What do you think, way overkill? If you prefer I could take your advice and
> first find the children, then layout them in the wanted order. It'd probably
> still be a for-loop though, when I tried just peeking at first firstChild and
> lastChild it wasn't really make it clearer.

I don't mind that much, but it does give the impression of being more generic
than it actually is. All the ifdeffing is kind of ugly though...

[philipj_slow, 2015-03-20 16:46:33]

https://codereview.chromium.org/1018593004/diff/60001/Source/core/html/shadow...
File Source/core/html/shadow/MediaControlElements.cpp (right):

https://codereview.chromium.org/1018593004/diff/60001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:171: if (m_isDisplayed &&
m_opaque) {
On 2015/03/20 09:58:54, fs wrote:
> Nit: Unneeded {}

Oops.

https://codereview.chromium.org/1018593004/diff/60001/Source/core/layout/Layo...
File Source/core/layout/LayoutMedia.cpp (right):

https://codereview.chromium.org/1018593004/diff/60001/Source/core/layout/Layo...
Source/core/layout/LayoutMedia.cpp:65: bool seenTextTrackContainer = false;
On 2015/03/20 09:58:54, fs wrote:
> On 2015/03/20 05:58:19, philipj_UTC7 wrote:
> > What do you think, way overkill? If you prefer I could take your advice and
> > first find the children, then layout them in the wanted order. It'd probably
> > still be a for-loop though, when I tried just peeking at first firstChild
and
> > lastChild it wasn't really make it clearer.
> 
> I don't mind that much, but it does give the impression of being more generic
> than it actually is. All the ifdeffing is kind of ugly though...

Yeah, it's ugly alright. I've tinkered a bit on a follow-up and will send that
out, but leave it like this in this CL.

[philipj_slow, 2015-03-20 16:46:59]

braces

[philipj_slow, 2015-03-20 17:00:10]

The CQ bit was checked by philipj@opera.com

[philipj_slow, 2015-03-20 17:00:10]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com
Link to the patchset: https://codereview.chromium.org/1018593004/#ps80001
(title: "braces")

[commit-bot: I haz the power, 2015-03-20 17:00:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1018593004/80001

[commit-bot: I haz the power, 2015-03-20 19:46:13]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192281