Bug 219698 - [iOS][FCR] Add new look for input type=range
Summary: [iOS][FCR] Add new look for input type=range
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: Other
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: Aditya Keerthi
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-12-09 11:12 PST by Aditya Keerthi
Modified: 2020-12-10 07:12 PST (History)
15 users (show)

See Also:


Attachments
Patch (21.17 KB, patch)
2020-12-09 11:19 PST, Aditya Keerthi
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (21.15 KB, patch)
2020-12-09 11:29 PST, Aditya Keerthi
wenson_hsieh: review+
Details | Formatted Diff | Diff
Patch (21.22 KB, patch)
2020-12-09 19:10 PST, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Patch for landing (22.94 KB, patch)
2020-12-09 19:20 PST, Aditya Keerthi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aditya Keerthi 2020-12-09 11:12:37 PST
...
Comment 1 Aditya Keerthi 2020-12-09 11:13:04 PST
<rdar://problem/72144727>
Comment 2 Aditya Keerthi 2020-12-09 11:19:07 PST
Created attachment 415776 [details]
Patch
Comment 3 Aditya Keerthi 2020-12-09 11:29:28 PST
Created attachment 415779 [details]
Patch
Comment 4 Wenson Hsieh 2020-12-09 16:33:58 PST
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).
Comment 5 Wenson Hsieh 2020-12-09 16:34:35 PST
> refrain from using acronyms and acronyms in

I meant to write "acronyms and abbreviations" 😅
Comment 6 Tim Horton 2020-12-09 16:35:49 PST
(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.
Comment 7 Aditya Keerthi 2020-12-09 19:10:42 PST
Created attachment 415821 [details]
Patch
Comment 8 Aditya Keerthi 2020-12-09 19:20:57 PST
Created attachment 415822 [details]
Patch for landing
Comment 9 Aditya Keerthi 2020-12-09 19:22:20 PST
(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.
Comment 10 EWS 2020-12-10 07:12:39 PST
Committed r270625: <https://trac.webkit.org/changeset/270625>

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