Bug 219698 – [iOS][FCR] Add new look for input type=range

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Aditya Keerthi]

...

[Aditya Keerthi]

<rdar://problem/72144727>

[Aditya Keerthi]

Created attachment 415776 [details]
Patch

[Aditya Keerthi]

Created attachment 415779 [details]
Patch

[Wenson Hsieh]

Comment on attachment 415779 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeIOS.mm:2225
> +bool RenderThemeIOS::paintSliderTrackFCR(const RenderObject& box, const PaintInfo& paintInfo, const IntRect& rect)

Nit - we typically refrain from using acronyms and acronyms in function names like this (https://webkit.org/code-style-guidelines/#names-full-words). I think we should either just roll this into the call site, or rename this to `paintSliderTrackWithFormControlRedesign()`. If we do decide to keep this helper, we should also move its declaration in to the `private` section in the header (RenderThemeIOS.h).

[Wenson Hsieh]

> refrain from using acronyms and acronyms in

I meant to write "acronyms and abbreviations" 😅

[Tim Horton]

(In reply to Wenson Hsieh from comment #4)
> Comment on attachment 415779 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415779&action=review
> 
> > Source/WebCore/rendering/RenderThemeIOS.mm:2225
> > +bool RenderThemeIOS::paintSliderTrackFCR(const RenderObject& box, const PaintInfo& paintInfo, const IntRect& rect)
> 
> Nit - we typically refrain from using acronyms and acronyms in function
> names like this
> (https://webkit.org/code-style-guidelines/#names-full-words). I think we
> should either just roll this into the call site, or rename this to
> `paintSliderTrackWithFormControlRedesign()`. If we do decide to keep this
> helper, we should also move its declaration in to the `private` section in
> the header (RenderThemeIOS.h).

And of course, even better is renaming the old thing to indicate that it's old, and the new thing to have the "normal" name without a qualifier, so that when the old thing goes away, the code is in the "normal" state.

[Aditya Keerthi]

Created attachment 415821 [details]
Patch

[Aditya Keerthi]

Created attachment 415822 [details]
Patch for landing

[Aditya Keerthi]

(In reply to Tim Horton from comment #6)
> (In reply to Wenson Hsieh from comment #4)
> > Comment on attachment 415779 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=415779&action=review
> > 
> > > Source/WebCore/rendering/RenderThemeIOS.mm:2225
> > > +bool RenderThemeIOS::paintSliderTrackFCR(const RenderObject& box, const PaintInfo& paintInfo, const IntRect& rect)
> > 
> > Nit - we typically refrain from using acronyms and acronyms in function
> > names like this
> > (https://webkit.org/code-style-guidelines/#names-full-words). I think we
> > should either just roll this into the call site, or rename this to
> > `paintSliderTrackWithFormControlRedesign()`. If we do decide to keep this
> > helper, we should also move its declaration in to the `private` section in
> > the header (RenderThemeIOS.h).
> 
> And of course, even better is renaming the old thing to indicate that it's
> old, and the new thing to have the "normal" name without a qualifier, so
> that when the old thing goes away, the code is in the "normal" state.

For now, I’ve renamed the method to `paintSliderTrackWithFormControlRefresh` for consistency with existing code for the refresh.

If I did rename the old thing, I’d need to add something like this to every method:

void paintControl() {
#if REDESIGN_BUILD_FLAG
    if (!redesign_runtime_flag())
        return paintOldControl();
#else
    return paintOldControl();
#endif

    // New painting code.
}

Which is more complicated since we have both a compile time and runtime flag.

The new methods will be renamed when we remove the old ones.

[EWS]

Committed r270625: <https://trac.webkit.org/changeset/270625>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415822 [details].