Handle exclusive end offsets when translating from flow thread coordinates.

Handle exclusive end offsets when translating from flow thread coordinates.

If we're in flipped blocks writing mode (i.e. vertical-rl), the flow thread
block offset we're dealing with may be a logical end point, and end points are
exclusive. This means that we need to pick the previous column, not the next,
if the offset is exactly at a column boundary.

Let flowThreadTranslationAtOffset() and columnIndexAtOffset() take a
PageBoundaryRule argument to handle this.

This makes offsetLeft and offsetTop work properly in vertical-rl writing mode
for elements that end at column boundaries. Added a test for that, and threw in
a vertical-lr test too, for good measure.

Remove ColumnIndexCalculationMode from columnIndexAtOffset(). It was partially
and inaccurately used to make sure we didn't escape the valid column range in
case an exclusive end offset was passed. Have the call sites that really need
to clamp the column index do it themselves. It's up to the callers to decide
how to treat offsets outside the range of columns anyway.

Committed: https://crrev.com/8403c15086b287d0db07c7fe9bcf104d02c99052
Cr-Commit-Position: refs/heads/master@{#418800}

[mstensho (USE GERRIT), 2016-09-14 15:06:16]

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

[commit-bot: I haz the power, 2016-09-14 15:06:35]

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-09-14 15:33:04]

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

[commit-bot: I haz the power, 2016-09-14 16:28:53]

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

[commit-bot: I haz the power, 2016-09-14 16:28:54]

Dry run: This issue passed the CQ dry run.

[eae, 2016-09-14 22:08:48]

LGTM w/suggestion

https://codereview.chromium.org/2339973002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.cpp
(right):

https://codereview.chromium.org/2339973002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.cpp:109:
unsigned columnIndex = offsetInFlowThread >= logicalBottomInFlowThread() ?
actualColumnCount() - 1 : columnIndexAtOffset(offsetInFlowThread, rule);
Maybe add a DCHECK(actualColumnCount() > 0) before to ensure it doesn't
undeflow?

[mstensho (USE GERRIT), 2016-09-15 06:37:50]

https://codereview.chromium.org/2339973002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.cpp
(right):

https://codereview.chromium.org/2339973002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.cpp:109:
unsigned columnIndex = offsetInFlowThread >= logicalBottomInFlowThread() ?
actualColumnCount() - 1 : columnIndexAtOffset(offsetInFlowThread, rule);
On 2016/09/14 22:08:47, eae wrote:
> Maybe add a DCHECK(actualColumnCount() > 0) before to ensure it doesn't
> undeflow?

There are already other actualColumnCount() - 1 expressions, so I see little
point in DCHECKing only here. Note that the actualColumnCount() implementation
has an assert on being 1 or greater, though.

Hmm, I'll document that actualColumnCount() returns 1 or greater instead of
adding a DCHECK here.

[mstensho (USE GERRIT), 2016-09-15 06:37:56]

The CQ bit was checked by mstensho@opera.com

[mstensho (USE GERRIT), 2016-09-15 06:37:57]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2339973002/#ps40001
(title: "Documentation")

[commit-bot: I haz the power, 2016-09-15 06:38:16]

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-09-15 07:42:32]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-15 07:44:53]

Description was changed from

==========
Handle exclusive end offsets when translating from flow thread coordinates.

If we're in flipped blocks writing mode (i.e. vertical-rl), the flow thread
block offset we're dealing with may be a logical end point, and end points are
exclusive. This means that we need to pick the previous column, not the next,
if the offset is exactly at a column boundary.

Let flowThreadTranslationAtOffset() and columnIndexAtOffset() take a
PageBoundaryRule argument to handle this.

This makes offsetLeft and offsetTop work properly in vertical-rl writing mode
for elements that end at column boundaries. Added a test for that, and threw in
a vertical-lr test too, for good measure.

Remove ColumnIndexCalculationMode from columnIndexAtOffset(). It was partially
and inaccurately used to make sure we didn't escape the valid column range in
case an exclusive end offset was passed. Have the call sites that really need
to clamp the column index do it themselves. It's up to the callers to decide
how to treat offsets outside the range of columns anyway.
==========

to

==========
Handle exclusive end offsets when translating from flow thread coordinates.

If we're in flipped blocks writing mode (i.e. vertical-rl), the flow thread
block offset we're dealing with may be a logical end point, and end points are
exclusive. This means that we need to pick the previous column, not the next,
if the offset is exactly at a column boundary.

Let flowThreadTranslationAtOffset() and columnIndexAtOffset() take a
PageBoundaryRule argument to handle this.

This makes offsetLeft and offsetTop work properly in vertical-rl writing mode
for elements that end at column boundaries. Added a test for that, and threw in
a vertical-lr test too, for good measure.

Remove ColumnIndexCalculationMode from columnIndexAtOffset(). It was partially
and inaccurately used to make sure we didn't escape the valid column range in
case an exclusive end offset was passed. Have the call sites that really need
to clamp the column index do it themselves. It's up to the callers to decide
how to treat offsets outside the range of columns anyway.

Committed: https://crrev.com/8403c15086b287d0db07c7fe9bcf104d02c99052
Cr-Commit-Position: refs/heads/master@{#418800}
==========

[commit-bot: I haz the power, 2016-09-15 07:44:54]

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