Convert media controls from DeprecatedFlexibleBox to FlexibleBox

• Component: New Bugs
• Status: RESOLVED FIXED

[Christian Biesinger]

Convert media controls from DeprecatedFlexibleBox to FlexibleBox

[Christian Biesinger]

Created attachment 188232 [details]
Patch

[Christian Biesinger]

I need to re-baseline (or add to TestExpectations) all the non-chromium platforms except mac. I also didn't visually verify those platforms.

Other than that, this should be ready.

[Christian Biesinger]

Created attachment 188247 [details]
Patch

[Christian Biesinger]

FYI the mac-wk2 media/media-captions.html crash happens even without my patch for me. trying to dereference a null track list.

[Ojan Vafai]

Comment on attachment 188247 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188247&action=review

> Source/WebCore/ChangeLog:21
> +        (::-webkit-media-controls):
> +        (audio::-webkit-media-controls-panel, video::-webkit-media-controls-panel):
> +        (audio::-webkit-media-controls-mute-button, video::-webkit-media-controls-mute-button):
> +        (audio::-webkit-media-controls-play-button, video::-webkit-media-controls-play-button):
> +        (audio::-webkit-media-controls-timeline-container, video::-webkit-media-controls-timeline-container):
> +        (audio::-webkit-media-controls-timeline, video::-webkit-media-controls-timeline):
> +        (audio::-webkit-media-controls-seek-back-button, video::-webkit-media-controls-seek-back-button):
> +        (audio::-webkit-media-controls-seek-forward-button, video::-webkit-media-controls-seek-forward-button):
> +        (audio::-webkit-media-controls-fullscreen-button, video::-webkit-media-controls-fullscreen-button):
> +        (audio::-webkit-media-controls-toggle-closed-captions-button, video::-webkit-media-controls-toggle-closed-captions-button):
> +        (video::-webkit-media-text-track-container):

In situations like this you can delete all this junk and just leave the list of files.

> LayoutTests/ChangeLog:8
> +        * media/media-document-audio-repaint-expected.txt:

In cases like this where there's a long list of files you can just delete the list of files, leaving only the ones that have extra explanatory comments.

Also, it'd be good to have a description of what changed in these expected results. Off the top of my head:
-Names of objects in the render tree dump change.
-Some widths of items are slightly different due to new flexbox and old flexbox shrinking over-constrained flex-items using a slightly different algorithm. Old flexbox shrinks all flex-items evenly. New flexbox shrinks them in proportion to the size of the flex item.
-LayoutTests/platform/chromium-linux/media/video-zoom-controls-expected.png looks strange. It has no play button in the new result. Both the old and new behavior look wrong though. Do you know what's going on there? It at least deserves an explanation in the ChangeLog.

[Victor Carbune]

The new baselines for the controls when the CC button is present are wrong,
but this shouldn't be related to your patch at all.

I've filed this bug to track the issue:
https://bugs.webkit.org/show_bug.cgi?id=109871

[Christian Biesinger]

Comment on attachment 188247 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188247&action=review

>> LayoutTests/ChangeLog:8
>> +        * media/media-document-audio-repaint-expected.txt:
> 
> In cases like this where there's a long list of files you can just delete the list of files, leaving only the ones that have extra explanatory comments.
> 
> Also, it'd be good to have a description of what changed in these expected results. Off the top of my head:
> -Names of objects in the render tree dump change.
> -Some widths of items are slightly different due to new flexbox and old flexbox shrinking over-constrained flex-items using a slightly different algorithm. Old flexbox shrinks all flex-items evenly. New flexbox shrinks them in proportion to the size of the flex item.
> -LayoutTests/platform/chromium-linux/media/video-zoom-controls-expected.png looks strange. It has no play button in the new result. Both the old and new behavior look wrong though. Do you know what's going on there? It at least deserves an explanation in the ChangeLog.

Turns out this can be fixed using justify-content: flex-start;. I'll make that change (and the other changes you requested)

[Christian Biesinger]

Created attachment 188453 [details]
Patch

[Ojan Vafai]

Comment on attachment 188453 [details]
Patch

Yay!

[WebKit Review Bot]

Comment on attachment 188453 [details]
Patch

Clearing flags on attachment: 188453

Committed r142947: <http://trac.webkit.org/changeset/142947>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Andrew Wilson]

I'm updating expectations from this change - looks like the resulting dialog does not have the [CC] closed caption UI any more. Just wanted to make sure this was expected, because I'm not embedding that in the expectations.

[Andrew Wilson]

(In reply to comment #12)
>I'm not embedding that in the expectations.

I meant "I am *NOW* embedding that in the expectations".

[Christian Biesinger]

Andrew: See comment 6 from Victor and the bug that's referenced there

[Andrew Wilson]

OK, I'll leave those incorrect expectations in the tree then, assuming you'll update them after fixing 109871.

[Simon Fraser (smfr)]

This broke controls sizing on http://m.imore.com/imore-show-338-pre-post-pc

[Christian Biesinger]

That's on the mac port? Looking...