Let positionNewFloats() take a logicalTop parameter.

Let positionNewFloats() take a logicalTop parameter.

It seemed ugly to temporarily change the logical height before calling
positionNewFloats(), just to make the method behave.

Committed: https://crrev.com/3bda38bf56b4e1abc27d545bd5e04d8290687206
Cr-Commit-Position: refs/heads/master@{#431115}

[mstensho (USE GERRIT), 2016-11-07 20:51:33]

The CQ bit was checked by mstensho@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 20:51:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mstensho (USE GERRIT), 2016-11-07 22:37:15]

mstensho@opera.com changed reviewers:
+ szager@chromium.org

[commit-bot: I haz the power, 2016-11-07 23:09:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-07 23:09:50]

Dry run: This issue passed the CQ dry run.

[szager1, 2016-11-08 21:57:36]

lgtm, but as a nit, I think it would be cleaner to do this:

bool positionNewFloats(LayoutUnit topOffset = LayoutUnit(), LineWidth* =
nullptr);

bool LayoutBlockFlow::positionNewFloats(LayoutUnit topOffset, LineWidth* width)
{
  LayoutUnit logicalTop = logicalHeight() + topOffset;
  ...


Then, all of the call sites that just pass logicalHeight() can remain unchanged.

[mstensho (USE GERRIT), 2016-11-09 08:13:10]

On 2016/11/08 21:57:36, szager1 wrote:
> lgtm, but as a nit, I think it would be cleaner to do this:
> 
> bool positionNewFloats(LayoutUnit topOffset = LayoutUnit(), LineWidth* =
> nullptr);
> 
> bool LayoutBlockFlow::positionNewFloats(LayoutUnit topOffset, LineWidth*
width)
> {
>   LayoutUnit logicalTop = logicalHeight() + topOffset;
>   ...
> 
> 
> Then, all of the call sites that just pass logicalHeight() can remain
unchanged.

Yeah, I was thinking about not having to change those that only want to use
logicalHeight(), but I landed on this solution, since it seemed good if callers
were conscious about where to position their floats. But maybe that's not so
very important.

Passing an offset relatively to logicalHeight() probably isn't the way to go, in
any case, though. Look at LayoutBlockFlow::linkToEndLineIfNeeded(), for instance
[1]; it'd just be tedious to calculate value relatively to logicalHeight(), and
I also find it a bit exotic to have anything relative to the current layout
position.

[1] logicalTopForChild(*box) - marginBeforeForChild(*box) + delta

I suggest either:
A: Add a positionNewFloats(LineWidth* lineWidth=nullptr) overload that calls
positionNewFloats(logicalHeight(), lineWidth)
B: Land the patch as it is

What do you think?

[szager1, 2016-11-09 21:57:06]

On 2016/11/09 08:13:10, mstensho wrote:
> On 2016/11/08 21:57:36, szager1 wrote:
> > lgtm, but as a nit, I think it would be cleaner to do this:
> > 
> > bool positionNewFloats(LayoutUnit topOffset = LayoutUnit(), LineWidth* =
> > nullptr);
> > 
> > bool LayoutBlockFlow::positionNewFloats(LayoutUnit topOffset, LineWidth*
> width)
> > {
> >   LayoutUnit logicalTop = logicalHeight() + topOffset;
> >   ...
> > 
> > 
> > Then, all of the call sites that just pass logicalHeight() can remain
> unchanged.
> 
> Yeah, I was thinking about not having to change those that only want to use
> logicalHeight(), but I landed on this solution, since it seemed good if
callers
> were conscious about where to position their floats. But maybe that's not so
> very important.
> 
> Passing an offset relatively to logicalHeight() probably isn't the way to go,
in
> any case, though. Look at LayoutBlockFlow::linkToEndLineIfNeeded(), for
instance
> [1]; it'd just be tedious to calculate value relatively to logicalHeight(),
and
> I also find it a bit exotic to have anything relative to the current layout
> position.
> 
> [1] logicalTopForChild(*box) - marginBeforeForChild(*box) + delta
> 
> I suggest either:
> A: Add a positionNewFloats(LineWidth* lineWidth=nullptr) overload that calls
> positionNewFloats(logicalHeight(), lineWidth)
> B: Land the patch as it is
> 
> What do you think?

I think landing as-is is fine, still lgtm.

[mstensho (USE GERRIT), 2016-11-09 21:59:17]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2016-11-09 21:59:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-10 00:53:41]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-11-10 01:01:17]

Description was changed from

==========
Let positionNewFloats() take a logicalTop parameter.

It seemed ugly to temporarily change the logical height before calling
positionNewFloats(), just to make the method behave.
==========

to

==========
Let positionNewFloats() take a logicalTop parameter.

It seemed ugly to temporarily change the logical height before calling
positionNewFloats(), just to make the method behave.

Committed: https://crrev.com/3bda38bf56b4e1abc27d545bd5e04d8290687206
Cr-Commit-Position: refs/heads/master@{#431115}
==========

[commit-bot: I haz the power, 2016-11-10 01:01:18]

Patchset 1 (id:??) landed as
https://crrev.com/3bda38bf56b4e1abc27d545bd5e04d8290687206
Cr-Commit-Position: refs/heads/master@{#431115}