[CSS Exclusions] Zoom causes shape-inside to fail when shape-padding is specified

[CSS Exclusions] Zoom causes shape-inside to fail when shape-padding is specified

Snap computed margin and polygon boundaries to the LayoutUnit grid to ensure that they fall within the shape's LayoutUnit bounding box.

This is a provisional change, it will be replaced by the TBD patch for https://code.google.com/p/chromium/issues/detail?id=235085

BUG=230392

[Hans Muller, 2013-04-23 15:21:04]

This is a patch for https://code.google.com/p/chromium/issues/detail?id=230392
which syncs blink with the WebKit patch for
https://bugs.webkit.org/show_bug.cgi?id=113730.

[Julien - ping for review, 2013-04-23 17:09:04]

https://codereview.chromium.org/14289003/diff/1/Source/core/rendering/Exclusi...
File Source/core/rendering/ExclusionPolygon.cpp (right):

https://codereview.chromium.org/14289003/diff/1/Source/core/rendering/Exclusi...
Source/core/rendering/ExclusionPolygon.cpp:132: static inline void
snapVerticesToLayoutUnitGrid(Vector<FloatPoint>& vertices)
Wouldn't it make more sense to have your vertices be LayoutUnits instead of
floats? Alternatively have FloatPolygon keep them as LayoutUnits?

As done, any subsequent operation(s) on your floats can yield to more instances
of this bug (not to mention that it is inefficient but that's not really a
problem for now).

https://codereview.chromium.org/14289003/diff/1/Source/core/rendering/Exclusi...
Source/core/rendering/ExclusionPolygon.cpp:135:
vertices[i].set(LayoutUnit(vertices[i].x()).toFloat(),
LayoutUnit(vertices[i].y()).toFloat());
Note that this could (and will) yield to big roundings as we have saturated
arithmetic enabled and the conversion to LayoutUnit involves a multiplication.

Also it may be nice to just use the existing FloatPoint -> LayoutPoint explicit
conversions (to choose whether you want to ceil / floor / something else)
instead of manually doing this conversion.

[Hans Muller, 2013-04-23 18:18:57]

2013/04/23 17:09:04, Julien Chaffraix wrote:

>
https://codereview.chromium.org/14289003/diff/1/Source/core/rendering/Exclusi...
> Source/core/rendering/ExclusionPolygon.cpp:132: static inline void
> snapVerticesToLayoutUnitGrid(Vector<FloatPoint>& vertices)
> Wouldn't it make more sense to have your vertices be LayoutUnits instead of
> floats? Alternatively have FloatPolygon keep them as LayoutUnits?
> 
> As done, any subsequent operation(s) on your floats can yield to more
instances
> of this bug (not to mention that it is inefficient but that's not really a
> problem for now).

I agree that there are other potential float/LayoutUnit bugs. For example CSS
shape coordinates should probably be snapped to the LayoutUnit grid before
they're processed. I haven't made that change yet because I haven't found a test
case that exposes the problem. In general I think that the shape classes'
incoming and outgoing values should be snapped to the LayoutUnit grid. I'd like
to get to that point incrementally. 

I don't think that wholesale replacing floats with LayoutUnits in the shape code
would make things more robust. To do so for FloatPolygon (vertices) would make
it inconsistent with the other FloatFoo classes. And at this stage, significant
performance gains are more likely to come from algorithm improvements than by
trying to do more computations with LayoutUnits. 

> 
>
https://codereview.chromium.org/14289003/diff/1/Source/core/rendering/Exclusi...
> Source/core/rendering/ExclusionPolygon.cpp:135:
> vertices[i].set(LayoutUnit(vertices[i].x()).toFloat(),
> LayoutUnit(vertices[i].y()).toFloat());
> Note that this could (and will) yield to big roundings as we have saturated
> arithmetic enabled and the conversion to LayoutUnit involves a multiplication.

Yes. The multiplication doesn't seem avoidable.

> Also it may be nice to just use the existing FloatPoint -> LayoutPoint
explicit
> conversions (to choose whether you want to ceil / floor / something else)
> instead of manually doing this conversion.

Is this what you're suggesting?

    vertices[i].set(LayoutUnit::fromFloatFloor(vertices[i].x),
LayoutUnit::fromFloatFloor(vertices[i].y()));

Seems reasonable to me.

[leviw_travelin_and_unemployed, 2013-04-23 18:27:20]

https://codereview.chromium.org/14289003/diff/1/Source/core/rendering/Exclusi...
> > Source/core/rendering/ExclusionPolygon.cpp:135:
> > vertices[i].set(LayoutUnit(vertices[i].x()).toFloat(),
> > LayoutUnit(vertices[i].y()).toFloat());
> > Note that this could (and will) yield to big roundings as we have saturated
> > arithmetic enabled and the conversion to LayoutUnit involves a
multiplication.
> 
> Yes. The multiplication doesn't seem avoidable.
> 
> > Also it may be nice to just use the existing FloatPoint -> LayoutPoint
> explicit
> > conversions (to choose whether you want to ceil / floor / something else)
> > instead of manually doing this conversion.
> 
> Is this what you're suggesting?
> 
>     vertices[i].set(LayoutUnit::fromFloatFloor(vertices[i].x),
> LayoutUnit::fromFloatFloor(vertices[i].y()));
> 
> Seems reasonable to me.

I'd agree this is clearer, but it still seems odd to do the conversion instead
of using the same data type.

https://codereview.chromium.org/14289003/diff/1/Source/core/rendering/Exclusi...
File Source/core/rendering/ExclusionPolygon.cpp (right):

https://codereview.chromium.org/14289003/diff/1/Source/core/rendering/Exclusi...
Source/core/rendering/ExclusionPolygon.cpp:132: static inline void
snapVerticesToLayoutUnitGrid(Vector<FloatPoint>& vertices)
On 2013/04/23 17:09:04, Julien Chaffraix wrote:
> Wouldn't it make more sense to have your vertices be LayoutUnits instead of
> floats? Alternatively have FloatPolygon keep them as LayoutUnits?
> 
> As done, any subsequent operation(s) on your floats can yield to more
instances
> of this bug (not to mention that it is inefficient but that's not really a
> problem for now).

For the record, I agree that since this issue shows that we'll have problems
when Floats don't align to the LayoutUnit grid, it would make more sense to use
them for our vertices.

[Hans Muller, 2013-04-23 18:35:54]

On 2013/04/23 18:27:20, Levi wrote:
>
https://codereview.chromium.org/14289003/diff/1/Source/core/rendering/Exclusi...
> > As done, any subsequent operation(s) on your floats can yield to more
instances
> > of this bug (not to mention that it is inefficient but that's not really a
> > problem for now).
> 
> For the record, I agree that since this issue shows that we'll have problems
> when Floats don't align to the LayoutUnit grid, it would make more sense to
use
> them for our vertices.

Are you suggesting that FloatRects also be defined in terms of LayoutUnits?

[Julien - ping for review, 2013-04-23 19:30:34]

On 2013/04/23 18:35:54, Hans Muller wrote:
> On 2013/04/23 18:27:20, Levi wrote:
> >
>
https://codereview.chromium.org/14289003/diff/1/Source/core/rendering/Exclusi...
> > > As done, any subsequent operation(s) on your floats can yield to more
> instances
> > > of this bug (not to mention that it is inefficient but that's not really a
> > > problem for now).
> > 
> > For the record, I agree that since this issue shows that we'll have problems
> > when Floats don't align to the LayoutUnit grid, it would make more sense to
> use
> > them for our vertices.
> 
> Are you suggesting that FloatRects also be defined in terms of LayoutUnits?

No, there are cases where you want full float precision. What we are saying is
the code you are touching seems like it needs to be LayoutUnit aligned which
makes it a good candidate for LayoutRect (instead of using FloatRect)

[Julien - ping for review, 2013-04-23 19:37:04]

> > Also it may be nice to just use the existing FloatPoint -> LayoutPoint
> explicit
> > conversions (to choose whether you want to ceil / floor / something else)
> > instead of manually doing this conversion.
> 
> Is this what you're suggesting?
> 
>     vertices[i].set(LayoutUnit::fromFloatFloor(vertices[i].x),
> LayoutUnit::fromFloatFloor(vertices[i].y()));
> 
> Seems reasonable to me.

It's step in the right direction but we could do more simple:

vertices[i] = flooredLayoutPoint(vertices[i]);

(flooredLayoutPoint is defined in LayoutPoint.h)

[Hans Muller, 2013-04-23 21:41:54]

On 2013/04/23 19:37:04, Julien Chaffraix wrote:
> > > Also it may be nice to just use the existing FloatPoint -> LayoutPoint
> > > explicit conversions (to choose whether you want to ceil / floor /
something else)
> > > instead of manually doing this conversion.
> ...
> It's step in the right direction but we could do more simple:
> 
> vertices[i] = flooredLayoutPoint(vertices[i]);
> 
> (flooredLayoutPoint is defined in LayoutPoint.h)

That's much cleaner, I've made the change.

[Hans Muller, 2013-04-23 21:51:32]

On 2013/04/23 19:30:34, Julien Chaffraix wrote:
> > Are you suggesting that FloatRects also be defined in terms of LayoutUnits?
> 
> No, there are cases where you want full float precision. What we are saying is
> the code you are touching seems like it needs to be LayoutUnit aligned which
> makes it a good candidate for LayoutRect (instead of using FloatRect)

Using LayoutRect instead of FloatRect as the return value for the ExclusionShape
bounding box methods makes sense, since the values returned by the
ExclusionShape methods are supposed to be aligned with the LayoutUnit Grid. 

I'd prefer to make that change in a separate patch, just for my sanity's sake,
since I'm still trying to get Blink caught up to WebKit, in terms of my changes
(only one patch to go!).

[Hans Muller, 2013-04-23 22:40:31]

Using flooredLayoutPoint() to snap polygon vertices to the LayoutUnit grid.

I'd like to defer LayoutRectifying the ExclusionShape bounding box methods to a
separate patch.

[Julien - ping for review, 2013-04-24 16:47:16]

LGTM.

Preferably (ie nit) the description should mention the follow-up bug for the
bigger refactoring as this is a temporary fix.

[commit-bot: I haz the power, 2013-04-24 17:44:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hmuller@adobe.com/14289003/11001

[commit-bot: I haz the power, 2013-04-24 17:50:26]

Presubmit check for 14289003-11001 failed and returned exit status -2001.
The presubmit check was hung. It took 360.0 seconds to execute and the time
limit is 360.0 seconds.

INFO:root:Found 3 file(s).

[commit-bot: I haz the power, 2013-04-24 18:04:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hmuller@adobe.com/14289003/17001

[commit-bot: I haz the power, 2013-04-24 18:10:28]

Presubmit check for 14289003-17001 failed and returned exit status -2001.
The presubmit check was hung. It took 360.0 seconds to execute and the time
limit is 360.0 seconds.

INFO:root:Found 3 file(s).

[commit-bot: I haz the power, 2013-04-24 18:44:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hmuller@adobe.com/14289003/17001

[commit-bot: I haz the power, 2013-04-24 18:50:16]

Presubmit check for 14289003-17001 failed and returned exit status -2001.
The presubmit check was hung. It took 360.0 seconds to execute and the time
limit is 360.0 seconds.

INFO:root:Found 3 file(s).

[commit-bot: I haz the power, 2013-04-24 18:57:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hmuller@adobe.com/14289003/17001

[commit-bot: I haz the power, 2013-04-24 19:03:09]

Presubmit check for 14289003-17001 failed and returned exit status -2001.
The presubmit check was hung. It took 360.0 seconds to execute and the time
limit is 360.0 seconds.

INFO:root:Found 3 file(s).

[commit-bot: I haz the power, 2013-04-25 14:42:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hmuller@adobe.com/14289003/17001

[commit-bot: I haz the power, 2013-04-25 14:42:19]

Failed to apply patch for Source/core/rendering/ExclusionPolygon.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/rendering/ExclusionPolygon.cpp
  Hunk #1 FAILED at 29.
  1 out of 4 hunks FAILED -- saving rejects to file
Source/core/rendering/ExclusionPolygon.cpp.rej

Patch:       Source/core/rendering/ExclusionPolygon.cpp
Index: Source/core/rendering/ExclusionPolygon.cpp
diff --git a/Source/core/rendering/ExclusionPolygon.cpp
b/Source/core/rendering/ExclusionPolygon.cpp
index
136287ac40f54a90d70754c8defa857add7cc4cd..b3d5884bfcd2575d1501391c5141357e0b1bdd03
100644
--- a/Source/core/rendering/ExclusionPolygon.cpp
+++ b/Source/core/rendering/ExclusionPolygon.cpp
@@ -29,6 +29,7 @@
 
 #include "config.h"
 #include "ExclusionPolygon.h"
+#include "LayoutPoint.h"
 
 #include <wtf/MathExtras.h>
 
@@ -129,6 +130,12 @@ static inline void appendArc(Vector<FloatPoint>& vertices,
const FloatPoint& arc
     vertices.append(endArcVertex);
 }
 
+static inline void snapVerticesToLayoutUnitGrid(Vector<FloatPoint>& vertices)
+{
+    for (unsigned i = 0; i < vertices.size(); ++i)
+      vertices[i] = flooredLayoutPoint(vertices[i]);
+}
+
 static inline FloatPolygon* computeShapePaddingBounds(const FloatPolygon&
polygon, float padding, WindRule fillRule)
 {
     Vector<FloatPoint>* paddedVertices = new Vector<FloatPoint>();
@@ -146,6 +153,7 @@ static inline FloatPolygon* computeShapePaddingBounds(const
FloatPolygon& polygo
             appendArc(*paddedVertices, thisEdge.vertex1(), padding,
prevOffsetEdge.vertex2(), thisOffsetEdge.vertex1(), true);
     }
 
+    snapVerticesToLayoutUnitGrid(*paddedVertices);
     return new FloatPolygon(adoptPtr(paddedVertices), fillRule);
 }
 
@@ -166,6 +174,7 @@ static inline FloatPolygon* computeShapeMarginBounds(const
FloatPolygon& polygon
             appendArc(*marginVertices, thisEdge.vertex1(), margin,
prevOffsetEdge.vertex2(), thisOffsetEdge.vertex1(), false);
     }
 
+    snapVerticesToLayoutUnitGrid(*marginVertices);
     return new FloatPolygon(adoptPtr(marginVertices), fillRule);
 }