[webkit-reviews] review granted: [Bug 58866] [EFL] Add current time to media control panel : [Attachment 90177] Modified Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Gyuyoung Kim
<gyuyoung.kim at samsung.com>'s request for review:
Bug 58866: [EFL] Add current time to media control panel
https://bugs.webkit.org/show_bug.cgi?id=58866

Attachment 90177: Modified Patch
https://bugs.webkit.org/attachment.cgi?id=90177&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
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.