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.
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.
Created attachment 96399 [details] Screen Capture - Slider Thumb Image
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?
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.
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.
Comment on attachment 96572 [details] Modified Patch LGTM.
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?
(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" ?
(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
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.
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 ?
Comment on attachment 97052 [details] Patch looks good
Comment on attachment 97052 [details] Patch Clearing flags on attachment: 97052 Committed r88753: <http://trac.webkit.org/changeset/88753>
All reviewed patches have been landed. Closing bug.
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)?
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.
Previous patch had a problem. thumb icon is not showed.
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.
Created attachment 97084 [details] Screen Capture - Modified Thumb Icon
Created attachment 97085 [details] Fixed Pach
Comment on attachment 97085 [details] Fixed Pach looks ok.
Comment on attachment 97085 [details] Fixed Pach Clearing flags on attachment: 97085 Committed r88788: <http://trac.webkit.org/changeset/88788>