The caller always wants all padding sides, so we can simplify both the caller and the implementations by using a single method. It's also more efficient for the GTK+ port that creates and destroys the same style contexts on every call.
Created attachment 275374 [details] Patch
Created attachment 275375 [details] Try to fix mac builds
Comment on attachment 275375 [details] Try to fix mac builds View in context: https://bugs.webkit.org/attachment.cgi?id=275375&action=review > Source/WebCore/rendering/RenderTheme.h:187 > + virtual LengthBox popupInternalPaddingBox(const RenderStyle&) const { return LengthBox(0); } Not sure this function needs the word “Box” in its name. Also could write: return { 0 }; Instead of return LengthBox(0); > Source/WebCore/rendering/RenderThemeGtk.cpp:767 > + return LengthBox(0); Again, I prefer: return { 0 }; > Source/WebCore/rendering/RenderThemeGtk.cpp:782 > + return LengthBox(borderWidth.top + focusWidth, borderWidth.right + focusWidth + (style.direction() == LTR ? minArrowSize : 0), > + borderWidth.bottom + focusWidth, borderWidth.left + focusWidth + (style.direction() == RTL ? minArrowSize : 0)); I like the syntax where we just write: return { a, b, c, d }; Rather than naming the type LengthBox. > Source/WebCore/rendering/RenderThemeIOS.mm:535 > + return LengthBox(0, MenuListButtonPaddingRight + style.borderTopWidth(), 0, 0); > + return LengthBox(0); Ditto. > Source/WebCore/rendering/RenderThemeMac.mm:1374 > + return LengthBox(popupButtonPadding(controlSizeForFont(style))[topPadding] * style.effectiveZoom(), > + popupButtonPadding(controlSizeForFont(style))[rightPadding] * style.effectiveZoom(), > + popupButtonPadding(controlSizeForFont(style))[bottomPadding] * style.effectiveZoom(), > + popupButtonPadding(controlSizeForFont(style))[leftPadding] * style.effectiveZoom()); Ditto. > Source/WebCore/rendering/RenderThemeMac.mm:1381 > + return LengthBox(styledPopupPaddingTop * style.effectiveZoom(), > + static_cast<int>(ceilf(arrowWidth + (arrowPaddingLeft + arrowPaddingRight + paddingBeforeSeparator) * style.effectiveZoom())), > + styledPopupPaddingBottom * style.effectiveZoom(), styledPopupPaddingLeft * style.effectiveZoom()); Ditto. > Source/WebCore/rendering/RenderThemeMac.mm:1384 > + return LengthBox(0); Ditto.
(In reply to comment #3) > Comment on attachment 275375 [details] > Try to fix mac builds > > View in context: > https://bugs.webkit.org/attachment.cgi?id=275375&action=review > > > Source/WebCore/rendering/RenderTheme.h:187 > > + virtual LengthBox popupInternalPaddingBox(const RenderStyle&) const { return LengthBox(0); } > > Not sure this function needs the word “Box” in its name. I thought about it and decided to leave the Box for consistency with RenderStyle::setPaddingBox and because it returns a LengthBox. > Also could write: > > return { 0 }; > > Instead of return LengthBox(0); GCC doesn't like that: error: converting to 'WebCore::LengthBox' from initializer list would use explicit constructor 'WebCore::LengthBox::LengthBox(int)' I think it's because there are two explicit constructors receiving an integer, using { 0, 0, 0, 0 } works.
Created attachment 275491 [details] Patch for landing
It seems brace initializer list requires explicit casts everywhere: /Volumes/Data/EWS/WebKit/Source/WebCore/rendering/RenderThemeMac.mm:1371:18: error: type 'float' cannot be narrowed to 'int' in initializer list [-Wc++11-narrowing] return { popupButtonPadding(controlSizeForFont(style))[topPadding] * style.effectiveZoom(), ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /Volumes/Data/EWS/WebKit/Source/WebCore/rendering/RenderThemeMac.mm:1371:18: note: insert an explicit cast to silence this issue
Created attachment 275493 [details] Try to fix mac builds
Committed r198983: <http://trac.webkit.org/changeset/198983>