[CSS Exclusions] ExclusionShape bounding box methods should return LayoutRects

[CSS Exclusions] ExclusionShape bounding box methods should return LayoutRects

Redefined the ExclusionShape API in terms of LayoutUnits, instead of floats: all of the ExclusionShape methods now have LayoutUnit parameters and return LayoutUnit values. This is more natural, since the callers work exclusively in LayoutUnits. This is strictly a refactoring, no new tests were needed.

The impetus for this change was some feedback from Levi and Julien about an earlier change - https://codereview.chromium.org/14289003/.

The LayoutUnit to float conversions have always occured at the boundary between the layout system and the ExclusionShape classes. Previously these conversions were mostly implicit and ad-hoc, and the callers found them somewhat confusing. Refactoring the ExclusionShape API in terms of LayoutUnits simplifies life for the callers and ensures that the conversions are systematic.

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

[Hans Muller, 2013-05-03 22:20:23]

Although this not a small patch, it's making a logically small change. It
revises the ExclusionShape API, to use LayoutUnits for input and output values,
instead of floats.

[eseidel, 2013-05-04 01:15:47]

Emil and Levi are better reviewer here.

[Hans Muller, 2013-05-06 13:56:37]

Added Levi and Emil per Eric's request.

[Hans Muller, 2013-05-06 15:39:11]

[resending this since the reviewer list has changed, sorry if you're seeing a
dup]

Although this not a small patch, it's making a logically small change. It
revises the ExclusionShape API, to use LayoutUnits for input and output values,
instead of floats.

Note also: the first version of this patch also included a fix for:
Fix a broken ASSERT in StylePropertySerializer. (issue 14772013),
https://chromiumcodereview.appspot.com/14772013/

[eae, 2013-05-06 15:59:28]

I'm not entirely sure why you are making this change and I am a bit concerned
that it'll introduce subtle rounding errors given that many subsystems still use
floats. Could you please expand on the idea and how you've tested it?

I'm not against the change in itself but I want to make sure I fully understand
what you are trying to do to ensure we do not regress.

Thanks!

https://codereview.chromium.org/14892005/diff/5001/Source/core/rendering/excl...
File Source/core/rendering/exclusions/ExclusionPolygon.cpp (right):

https://codereview.chromium.org/14892005/diff/5001/Source/core/rendering/excl...
Source/core/rendering/exclusions/ExclusionPolygon.cpp:357: float y2 = logicalTop
+ logicalHeight;
This changes  the precision of the calculations and could theoretically
introduce an off by one errors (logicalTop + logicalHeight).toFloat() and
logicalTop.toFloat() + logicalHeight.toFloat() are not guaranteed to be the
same.

The same applies to the other changes in this file. This might not be a problem
or might even fix some edge cases. It would be nice to see a test using values
where round(y+h) and round(y)+round(h) are different.

Finally, please update the commit message to detail these logic changes as they
are not covered by the existing description.

https://codereview.chromium.org/14892005/diff/5001/Source/core/rendering/excl...
File Source/core/rendering/exclusions/ExclusionRectangle.h (right):

https://codereview.chromium.org/14892005/diff/5001/Source/core/rendering/excl...
Source/core/rendering/exclusions/ExclusionRectangle.h:79: FloatRoundedRect
shapeMarginBounds() const;
The API is changes to use LayoutUnits yet the internal representation still uses
floats, why is that? Seems undesirable.

https://codereview.chromium.org/14892005/diff/5001/Source/core/rendering/excl...
File Source/core/rendering/exclusions/ExclusionShape.h (right):

https://codereview.chromium.org/14892005/diff/5001/Source/core/rendering/excl...
Source/core/rendering/exclusions/ExclusionShape.h:48: LayoutUnit logicalLeft;
The line-box tree still uses floats (and the LineSegment constructor still takes
floats), changing the internal representation here without changing the
constructor or the line-box tree seems like it would introduce subtle rounding
errors.

[Hans Muller, 2013-05-06 17:08:28]

On 2013/05/06 15:59:28, eae wrote:
> I'm not entirely sure why you are making this change and I am a bit concerned
> that it'll introduce subtle rounding errors given that many subsystems still
use
> floats. Could you please expand on the idea and how you've tested it?
The impetus for this change was some feedback from Levi and Julien about an
earlier change - https://codereview.chromium.org/14289003/.

The LayoutUnit to float conversions have always occured at the boundary between
the layout system and the ExclusionShape classes. Previously these conversions
were mostly implicit and ad-hoc, and the callers found them somewhat confusing.
Refactoring the ExclusionShape API in terms of LayoutUnits simplifies life for
the callers and ensures that the conversions are systematic.

I don't believe I've introduced an instability by making this change, and the
existing tests, many of which are sensitive to floating point issues, still
pass.

I'm aware of potential problems like the one you mentioned:

> This changes the precision of the calculations and could theoretically
introduce an off by one errors (logicalTop + logicalHeight).toFloat() and
logicalTop.toFloat() + logicalHeight.toFloat() are not guaranteed to be the
same.

If it's OK with you, I'll add some tests that check this in a subsequent patch.

> Source/core/rendering/exclusions/ExclusionRectangle.h:79: FloatRoundedRect
> shapeMarginBounds() const;
> The API is changes to use LayoutUnits yet the internal representation still
uses floats, why is that? Seems undesirable.

All of the ExclusionShape classes uses float representations of geometry
internally because the extra precision is needed. What's happening here is that
the outgoing value is being snapped to the LayoutUnit grid.

> Source/core/rendering/exclusions/ExclusionShape.h:48: LayoutUnit
> The line-box tree still uses floats (and the LineSegment constructor still
takes floats), changing the internal representation here without changing the
constructor or the line-box tree seems like it would introduce subtle rounding
errors

Thanks for pointing that out, changing the types of the LineSegment
logicalLeft,logicalRight was a mistake.

[eae, 2013-05-06 19:45:16]

On 2013/05/06 17:08:28, Hans Muller wrote:
> On 2013/05/06 15:59:28, eae wrote:
> > I'm not entirely sure why you are making this change and I am a bit
concerned
> > that it'll introduce subtle rounding errors given that many subsystems still
> use
> > floats. Could you please expand on the idea and how you've tested it?
> The impetus for this change was some feedback from Levi and Julien about an
> earlier change - https://codereview.chromium.org/14289003/.
> 
> The LayoutUnit to float conversions have always occured at the boundary
between
> the layout system and the ExclusionShape classes. Previously these conversions
> were mostly implicit and ad-hoc, and the callers found them somewhat
confusing.
> Refactoring the ExclusionShape API in terms of LayoutUnits simplifies life for
> the callers and ensures that the conversions are systematic.

Could you please include the above in the CL description?

> 
> I don't believe I've introduced an instability by making this change, and the
> existing tests, many of which are sensitive to floating point issues, still
> pass.
> 
> I'm aware of potential problems like the one you mentioned:
> 
> > This changes the precision of the calculations and could theoretically
> introduce an off by one errors (logicalTop + logicalHeight).toFloat() and
> logicalTop.toFloat() + logicalHeight.toFloat() are not guaranteed to be the
> same.
> 
> If it's OK with you, I'll add some tests that check this in a subsequent
patch.

Seems reasonable. Assuming of course that the existing tests still pass.

> 
> > Source/core/rendering/exclusions/ExclusionRectangle.h:79: FloatRoundedRect
> > shapeMarginBounds() const;
> > The API is changes to use LayoutUnits yet the internal representation still
> uses floats, why is that? Seems undesirable.
> 
> All of the ExclusionShape classes uses float representations of geometry
> internally because the extra precision is needed. What's happening here is
that
> the outgoing value is being snapped to the LayoutUnit grid.
> 
> > Source/core/rendering/exclusions/ExclusionShape.h:48: LayoutUnit
> > The line-box tree still uses floats (and the LineSegment constructor still
> takes floats), changing the internal representation here without changing the
> constructor or the line-box tree seems like it would introduce subtle rounding
> errors
> 
> Thanks for pointing that out, changing the types of the LineSegment
> logicalLeft,logicalRight was a mistake.

Thanks.

[Hans Muller, 2013-05-06 21:21:31]

On 2013/05/06 19:45:16, eae wrote:
> On 2013/05/06 17:08:28, Hans Muller wrote:
> > On 2013/05/06 15:59:28, eae wrote:
> > Refactoring the ExclusionShape API in terms of LayoutUnits simplifies life
for
> > the callers and ensures that the conversions are systematic.
> 
> Could you please include the above in the CL description?

Done.  I'd meant to update the description in the first go-round, sorry about
the delay.

> Seems reasonable. Assuming of course that the existing tests still pass.

All tests still pass on my OSX build.

[eae, 2013-05-06 21:22:21]

LGTM

Thanks!

[commit-bot: I haz the power, 2013-05-06 21:57:47]

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

[commit-bot: I haz the power, 2013-05-06 22:04:40]

Change committed as 149809