Bug 109775 - Convert media controls from DeprecatedFlexibleBox to FlexibleBox
Summary: Convert media controls from DeprecatedFlexibleBox to FlexibleBox
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Christian Biesinger
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-13 17:15 PST by Christian Biesinger
Modified: 2013-02-26 12:05 PST (History)
15 users (show)

See Also:


Attachments
Patch (1.04 MB, patch)
2013-02-13 17:17 PST, Christian Biesinger
no flags Details | Formatted Diff | Diff
Patch (1.05 MB, patch)
2013-02-13 18:28 PST, Christian Biesinger
no flags Details | Formatted Diff | Diff
Patch (964.25 KB, patch)
2013-02-14 17:01 PST, Christian Biesinger
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Christian Biesinger 2013-02-13 17:15:34 PST
Convert media controls from DeprecatedFlexibleBox to FlexibleBox
Comment 1 Christian Biesinger 2013-02-13 17:17:34 PST
Created attachment 188232 [details]
Patch
Comment 2 Christian Biesinger 2013-02-13 17:26:12 PST
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.
Comment 3 Christian Biesinger 2013-02-13 18:28:13 PST
Created attachment 188247 [details]
Patch
Comment 4 Christian Biesinger 2013-02-14 13:59:56 PST
FYI the mac-wk2 media/media-captions.html crash happens even without my patch for me. trying to dereference a null track list.
Comment 5 Ojan Vafai 2013-02-14 15:01:44 PST
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.
Comment 6 Victor Carbune 2013-02-14 16:02:53 PST
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
Comment 7 Christian Biesinger 2013-02-14 16:47:27 PST
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)
Comment 8 Christian Biesinger 2013-02-14 17:01:51 PST
Created attachment 188453 [details]
Patch
Comment 9 Ojan Vafai 2013-02-14 17:06:28 PST
Comment on attachment 188453 [details]
Patch

Yay!
Comment 10 WebKit Review Bot 2013-02-14 17:41:08 PST
Comment on attachment 188453 [details]
Patch

Clearing flags on attachment: 188453

Committed r142947: <http://trac.webkit.org/changeset/142947>
Comment 11 WebKit Review Bot 2013-02-14 17:41:14 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Andrew Wilson 2013-02-15 05:10:37 PST
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.
Comment 13 Andrew Wilson 2013-02-15 06:54:49 PST
(In reply to comment #12)
>I'm not embedding that in the expectations.

I meant "I am *NOW* embedding that in the expectations".
Comment 14 Christian Biesinger 2013-02-15 11:36:45 PST
Andrew: See comment 6 from Victor and the bug that's referenced there
Comment 15 Andrew Wilson 2013-02-18 00:56:25 PST
OK, I'll leave those incorrect expectations in the tree then, assuming you'll update them after fixing 109871.
Comment 16 Simon Fraser (smfr) 2013-02-26 11:59:48 PST
This broke controls sizing on http://m.imore.com/imore-show-338-pre-post-pc
Comment 17 Christian Biesinger 2013-02-26 12:05:42 PST
That's on the mac port? Looking...