...
<rdar://problem/72144727>
Created attachment 415776 [details] Patch
Created attachment 415779 [details] Patch
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).
> refrain from using acronyms and acronyms in I meant to write "acronyms and abbreviations" 😅
(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.
Created attachment 415821 [details] Patch
Created attachment 415822 [details] Patch for landing
(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.
Committed r270625: <https://trac.webkit.org/changeset/270625> All reviewed patches have been landed. Closing bug and clearing flags on attachment 415822 [details].