[webkit-reviews] review denied: [Bug 37514] [CSS3 Backgrounds and Borders] Implement CSS3 background-position offsets : [Attachment 135560] Proposed 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 135560: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=135560&action=review

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