Bug 58866 - [EFL] Add current time to media control panel
Summary: [EFL] Add current time to media control panel
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on:
Blocks: 58865
  Show dependency tree / graph
 
Reported: 2011-04-18 23:41 PDT by Gyuyoung Kim
Modified: 2011-04-20 00:19 PDT (History)
5 users (show)

See Also:

Attachments
Proposed Patch (2.62 KB, patch)
2011-04-18 23:45 PDT, Gyuyoung Kim 		no flags Details | Formatted Diff | Diff
Screen Capture - Timeline (7.50 KB, image/png)
2011-04-18 23:54 PDT, Gyuyoung Kim 		no flags Details
Modified Patch (2.64 KB, patch)
2011-04-19 04:36 PDT, Gyuyoung Kim 		dbates: review+
dbates: commit-queue-
Details | Formatted Diff | Diff
Modified Patch (2.83 KB, patch)
2011-04-19 23:32 PDT, Gyuyoung Kim 		no flags Details | Formatted Diff | Diff
Modified Patch - Add reviewer field (2.82 KB, patch)
2011-04-19 23:34 PDT, Gyuyoung Kim 		no flags Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2011-04-18 23:41:28 PDT 
I implement paintMediaCurrentTime() to show play time of media content. I refer to gtk implementation.
Comment 1 Gyuyoung Kim 2011-04-18 23:45:51 PDT 
Created attachment 90153 [details]
Proposed Patch
Comment 2 Gyuyoung Kim 2011-04-18 23:54:07 PDT 
Created attachment 90156 [details]
Screen Capture - Timeline
Comment 3 Gyuyoung Kim 2011-04-19 04:36:13 PDT 
Created attachment 90177 [details]
Modified Patch

Fix return value in paintMediaFullscreenButton()
Comment 4 Gyuyoung Kim 2011-04-19 04:38:19 PDT 
(In reply to comment #3)
> Created an attachment (id=90177) [details]
> Modified Patch
> 
> Fix return value in paintMediaFullscreenButton()

(In reply to comment #3)
> Created an attachment (id=90177) [details]
> Modified Patch
> 
> Fix return value in paintMediaFullscreenButton()

Oops, I fix paintMediaCurrentTime(), not paintMediaFullscreenButton().
Comment 5 Lucas De Marchi 2011-04-19 06:00:21 PDT 
Comment on attachment 90177 [details]
Modified Patch

LGTM
Comment 6 Daniel Bates 2011-04-19 11:34:18 PDT 
Comment on attachment 90177 [details]
Modified Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=90177&action=review

r=me

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:694
> +    , m_panelColor(220, 220, 195)

Nit: It might be nice to write an inline comment that explains that the RGB values describe some kind of beige/tan-ish color.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:1101
> +    return formatMediaControlsTime(currentTime) + " / " + formatMediaControlsTime(duration);

It is more efficient to use makeString() (defined in wtf/text/StringConcatenate.h) for such string concatenation. Notice, there is a three argument variant.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:1196
> +

This variable is being underutilized since it's only referenced once in this function body. I suggest inlining the value of this variable into the line below and removing this variable.
Comment 7 Gyuyoung Kim 2011-04-19 23:32:48 PDT 
Created attachment 90310 [details]
Modified Patch

Daniel, thank you for your review.  

I modify this patch according to your comment. It seems no nits. So, I land this patch.
Comment 8 Gyuyoung Kim 2011-04-19 23:34:16 PDT 
Created attachment 90311 [details]
Modified Patch - Add reviewer field
Comment 9 WebKit Commit Bot 2011-04-20 00:19:12 PDT 
Comment on attachment 90311 [details]
Modified Patch - Add reviewer field

Clearing flags on attachment: 90311

Committed r84348: <http://trac.webkit.org/changeset/84348>
Comment 10 WebKit Commit Bot 2011-04-20 00:19:19 PDT 
All reviewed patches have been landed.  Closing bug.