Bug 103440 – [CSS3 Backgrounds and Borders] Implement CSS3 background-position offsets rendering.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Alexis Menard (darktears)]

[CSS3 Backgrounds and Borders] Implement CSS3 background-position offsets rendering.

[Alexis Menard (darktears)]

Created attachment 176314 [details]
Patch

[Early Warning System Bot]

Comment on attachment 176314 [details]
Patch

Attachment 176314 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15026123

[Early Warning System Bot]

Comment on attachment 176314 [details]
Patch

Attachment 176314 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15000751

[Alexis Menard (darktears)]

Created attachment 176319 [details]
Fix build

[Julien Chaffraix]

Comment on attachment 176319 [details]
Fix build

View in context: https://bugs.webkit.org/attachment.cgi?id=176319&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:599
> +static PassRefPtr<CSSValueList> createPositionListForLayer(CSSPropertyID propertyID, const FillLayer* layer, const RenderStyle* style)

This won't build when CSS3_BACKGROUND is disabled as propertyId is unused.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:603
> +    if (propertyID == CSSPropertyBackgroundPosition && layer->isBackgroundOriginSet())

isBackgroundOriginSet implies propertyID == CSSPropertyBackgroundPosition so we could simply the if and have an ASSERT.

> Source/WebCore/css/CSSToStyleMap.cpp:232
> +    if (propId == CSSPropertyBackgroundPositionX && pair) {

Shouldn't it be impossible to get a Pair for any other value but background-position?

> Source/WebCore/css/CSSToStyleMap.cpp:236
> +        xOrigin = *(pair->first());

I don't see the need for xOrigin, knowing that you need to check pair->first() below anyway.

> Source/WebCore/rendering/style/FillLayer.cpp:41
> +#if ENABLE(CSS3_BACKGROUND)
> +    unsigned m_bitfields2: 1;
> +#endif

Doh' that means that we are growing the structure by 4 bytes which is uncool.

Some (probably crazy) ideas:
* Couldn't we find some relation between the existing bit-sets to save one bit and thus make this neutral?
* do we really need m_backgroundOriginSet as reading the default value (if unset) would probably work
* there are 2 types of FillLayer and maybe splitting them in 2 would help save some bits?

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

AFAICT the new enums don't use the E-prefix.

[Build Bot]

Comment on attachment 176319 [details]
Fix build

Attachment 176319 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15025188

[Alexis Menard (darktears)]

Created attachment 176461 [details]
Patch

[EFL EWS Bot]

Comment on attachment 176461 [details]
Patch

Attachment 176461 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15031215

[Early Warning System Bot]

Comment on attachment 176461 [details]
Patch

Attachment 176461 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15025344

[Early Warning System Bot]

Comment on attachment 176461 [details]
Patch

Attachment 176461 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15028382

[Alexis Menard (darktears)]

(In reply to comment #5)
> (From update of attachment 176319 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176319&action=review
> 
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:599
> > +static PassRefPtr<CSSValueList> createPositionListForLayer(CSSPropertyID propertyID, const FillLayer* layer, const RenderStyle* style)
> 
> This won't build when CSS3_BACKGROUND is disabled as propertyId is unused.

Fixed.

> 
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:603
> > +    if (propertyID == CSSPropertyBackgroundPosition && layer->isBackgroundOriginSet())
> 
> isBackgroundOriginSet implies propertyID == CSSPropertyBackgroundPosition so we could simply the if and have an ASSERT.

Fixed.

> 
> > Source/WebCore/css/CSSToStyleMap.cpp:232
> > +    if (propId == CSSPropertyBackgroundPositionX && pair) {
> 
> Shouldn't it be impossible to get a Pair for any other value but background-position?
> 
> > Source/WebCore/css/CSSToStyleMap.cpp:236
> > +        xOrigin = *(pair->first());
> 
> I don't see the need for xOrigin, knowing that you need to check pair->first() below anyway.
> 

Fixed.

> > Source/WebCore/rendering/style/FillLayer.cpp:41
> > +#if ENABLE(CSS3_BACKGROUND)
> > +    unsigned m_bitfields2: 1;
> > +#endif
> 
> Doh' that means that we are growing the structure by 4 bytes which is uncool.
> 
> Some (probably crazy) ideas:
> * Couldn't we find some relation between the existing bit-sets to save one bit and thus make this neutral?

The only relation I could find is xPos,yPos. But it doesn't help, see below

> * do we really need m_backgroundOriginSet as reading the default value (if unset) would probably work

Sure as the CSS 2.1 is in fact using top left corner as a reference. It could work but it will bring a problem to return the computed style for this case.

30px 50px;

is equivalent in term of rendering as :

left 30px top 50px;

but the return value of the computed style is different. Without the flag m_backgroundOriginSet I won't be able to return left 30px top 50px as I won't be able to tell the difference between the two (from FillLayer POV).

> * there are 2 types of FillLayer and maybe splitting them in 2 would help save some bits?

We could save the type bit only as the rest is used for both mask and background. In the other hand it would make every call site using FillLayer much more complex (as I would need to static_cast or some other trickery).

> 
> > Source/WebCore/rendering/style/RenderStyleConstants.h:157
> > +// CSS3 Background Position
> > +enum EBackgroundEdgeOrigin { TopEdge, RightEdge, BottomEdge, LeftEdge };
> 
> AFAICT the new enums don't use the E-prefix.

In fact none of the background related enums are using the E prefix for their enum values, e.g. EFillAttachment, EFillBox, EFillRepeat, EFillLayerType, EFillSizeType. In fact in the file I was trying to understand the pattern of naming but couldn't figure it out.

[Alexis Menard (darktears)]

Created attachment 176465 [details]
Patch

[Julien Chaffraix]

Comment on attachment 176465 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176465&action=review

Sorry but I didn't have time to look at the test cases.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:612
> +    if (propertyID == CSSPropertyBackgroundPosition && layer->isBackgroundOriginSet()) {

You forgot the propertyID check.

> Source/WebCore/css/CSSToStyleMap.cpp:233
> +        if (!pair->second() || !pair->first())
> +            return;

AFAICT you are guaranteed that if you have a pair, both entries will be filled. Thus this code could be removed, replaced by an ASSERT (or kept for defense in depth).

> Source/WebCore/css/CSSToStyleMap.cpp:250
> +    if (pair && pair->first())

You already checked first above.

> Source/WebCore/css/CSSToStyleMap.cpp:272
> +        if (!pair->second() || !pair->first())
> +            return;

Same comments.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1233
> +        int xp = xPosition + left;

I don't really like xp as it isn't very explicit. I would probably just revamp the naming:
* xPosition -> computedXPosition or usedXPosition
* xp -> finalXPosition or just xPosition (alternatively: positionInsideLayer or positionInsideBackground as you add left after see below)

I would also introduce a variable for positioningAreaSize.width() - geometry.tileSize().width(), likely availableWidth as you could reuse it below.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1237
> +#if ENABLE(CSS3_BACKGROUND)
> +        if (fillLayer->backgroundXOrigin() == RightEdge)
> +            xp = left + positioningAreaSize.width() - geometry.tileSize().width() - xPosition;
> +#endif

It looks dumb to add |left| to |xp| above and then completely change |xp|. How about:
int finalPosition = 0;
#if ENABLE(CSS3_BACKGROUND)
   if (fillLayer->backgroundXOrigin() == RightEdge)
       finalPosition = positioningAreaSize.width() - geometry.tileSize().width() - xPosition;
   else
#endif
      finalPosition = xPosition;

[Alexis Menard (darktears)]

Created attachment 176700 [details]
Patch

[Julien Chaffraix]

Comment on attachment 176700 [details]
Patch

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?

[Alexis Menard (darktears)]

Created attachment 177010 [details]
Patch for landing

[Alexis Menard (darktears)]

Created attachment 177011 [details]
Patch for landing

[Early Warning System Bot]

Comment on attachment 177011 [details]
Patch for landing

Attachment 177011 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15061450

[Early Warning System Bot]

Comment on attachment 177011 [details]
Patch for landing

Attachment 177011 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15059473

[Alexis Menard (darktears)]

Created attachment 177013 [details]
Patch for landing

[Build Bot]

Comment on attachment 177013 [details]
Patch for landing

Attachment 177013 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15065348

[Alexis Menard (darktears)]

Created attachment 177058 [details]
Patch for landing

[Alexis Menard (darktears)]

Created attachment 177060 [details]
Patch for landing (Mac build fix)

[Alexis Menard (darktears)]

Created attachment 177067 [details]
Patch for landing (Mac build fix)

[Alexis Menard (darktears)]

Created attachment 177068 [details]
Patch for landing (Mac build fix)

[Alexis Menard (darktears)]

Created attachment 177100 [details]
Patch for landing (Mac build fix)

[WebKit Review Bot]

Attachment 177100 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
LayoutTests/platform/chromium/TestExpectations:220:  Path does not exist.  [test/expectations] [5]
LayoutTests/platform/mac/TestExpectations:1005:  Path does not exist.  [test/expectations] [5]
LayoutTests/platform/efl/TestExpectations:397:  Path does not exist.  [test/expectations] [5]
Source/WebCore/css/CSSToStyleMap.cpp:236:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/css/CSSToStyleMap.cpp:276:  Tab found; better to use spaces  [whitespace/tab] [1]
LayoutTests/platform/win/TestExpectations:57:  Path does not exist.  [test/expectations] [5]
LayoutTests/platform/gtk/TestExpectations:285:  Path does not exist.  [test/expectations] [5]
LayoutTests/platform/qt/TestExpectations:338:  Path does not exist.  [test/expectations] [5]
Total errors found: 8 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexis Menard (darktears)]

I give up. I'll make the patch on Monday. Here at home windows doesn't want to do the right thing

[Build Bot]

Comment on attachment 177100 [details]
Patch for landing (Mac build fix)

Attachment 177100 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15058873

[Alexis Menard (darktears)]

Created attachment 177220 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 177220 [details]
Patch for landing

Clearing flags on attachment: 177220

Committed r136378: <http://trac.webkit.org/changeset/136378>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.