Support slider tick mark snapping by implementing RenderThemeEfl::sliderTickSnappingThreshold().
Created attachment 159591 [details] Patch I set threshold value 5. It's natural, I think. Chromium port uses 5 also.
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?
(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 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.
(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.
Created attachment 159843 [details] Patch
(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 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.
Created attachment 159857 [details] Patch Modified comment.
Comment on attachment 159857 [details] Patch Looks OK to me now.
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 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 on attachment 159857 [details] Patch Clearing flags on attachment: 159857 Committed r126292: <http://trac.webkit.org/changeset/126292>
All reviewed patches have been landed. Closing bug.