Bug 94560 – [EFL] Support slider tick mark snapping

• Component: WebKit EFL
• Status: RESOLVED FIXED

[KwangYong Choi]

Support slider tick mark snapping by implementing RenderThemeEfl::sliderTickSnappingThreshold().

[KwangYong Choi]

Created attachment 159591 [details]
Patch

I set threshold value 5. It's natural, I think. Chromium port uses 5 also.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 159591 [details]
Patch

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

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:817
> +    static const int threshold = 5;
> +
> +    return threshold;

Why not `return 5', perhaps with a comment explaining where the number came from?

[KwangYong Choi]

(In reply to comment #2)
> (From update of attachment 159591 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159591&action=review
> 
> > Source/WebCore/platform/efl/RenderThemeEfl.cpp:817
> > +    static const int threshold = 5;
> > +
> > +    return threshold;
> 
> Why not `return 5', perhaps with a comment explaining where the number came from?

I refer to already merged code above this method.

int RenderThemeEfl::sliderTickOffsetFromTrackCenter() const
{
    static const int sliderTickOffset = -12;

    return sliderTickOffset;
}

What shall I do? change it to 'return 5;' with a comment?

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 159591 [details]
Patch

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

>>> Source/WebCore/platform/efl/RenderThemeEfl.cpp:817
>>> +    return threshold;
>> 
>> Why not `return 5', perhaps with a comment explaining where the number came from?
> 
> I refer to already merged code above this method.
> 
> int RenderThemeEfl::sliderTickOffsetFromTrackCenter() const
> {
>     static const int sliderTickOffset = -12;
> 
>     return sliderTickOffset;
> }
> 
> What shall I do? change it to 'return 5;' with a comment?

Yes, that looks clearer.

[KwangYong Choi]

(In reply to comment #4)
> (From update of attachment 159591 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159591&action=review
> 
> >>> Source/WebCore/platform/efl/RenderThemeEfl.cpp:817
> >>> +    return threshold;
> >> 
> >> Why not `return 5', perhaps with a comment explaining where the number came from?
> > 
> > I refer to already merged code above this method.
> > 
> > int RenderThemeEfl::sliderTickOffsetFromTrackCenter() const
> > {
> >     static const int sliderTickOffset = -12;
> > 
> >     return sliderTickOffset;
> > }
> > 
> > What shall I do? change it to 'return 5;' with a comment?
> 
> Yes, that looks clearer.

OK. I will.

[KwangYong Choi]

Created attachment 159843 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #3)

int RenderThemeEfl::sliderTickOffsetFromTrackCenter() const
{
    static const int sliderTickOffset = -12;

    return sliderTickOffset;
}

In my opinion, above style is also good. But, *return 5* is being used by chromium port. So, if there is no objection, looks good to me.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 159843 [details]
Patch

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

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:815
> +    // It's natural to use 5 px for threshold.

Erm, that doesn't sound very convincing. You can say you are using the same threshold value as the Chromium port, or explain why 5px makes more sense than any other value.

[KwangYong Choi]

Created attachment 159857 [details]
Patch

Modified comment.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 159857 [details]
Patch

Looks OK to me now.

[Kenneth Rohde Christiansen]

Comment on attachment 159857 [details]
Patch

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

> LayoutTests/platform/efl/fast/forms/datalist/range-snap-to-datalist-expected.txt:-8
>  value for 42 is <500
>  value for 43 is <500
>  value for 44 is <500
> -value for 45 is <500

Why do we need a platform expected result? like do we still differ? Like how are the Android values for instnace?

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:817
> +LayoutUnit RenderThemeEfl::sliderTickSnappingThreshold() const
> +{
> +    // The same threshold value as the Chromium port.
> +    return 5;
> +}

So this sounds as we can share the result

[KwangYong Choi]

Comment on attachment 159857 [details]
Patch

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

>> LayoutTests/platform/efl/fast/forms/datalist/range-snap-to-datalist-expected.txt:-8
>> -value for 45 is <500
> 
> Why do we need a platform expected result? like do we still differ? Like how are the Android values for instnace?

Actually, there is no common expected result for this test. And, only Chrome port has a platform expected result which is same as EFL for now.

[WebKit Review Bot]

Comment on attachment 159857 [details]
Patch

Clearing flags on attachment: 159857

Committed r126292: <http://trac.webkit.org/changeset/126292>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.