Bug 88297

Summary: Change the styling of the Chromium video controls
Product: WebKit Reporter: Silvia Pfeiffer <silviapf>
Component: MediaAssignee: Silvia Pfeiffer <silviapf>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, eric.carlson, eric, feature-media-reviews, macpherson, menard, ojan, tkent, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 87683, 87835    
Bug Blocks: 84672, 88623, 88724, 88743, 88818, 89050    
Attachments:
Description Flags
Full patch for layout tests
none
Review this for the differences to the second patch
none
Patch for cq none

Silvia Pfeiffer
Reported 2012-06-04 23:40:40 PDT
This is a third patch for the introduction of the new Chromium video controls, master bug at https://bugs.webkit.org/show_bug.cgi?id=84672 . This does the actual visual update. As such, it includes the change of the CSS using the new flexbox and new styles, the removal of the now not needed controls background div, and the introduction of the new images for the buttons. The visual update of the range sliders will be in a separate patch. Not to land separately and to land after 87835.
Attachments
Full patch for layout tests (50.36 KB, patch)
2012-06-06 00:53 PDT, Silvia Pfeiffer 		no flags
DetailsFormatted DiffDiff
Review this for the differences to the second patch (23.36 KB, patch)
2012-06-06 00:56 PDT, Silvia Pfeiffer 		no flags
DetailsFormatted DiffDiff
Patch for cq (24.09 KB, patch)
2012-06-12 16:14 PDT, Silvia Pfeiffer 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Silvia Pfeiffer
Comment 1 2012-06-06 00:53:46 PDT
Created attachment 145954 [details] Full patch for layout tests
Silvia Pfeiffer
Comment 2 2012-06-06 00:56:45 PDT
Created attachment 145956 [details] Review this for the differences to the second patch As it turns out, there are still too many changes to the new flexbox model, so I've gone back to the old one, which is also more consistent with the other WebKit styles for video controls shadow DOM elements.
Eric Carlson
Comment 3 2012-06-06 09:55:22 PDT
Comment on attachment 145956 [details] Review this for the differences to the second patch View in context: https://bugs.webkit.org/attachment.cgi?id=145956&action=review > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:278 > +const int mediaSliderThumbWidth = 32; > +const int mediaSliderThumbHeight = 24; Nit: you might want to add "Timeline" to these names to match the pattern used for the other names.
Silvia Pfeiffer
Comment 4 2012-06-06 22:47:56 PDT
(In reply to comment #3) > (From update of attachment 145956 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145956&action=review > > > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:278 > > +const int mediaSliderThumbWidth = 32; > > +const int mediaSliderThumbHeight = 24; > > Nit: you might want to add "Timeline" to these names to match the pattern used for the other names. Thanks, will do when uploading the patch for landing rebased to the current codebase.
Silvia Pfeiffer
Comment 5 2012-06-12 16:06:17 PDT
(In reply to comment #3) > (From update of attachment 145956 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145956&action=review > > > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:278 > > +const int mediaSliderThumbWidth = 32; > > +const int mediaSliderThumbHeight = 24; > > Nit: you might want to add "Timeline" to these names to match the pattern used for the other names. Decided not to do this for consistency, since all constants and variable names in the media controls system for the timeline/transport are called "mediaslider".
Silvia Pfeiffer
Comment 6 2012-06-12 16:14:11 PDT
Created attachment 147185 [details] Patch for cq
WebKit Review Bot
Comment 7 2012-06-13 01:19:37 PDT
Comment on attachment 147185 [details] Patch for cq Clearing flags on attachment: 147185 Committed r120171: <http://trac.webkit.org/changeset/120171>
WebKit Review Bot
Comment 8 2012-06-13 01:19:42 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.