Bug 51874 – [GTK] Port slider painting to GtkStyleContext

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

This depends on bug #51764 since the patch for stock icons contains the initial GtkStyleContext port.

[Carlos Garcia Campos]

Created attachment 77889 [details]
Use GtkStyleContext to paint sliders

[Martin Robinson]

Comment on attachment 77889 [details]
Use GtkStyleContext to paint sliders

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

Looks great, but I think it makes sense to create a RenderThemeGtk::adjustMediaSliderThumbSize. See below.

> WebCore/platform/gtk/RenderThemeGtk3.cpp:621
> +    gint sliderWidth, sliderLength, troughBorder;
> +    gint focusWidth, focusPad;

Please put these all on one line.

> WebCore/platform/gtk/RenderThemeGtk3.cpp:628
> +                                0);

Doesn't this produce a warning?

> WebCore/platform/gtk/RenderThemeGtk3.cpp:660
>  #if ENABLE(VIDEO)
> -    if (part == MediaSliderThumbPart) {
> -        o->style()->setWidth(Length(m_mediaSliderThumbWidth, Fixed));
> -        o->style()->setHeight(Length(m_mediaSliderThumbHeight, Fixed));
> -        return;
> -    }
> -    if (part == MediaVolumeSliderThumbPart)
> -        return;
> +        if (part == MediaSliderThumbPart) {
> +            renderObject->style()->setWidth(Length(m_mediaSliderThumbWidth, Fixed));
> +            renderObject->style()->setHeight(Length(m_mediaSliderThumbHeight, Fixed));
> +        }
>  #endif

Instead of using a switch statement here and duplicating this logic between RenderThemeGtk2 and RenderThemeGtk3, let's take the approach that RenderThemeMac.mm takes, but put RenderThemeGtk::adjustMediaSliderThumbSize in the platform-independent file. That way we can avoid duplicating this section.

> WebCore/platform/gtk/RenderThemeGtk3.cpp:680
> +            renderObject->style()->setWidth(Length((focusWidth + focusPad+ troughBorder) * 2 + sliderWidth, Fixed));

There is a space missing after focusPad.

[Carlos Garcia Campos]

(In reply to comment #2)
> (From update of attachment 77889 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77889&action=review
> 
> Looks great, but I think it makes sense to create a RenderThemeGtk::adjustMediaSliderThumbSize. See below.
> 
> > WebCore/platform/gtk/RenderThemeGtk3.cpp:621
> > +    gint sliderWidth, sliderLength, troughBorder;
> > +    gint focusWidth, focusPad;
> 
> Please put these all on one line.

Ok

> > WebCore/platform/gtk/RenderThemeGtk3.cpp:628
> > +                                0);
> 
> Doesn't this produce a warning?

No.

> > WebCore/platform/gtk/RenderThemeGtk3.cpp:660
> >  #if ENABLE(VIDEO)
> > -    if (part == MediaSliderThumbPart) {
> > -        o->style()->setWidth(Length(m_mediaSliderThumbWidth, Fixed));
> > -        o->style()->setHeight(Length(m_mediaSliderThumbHeight, Fixed));
> > -        return;
> > -    }
> > -    if (part == MediaVolumeSliderThumbPart)
> > -        return;
> > +        if (part == MediaSliderThumbPart) {
> > +            renderObject->style()->setWidth(Length(m_mediaSliderThumbWidth, Fixed));
> > +            renderObject->style()->setHeight(Length(m_mediaSliderThumbHeight, Fixed));
> > +        }
> >  #endif
> 
> Instead of using a switch statement here and duplicating this logic between RenderThemeGtk2 and RenderThemeGtk3, let's take the approach that RenderThemeMac.mm takes, but put RenderThemeGtk::adjustMediaSliderThumbSize in the platform-independent file. That way we can avoid duplicating this section.

Ok, I'll look at the mac code then. 

> > WebCore/platform/gtk/RenderThemeGtk3.cpp:680
> > +            renderObject->style()->setWidth(Length((focusWidth + focusPad+ troughBorder) * 2 + sliderWidth, Fixed));
> 
> There is a space missing after focusPad.

Ok.

[Carlos Garcia Campos]

Created attachment 77986 [details]
Updated patch

[Martin Robinson]

Created attachment 78026 [details]
Patch moving focus width and focus pad outside of the slider rect

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/platform/gtk/RenderThemeGtk.cpp', u'WebCore/platform/gtk/RenderThemeGtk.h', u'WebCore/platform/gtk/RenderThemeGtk2.cpp', u'WebCore/platform/gtk/RenderThemeGtk3.cpp']" exit_code: 1
WebCore/platform/gtk/RenderThemeGtk3.cpp:114:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/platform/gtk/RenderThemeGtk3.cpp:318:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/platform/gtk/RenderThemeGtk3.cpp:372:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 3 in 5 files


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

[Martin Robinson]

(In reply to comment #5)
> Created an attachment (id=78026) [details]
> Patch moving focus width and focus pad outside of the slider rect

This updated version of the patch has a few changes:
1. Focus width and focus padding are now drawn outside the slider rect, which makes calculating the size of slides pieces much more straight-forward. This is perfectly valid as long as you let WebCore know via adjustRepaintRect, I think.
2. Use NULL as the sentinel for calls to gtk_style_context_get_style. Zero doesn't produce a warnings, but can lead to crashes on 64-bit machines. Thus we should always use NULL.
3. Remove the static_casts wrapping gktTextDirection calls. They are no longer needed.

Let me know if I've futzed anything up too badly! :)

[Carlos Garcia Campos]

(In reply to comment #7)
> (In reply to comment #5)
> > Created an attachment (id=78026) [details] [details]
> > Patch moving focus width and focus pad outside of the slider rect
> 
> This updated version of the patch has a few changes:
> 1. Focus width and focus padding are now drawn outside the slider rect, which makes calculating the size of slides pieces much more straight-forward. This is perfectly valid as long as you let WebCore know via adjustRepaintRect, I think.

Yes, it works. 

> 2. Use NULL as the sentinel for calls to gtk_style_context_get_style. Zero doesn't produce a warnings, but can lead to crashes on 64-bit machines. Thus we should always use NULL.

Great, never liked using 0 instead of NULL, at least there's an exception here :-)

> 3. Remove the static_casts wrapping gktTextDirection calls. They are no longer needed.

Right.

> Let me know if I've futzed anything up too badly! :)

The only problem is that the slider thumb is painted outside the slider track, you still need to take into account the trough border to make the thumb fit into the slider track.

[Carlos Garcia Campos]

Created attachment 78216 [details]
Updated patch taking into account the trough border

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/platform/gtk/RenderThemeGtk.cpp', u'WebCore/platform/gtk/RenderThemeGtk.h', u'WebCore/platform/gtk/RenderThemeGtk2.cpp', u'WebCore/platform/gtk/RenderThemeGtk3.cpp']" exit_code: 1
WebCore/platform/gtk/RenderThemeGtk3.cpp:114:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/platform/gtk/RenderThemeGtk3.cpp:318:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/platform/gtk/RenderThemeGtk3.cpp:342:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/platform/gtk/RenderThemeGtk3.cpp:378:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 4 in 5 files


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

[Xan Lopez]

Comment on attachment 78216 [details]
Updated patch taking into account the trough border

Looks good to me.

[Carlos Garcia Campos]

Committed r75241: <http://trac.webkit.org/changeset/75241>