Bug 58866

Summary: [EFL] Add current time to media control panel
Product: WebKit Reporter: Gyuyoung Kim <gyuyoung.kim>
Component: WebKit EFLAssignee: Gyuyoung Kim <gyuyoung.kim>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric, kenneth, lucas.de.marchi, tonikitoo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 58865    
Attachments:
Description Flags
Proposed Patch
none
Screen Capture - Timeline
none
Modified Patch
dbates: review+, dbates: commit-queue-
Modified Patch
none
Modified Patch - Add reviewer field none

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.