Bug 103440 - [CSS3 Backgrounds and Borders] Implement CSS3 background-position offsets rendering.
: [CSS3 Backgrounds and Borders] Implement CSS3 background-position offsets ren...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Alexis Menard (darktears)
: WebExposed
Depends on:
Blocks: 37514
  Show dependency treegraph
 
Reported: 2012-11-27 11:29 PST by Alexis Menard (darktears)
Modified: 2012-12-03 03:56 PST (History)
13 users (show)

See Also:


Attachments
Patch (52.70 KB, patch)
2012-11-27 11:55 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Fix build (52.74 KB, patch)
2012-11-27 12:12 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (52.02 KB, patch)
2012-11-28 05:29 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (52.02 KB, patch)
2012-11-28 05:43 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (53.20 KB, patch)
2012-11-29 04:37 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch for landing (55.69 KB, patch)
2012-11-30 12:27 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch for landing (55.69 KB, patch)
2012-11-30 12:29 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch for landing (55.76 KB, patch)
2012-11-30 12:43 PST, Alexis Menard (darktears)
buildbot: commit‑queue-
Details | Formatted Diff | Diff
Patch for landing (56.92 KB, patch)
2012-11-30 17:16 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch for landing (Mac build fix) (56.93 KB, patch)
2012-11-30 17:17 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch for landing (Mac build fix) (57.01 KB, patch)
2012-11-30 17:47 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch for landing (Mac build fix) (55.86 KB, patch)
2012-11-30 17:53 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch for landing (Mac build fix) (46.69 KB, patch)
2012-12-01 04:09 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch for landing (55.93 KB, patch)
2012-12-03 03:21 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexis Menard (darktears) 2012-11-27 11:29:36 PST
[CSS3 Backgrounds and Borders] Implement CSS3 background-position offsets rendering.
Comment 1 Alexis Menard (darktears) 2012-11-27 11:55:44 PST
Created attachment 176314 [details]
Patch
Comment 2 Early Warning System Bot 2012-11-27 12:06:07 PST
Comment on attachment 176314 [details]
Patch

Attachment 176314 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15026123
Comment 3 Early Warning System Bot 2012-11-27 12:08:05 PST
Comment on attachment 176314 [details]
Patch

Attachment 176314 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15000751
Comment 4 Alexis Menard (darktears) 2012-11-27 12:12:38 PST
Created attachment 176319 [details]
Fix build
Comment 5 Julien Chaffraix 2012-11-27 15:25:10 PST
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.
Comment 6 Build Bot 2012-11-27 18:21:27 PST
Comment on attachment 176319 [details]
Fix build

Attachment 176319 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15025188
Comment 7 Alexis Menard (darktears) 2012-11-28 05:29:58 PST
Created attachment 176461 [details]
Patch
Comment 8 EFL EWS Bot 2012-11-28 05:37:07 PST
Comment on attachment 176461 [details]
Patch

Attachment 176461 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15031215
Comment 9 Early Warning System Bot 2012-11-28 05:38:27 PST
Comment on attachment 176461 [details]
Patch

Attachment 176461 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15025344
Comment 10 Early Warning System Bot 2012-11-28 05:39:55 PST
Comment on attachment 176461 [details]
Patch

Attachment 176461 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15028382
Comment 11 Alexis Menard (darktears) 2012-11-28 05:40:04 PST
(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.
Comment 12 Alexis Menard (darktears) 2012-11-28 05:43:10 PST
Created attachment 176465 [details]
Patch
Comment 13 Julien Chaffraix 2012-11-28 17:33:57 PST
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;
Comment 14 Alexis Menard (darktears) 2012-11-29 04:37:02 PST
Created attachment 176700 [details]
Patch
Comment 15 Julien Chaffraix 2012-11-29 13:45:47 PST
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?
Comment 16 Alexis Menard (darktears) 2012-11-30 12:27:01 PST
Created attachment 177010 [details]
Patch for landing
Comment 17 Alexis Menard (darktears) 2012-11-30 12:29:02 PST
Created attachment 177011 [details]
Patch for landing
Comment 18 Early Warning System Bot 2012-11-30 12:39:17 PST
Comment on attachment 177011 [details]
Patch for landing

Attachment 177011 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15061450
Comment 19 Early Warning System Bot 2012-11-30 12:40:42 PST
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
Comment 20 Alexis Menard (darktears) 2012-11-30 12:43:21 PST
Created attachment 177013 [details]
Patch for landing
Comment 21 Build Bot 2012-11-30 14:15:58 PST
Comment on attachment 177013 [details]
Patch for landing

Attachment 177013 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15065348
Comment 22 Alexis Menard (darktears) 2012-11-30 17:16:37 PST
Created attachment 177058 [details]
Patch for landing
Comment 23 Alexis Menard (darktears) 2012-11-30 17:17:45 PST
Created attachment 177060 [details]
Patch for landing (Mac build fix)
Comment 24 Alexis Menard (darktears) 2012-11-30 17:47:47 PST
Created attachment 177067 [details]
Patch for landing (Mac build fix)
Comment 25 Alexis Menard (darktears) 2012-11-30 17:53:53 PST
Created attachment 177068 [details]
Patch for landing (Mac build fix)
Comment 26 Alexis Menard (darktears) 2012-12-01 04:09:28 PST
Created attachment 177100 [details]
Patch for landing (Mac build fix)
Comment 27 WebKit Review Bot 2012-12-01 04:12:50 PST
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.
Comment 28 Alexis Menard (darktears) 2012-12-01 06:21:53 PST
I give up. I'll make the patch on Monday. Here at home windows doesn't want to do the right thing
Comment 29 Build Bot 2012-12-01 10:05:54 PST
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
Comment 30 Alexis Menard (darktears) 2012-12-03 03:21:52 PST
Created attachment 177220 [details]
Patch for landing
Comment 31 WebKit Review Bot 2012-12-03 03:56:23 PST
Comment on attachment 177220 [details]
Patch for landing

Clearing flags on attachment: 177220

Committed r136378: <http://trac.webkit.org/changeset/136378>
Comment 32 WebKit Review Bot 2012-12-03 03:56:30 PST
All reviewed patches have been landed.  Closing bug.