Bug 62174 – [EFL] Support for painting thumb of media slider

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Gyuyoung Kim]

paintMediaSliderThumb() should be implemented in order to support media control UI. However, the function is not invoked during the media playing. I am investigating it.

[Gyuyoung Kim]

Created attachment 96398 [details]
Proposed Patch

At last, I find why the paintMediaSliderthumb() is not called. adjustSliderThumbSize() function should be implemented first. So, I make the adjustSliderThumbSize() referencing to other ports.

[Gyuyoung Kim]

Created attachment 96399 [details]
Screen Capture - Slider Thumb Image

[Lucas De Marchi]

Comment on attachment 96398 [details]
Proposed Patch

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

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:703
> +    , m_sliderThumbColor(255, 0, 0) // red color.
> +    , m_sliderThumbWidth(12)
> +    , m_sliderThumbHeight(12)
> +#if ENABLE(VIDEO)
>      , m_mediaSliderHeight(14)
> +    , m_mediaSliderThumbWidth(12)
> +    , m_mediaSliderThumbHeight(12)

Why do you use m_slider* and m_panel* if VIDEO is disabled?

[Kent Tamura]

Comment on attachment 96398 [details]
Proposed Patch

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

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:845
> +void RenderThemeEfl::adjustSliderThumbSize(RenderObject* o) const

FYI. We're trying to change the parameter type of adjustSliderThumbSize().  See Bug 62208.

[Gyuyoung Kim]

Created attachment 96572 [details]
Modified Patch

I wanted to use the m_slider* and m_panel* when VIDEO is disabled. However, the variables only are used by VIDEO. So, I modify the names with "media" as below,

m_panelColor -> m_mediaPanelColor
m_sliderColor -> m_mediaSliderColor


Kent, Thank you for your comment. I adjust it to this patch.

[Leandro Pereira]

Comment on attachment 96572 [details]
Modified Patch

LGTM.

[Kent Tamura]

Comment on attachment 96572 [details]
Modified Patch

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

> Source/WebCore/platform/efl/RenderThemeEfl.h:219
> +    const int m_sliderThumbWidth;
> +    const int m_sliderThumbHeight;

Why are they const data members? They look very strange.
Can't you make them file-scoped const variables like http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderThemeChromiumLinux.cpp#L51
or static const data members?

[Gyuyoung Kim]

(In reply to comment #7)
> (From update of attachment 96572 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96572&action=review
> 
> > Source/WebCore/platform/efl/RenderThemeEfl.h:219
> > +    const int m_sliderThumbWidth;
> > +    const int m_sliderThumbHeight;
> 
> Why are they const data members? They look very strange.
> Can't you make them file-scoped const variables like http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderThemeChromiumLinux.cpp#L51
> or static const data members?

Kent, thank you for your review. :)  

When I make this patch, I refer to GTK port as below,

    const int m_mediaIconSize;
    const int m_mediaSliderHeight;
    const int m_mediaSliderThumbWidth;
    const int m_mediaSliderThumbHeight;

I thought "const" keyword is enough for this patch. Do you think the variables should be declared "static const" ?

[Gyuyoung Kim]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 96572 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=96572&action=review
> > 
> > > Source/WebCore/platform/efl/RenderThemeEfl.h:219
> > > +    const int m_sliderThumbWidth;
> > > +    const int m_sliderThumbHeight;
> > 
> > Why are they const data members? They look very strange.
> > Can't you make them file-scoped const variables like http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderThemeChromiumLinux.cpp#L51
> > or static const data members?
> 
> Kent, thank you for your review. :)  
> 
> When I make this patch, I refer to GTK port as below,
> 
>     const int m_mediaIconSize;
>     const int m_mediaSliderHeight;
>     const int m_mediaSliderThumbWidth;
>     const int m_mediaSliderThumbHeight;
> 
> I thought "const" keyword is enough for this patch. Do you think the variables should be declared "static const" ?

http://trac.webkit.org/browser/trunk/Source/WebCore/platform/gtk/RenderThemeGtk.h#L193

[Kent Tamura]

Comment on attachment 96572 [details]
Modified Patch

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

>>>> Source/WebCore/platform/efl/RenderThemeEfl.h:219
>>>> +    const int m_sliderThumbHeight;
>>> 
>>> Why are they const data members? They look very strange.
>>> Can't you make them file-scoped const variables like http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderThemeChromiumLinux.cpp#L51
>>> or static const data members?
>> 
>> Kent, thank you for your review. :)  
>> 
>> When I make this patch, I refer to GTK port as below,
>> 
>>     const int m_mediaIconSize;
>>     const int m_mediaSliderHeight;
>>     const int m_mediaSliderThumbWidth;
>>     const int m_mediaSliderThumbHeight;
>> 
>> I thought "const" keyword is enough for this patch. Do you think the variables should be declared "static const" ?
> 
> http://trac.webkit.org/browser/trunk/Source/WebCore/platform/gtk/RenderThemeGtk.h#L193

I see.

In general, non-static const data member for a fixed value wastes resources.  It wastes memory space to store the member and waste CPU time to substitute the initial value.
On the other hand, we can expect inline expansion of a static const data member or a file-scoped const variable.

However, RenderTheme is a singleton class. It wouldn't waste much resources.
IMO we should not use use const data members to hold fixed values anyway because other contributors might follow the wrong usage.

[Gyuyoung Kim]

Created attachment 97052 [details]
Patch

Ok, I understand what your point. So, I modify this patch according to your guidance.
How do you think this patch ?

[Kent Tamura]

Comment on attachment 97052 [details]
Patch

looks good

[WebKit Review Bot]

Comment on attachment 97052 [details]
Patch

Clearing flags on attachment: 97052

Committed r88753: <http://trac.webkit.org/changeset/88753>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Comment on attachment 96572 [details]
Modified Patch

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

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:695
> +    , m_sliderThumbColor(255, 0, 0) // red color.

Would we want to use Color::red?

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:1253
> +    info.context->fillRoundedRect(rect, IntSize(3, 3), IntSize(3, 3), IntSize(3, 3), IntSize(3, 3), m_sliderThumbColor, ColorSpaceDeviceRGB);

Maybe we should have a local for IntSize(3,3)?

[Gyuyoung Kim]

Created attachment 97083 [details]
Additional Patch

Landed patch had a problem. parameter of adjustSliderThumbSize() should be RenderStyle instead of RenderObject.

Eric,
I adjust your advice to this patch. But, there is no Color:red. So, I use Color::darkGray instead of Color::red.

[Gyuyoung Kim]

Previous patch had a problem. thumb icon is not showed.

[WebKit Review Bot]

Attachment 97083 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/efl/RenderThemeEfl.cpp:73:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Created attachment 97084 [details]
Screen Capture - Modified Thumb Icon

[Gyuyoung Kim]

Created attachment 97085 [details]
Fixed Pach

[Kent Tamura]

Comment on attachment 97085 [details]
Fixed Pach

looks ok.

[WebKit Review Bot]

Comment on attachment 97085 [details]
Fixed Pach

Clearing flags on attachment: 97085

Committed r88788: <http://trac.webkit.org/changeset/88788>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.