[webkit-reviews] review requested: [Bug 238035] nj.gov: Background color incorrect for 'State Vehicles' section : [Attachment 455054] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has asked  for review:
Bug 238035: nj.gov: Background color incorrect for 'State Vehicles' section
https://bugs.webkit.org/show_bug.cgi?id=238035

Attachment 455054: Patch

https://bugs.webkit.org/attachment.cgi?id=455054&action=review




--- Comment #11 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 455054
  --> https://bugs.webkit.org/attachment.cgi?id=455054
Patch

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

(just style comments, leaving substantive review to Aditya)

> Source/WebCore/rendering/RenderTheme.cpp:87
> +ControlPart RenderTheme::adjustAppearanceForElement(RenderStyle& style,
const Element* element, ControlPart& autoAppearance) const

It would be more stylish to return both ControlPart as return values, either as
struct or std::pair. Caller can then use structured bindings to unpack

auto [appareance, autoAppearance] = adjustAppearanceForElement(...).

> Source/WebCore/rendering/RenderTheme.cpp:114
> +static bool styleBorderAndBackgroundDiffer(const RenderStyle& style, const
RenderStyle& otherStyle)

"differ" is an odd function name. Maybe something more descriptive like
shouldIgnoreApparanceForStyle(style, baseStyle)?

> Source/WebCore/rendering/RenderTheme.cpp:151
> +    if (!userAgentAppearanceStyle && autoAppearanceForElement ==
NoControlPart && styleBorderAndBackgroundDiffer(style, style.defaultStyle()))

defaultStyle() is a static function, it is clearer to use
RenderStyle::defaultStyle() to call it.