This is a fifth patch for the introduction of the new Chromium video controls, master bug at https://bugs.webkit.org/show_bug.cgi?id=84672 . This changes the format of the time digits that are displayed. They are different for media files of different duration. For non-autoplaying videos, the duration is shown before playback is started.
Created attachment 146727 [details] Full patch for layout tests
Created attachment 146728 [details] Use this for review This is the difference to the previous patch. It will not pass the test - refer to the full patch for that.
Created attachment 146774 [details] Full patch for layout tests - rebased to TOT
Comment on attachment 146728 [details] Use this for review Still the correct diff.
Comment on attachment 146774 [details] Full patch for layout tests - rebased to TOT Attachment 146774 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12939268
Created attachment 146840 [details] Full patch for layout tests - with skia fixes from test
Created attachment 146842 [details] Review this, including the fixes for skia. Review this, including the fixes for skia.
Comment on attachment 146842 [details] Review this, including the fixes for skia. View in context: https://bugs.webkit.org/attachment.cgi?id=146842&action=review r=me with the suggested changes. > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:357 > + int seconds = (int)fabsf(time); WebKit C++ code should use C++-style casts rather than C-style cast > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:363 > + int durationSecs = (int)fabsf(duration); Ditto.
Created attachment 147032 [details] full patch with int cast fix
(In reply to comment #8) > (From update of attachment 146842 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=146842&action=review > > r=me with the suggested changes. > > > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:357 > > + int seconds = (int)fabsf(time); > > WebKit C++ code should use C++-style casts rather than C-style cast > > > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:363 > > + int durationSecs = (int)fabsf(duration); > > Ditto. Done.
Comment on attachment 147032 [details] full patch with int cast fix Attachment 147032 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12941574
Created attachment 147444 [details] patch for cq
Comment on attachment 147444 [details] patch for cq unfortunately introduced a bug in skia - gotta re-upload
Created attachment 147464 [details] patch for cq
Comment on attachment 147464 [details] patch for cq Clearing flags on attachment: 147464 Committed r120322: <http://trac.webkit.org/changeset/120322>
All reviewed patches have been landed. Closing bug.
Comment on attachment 147464 [details] patch for cq View in context: https://bugs.webkit.org/attachment.cgi?id=147464&action=review > Source/WebCore/rendering/RenderThemeChromiumMac.mm:239 > + return RenderThemeChromiumMac::formatMediaControlsRemainingTime(currentTime, duration); Do you mean RenderMediaControlsChromium::formatMediaControlsRemainingTime() here? As is, this is an infinite recursion, right? (Is this not covered by tests?)
(In reply to comment #17) > (From update of attachment 147464 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147464&action=review > > > Source/WebCore/rendering/RenderThemeChromiumMac.mm:239 > > + return RenderThemeChromiumMac::formatMediaControlsRemainingTime(currentTime, duration); > > Do you mean RenderMediaControlsChromium::formatMediaControlsRemainingTime() here? As is, this is an infinite recursion, right? (Is this not covered by tests?) Right, nice find. This was in preparation for a feature where we would toggle between current time and remaining time, but wasn't ever used. Thanks for taking care of it.