Bug 94560 - [EFL] Support slider tick mark snapping
Summary: [EFL] Support slider tick mark snapping
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-20 19:13 PDT by KwangYong Choi
Modified: 2012-08-22 05:48 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.70 KB, patch)
2012-08-20 19:28 PDT, KwangYong Choi
no flags Details | Formatted Diff | Diff
Patch (3.74 KB, patch)
2012-08-21 19:04 PDT, KwangYong Choi
no flags Details | Formatted Diff | Diff
Patch (3.74 KB, patch)
2012-08-21 22:08 PDT, KwangYong Choi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description KwangYong Choi 2012-08-20 19:13:33 PDT
Support slider tick mark snapping by implementing RenderThemeEfl::sliderTickSnappingThreshold().
Comment 1 KwangYong Choi 2012-08-20 19:28:43 PDT
Created attachment 159591 [details]
Patch

I set threshold value 5. It's natural, I think. Chromium port uses 5 also.
Comment 2 Raphael Kubo da Costa (:rakuco) 2012-08-21 07:39:39 PDT
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?
Comment 3 KwangYong Choi 2012-08-21 17:43:07 PDT
(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?
Comment 4 Raphael Kubo da Costa (:rakuco) 2012-08-21 18:29:12 PDT
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.
Comment 5 KwangYong Choi 2012-08-21 18:32:26 PDT
(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.
Comment 6 KwangYong Choi 2012-08-21 19:04:47 PDT
Created attachment 159843 [details]
Patch
Comment 7 Gyuyoung Kim 2012-08-21 19:20:30 PDT
(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.
Comment 8 Raphael Kubo da Costa (:rakuco) 2012-08-21 19:34:21 PDT
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.
Comment 9 KwangYong Choi 2012-08-21 22:08:12 PDT
Created attachment 159857 [details]
Patch

Modified comment.
Comment 10 Raphael Kubo da Costa (:rakuco) 2012-08-21 22:20:44 PDT
Comment on attachment 159857 [details]
Patch

Looks OK to me now.
Comment 11 Kenneth Rohde Christiansen 2012-08-22 05:14:16 PDT
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
Comment 12 KwangYong Choi 2012-08-22 05:41:51 PDT
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.
Comment 13 WebKit Review Bot 2012-08-22 05:48:24 PDT
Comment on attachment 159857 [details]
Patch

Clearing flags on attachment: 159857

Committed r126292: <http://trac.webkit.org/changeset/126292>
Comment 14 WebKit Review Bot 2012-08-22 05:48:28 PDT
All reviewed patches have been landed.  Closing bug.