Move overflow in InlineTextBox::adjustPosition() if it has glyph overflows

Move overflow in InlineTextBox::adjustPosition() if it has glyph overflows

If the InlineTextBox has glyph overflows, without overflow adjustment,
the overflow will be inconsistent with the changed position of the
inline box. Now also move the overflow.

BTW, renamed adjustPosition() to move() to make the meaning clearer. 

BUG=499690
TEST=fast/repaint/line-flow-with-floats-2.html --enable-slimming-paint

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

[Xianzhu, 2015-06-15 23:15:57]

wangxianzhu@chromium.org changed reviewers:
+ chrishtr@chromium.org, eae@chromium.org

[chrishtr, 2015-06-16 14:50:17]

What is adjustPosition for exactly? Could you add a comment to its definition in
InlineBox.h?

[Xianzhu, 2015-06-16 17:50:57]

On 2015/06/16 14:50:17, chrishtr wrote:
> What is adjustPosition for exactly? Could you add a comment to its definition
in
> InlineBox.h?

The function is just for changing the position of the InlineBox during layout.
Perhaps the unclearness comes from its naming. How about renaming it to
'movePosition'?

[leviw_travelin_and_unemployed, 2015-06-16 17:59:21]

On 2015/06/16 at 17:50:57, wangxianzhu wrote:
> On 2015/06/16 14:50:17, chrishtr wrote:
> > What is adjustPosition for exactly? Could you add a comment to its
definition in
> > InlineBox.h?
> 
> The function is just for changing the position of the InlineBox during layout.
Perhaps the unclearness comes from its naming. How about renaming it to
'movePosition'?

drive-by: If we rename the method, please nuke "dx, dy" from orbit, and use a
LayoutSize. Then you can use the built-in transposing methods instead of
switching argument order.

[chrishtr, 2015-06-16 18:49:52]

On 2015/06/16 at 17:59:21, leviw wrote:
> On 2015/06/16 at 17:50:57, wangxianzhu wrote:
> > On 2015/06/16 14:50:17, chrishtr wrote:
> > > What is adjustPosition for exactly? Could you add a comment to its
definition in
> > > InlineBox.h?
> > 
> > The function is just for changing the position of the InlineBox during
layout. Perhaps the unclearness comes from its naming. How about renaming it to
'movePosition'?
> 
> drive-by: If we rename the method, please nuke "dx, dy" from orbit, and use a
LayoutSize. Then you can use the built-in transposing methods instead of
switching argument order.

The kind of comment I wanted was something like: adjustPosition modifies the
position based on factors a, b and c, such as in <example>. Maybe in this case
it's always called though? In that case maybe it should be setPosition() or
applyPosition() to indicate that.

[Xianzhu, 2015-06-16 18:55:29]

On 2015/06/16 18:49:52, chrishtr wrote:
> On 2015/06/16 at 17:59:21, leviw wrote:
> > On 2015/06/16 at 17:50:57, wangxianzhu wrote:
> > > On 2015/06/16 14:50:17, chrishtr wrote:
> > > > What is adjustPosition for exactly? Could you add a comment to its
> definition in
> > > > InlineBox.h?
> > > 
> > > The function is just for changing the position of the InlineBox during
> layout. Perhaps the unclearness comes from its naming. How about renaming it
to
> 'movePosition'?
> > 
> > drive-by: If we rename the method, please nuke "dx, dy" from orbit, and use
a
> LayoutSize. Then you can use the built-in transposing methods instead of
> switching argument order.
> 
> The kind of comment I wanted was something like: adjustPosition modifies the
> position based on factors a, b and c, such as in <example>.

It's just like the move() method of LayoutRect or LayoutBox. There is no factors
other than the parameters.

> Maybe in this case
> it's always called though? In that case maybe it should be setPosition() or
> applyPosition() to indicate that.

[Xianzhu, 2015-06-16 18:58:15]

Patch Set 4 contains the following renaming:
adjustPosition(LayoutUnit dx, LayoutUnit dy) => move(LayoutSize)
adjustLogicalPosition => moveInLogicalDirection
adjustLineDirectionPosition => moveInLineDirection
adjustBlockDirectionPosition => moveInBlockDirection

[Xianzhu, 2015-06-16 19:16:20]

On 2015/06/16 18:49:52, chrishtr wrote:
> On 2015/06/16 at 17:59:21, leviw wrote:
> > On 2015/06/16 at 17:50:57, wangxianzhu wrote:
> > > On 2015/06/16 14:50:17, chrishtr wrote:
> > > > What is adjustPosition for exactly? Could you add a comment to its
> definition in
> > > > InlineBox.h?
> > > 
> > > The function is just for changing the position of the InlineBox during
> layout. Perhaps the unclearness comes from its naming. How about renaming it
to
> 'movePosition'?
> > 
> > drive-by: If we rename the method, please nuke "dx, dy" from orbit, and use
a
> LayoutSize. Then you can use the built-in transposing methods instead of
> switching argument order.
> 
> The kind of comment I wanted was something like: adjustPosition modifies the
> position based on factors a, b and c, such as in <example>. Maybe in this case
> it's always called though? In that case maybe it should be setPosition() or
> applyPosition() to indicate that.

Added comments in Patch Set 5.

[leviw_travelin_and_unemployed, 2015-06-16 20:09:16]

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

[leviw_travelin_and_unemployed, 2015-06-16 20:09:17]

https://codereview.chromium.org/1183003003/diff/80001/Source/core/layout/Layo...
File Source/core/layout/LayoutBlockFlowLine.cpp (right):

https://codereview.chromium.org/1183003003/diff/80001/Source/core/layout/Layo...
Source/core/layout/LayoutBlockFlowLine.cpp:1924:
curr->moveInLogicalDirection(LayoutSize(logicalLeft - curr->logicalLeft(), 0));
Nit: s/0/LayoutUnit()/

https://codereview.chromium.org/1183003003/diff/80001/Source/core/layout/Layo...
Source/core/layout/LayoutBlockFlowLine.cpp:1994:
curr->moveInLogicalDirection(LayoutSize(logicalLeft, 0));
Ditto

https://codereview.chromium.org/1183003003/diff/80001/Source/core/layout/Layo...
Source/core/layout/LayoutBlockFlowLine.cpp:1996:
curr->moveInLogicalDirection(LayoutSize(logicalLeft - (availableLogicalWidth -
totalLogicalWidth), 0));
Ditto.

https://codereview.chromium.org/1183003003/diff/80001/Source/core/layout/line...
File Source/core/layout/line/InlineFlowBox.cpp (right):

https://codereview.chromium.org/1183003003/diff/80001/Source/core/layout/line...
Source/core/layout/line/InlineFlowBox.cpp:267: m_overflow->move(delta.width(),
delta.height()); // FIXME: Rounding error here since overflow was pixel snapped,
but nobody other than list markers passes non-integral values here.
Grumble grumble about more dx dy (not an issue with this patch)

[Xianzhu, 2015-06-16 20:43:23]

https://codereview.chromium.org/1183003003/diff/80001/Source/core/layout/Layo...
File Source/core/layout/LayoutBlockFlowLine.cpp (right):

https://codereview.chromium.org/1183003003/diff/80001/Source/core/layout/Layo...
Source/core/layout/LayoutBlockFlowLine.cpp:1924:
curr->moveInLogicalDirection(LayoutSize(logicalLeft - curr->logicalLeft(), 0));
On 2015/06/16 20:09:17, leviw wrote:
> Nit: s/0/LayoutUnit()/

Changed to use moveInLineDirection().

https://codereview.chromium.org/1183003003/diff/80001/Source/core/layout/Layo...
Source/core/layout/LayoutBlockFlowLine.cpp:1994:
curr->moveInLogicalDirection(LayoutSize(logicalLeft, 0));
On 2015/06/16 20:09:17, leviw wrote:
> Ditto

Ditto

https://codereview.chromium.org/1183003003/diff/80001/Source/core/layout/Layo...
Source/core/layout/LayoutBlockFlowLine.cpp:1996:
curr->moveInLogicalDirection(LayoutSize(logicalLeft - (availableLogicalWidth -
totalLogicalWidth), 0));
On 2015/06/16 20:09:17, leviw wrote:
> Ditto.

Ditto

https://codereview.chromium.org/1183003003/diff/80001/Source/core/layout/line...
File Source/core/layout/line/InlineBox.h (right):

https://codereview.chromium.org/1183003003/diff/80001/Source/core/layout/line...
Source/core/layout/line/InlineBox.h:83: void moveInLineDirection(LayoutUnit
delta) { moveInLogicalDirection(LayoutSize(delta, 0)); }
Changed 0 to LayoutUnit().

https://codereview.chromium.org/1183003003/diff/80001/Source/core/layout/line...
File Source/core/layout/line/InlineFlowBox.cpp (right):

https://codereview.chromium.org/1183003003/diff/80001/Source/core/layout/line...
Source/core/layout/line/InlineFlowBox.cpp:267: m_overflow->move(delta.width(),
delta.height()); // FIXME: Rounding error here since overflow was pixel snapped,
but nobody other than list markers passes non-integral values here.
On 2015/06/16 20:09:17, leviw wrote:
> Grumble grumble about more dx dy (not an issue with this patch)

Acknowledged.

[Xianzhu, 2015-06-17 15:43:14]

Updated descriptions. Ptal.

[chrishtr, 2015-06-17 16:36:07]

I'll defer to Levi for final call on the names of these layout methods.

[leviw_travelin_and_unemployed, 2015-06-17 18:35:12]

https://codereview.chromium.org/1183003003/diff/120001/Source/core/layout/lin...
File Source/core/layout/line/InlineBox.h (right):

https://codereview.chromium.org/1183003003/diff/120001/Source/core/layout/lin...
Source/core/layout/line/InlineBox.h:81: virtual void move(const LayoutSize&
delta);
It'd be nice if this gave some context about the notion that it's moving not
just this box, but all descendants as well.

https://codereview.chromium.org/1183003003/diff/120001/Source/core/layout/lin...
Source/core/layout/line/InlineBox.h:83: void moveInLineDirection(LayoutUnit
delta) { moveInLogicalDirection(LayoutSize(delta, LayoutUnit())); }
For consistency, this should be moveInInlineDirection.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[Xianzhu, 2015-06-17 18:51:10]

https://codereview.chromium.org/1183003003/diff/120001/Source/core/layout/lin...
File Source/core/layout/line/InlineBox.h (right):

https://codereview.chromium.org/1183003003/diff/120001/Source/core/layout/lin...
Source/core/layout/line/InlineBox.h:81: virtual void move(const LayoutSize&
delta);
On 2015/06/17 18:35:11, leviw wrote:
> It'd be nice if this gave some context about the notion that it's moving not
> just this box, but all descendants as well.

Done.

https://codereview.chromium.org/1183003003/diff/120001/Source/core/layout/lin...
Source/core/layout/line/InlineBox.h:83: void moveInLineDirection(LayoutUnit
delta) { moveInLogicalDirection(LayoutSize(delta, LayoutUnit())); }
On 2015/06/17 18:35:11, leviw wrote:
> For consistency, this should be moveInInlineDirection.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Done.

[leviw_travelin_and_unemployed, 2015-06-17 20:00:10]

LGTM.

[Xianzhu, 2015-06-17 21:02:17]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2015-06-17 21:03:10]

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

[commit-bot: I haz the power, 2015-06-17 22:42:20]

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