...
<rdar://problem/71951874>
Created attachment 415345 [details] Patch
Created attachment 415349 [details] Patch
Comment on attachment 415349 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415349&action=review > Source/WebCore/rendering/RenderMenuList.cpp:672 > + RenderThemeIOS::adjustRoundBorderRadius(mutableStyle(), *this); It seems a bit strange to be adjusting style during layout, but I do see that this is already done in a similar way in RenderButton and RenderTextControlSingleLine. Perhaps other folks might have some more input on whether this is fine (and what alternatives we should consider otherwise).
Comment on attachment 415349 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415349&action=review > Source/WebCore/rendering/RenderThemeIOS.mm:553 > +LengthBox RenderThemeIOS::popupInternalPaddingBox(const RenderStyle& style, const Element& element) const Please do not pass in the Element just to consult with the iOSFormControlRefreshEnabled bit.
Created attachment 415672 [details] Patch
(In reply to zalan from comment #5) > Comment on attachment 415349 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=415349&action=review > > > Source/WebCore/rendering/RenderThemeIOS.mm:553 > > +LengthBox RenderThemeIOS::popupInternalPaddingBox(const RenderStyle& style, const Element& element) const > > Please do not pass in the Element just to consult with the > iOSFormControlRefreshEnabled bit. Changed to pass in the Settings.
Comment on attachment 415672 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415672&action=review > Source/WebCore/rendering/RenderThemeIOS.mm:2219 > +void RenderThemeIOS::paintMenuListButtonDecorationsFCR(const RenderBox& box, const PaintInfo& paintInfo, const FloatRect& rect) (Same comment here about method naming and acronyms/abbreviations)
(In reply to Wenson Hsieh from comment #8) > Comment on attachment 415672 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=415672&action=review > > > Source/WebCore/rendering/RenderThemeIOS.mm:2219 > > +void RenderThemeIOS::paintMenuListButtonDecorationsFCR(const RenderBox& box, const PaintInfo& paintInfo, const FloatRect& rect) > > (Same comment here about method naming and acronyms/abbreviations) Will update after landing https://bugs.webkit.org/show_bug.cgi?id=219698, since this patch will need to be rebased.
Created attachment 415879 [details] Patch
(In reply to Wenson Hsieh from comment #8) > Comment on attachment 415672 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=415672&action=review > > > Source/WebCore/rendering/RenderThemeIOS.mm:2219 > > +void RenderThemeIOS::paintMenuListButtonDecorationsFCR(const RenderBox& box, const PaintInfo& paintInfo, const FloatRect& rect) > > (Same comment here about method naming and acronyms/abbreviations) Renamed method to paintMenuListButtonDecorationsWithFormControlRefresh.
Created attachment 416020 [details] Patch
Comment on attachment 416020 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416020&action=review r=mews > Source/WebCore/rendering/RenderThemeIOS.mm:2388 > + constexpr int size = 18; Perhaps `length` or `sideLength` instead of `size`? Size more commonly refers to a { width, height }, especially in this area of code.
(In reply to Wenson Hsieh from comment #13) > Comment on attachment 416020 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=416020&action=review > > r=mews > > > Source/WebCore/rendering/RenderThemeIOS.mm:2388 > > + constexpr int size = 18; > > Perhaps `length` or `sideLength` instead of `size`? Size more commonly > refers to a { width, height }, especially in this area of code. Thanks for the review! Will update to `length`.
Created attachment 416023 [details] Patch for landing
Committed r270713: <https://trac.webkit.org/changeset/270713> All reviewed patches have been landed. Closing bug and clearing flags on attachment 416023 [details].