Bug 34405

Summary: MediaControlTimeDisplayElement should defer time formatting to the theme
Product: WebKit Reporter: Nick Young <nicholas.young>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Enhancement CC: commit-queue, eric.carlson, eric
Priority: P2 Keywords: HTML5
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Initial Patch
none
Updated Patch
eric: review-, commit-queue: commit-queue-
Update Patch - Fixes build on mac none

Description Nick Young 2010-01-31 20:48:06 PST
Currently, there exists two media control elements which display time related information: The current time display and the remaining time display.
The inner text of these elements is defined in the MediaControlTimeDisplayElement (platform independent code).

It should be possible for the theme to decide what times are shown in these elements, and how these times are formatted.
Comment 1 Nick Young 2010-01-31 20:53:03 PST
Created attachment 47811 [details]
Initial Patch

This patch addresses the problem by introducing three new virtual methods to RenderTheme:

virtual String formatMediaControlsTime(float time);
virtual String formatMediaControlsCurrentTime(float currentTime, float duration);
virtual String formatMediaControlsRemainingTime(float currentTime, float duration);

The first method can be reimplemented by those only wishing to change how the times are formatted.
The latter two methods can be reimplemented by those wishing to change what times are shown in each element.

The motivating use case for this patch is to hide the time remaining element and augment the current time element -

String RenderThemeXX::formatMediaCurrentTime(float currentTime, float duration)
{
    return formatMediaTime(currentTime) + " / " + formatMediaTime(duration);
}
Comment 2 Eric Carlson 2010-02-01 08:00:22 PST
Comment on attachment 47811 [details]
Initial Patch

> +    virtual String formatMediaControlsTime(float time);
> +    virtual String formatMediaControlsCurrentTime(float currentTime, float duration);
> +    virtual String formatMediaControlsRemainingTime(float currentTime, float duration);

These should all be const.

r=me with this change, thanks!
Comment 3 Nick Young 2010-02-01 18:28:19 PST
Created attachment 47896 [details]
Updated Patch

Updated Patch. New RenderTheme methods are now const methods.

Thanks for the review.
Comment 4 Eric Carlson 2010-02-01 19:44:44 PST
Comment on attachment 47896 [details]
Updated Patch

r=me
Comment 5 WebKit Commit Bot 2010-02-02 03:19:59 PST
Comment on attachment 47896 [details]
Updated Patch

Rejecting patch 47896 from commit-queue.

Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1
Last 500 characters of output:
s/CommitQueue/WebCore/rendering/RenderButton.cpp -o /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/i386/RenderButton.o
** BUILD FAILED **

The following build commands failed:
WebCore:
	Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/i386/RenderTheme.o /Users/eseidel/Projects/CommitQueue/WebCore/rendering/RenderTheme.cpp normal i386 c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://webkit-commit-queue.appspot.com/results/229431
Comment 6 Eric Seidel (no email) 2010-02-02 13:54:45 PST
Comment on attachment 47896 [details]
Updated Patch

Looks like this patch does not compile on Leopard.  Marking r-
Comment 7 Eric Seidel (no email) 2010-02-02 14:08:04 PST
Comment on attachment 47811 [details]
Initial Patch

Clearing Eric Carlson's review on this obsolete patch.
Comment 8 Nick Young 2010-02-02 16:18:54 PST
Created attachment 47982 [details]
Update Patch - Fixes build on mac

There's an unused parameter there. I've left it in (but commented it out) so that reimplementers don't have to guess what it is.
Comment 9 Eric Seidel (no email) 2010-02-03 16:25:23 PST
Why is this one the RenderTheme (which is runtime switchable) instead of part of WebCore/platform?
Comment 10 Eric Carlson 2010-02-03 16:39:17 PST
Because some platforms support more than one media controller theme?
Comment 11 WebKit Commit Bot 2010-02-03 23:48:43 PST
Comment on attachment 47982 [details]
Update Patch - Fixes build on mac

Clearing flags on attachment: 47982

Committed r54326: <http://trac.webkit.org/changeset/54326>
Comment 12 WebKit Commit Bot 2010-02-03 23:48:54 PST
All reviewed patches have been landed.  Closing bug.