Bug 208442

Summary:

[GTK][WPE] Fix current time and duration formatting in media controls

Product:

WebKit

Reporter:

Carlos Garcia Campos <cgarcia>

Component:

WebKitGTK

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bugs-noreply, calvaris, eric.carlson, ews-watchlist, glenn, jer.noble, joepeck, philipj, sergio

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	calvaris: review+, calvaris: commit-queue-

Description Carlos Garcia Campos 2020-03-02 04:20:52 PST

We are always using mm:ss for current time and duration. We should use different amount of digits depending on the duration:

m:ss <- duration < 10 minutes
mm:ss <- duration < 1 hour
h:mm:ss <- duration < 10 hours
hh:mm:ss <- duration >= 10 hours

Comment 1 Carlos Garcia Campos 2020-03-02 04:25:47 PST

Created attachment 392128 [details]
Patch

Comment 2 Xabier Rodríguez Calvar 2020-03-02 07:24:27 PST

Comment on attachment 392128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392128&action=review

> Source/WebCore/Modules/mediacontrols/mediaControlsAdwaita.js:411
> +        else if (duration < 10 * 60) /* Ten minutes */
> +            this.timeDigitsCount = 3;

I personally dislike having this on a time display. I would strongly recommend you remove this or run it thru more people who can give an opinion on this.

Comment 3 Carlos Garcia Campos 2020-03-03 01:12:06 PST

(In reply to Xabier Rodríguez Calvar from comment #2)
> Comment on attachment 392128 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=392128&action=review
> 
> > Source/WebCore/Modules/mediacontrols/mediaControlsAdwaita.js:411
> > +        else if (duration < 10 * 60) /* Ten minutes */
> > +            this.timeDigitsCount = 3;
> 
> I personally dislike having this on a time display. I would strongly
> recommend you remove this or run it thru more people who can give an opinion
> on this.

What do you mean? You don't like m:ss and h:mm:ss?

Comment 4 Xabier Rodríguez Calvar 2020-03-03 03:48:07 PST

(In reply to Carlos Garcia Campos from comment #3)
> (In reply to Xabier Rodríguez Calvar from comment #2)
> > Comment on attachment 392128 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=392128&action=review
> > 
> > > Source/WebCore/Modules/mediacontrols/mediaControlsAdwaita.js:411
> > > +        else if (duration < 10 * 60) /* Ten minutes */
> > > +            this.timeDigitsCount = 3;
> > 
> > I personally dislike having this on a time display. I would strongly
> > recommend you remove this or run it thru more people who can give an opinion
> > on this.
> 
> What do you mean? You don't like m:ss and h:mm:ss?

I don't like m:ss but I don't mind h:mm:ss. That was the design choice and I don't see a reason why it should not be respected now as well.

Comment 5 Carlos Garcia Campos 2020-03-03 05:06:33 PST

(In reply to Xabier Rodríguez Calvar from comment #4)
> (In reply to Carlos Garcia Campos from comment #3)
> > (In reply to Xabier Rodríguez Calvar from comment #2)
> > > Comment on attachment 392128 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=392128&action=review
> > > 
> > > > Source/WebCore/Modules/mediacontrols/mediaControlsAdwaita.js:411
> > > > +        else if (duration < 10 * 60) /* Ten minutes */
> > > > +            this.timeDigitsCount = 3;
> > > 
> > > I personally dislike having this on a time display. I would strongly
> > > recommend you remove this or run it thru more people who can give an opinion
> > > on this.
> > 
> > What do you mean? You don't like m:ss and h:mm:ss?
> 
> I don't like m:ss but I don't mind h:mm:ss. That was the design choice and I
> don't see a reason why it should not be respected now as well.

For consistency with other browsers, GNOME and some other popular video players.

Comment 6 Carlos Garcia Campos 2020-03-03 07:28:27 PST

Committed r257778: <https://trac.webkit.org/changeset/257778>