Avoid floating point when calculating the actual column count.

Avoid floating point when calculating the actual column count.

It's too inaccurate.

BUG=502407
R=jchaffraix@chromium.org,leviw@chromium.org

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

[mstensho (USE GERRIT), 2015-08-17 11:18:00]

https://codereview.chromium.org/1285273004/diff/1/Source/core/layout/MultiCol...
File Source/core/layout/MultiColumnFragmentainerGroup.cpp (right):

https://codereview.chromium.org/1285273004/diff/1/Source/core/layout/MultiCol...
Source/core/layout/MultiColumnFragmentainerGroup.cpp:285: if (count *
m_columnHeight < flowThreadPortionHeight)
If you can suggest something that looks less silly, I'm all ears. :)

Could have used intMod(m_columnHeight, flowThreadPortionHeight), although that's
more expensive.

Not that this is a hot code path or anything, though.

Cannot use ceil() instead of floor() on the expression above, because the
fractional part may be so small it's not possible for LayoutUnit to hold it.
E.g. 1001px / 1000px = 1 (but we DO need 2 columns in that case).

[leviw_travelin_and_unemployed, 2015-08-17 20:21:16]

leviw@chromium.org changed reviewers:
+ eae@chromium.org

[eae, 2015-08-17 21:34:53]

LGTM

https://codereview.chromium.org/1285273004/diff/1/Source/core/layout/MultiCol...
File Source/core/layout/MultiColumnFragmentainerGroup.cpp (right):

https://codereview.chromium.org/1285273004/diff/1/Source/core/layout/MultiCol...
Source/core/layout/MultiColumnFragmentainerGroup.cpp:285: if (count *
m_columnHeight < flowThreadPortionHeight)
On 2015/08/17 11:18:00, mstensho wrote:
> If you can suggest something that looks less silly, I'm all ears. :)
> 
> Could have used intMod(m_columnHeight, flowThreadPortionHeight), although
that's
> more expensive.
> 
> Not that this is a hot code path or anything, though.
> 
> Cannot use ceil() instead of floor() on the expression above, because the
> fractional part may be so small it's not possible for LayoutUnit to hold it.
> E.g. 1001px / 1000px = 1 (but we DO need 2 columns in that case).

We could check if the value is LayoutUnit::max but that way we cannot
distinguish saturated from max. Perhaps we should reserve a value to indicate
saturation.

That's a question for another day though, for now I think this is probably the
best we can do.

[leviw_travelin_and_unemployed, 2015-08-17 21:45:28]

On 2015/08/17 at 21:34:53, eae wrote:
> LGTM
> 
>
https://codereview.chromium.org/1285273004/diff/1/Source/core/layout/MultiCol...
> File Source/core/layout/MultiColumnFragmentainerGroup.cpp (right):
> 
>
https://codereview.chromium.org/1285273004/diff/1/Source/core/layout/MultiCol...
> Source/core/layout/MultiColumnFragmentainerGroup.cpp:285: if (count *
m_columnHeight < flowThreadPortionHeight)
> On 2015/08/17 11:18:00, mstensho wrote:
> > If you can suggest something that looks less silly, I'm all ears. :)
> > 
> > Could have used intMod(m_columnHeight, flowThreadPortionHeight), although
that's
> > more expensive.
> > 
> > Not that this is a hot code path or anything, though.
> > 
> > Cannot use ceil() instead of floor() on the expression above, because the
> > fractional part may be so small it's not possible for LayoutUnit to hold it.
> > E.g. 1001px / 1000px = 1 (but we DO need 2 columns in that case).
> 
> We could check if the value is LayoutUnit::max but that way we cannot
distinguish saturated from max. Perhaps we should reserve a value to indicate
saturation.
> 
> That's a question for another day though, for now I think this is probably the
best we can do.

We do have LayoutUnit::mightBeSaturated()

[mstensho (USE GERRIT), 2015-08-17 22:43:19]

On 2015/08/17 21:45:28, leviw wrote:
> On 2015/08/17 at 21:34:53, eae wrote:
> > LGTM
> > 
> >
>
https://codereview.chromium.org/1285273004/diff/1/Source/core/layout/MultiCol...
> > File Source/core/layout/MultiColumnFragmentainerGroup.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1285273004/diff/1/Source/core/layout/MultiCol...
> > Source/core/layout/MultiColumnFragmentainerGroup.cpp:285: if (count *
> m_columnHeight < flowThreadPortionHeight)
> > On 2015/08/17 11:18:00, mstensho wrote:
> > > If you can suggest something that looks less silly, I'm all ears. :)
> > > 
> > > Could have used intMod(m_columnHeight, flowThreadPortionHeight), although
> that's
> > > more expensive.
> > > 
> > > Not that this is a hot code path or anything, though.
> > > 
> > > Cannot use ceil() instead of floor() on the expression above, because the
> > > fractional part may be so small it's not possible for LayoutUnit to hold
it.
> > > E.g. 1001px / 1000px = 1 (but we DO need 2 columns in that case).
> > 
> > We could check if the value is LayoutUnit::max but that way we cannot
> distinguish saturated from max. Perhaps we should reserve a value to indicate
> saturation.
> > 
> > That's a question for another day though, for now I think this is probably
the
> best we can do.
> 
> We do have LayoutUnit::mightBeSaturated()

I want to return the correct column count also if flowThreadPortionHeight is
LayoutUnit::max(), or near LayoutUnit::max(). If I didn't have to worry about
that, I could have gone ((flowThreadPortionHeight + m_columnHeight -
LayoutUnit::shinyEpsilonThatReturnsLayoutUnit()) / m_columnHeight).floor(), but
since adding something to a saturated or nearly saturated value doesn't give the
desired result, I cannot.

mightBeSaturated() isn't going to help here.

[eae, 2015-08-19 19:49:38]

This is fine, go for it.
LGTM

[eae, 2015-08-19 19:49:42]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2015-08-19 19:50:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1285273004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1285273004/1

[commit-bot: I haz the power, 2015-08-19 21:44:21]

Committed patchset #1 (id:1) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200848