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

Gyuyoung Kim
Reported 2011-04-18 23:41:28 PDT
I implement paintMediaCurrentTime() to show play time of media content. I refer to gtk implementation.
Attachments
Proposed Patch (2.62 KB, patch)
2011-04-18 23:45 PDT, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
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-
DetailsFormatted DiffDiff
Modified Patch (2.83 KB, patch)
2011-04-19 23:32 PDT, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
Modified Patch - Add reviewer field (2.82 KB, patch)
2011-04-19 23:34 PDT, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Gyuyoung Kim
Comment 1 2011-04-18 23:45:51 PDT
Created attachment 90153 [details] Proposed Patch
Gyuyoung Kim
Comment 2 2011-04-18 23:54:07 PDT
Created attachment 90156 [details] Screen Capture - Timeline
Gyuyoung Kim
Comment 3 2011-04-19 04:36:13 PDT
Created attachment 90177 [details] Modified Patch Fix return value in paintMediaFullscreenButton()
Gyuyoung Kim
Comment 4 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().
Lucas De Marchi
Comment 5 2011-04-19 06:00:21 PDT
Comment on attachment 90177 [details] Modified Patch LGTM
Daniel Bates
Comment 6 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.
Gyuyoung Kim
Comment 7 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.
Gyuyoung Kim
Comment 8 2011-04-19 23:34:16 PDT
Created attachment 90311 [details] Modified Patch - Add reviewer field
WebKit Commit Bot
Comment 9 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>
WebKit Commit Bot
Comment 10 2011-04-20 00:19:19 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.