Bug 238035

Created attachment 455006 [details]
Patch

Comment on attachment 455006 [details]
Patch

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

> Source/WebCore/rendering/RenderTheme.cpp:114
> +static bool shouldIgnoreAppearance(RenderStyle& style)

Not really my area but it feels like this parameter could/should be const.

Created attachment 455009 [details]
Patch

Comment on attachment 455006 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        controlled elements when they are styled by the author.

"controlled" -> "control"

>> Source/WebCore/rendering/RenderTheme.cpp:114
>> +static bool shouldIgnoreAppearance(RenderStyle& style)
> 
> Not really my area but it feels like this parameter could/should be const.

Would be nice if this method took two RenderStyle parameters, and was renamed, so that `RenderTheme::isControlStyled` can share the logic.

(and make the parameters const, as Chris said)

> LayoutTests/fast/css/non-form-control-element-drop-appearance.html:3
> +  -webkit-appearance: button;

Let's use the unprefixed "appearance".

(In reply to Aditya Keerthi from comment #4)
> Comment on attachment 455006 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455006&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        controlled elements when they are styled by the author.
> 
> "controlled" -> "control"
> 

will fix. 

> >> Source/WebCore/rendering/RenderTheme.cpp:114
> >> +static bool shouldIgnoreAppearance(RenderStyle& style)
> > 
> > Not really my area but it feels like this parameter could/should be const.
> 
> Would be nice if this method took two RenderStyle parameters, and was
> renamed, so that `RenderTheme::isControlStyled` can share the logic.
> 
> (and make the parameters const, as Chris said)
> 
sounds good, will fix

> > LayoutTests/fast/css/non-form-control-element-drop-appearance.html:3
> > +  -webkit-appearance: button;
> 
> Let's use the unprefixed "appearance".

ok. why?

(In reply to Kate Cheney from comment #5)
> (In reply to Aditya Keerthi from comment #4)
> > Comment on attachment 455006 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=455006&action=review
> > 
> > > Source/WebCore/ChangeLog:12
> > > +        controlled elements when they are styled by the author.
> > 
> > "controlled" -> "control"
> > 
> 
> will fix. 
> 
> > >> Source/WebCore/rendering/RenderTheme.cpp:114
> > >> +static bool shouldIgnoreAppearance(RenderStyle& style)
> > > 
> > > Not really my area but it feels like this parameter could/should be const.
> > 
> > Would be nice if this method took two RenderStyle parameters, and was
> > renamed, so that `RenderTheme::isControlStyled` can share the logic.
> > 
> > (and make the parameters const, as Chris said)
> > 
> sounds good, will fix
> 
> > > LayoutTests/fast/css/non-form-control-element-drop-appearance.html:3
> > > +  -webkit-appearance: button;
> > 
> > Let's use the unprefixed "appearance".
> 
> ok. why?

There's no behavioral difference, because it's an alias, but I think it's better to use the unprefixed version of the property in new code.

Created attachment 455021 [details]
Patch

failing layout tests seem possibly related..

Created attachment 455054 [details]
Patch

Comment on attachment 455054 [details]
Patch

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

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

I saw that we have the following methods in RenderStyle which compare two styles:

```
bool inheritedEqual(const RenderStyle&) const;
bool descendantAffectingNonInheritedPropertiesEqual(const RenderStyle&) const;
```

Consider declaring this as `bool borderAndBackgroundEqual(const RenderStyle&) const;` and implementing in `RenderStyle`.

> Source/WebCore/rendering/RenderTheme.cpp:124
> +    auto part = adjustAppearanceForElement(style, element, autoAppearanceForElement);

I prefer to avoid out-parameters when possible, as they add indirection when reading code, and can give a single method too much responsibility.

Consider moving the call to `autoAppearanceForElement()` to the line above, and passing the result into `adjustAppearanceForElement`.

Comment on attachment 455054 [details]
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.

Created attachment 455102 [details]
Patch

Created attachment 455103 [details]
PFL once EWS is happy

Created attachment 455266 [details]
Patch

Committed r291588 (248683@main): <https://commits.webkit.org/248683@main>

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

<rdar://problem/90600712>

*** Bug 223980 has been marked as a duplicate of this bug. ***