Update the time display of Chromium media controls

• Component: Media
• Status: RESOLVED FIXED

[Silvia Pfeiffer]

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.

[Silvia Pfeiffer]

Created attachment 146727 [details]
Full patch for layout tests

[Silvia Pfeiffer]

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.

[Silvia Pfeiffer]

Created attachment 146774 [details]
Full patch for layout tests - rebased to TOT

[Silvia Pfeiffer]

Comment on attachment 146728 [details]
Use this for review

Still the correct diff.

[WebKit Review Bot]

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

[Silvia Pfeiffer]

Created attachment 146840 [details]
Full patch for layout tests - with skia fixes from test

[Silvia Pfeiffer]

Created attachment 146842 [details]
Review this, including the fixes for skia.

Review this, including the fixes for skia.

[Eric Carlson]

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.

[Silvia Pfeiffer]

Created attachment 147032 [details]
full patch with int cast fix

[Silvia Pfeiffer]

(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.

[WebKit Review Bot]

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

[Silvia Pfeiffer]

Created attachment 147444 [details]
patch for cq

[Silvia Pfeiffer]

Comment on attachment 147444 [details]
patch for cq

unfortunately introduced a bug in skia - gotta re-upload

[Silvia Pfeiffer]

Created attachment 147464 [details]
patch for cq

[WebKit Review Bot]

Comment on attachment 147464 [details]
patch for cq

Clearing flags on attachment: 147464

Committed r120322: <http://trac.webkit.org/changeset/120322>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Nico Weber]

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

[Silvia Pfeiffer]

(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.