Continue with InlineBox float->LayoutUnit migration.

Continue with InlineBox float->LayoutUnit migration.

This is the second patch in the series to migrate
to LayoutUnits in all InlineBox classes.

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

[hartmanng, 2014-11-03 18:57:29]

Patchset #1 (id:1) has been deleted

[hartmanng, 2014-11-03 19:06:31]

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

[hartmanng, 2014-11-03 19:19:24]

Levi, please take a look. I'm not sure how easy or hard these patches are to
review, so let me know if it's better to make them bigger or smaller than this -
it's easy to split it to per-class patches if necessary.

[leviw_travelin_and_unemployed, 2014-11-03 21:35:02]

https://codereview.chromium.org/699683002/diff/20001/Source/core/rendering/Fl...
File Source/core/rendering/FloatToLayoutUnit.h (right):

https://codereview.chromium.org/699683002/diff/20001/Source/core/rendering/Fl...
Source/core/rendering/FloatToLayoutUnit.h:26: #define
WILL_BE_LAYOUT_UNIT_TO_FLOAT(layoutUnit) (layoutUnit.toFloat())
These names don't actually seem self-identifying to me. LAYOUT_UNIT_TO_FLOAT and
WILL_BE_LAYOUT_UNIT_TO_FLOAT don't imply the actual difference.

It seems to me that you want a class to make this indirection not awful. With
the conversion to sub-pixel layout, we used LayoutUnit (originally an integer),
which you could optionally point at FractionalLayoutUnit (sub-pixel magic).

If you did that, it seems to me that you could have much more reasonable and
understandable conversions to other types. This model is hard to parse for me.

[hartmanng, 2014-11-06 23:11:38]

On 2014/11/03 21:35:02, leviw wrote:
>
https://codereview.chromium.org/699683002/diff/20001/Source/core/rendering/Fl...
> File Source/core/rendering/FloatToLayoutUnit.h (right):
> 
>
https://codereview.chromium.org/699683002/diff/20001/Source/core/rendering/Fl...
> Source/core/rendering/FloatToLayoutUnit.h:26: #define
> WILL_BE_LAYOUT_UNIT_TO_FLOAT(layoutUnit) (layoutUnit.toFloat())
> These names don't actually seem self-identifying to me. LAYOUT_UNIT_TO_FLOAT
and
> WILL_BE_LAYOUT_UNIT_TO_FLOAT don't imply the actual difference.
> 
> It seems to me that you want a class to make this indirection not awful. With
> the conversion to sub-pixel layout, we used LayoutUnit (originally an
integer),
> which you could optionally point at FractionalLayoutUnit (sub-pixel magic).
> 
> If you did that, it seems to me that you could have much more reasonable and
> understandable conversions to other types. This model is hard to parse for me.

Yeah, that's pretty reasonable. I don't think it's as easy to do incrementally,
though.

How does this sound:

1) I've removed all the ugly macros.
2) I'll have to do a pass of just aliasing float* types to Float*WillBeLayout*.
3) Once that's all done, I can add classes (say, LineLayout*) with implicit
conversions to/from float as well as explicit toFloat and toLayoutUnit functions
(and ceil, round, etc), then switch the aliases over to that type. I've added a
small sample in this CL of what this type of class might look like.
4) Once switched over, I can convert any implicit casts in the code into the
explicit functions, and remove the implicit conversions from the class.
5) Then it should be trivial to swap the backend of LineLayout* from float-based
types to LayoutUnit-based types.
6) Finally, we can switch over to real LayoutUnit types and get rid of the
intermediate classes.

It'll take some extra patches this way, but it'll keep the code cleaner in the
interim. I'm willing to write them if you're willing to review them :)

Is that closer to what you had in mind?

[leviw_travelin_and_unemployed, 2014-11-12 21:27:46]

https://codereview.chromium.org/699683002/diff/40001/Source/core/rendering/Fl...
File Source/core/rendering/FloatToLayoutUnit.h (right):

https://codereview.chromium.org/699683002/diff/40001/Source/core/rendering/Fl...
Source/core/rendering/FloatToLayoutUnit.h:20: class LineLayoutUnit {
This may be easier if you create 2 separate classes, FloatLineLayoutUnit and
LayoutUnitLineLayoutUnit (or something less hideous), and have LineLayoutUnit
just be a "using LineLayoutUnit = FloatLineLayoutUnit". This would make
reasoning around the actual class a lot easier, as you'd only need one ifdef
switch to change the using statement.

[hartmanng, 2014-11-13 16:08:27]

https://codereview.chromium.org/699683002/diff/40001/Source/core/rendering/Fl...
File Source/core/rendering/FloatToLayoutUnit.h (right):

https://codereview.chromium.org/699683002/diff/40001/Source/core/rendering/Fl...
Source/core/rendering/FloatToLayoutUnit.h:20: class LineLayoutUnit {
On 2014/11/12 21:27:45, leviw wrote:
> This may be easier if you create 2 separate classes, FloatLineLayoutUnit and
> LayoutUnitLineLayoutUnit (or something less hideous), and have LineLayoutUnit
> just be a "using LineLayoutUnit = FloatLineLayoutUnit". This would make
> reasoning around the actual class a lot easier, as you'd only need one ifdef
> switch to change the using statement.

Done.

I did go with the hideous LayoutUnitLineLayoutUnit for now, because anything
else wouldn't really fit the pattern...

One alternative might be LineFloat and LineLayoutUnit? Personally I'm not too
concerned about the naming here, since these internal classes will only ever be
referred to in the "using" statement.

[leviw_travelin_and_unemployed, 2014-11-13 19:43:11]

lgtm

https://codereview.chromium.org/699683002/diff/60001/Source/core/rendering/Fl...
File Source/core/rendering/FloatToLayoutUnit.h (right):

https://codereview.chromium.org/699683002/diff/60001/Source/core/rendering/Fl...
Source/core/rendering/FloatToLayoutUnit.h:22: FloatLineLayoutUnit(float f) :
m_value(f) { }
Nit: Can you add default constructors that initialize to zero (LayoutUnit does
this automatically)? We do additional math to construct a LayoutUnit from an
int. It's faster to just use the default constructor.

[hartmanng, 2014-11-13 19:48:35]

Thanks!

https://codereview.chromium.org/699683002/diff/60001/Source/core/rendering/Fl...
File Source/core/rendering/FloatToLayoutUnit.h (right):

https://codereview.chromium.org/699683002/diff/60001/Source/core/rendering/Fl...
Source/core/rendering/FloatToLayoutUnit.h:22: FloatLineLayoutUnit(float f) :
m_value(f) { }
On 2014/11/13 19:43:11, leviw wrote:
> Nit: Can you add default constructors that initialize to zero (LayoutUnit does
> this automatically)? We do additional math to construct a LayoutUnit from an
> int. It's faster to just use the default constructor.

Done.

[hartmanng, 2014-11-13 19:48:51]

The CQ bit was checked by hartmanng@chromium.org

[commit-bot: I haz the power, 2014-11-13 19:49:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/699683002/80001

[leviw_travelin_and_unemployed, 2014-11-13 19:51:23]

https://codereview.chromium.org/699683002/diff/80001/Source/core/rendering/Fl...
File Source/core/rendering/FloatToLayoutUnit.h (right):

https://codereview.chromium.org/699683002/diff/80001/Source/core/rendering/Fl...
Source/core/rendering/FloatToLayoutUnit.h:41: LayoutUnitLineLayoutUnit() :
m_value() { }
Nit: I don't think you even need the m_value() part :)

https://codereview.chromium.org/699683002/diff/80001/Source/core/rendering/In...
File Source/core/rendering/InlineBox.cpp (right):

https://codereview.chromium.org/699683002/diff/80001/Source/core/rendering/In...
Source/core/rendering/InlineBox.cpp:126: return m_bitfields.isText() ?
renderer().style(isFirstLineStyle())->fontMetrics().height() : 0;
ditto.

https://codereview.chromium.org/699683002/diff/80001/Source/core/rendering/In...
File Source/core/rendering/InlineBox.h (right):

https://codereview.chromium.org/699683002/diff/80001/Source/core/rendering/In...
Source/core/rendering/InlineBox.h:47: , m_logicalWidth(0)
Use default constructor.

https://codereview.chromium.org/699683002/diff/80001/Source/core/rendering/In...
Source/core/rendering/InlineBox.h:134: return 0;
Ditto.

https://codereview.chromium.org/699683002/diff/80001/Source/core/rendering/In...
File Source/core/rendering/InlineFlowBox.cpp (right):

https://codereview.chromium.org/699683002/diff/80001/Source/core/rendering/In...
Source/core/rendering/InlineFlowBox.cpp:387: FloatWillBeLayoutUnit space = 0;
Don't need the = 0 at all

https://codereview.chromium.org/699683002/diff/80001/Source/core/rendering/In...
Source/core/rendering/InlineFlowBox.cpp:756: FloatWillBeLayoutUnit
localMaxLogicalTop = 0;
Ditto.

https://codereview.chromium.org/699683002/diff/80001/Source/core/rendering/In...
File Source/core/rendering/InlineTextBox.cpp (right):

https://codereview.chromium.org/699683002/diff/80001/Source/core/rendering/In...
Source/core/rendering/InlineTextBox.cpp:487: FloatWillBeLayoutUnit lastWidth =
0;
Ditto.

https://codereview.chromium.org/699683002/diff/80001/Source/core/rendering/Ro...
File Source/core/rendering/RootInlineBox.cpp (right):

https://codereview.chromium.org/699683002/diff/80001/Source/core/rendering/Ro...
Source/core/rendering/RootInlineBox.cpp:251: FloatWillBeLayoutUnit maxLogicalTop
= 0;
Ditto.

[leviw_travelin_and_unemployed, 2014-11-13 19:51:29]

The CQ bit was unchecked by leviw@chromium.org

[leviw_travelin_and_unemployed, 2014-11-13 19:51:38]

Use that fancy new constructor first ;)

[hartmanng, 2014-11-13 19:56:33]

https://codereview.chromium.org/699683002/diff/80001/Source/core/rendering/In...
File Source/core/rendering/InlineBox.h (right):

https://codereview.chromium.org/699683002/diff/80001/Source/core/rendering/In...
Source/core/rendering/InlineBox.h:47: , m_logicalWidth(0)
On 2014/11/13 19:51:23, leviw wrote:
> Use default constructor.

For now these are still aliased to float... I could go back to using a
ZERO_LINE_LAYOUT_UNIT macro?

[leviw_travelin_and_unemployed, 2014-11-13 19:59:11]

On 2014/11/13 at 19:56:33, hartmanng wrote:
>
https://codereview.chromium.org/699683002/diff/80001/Source/core/rendering/In...
> File Source/core/rendering/InlineBox.h (right):
> 
>
https://codereview.chromium.org/699683002/diff/80001/Source/core/rendering/In...
> Source/core/rendering/InlineBox.h:47: , m_logicalWidth(0)
> On 2014/11/13 19:51:23, leviw wrote:
> > Use default constructor.
> 
> For now these are still aliased to float... I could go back to using a
ZERO_LINE_LAYOUT_UNIT macro?

Ah that's fine then. Sorry for the noise :)

[hartmanng, 2014-11-13 20:03:04]

On 2014/11/13 19:59:11, leviw wrote:
> On 2014/11/13 at 19:56:33, hartmanng wrote:
> >
>
https://codereview.chromium.org/699683002/diff/80001/Source/core/rendering/In...
> > File Source/core/rendering/InlineBox.h (right):
> > 
> >
>
https://codereview.chromium.org/699683002/diff/80001/Source/core/rendering/In...
> > Source/core/rendering/InlineBox.h:47: , m_logicalWidth(0)
> > On 2014/11/13 19:51:23, leviw wrote:
> > > Use default constructor.
> > 
> > For now these are still aliased to float... I could go back to using a
> ZERO_LINE_LAYOUT_UNIT macro?
> 
> Ah that's fine then. Sorry for the noise :)

np, thanks :)

https://codereview.chromium.org/699683002/diff/80001/Source/core/rendering/Fl...
File Source/core/rendering/FloatToLayoutUnit.h (right):

https://codereview.chromium.org/699683002/diff/80001/Source/core/rendering/Fl...
Source/core/rendering/FloatToLayoutUnit.h:41: LayoutUnitLineLayoutUnit() :
m_value() { }
On 2014/11/13 19:51:23, leviw wrote:
> Nit: I don't think you even need the m_value() part :)

Done.

[hartmanng, 2014-11-13 20:03:40]

The CQ bit was checked by hartmanng@chromium.org

[commit-bot: I haz the power, 2014-11-13 20:04:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/699683002/100001

[commit-bot: I haz the power, 2014-11-13 21:09:46]

Committed patchset #5 (id:100001) as 185320