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

Wed Apr 18 21:13:35 PDT 2012

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

Julien Chaffraix <jchaffraix at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #135560|review?, commit-queue?      |review-
               Flag|                            |

--- Comment #13 from Julien Chaffraix <jchaffraix at webkit.org>  2012-04-18 21:13:34 PST ---
(From update of attachment 135560)
View in context: https://bugs.webkit.org/attachment.cgi?id=135560&action=review

It's difficult to actually see all that you changed because this change is huge. I would advise you to split it into a refactoring that aligns the code with what you want and the real change that introduces the new syntax. As-is, it's definitely not a good change due to the un-intended changes, the loss of CSSValuePool and everything I have missed as I haven't spend much time looking at whether the extended grammar matches the spec.

> Source/WebCore/ChangeLog:3
> +        Background-position parsing more than 2 values

This should be the bug number here. If you just run Tools/Scripts/prepare-ChangeLog, it would fill this part.

> Source/WebCore/ChangeLog:24
> +        Create value from keyword left/top - 0%, center - 50%, right/bottom - 100%.

Your ChangeLog is lacking the details: what are you allowing as part of this change?

> Source/WebCore/css/CSSParser.cpp:3283
> +            return CSSPrimitiveValue::createIdentifier(id);

AFAICT you want to use CSSValuePool as much as possible as it does some internal caching so this should go through CSSValuePool.

> Source/WebCore/css/CSSParser.cpp:3291
> +void CSSParser::parseFillPosition(CSSParserValueList* valueList, RefPtr<CSSValue>& value1, RefPtr<CSSValue>& value2, unsigned int numValues)

parseFillPosition is shared with -webkit-mask-position and several other properties are also using the function. This is never mentioned in your change that this is a side effect and the lack of testing makes me think you didn't see that.

I haven't found some standard information about -webkit-mask-position so I cannot tell if this is a desirable change. Each properties needs to be evaluated as you may be deviating from the existing standards.

> Source/WebCore/css/CSSParser.cpp:3301
> +        if (values.size() == numValues)

I am not sure what is the use of numValues here. First the name is so vague as to be confusing (what does it represents? Could you use that to rename the variable to something helping). Then I would have expected it to reflect something (like the size of some vectors or passed in variable) but it bears no relation to anything. Third it seems completely arbitrary that the default value is 0 (for which btw this condition will never be true as you always append something above).

> Source/WebCore/css/CSSParser.cpp:-3297
> -        // Only one value was specified.  If that value was not a keyword, then it sets the x position, and the y position
> -        // is simply 50%.  This is our default.
> -        // For keywords, the keyword was either an x-keyword (left/right), a y-keyword (top/bottom), or an ambiguous keyword (center).
> -        // For left/right/center, the default of 50% in the y is still correct.

Removing useful comments is really NOT OK. This comment explains why the magic constant 50% below in the '1' case.

> Source/WebCore/rendering/style/FillLayer.cpp:25
> +#include "CSSValueKeywords.h"

That's a layering violation. The whole purpose of the rendering/style/ classes is to hold the computed style but also to isolate the rendering code from the CSS code.

The proper way is to introduce an enum in RenderStyleConstants.h to hold the computed values.

> LayoutTests/ChangeLog:14
> +        * css3/calc/resources/smiley.png: Added.

Where did you get this file? You need the right to this image for us to integrate it and I am not sure this is the case here. Preferably re-use an existing image.

> LayoutTests/ChangeLog:15
> +        * platform/chromium/test_expectations.txt: Skip background-position test

Why is that fine to make one of the IE test to fail? You need a good justification here before skipping it. Also such changes need to be done on *all* platforms that have a baseline, not on the only EWS that runs the test.

> LayoutTests/css3/calc/background-position.html:1
> +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">

We usually prefer the HTML5 doctype for WebKit-specific test: <!DOCTYPE html>

> LayoutTests/css3/calc/background-position.html:2
> +<html lang="en">

No lang please.

> LayoutTests/css3/calc/background-position.html:5
> +  <style type="text/css">

No type.

> LayoutTests/css3/calc/background-position.html:6
> +    .case { width: 128px; height: 128px; float: left; border: solid; margin: 0.2em; }

You could easily remove the case class and use instead the div tag selector (div { .... })

> LayoutTests/css3/calc/background-position.html:7
> +    .a, .b, .c, .d, .e, .f, .g, .h, .i, .j, .k, .l, .m, .n, .o, .p, .r, .s, .t, .u, .v, .w, .x1, .x2, .x3, .x4, .x5, .x6

no .y? Anyway, it's pretty confusing to use just letters for those classes and it doesn't make the test case easy to read.

> LayoutTests/css3/calc/background-position.html:14
> +    .case .a { background-position: left 25% top 25%; }

Not a huge fan of the combined selectors here: .a should be enough no?

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