[CSS3 Backgrounds and Borders] Implement CSS3 background-position offsets rendering.
Created attachment 176314 [details] Patch
Comment on attachment 176314 [details] Patch Attachment 176314 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15026123
Comment on attachment 176314 [details] Patch Attachment 176314 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15000751
Created attachment 176319 [details] Fix build
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 on attachment 176319 [details] Fix build Attachment 176319 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15025188
Created attachment 176461 [details] Patch
Comment on attachment 176461 [details] Patch Attachment 176461 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15031215
Comment on attachment 176461 [details] Patch Attachment 176461 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15025344
Comment on attachment 176461 [details] Patch Attachment 176461 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15028382
(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.
Created attachment 176465 [details] Patch
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;
Created attachment 176700 [details] Patch
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?
Created attachment 177010 [details] Patch for landing
Created attachment 177011 [details] Patch for landing
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 on attachment 177011 [details] Patch for landing Attachment 177011 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15059473
Created attachment 177013 [details] Patch for landing
Comment on attachment 177013 [details] Patch for landing Attachment 177013 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15065348
Created attachment 177058 [details] Patch for landing
Created attachment 177060 [details] Patch for landing (Mac build fix)
Created attachment 177067 [details] Patch for landing (Mac build fix)
Created attachment 177068 [details] Patch for landing (Mac build fix)
Created attachment 177100 [details] Patch for landing (Mac build fix)
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.
I give up. I'll make the patch on Monday. Here at home windows doesn't want to do the right thing
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
Created attachment 177220 [details] Patch for landing
Comment on attachment 177220 [details] Patch for landing Clearing flags on attachment: 177220 Committed r136378: <http://trac.webkit.org/changeset/136378>
All reviewed patches have been landed. Closing bug.