Summary: | MediaControlTimeDisplayElement should defer time formatting to the theme | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nick Young <nicholas.young> | ||||||||
Component: | Media | Assignee: | 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
Nick Young
2010-01-31 20:48:06 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 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! Created attachment 47896 [details]
Updated Patch
Updated Patch. New RenderTheme methods are now const methods.
Thanks for the review.
Comment on attachment 47896 [details]
Updated Patch
r=me
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 on attachment 47896 [details]
Updated Patch
Looks like this patch does not compile on Leopard. Marking r-
Comment on attachment 47811 [details]
Initial Patch
Clearing Eric Carlson's review on this obsolete patch.
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.
Why is this one the RenderTheme (which is runtime switchable) instead of part of WebCore/platform? Because some platforms support more than one media controller theme? Comment on attachment 47982 [details] Update Patch - Fixes build on mac Clearing flags on attachment: 47982 Committed r54326: <http://trac.webkit.org/changeset/54326> All reviewed patches have been landed. Closing bug. |