[webkit-reviews] review denied: [Bug 111121] getComputedStyle not working with more than one background image. : [Attachment 445160] Patch

[bugzilla-daemon at webkit.org]

Cameron McCormack (:heycam) <heycam at apple.com> has denied Matt Woodrow
<m_woodrow at apple.com>'s request for review:
Bug 111121: getComputedStyle not working with more than one background image.
https://bugs.webkit.org/show_bug.cgi?id=111121

Attachment 445160: Patch

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




--- Comment #8 from Cameron McCormack (:heycam) <heycam at apple.com> ---
Comment on attachment 445160
  --> https://bugs.webkit.org/attachment.cgi?id=445160
Patch

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

Patch is good but I'd like to see the next version. Some comments and questions
below.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4451
> +size_t ComputedStyleExtractor::getLayerCount(CSSPropertyID property)
> +{

You have the assertion about `property` in getBackgroundPropertyShorthandValue,
but I'd repeat it in here too.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4452
> +    auto* styledElement = this->styledElement();

Can drop the "this->". (Not sure why some other functions in this file do
that.)

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4469
> +Ref<CSSValueList>
ComputedStyleExtractor::getBackgroundPropertyShorthandValue(CSSPropertyID
property, const StylePropertyShorthand& propertiesBeforeSlashSeperator, const
StylePropertyShorthand& propertiesAfterSlashSeperator, CSSPropertyID
lastLayerProperty)

Maybe "getFillLayerPropertyShorthandValue" (after the naming of the FillLayer
type)? I don't really think of "mask" as a being background-ish.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4475
> +    auto bgColor = lastLayerProperty != CSSPropertyInvalid ?
propertyValue(CSSPropertyBackgroundColor, DoNotUpdateLayout) : nullptr;

Feels a bit weird to pass in a property for lastLayerProperty but use
CSSPropertyBackgroundColor here. Maybe call bgColor "lastValue" and pass in
lastLayerProperty to propertyValue()?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4488
> +	   // Background color is a singleton, and should only be prepended to
the last layer.

WebKit style is to avoid comments that describe what the code is doing, if it's
reasonably clear what the code is doing by reading it. (And to make code clear
enough that a comment isn't required, if possible.) So I would probably drop
this comment.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4494
> +	       beforeList->append(layerCount == 1 ? value :
*downcast<CSSValueList>(value).item(i));

When layerCount == 1, is value a CSSValueList or a single value? If it's a
list, we should append item(i).

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4497
> +

Do we need to append the slash in here?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4506
> +	   // If there was only a single layer, then don't bother wrapping in
the
> +	   // layers list.

And I'd drop this one too.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4520
> +    static const CSSPropertyID propertiesBeforeSlashSeperator[4] = {
CSSPropertyBackgroundImage, CSSPropertyBackgroundRepeat,
CSSPropertyBackgroundAttachment, CSSPropertyBackgroundPosition };
>      static const CSSPropertyID propertiesAfterSlashSeperator[3] = {
CSSPropertyBackgroundSize, CSSPropertyBackgroundOrigin,
CSSPropertyBackgroundClip };

Do you know what other browsers do in terms of the ordering of the longhand
components? The general rule should be to follow the grammar order in the spec,
unless there is some other explicit rules about serialization, and this
ordering here doesn't match what's in the spec.

Also we can drop the explicit array lengths.