RESOLVED FIXED 62174
[EFL] Support for painting thumb of media slider
https://bugs.webkit.org/show_bug.cgi?id=62174
Summary [EFL] Support for painting thumb of media slider
Gyuyoung Kim
Reported 2011-06-06 18:28:33 PDT
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.
Attachments
Proposed Patch (4.27 KB, patch)
2011-06-08 01:24 PDT, Gyuyoung Kim
no flags
Screen Capture - Slider Thumb Image (7.73 KB, image/png)
2011-06-08 01:25 PDT, Gyuyoung Kim
no flags
Modified Patch (6.02 KB, patch)
2011-06-09 04:21 PDT, Gyuyoung Kim
no flags
Patch (6.44 KB, patch)
2011-06-13 18:23 PDT, Gyuyoung Kim
no flags
Additional Patch (4.07 KB, patch)
2011-06-14 01:27 PDT, Gyuyoung Kim
no flags
Screen Capture - Modified Thumb Icon (19.47 KB, image/png)
2011-06-14 01:29 PDT, Gyuyoung Kim
no flags
Fixed Pach (3.72 KB, patch)
2011-06-14 01:35 PDT, Gyuyoung Kim
no flags
Gyuyoung Kim
Comment 1 2011-06-08 01:24:00 PDT
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
Comment 2 2011-06-08 01:25:20 PDT
Created attachment 96399 [details] Screen Capture - Slider Thumb Image
Lucas De Marchi
Comment 3 2011-06-08 05:45:34 PDT
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 4 2011-06-08 06:04:58 PDT
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
Comment 5 2011-06-09 04:21:10 PDT
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 6 2011-06-09 07:25:19 PDT
Comment on attachment 96572 [details] Modified Patch LGTM.
Kent Tamura
Comment 7 2011-06-13 16:36:32 PDT
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
Comment 8 2011-06-13 17:44:59 PDT
(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
Comment 9 2011-06-13 17:48:20 PDT
(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 10 2011-06-13 18:02:12 PDT
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
Comment 11 2011-06-13 18:23:32 PDT
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 12 2011-06-13 18:41:10 PDT
Comment on attachment 97052 [details] Patch looks good
WebKit Review Bot
Comment 13 2011-06-13 19:19:56 PDT
Comment on attachment 97052 [details] Patch Clearing flags on attachment: 97052 Committed r88753: <http://trac.webkit.org/changeset/88753>
WebKit Review Bot
Comment 14 2011-06-13 19:20:01 PDT
All reviewed patches have been landed. Closing bug.
Eric Seidel (no email)
Comment 15 2011-06-13 21:22:21 PDT
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
Comment 16 2011-06-14 01:27:21 PDT
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
Comment 17 2011-06-14 01:28:11 PDT
Previous patch had a problem. thumb icon is not showed.
WebKit Review Bot
Comment 18 2011-06-14 01:29:35 PDT
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
Comment 19 2011-06-14 01:29:50 PDT
Created attachment 97084 [details] Screen Capture - Modified Thumb Icon
Gyuyoung Kim
Comment 20 2011-06-14 01:35:14 PDT
Created attachment 97085 [details] Fixed Pach
Kent Tamura
Comment 21 2011-06-14 02:10:29 PDT
Comment on attachment 97085 [details] Fixed Pach looks ok.
WebKit Review Bot
Comment 22 2011-06-14 02:50:12 PDT
Comment on attachment 97085 [details] Fixed Pach Clearing flags on attachment: 97085 Committed r88788: <http://trac.webkit.org/changeset/88788>
WebKit Review Bot
Comment 23 2011-06-14 02:50: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.