[webkit-reviews] review granted: [Bug 103440] [CSS3 Backgrounds and Borders] Implement CSS3 background-position offsets rendering. : [Attachment 176700] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Alexis Menard (darktears)
<alexis at webkit.org>'s request for review:
Bug 103440: [CSS3 Backgrounds and Borders] Implement CSS3 background-position
offsets rendering.
https://bugs.webkit.org/show_bug.cgi?id=103440

Attachment 176700: Patch
https://bugs.webkit.org/attachment.cgi?id=176700&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=176700&action=review


The code is good with some comments. The test needs to be fixed before landing,
please ensure that it fits into the viewport. If you do another round, I would
like to check the test.

> Source/WebCore/ChangeLog:32
> +	   variables for better readibility.

typo: readability.

> Source/WebCore/css/CSSToStyleMap.cpp:232
> +	   ASSERT(pair->second() && pair->first());

Didn't you want to assert that propertyID == CSSValueBackgroundPosition?

> Source/WebCore/css/CSSToStyleMap.cpp:270
> +	   ASSERT(pair->second() && pair->first());

Ditto?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1241
> +	   finalXPosition = computedXPosition + left;

Technically missing indentation here as it's part of the else. I would put left
in front to match what's inside the #if.

Looking at that again, this code could probably simplify be simplified if you
still exposed backgroundXOrigin() even if we disable CSS3_BACKGROUND (which
would be hardcoded to return LeftEdge):

int xOffset = fillLayer->backgroundXOrigin() == LeftEdge ? computedXPosition :
availableWidth - computedXPosition;
geometry.setNoRepeatX(left + xOffset);

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1254
> +	   finalYPosition = computedYPosition + top;

Same comment here.

> Source/WebCore/rendering/style/RenderStyleConstants.h:157
> +// CSS3 Background Position
> +enum EBackgroundEdgeOrigin { TopEdge, RightEdge, BottomEdge, LeftEdge };

As said, I would rather keep consistency with the rest of the code and our
coding style that with some super old enums by dropping the 'E' prefix. Feel
free to push back and ask that on webkit-dev if you disagree.

> LayoutTests/fast/backgrounds/background-position-rendering-expected.html:5
> +    <style type="text/css">

Unneeded type attribute.

> LayoutTests/fast/backgrounds/background-position-rendering.html:25
> +    #eighteen { background-image: url("./resources/diamond.png"),
url("./resources/ring.png"); background-position: center, center; }

Ideally you would want to test background-color too.

> LayoutTests/fast/backgrounds/background-position-rendering.html:31
> +	   <td><div id="one"></div></td>

Note that you could cut the div here and put the style on the table cells.
That's a nit though as table cells are different renderers with their own
quirks.

> LayoutTests/fast/backgrounds/background-position-rendering.html:57
> +    <tr>
> +	   <td><div id="thirteen"></div></td>
> +	   <td><div id="fourteen"></div></td>
> +	   <td><div id="fifteen"></div></td>
> +	   <td><div id="sixteen"></div></td>
> +    </tr>
> +    <tr>
> +	   <td><div id="seventeen"></div></td>
> +	   <td><div id="eighteen"></div></td>
> +    </tr>

I don't think these divs are tested: DRT uses a 800 * 600 pixel viewport and
thus anything outside the area will not be dumped.

Nit: let's try to keep the table regular (the last row has only 2 cells)

> LayoutTests/platform/chromium/TestExpectations:221
> +webkit.org/b/37514 fast/backgrounds/background-position-rendering.html [
Text ]

Shouldn't it be an [ Image ] difference?