[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





--- Comment #17 from Uday Kiran <udaykiran at motorola.com>  2012-05-03 23:13:47 PST ---
(From update of attachment 138085)
View in context: https://bugs.webkit.org/attachment.cgi?id=138085&action=review

Thanks for reviewing.
Rendering code for one or two <position> is already present. So my approach here was to reduce three or four <position> into two <position>. 
There is a special case when offset in pixels from right or bottom is specified where rendering support is needed.
I will upload a patch which only handles background-position and addressing review comments along with test cases.

>> Source/WebCore/ChangeLog:11
>> +        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.

I will make changes such that only background-position specification is handled. Other properties will be handled in different bug to keep the patch smaller and easier for review.

>> 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.

This is a draft patch where only parsing functions are written but not called from anywhere. As per your previous suggestion, I was splitting up bigger patch into parsing code and real patch that introduces the change. I meant to add tests to this bug itself when real patch would be uploaded as this draft patch itself was getting bigger.

>> Source/WebCore/css/CSSParser.cpp:3225
>> +    return false;
> 
> This should be:
> 
> return isBackgroundKeywordX(value) || isBackgroundKeyword(value) || isBackgroundKeywordC(value);

Done.

>> Source/WebCore/css/CSSParser.cpp:3243
>> +    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)

Done.

>> 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.

Done.

>> Source/WebCore/rendering/style/FillLayer.h:208
>> +    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.

Right. But shouldn't that be done as different patch as this patch is already getting bigger and we would be touching code related to other fill properties?

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