[Webkit-unassigned] [Bug 37514] [CSS3 Backgrounds and Borders] Implement CSS3 background-position offsets

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=37514


Julien Chaffraix <jchaffraix at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #138085|review?                     |review-
               Flag|                            |




--- Comment #16 from Julien Chaffraix <jchaffraix at webkit.org>  2012-05-03 15:49:52 PST ---
(From update of attachment 138085)
View in context: https://bugs.webkit.org/attachment.cgi?id=138085&action=review

To be frank, I think your approach of coming and implementing the whole grammar (even worse the grammars of different CSS values) is going to be painful: you have little experience in WebKit and > 20k patches from someone with little experience in the WebKit way have little change to get landed (also they are a reviewer time sink)

Think about this incrementally: which step as small as possible can you take towards supporting the full grammar while getting your change tested! It's important to keep your changes small and tested: this helps reviewers assess the quality of your change.

My take here would be to add parsing support for one <position> first, then add the needed rendering. Once this is done, scale to 2 <position>, ... Also this bug is about 'background-position', it is good that you are thinking about other values but don't get ahead of your self: getting 'background-position' is enough work for now.

I haven't read the whole patch because I think it should be split and tested.

> Source/WebCore/ChangeLog:11
> +        Currently parser supports only parsing of two values for background-position property.
> +        According to CSS specification http://www.w3.org/TR/css3-background/#the-background-position,
> +        it can take upto four values. This parsing function can also be reused for parsing other
> +        properties like -transform-origin, -perspective-origin, -webkit-mask-position and -radial-gradient.

Saying what grammar you *do* support would be helpful instead of having to extract it from the code.

> Source/WebCore/ChangeLog:13
> +        No functionality change. Tests to be added in next patch.

Technically this is a change in functionality. It's not exposed in JavaScript as you don't implement the getComputedStyle part though. Pushing the tests in another bug is not going to work as we expect changes in functionality to be tested.

> Source/WebCore/css/CSSParser.cpp:3225
> +    if (value->primitiveType() == CSSPrimitiveValue::CSS_IDENT) {
> +        int key = value->getIdent();
> +        return key == CSSValueLeft || key == CSSValueRight
> +            || key == CSSValueTop || key == CSSValueBottom || key == CSSValueCenter;
> +    }
> +    return false;

This should be:

return isBackgroundKeywordX(value) || isBackgroundKeyword(value) || isBackgroundKeywordC(value);

> Source/WebCore/css/CSSParser.cpp:3243
> +    if (value->primitiveType() == CSSPrimitiveValue::CSS_IDENT) {
> +        int key = value->getIdent();
> +        return key == CSSValueTop || key == CSSValueBottom;
> +    }
> +    return false;

We prefer early return (not repeated):
if (!value->isIdent())
   return false;
int identity = value->getIdent();
return identity == ...;

(btw, |key| is not a good variable name as it doesn't bear any meaning)

> Source/WebCore/css/CSSParser.cpp:3246
> +static bool isBackgroundKeywordC(CSSPrimitiveValue* value)

The naming doesn't cut. I guess C as Center but please don't abbreviate needlessly your names.

> Source/WebCore/rendering/style/FillLayer.h:208
> +    bool m_xOriginSet : 1;
> +    bool m_yOriginSet : 1;

The whole pattern of having a bool for being set + the value above makes me think we should have some structure to do that for us. We would need to be smart about it.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.