Mask/background-repeat: round should round the number of tiles of an image to the nearest natural n…

Mask/background-repeat: round should round the number of tiles of an image to the nearest natural number greater than 0.

BUG=260691
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=158017

[Andrei Parvu, 2013-09-13 09:05:37]

According to the spec [1] mask/background-repeat: round should round the number
of tiles of an image to the nearest natural number greater than 0. Thus,
replaced ceil function with roundToInt.

[1] - http://www.w3.org/TR/css3-background/#background-size

[Julien - ping for review, 2013-09-16 20:05:25]

https://codereview.chromium.org/24066007/diff/1/Source/core/rendering/RenderB...
File Source/core/rendering/RenderBoxModelObject.cpp (right):

https://codereview.chromium.org/24066007/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBoxModelObject.cpp:1065: int nrTiles =
roundToInt((float)positioningAreaSize.width() /
roundToInt takes a LayoutUnit which means that this line implicitly converts to
LayoutUnit and then back to int.

Can't you just use round from wtf/MathExtras.h which takes a double?

https://codereview.chromium.org/24066007/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBoxModelObject.cpp:1066: fillTileSize.width());
This line break is not needed nor advisable per Blink's coding style (we don't
have line limit)

[Andrei Parvu, 2013-09-17 09:14:53]

Thanks for the review, Julien. Didn't know about the round function from
wtf/MathExtras.h - replaced roundToInt with it.

[eseidel, 2013-09-17 15:48:44]

https://codereview.chromium.org/24066007/diff/6001/Source/core/rendering/Rend...
File Source/core/rendering/RenderBoxModelObject.cpp (right):

https://codereview.chromium.org/24066007/diff/6001/Source/core/rendering/Rend...
Source/core/rendering/RenderBoxModelObject.cpp:1065: int nrTiles =
static_cast<int>(round((float)positioningAreaSize.width() /
fillTileSize.width()));
We don't have a helper function do to this for us?  roundl?

[Julien - ping for review, 2013-09-17 16:26:26]

https://codereview.chromium.org/24066007/diff/6001/Source/core/rendering/Rend...
File Source/core/rendering/RenderBoxModelObject.cpp (right):

https://codereview.chromium.org/24066007/diff/6001/Source/core/rendering/Rend...
Source/core/rendering/RenderBoxModelObject.cpp:1065: int nrTiles =
static_cast<int>(round((float)positioningAreaSize.width() /
fillTileSize.width()));
On 2013/09/17 15:48:44, eseidel wrote:
> We don't have a helper function do to this for us? roundl?

lround would do the trick (or lroundf if we want to keep single precision
arithmetic). There is one issue with this code though: it can overflow the int.
Reading roundl (and ceil(3) manual page), you can't just cast to int without a
check.

Is there a good reason the logic doesn't use long?

Also if you use single precision, let's use the function that take a single
precision to avoid implicit conversions (I don't have mind use single vs double
precision but let's be consistent).

[eseidel, 2013-09-17 16:33:10]

We also have saturated arithmetic functions for LayoutUnit if that's applicable
here?

[Julien - ping for review, 2013-09-17 17:28:57]

On 2013/09/17 16:33:10, eseidel wrote:
> We also have saturated arithmetic functions for LayoutUnit if that's
applicable
> here?

It's unfortunately not, the original patch did an unneeded round-trip through
LayoutUnit, which is why we changed.

[Andrei Parvu, 2013-09-17 17:50:09]

Changed variable type to long and used lroundf function.

[eseidel, 2013-09-17 18:06:59]

https://codereview.chromium.org/24066007/diff/13001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBoxModelObject.cpp (right):

https://codereview.chromium.org/24066007/diff/13001/Source/core/rendering/Ren...
Source/core/rendering/RenderBoxModelObject.cpp:1065: long nrTiles =
lroundf((float)positioningAreaSize.width() / fillTileSize.width());
So this is intentionally causing whitespace when the author doesn't match the
background tile size precisely to the size of the div?  I'm confused.  Why
wouldn't we want to ceil here?

[eseidel, 2013-09-17 18:08:32]

lgtm

https://codereview.chromium.org/24066007/diff/13001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBoxModelObject.cpp (right):

https://codereview.chromium.org/24066007/diff/13001/Source/core/rendering/Ren...
Source/core/rendering/RenderBoxModelObject.cpp:1065: long nrTiles =
lroundf((float)positioningAreaSize.width() / fillTileSize.width());
On 2013/09/17 18:07:00, eseidel wrote:
> So this is intentionally causing whitespace when the author doesn't match the
> background tile size precisely to the size of the div?  I'm confused.  Why
> wouldn't we want to ceil here?

I see.  The author has *specifically* requested that we round instead of ceil
and that's the problem.  They want this whitespace for whatever reason. :)

[Julien - ping for review, 2013-09-17 18:37:48]

lgtm2

https://codereview.chromium.org/24066007/diff/13001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBoxModelObject.cpp (right):

https://codereview.chromium.org/24066007/diff/13001/Source/core/rendering/Ren...
Source/core/rendering/RenderBoxModelObject.cpp:1065: long nrTiles =
lroundf((float)positioningAreaSize.width() / fillTileSize.width());
On 2013/09/17 18:08:32, eseidel wrote:
> On 2013/09/17 18:07:00, eseidel wrote:
> > So this is intentionally causing whitespace when the author doesn't match
the
> > background tile size precisely to the size of the div?  I'm confused.  Why
> > wouldn't we want to ceil here?
> 
> I see.  The author has *specifically* requested that we round instead of ceil
> and that's the problem.  They want this whitespace for whatever reason. :)

This mandated by the specification.

Thanks for making the change to long even if it turns out I got confused and
it's not needed as we are taking the result of the division of 2 integers. The
new code is more future proof though.

[Andrei Parvu, 2013-09-18 06:19:46]

Thanks for the comments, guys!

[commit-bot: I haz the power, 2013-09-18 06:20:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/andrei.prv@gmail.com/24066007/13001

[commit-bot: I haz the power, 2013-09-18 07:43:47]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-09-18 11:19:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/andrei.prv@gmail.com/24066007/31001

[commit-bot: I haz the power, 2013-09-18 11:19:20]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 1305.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
82d569766a4ec272123df22de76af00930819719..9cc711452c7ed86094aad85ca877515153450316
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -1305,3 +1305,5 @@ crbug.com/292886 [ Retina ]
tables/mozilla/bugs/bug1318.html [ ImageOnlyFailure
 crbug.com/292886 [ Retina ] tables/mozilla/bugs/bug4527.html [ ImageOnlyFailure
NeedsManualRebaseline ]
 crbug.com/292886 [ Retina ]
tables/mozilla_expected_failures/core/captions1.html [ ImageOnlyFailure
NeedsManualRebaseline ]
 crbug.com/292886 [ Retina ]
tables/mozilla_expected_failures/core/captions2.html [ ImageOnlyFailure
NeedsManualRebaseline ]
+
+crbug.com/260691
ietestcenter/css3/bordersbackgrounds/background-size-aspect-ratio.htm [
NeedsRebaseline ]

[commit-bot: I haz the power, 2013-09-18 14:40:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/andrei.prv@gmail.com/24066007/36001

[commit-bot: I haz the power, 2013-09-18 15:46:57]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 1346.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
823cee6ffaa25b832a014e2a3df324225ce300bb..fd7d8f860fd1cfd9eda3c538694ff5b68101e81d
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -1346,3 +1346,5 @@ crbug.com/289833 [ Android ]
tables/hittesting/filltable-levels.html [ Timeout P
 crbug.com/289833 [ Android ]
transitions/transition-transform-translate-calculated-length-crash.html [
Timeout Pass ]
 crbug.com/289833 [ Android ] webaudio/stereo2mono-down-mixing.html [ Timeout
Pass ]
 crbug.com/289833 [ Android ] webexposed/custom-elements.html [ Timeout Pass ]
+
+crbug.com/260691
ietestcenter/css3/bordersbackgrounds/background-size-aspect-ratio.htm [
NeedsRebaseline ]

[commit-bot: I haz the power, 2013-09-19 06:16:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/andrei.prv@gmail.com/24066007/46001

[commit-bot: I haz the power, 2013-09-19 08:01:30]

Change committed as 158017