Bug 34405 - MediaControlTimeDisplayElement should defer time formatting to the theme
Summary: MediaControlTimeDisplayElement should defer time formatting to the theme
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Nobody
URL:
Keywords: HTML5
Depends on:
Blocks:
 
Reported: 2010-01-31 20:48 PST by Nick Young
Modified: 2010-02-03 23:48 PST (History)
3 users (show)

See Also:


Attachments
Initial Patch (5.58 KB, patch)
2010-01-31 20:53 PST, Nick Young
no flags Details | Formatted Diff | Diff
Updated Patch (5.62 KB, patch)
2010-02-01 18:28 PST, Nick Young
eric: review-
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Update Patch - Fixes build on mac (5.62 KB, patch)
2010-02-02 16:18 PST, Nick Young
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.