Bug 88724

Summary:

Update the time display of Chromium media controls

Product:

WebKit

Reporter:

Silvia Pfeiffer <silviapf>

Component:

Media

Assignee:

Silvia Pfeiffer <silviapf>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

annacc, cmarcelo, dglazkov, eric.carlson, eric, feature-media-reviews, macpherson, menard, scherkus, silviapfeiffer1, tkent, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

87683, 87835, 88297, 88623

Bug Blocks:

84672, 88743, 88818, 89050

Attachments:

Description	Flags
Full patch for layout tests	none
Use this for review	none
Full patch for layout tests - rebased to TOT	none
Full patch for layout tests - with skia fixes from test	none
Review this, including the fixes for skia.	none
full patch with int cast fix	none
patch for cq	none
patch for cq	none

Silvia Pfeiffer

Reported 2012-06-10 00:10:20 PDT

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.

Attachments
Full patch for layout tests (74.15 KB, patch) 2012-06-10 01:38 PDT, Silvia Pfeiffer	no flags	Details Formatted Diff Diff
Use this for review (15.87 KB, patch) 2012-06-10 01:50 PDT, Silvia Pfeiffer	no flags	Details Formatted Diff Diff
Full patch for layout tests - rebased to TOT (74.18 KB, patch) 2012-06-10 21:14 PDT, Silvia Pfeiffer	no flags	Details Formatted Diff Diff
Full patch for layout tests - with skia fixes from test (74.25 KB, patch) 2012-06-11 06:04 PDT, Silvia Pfeiffer	no flags	Details Formatted Diff Diff
Review this, including the fixes for skia. (15.93 KB, patch) 2012-06-11 06:11 PDT, Silvia Pfeiffer	no flags	Details Formatted Diff Diff
full patch with int cast fix (65.16 KB, patch) 2012-06-12 01:19 PDT, Silvia Pfeiffer	no flags	Details Formatted Diff Diff
patch for cq (16.68 KB, patch) 2012-06-13 16:54 PDT, Silvia Pfeiffer	no flags	Details Formatted Diff Diff
patch for cq (16.67 KB, patch) 2012-06-13 18:53 PDT, Silvia Pfeiffer	no flags	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Silvia Pfeiffer

Comment 1 2012-06-10 01:38:44 PDT

Created attachment 146727 [details] Full patch for layout tests

Silvia Pfeiffer

Comment 2 2012-06-10 01:50:42 PDT

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

Comment 3 2012-06-10 21:14:51 PDT

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

Silvia Pfeiffer

Comment 4 2012-06-10 21:15:25 PDT

Comment on attachment 146728 [details] Use this for review Still the correct diff.

WebKit Review Bot

Comment 5 2012-06-10 22:13:05 PDT

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

Comment 6 2012-06-11 06:04:22 PDT

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

Silvia Pfeiffer

Comment 7 2012-06-11 06:11:30 PDT

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

Eric Carlson

Comment 8 2012-06-11 09:43:41 PDT

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

Comment 9 2012-06-12 01:19:01 PDT

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

Silvia Pfeiffer

Comment 10 2012-06-12 01:20:15 PDT

(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 11 2012-06-12 01:40:23 PDT

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

Comment 12 2012-06-13 16:54:20 PDT

Created attachment 147444 [details] patch for cq

Silvia Pfeiffer

Comment 13 2012-06-13 18:48:10 PDT

Comment on attachment 147444 [details] patch for cq unfortunately introduced a bug in skia - gotta re-upload

Silvia Pfeiffer

Comment 14 2012-06-13 18:53:04 PDT

Created attachment 147464 [details] patch for cq

WebKit Review Bot

Comment 15 2012-06-14 07:01:53 PDT

Comment on attachment 147464 [details] patch for cq Clearing flags on attachment: 147464 Committed r120322: <http://trac.webkit.org/changeset/120322>

WebKit Review Bot

Comment 16 2012-06-14 07:02:00 PDT

All reviewed patches have been landed. Closing bug.

Nico Weber

Comment 17 2013-04-23 17:29:13 PDT

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

Comment 18 2013-04-27 01:03:12 PDT

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

Note You need to log in before you can comment on or make changes to this bug.