Bug 34405 – MediaControlTimeDisplayElement should defer time formatting to the theme

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Nick Young]

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.

[Nick Young]

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);
}

[Eric Carlson]

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!

[Nick Young]

Created attachment 47896 [details]
Updated Patch

Updated Patch. New RenderTheme methods are now const methods.

Thanks for the review.

[Eric Carlson]

Comment on attachment 47896 [details]
Updated Patch

r=me

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

Comment on attachment 47896 [details]
Updated Patch

Looks like this patch does not compile on Leopard.  Marking r-

[Eric Seidel (no email)]

Comment on attachment 47811 [details]
Initial Patch

Clearing Eric Carlson's review on this obsolete patch.

[Nick Young]

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.

[Eric Seidel (no email)]

Why is this one the RenderTheme (which is runtime switchable) instead of part of WebCore/platform?

[Eric Carlson]

Because some platforms support more than one media controller theme?

[WebKit Commit Bot]

Comment on attachment 47982 [details]
Update Patch - Fixes build on mac

Clearing flags on attachment: 47982

Committed r54326: <http://trac.webkit.org/changeset/54326>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.