Bug 109775

Created attachment 188232 [details] Patch

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.

Created attachment 188247 [details] Patch

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

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.

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 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)

Created attachment 188453 [details] Patch

Comment on attachment 188453 [details] Patch Yay!

Comment on attachment 188453 [details] Patch Clearing flags on attachment: 188453 Committed r142947: <http://trac.webkit.org/changeset/142947>

All reviewed patches have been landed. Closing bug.

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.

(In reply to comment #12) >I'm not embedding that in the expectations. I meant "I am *NOW* embedding that in the expectations".

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

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

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

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