[webkit-reviews] review denied: [Bug 37514] [CSS3 Backgrounds and Borders] Implement CSS3 background-position offsets : [Attachment 138085] Draft patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Uday Kiran
<udaykiran at motorola.com>'s request for review:
Bug 37514: [CSS3 Backgrounds and Borders] Implement CSS3 background-position
offsets
https://bugs.webkit.org/show_bug.cgi?id=37514

Attachment 138085: Draft patch
https://bugs.webkit.org/attachment.cgi?id=138085&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
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.