Make column snapping optional when translating to flow thread coordinates.

Make column snapping optional when translating to flow thread coordinates.

positionForPoint() wants this, but mapAncestorToLocal() requires that no
special behavior be applied.

While this CL doesn't fix bug 663062, it is a prerequisite for fixing it
without breaking existing tests (MulticolWithAbsPosNotContained in
MapCoordinatesTest unit test).

BUG=663062
Committed: https://crrev.com/90a520011b9e1f70355987371360540c922f4fc9
Cr-Commit-Position: refs/heads/master@{#439758}

[mstensho (USE GERRIT), 2016-12-19 13:04:27]

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

[commit-bot: I haz the power, 2016-12-19 13:04:43]

Dry run: 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-12-19 14:18:41]

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

[commit-bot: I haz the power, 2016-12-19 14:18:42]

Dry run: This issue passed the CQ dry run.

[mstensho (USE GERRIT), 2016-12-19 15:45:48]

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

[mstensho (USE GERRIT), 2016-12-19 15:45:49]

@szager I usually bother Emil with patches like this, but since he's OOO, it
would be great if you could take a look. Otherwise I'm sure it can wait until
next year.

[szager1, 2016-12-19 23:36:44]

https://codereview.chromium.org/2590463002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.h
(right):

https://codereview.chromium.org/2590463002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.h:92: enum
SnapToColumnPolicy { DontSnapToColumn, SnapToColumn };
nit: I prefer not to define an enum for what is effectively a boolean flag. 
Better, I think, to just use a boolean parameter with a self-documenting name. 
I don't feel strongly, so you can ignore this.

https://codereview.chromium.org/2590463002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.h:95:
SnapToColumnPolicy = DontSnapToColumn) const;
It's a bit strange that the default value doesn't preserve the previous
behavior.  Is this what you wanted?

[mstensho (USE GERRIT), 2016-12-20 08:55:02]

https://codereview.chromium.org/2590463002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.h
(right):

https://codereview.chromium.org/2590463002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.h:92: enum
SnapToColumnPolicy { DontSnapToColumn, SnapToColumn };
On 2016/12/19 23:36:44, szager1 wrote:
> nit: I prefer not to define an enum for what is effectively a boolean flag. 
> Better, I think, to just use a boolean parameter with a self-documenting name.

> I don't feel strongly, so you can ignore this.

A long time ago I learned that this was how to do things in Blink/WebKit.
Basically, we should never use bools in parameters, unless it's super-obvious
what it means (a bool setter, for instance). I've come to like this rule,
because it *is* hard to tell what it does when looking at the code from the call
site.

The requirement is still there (if the document is still valid). See item 10
here:
https://www.chromium.org/blink/coding-style#TOC-Names

https://codereview.chromium.org/2590463002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.h:95:
SnapToColumnPolicy = DontSnapToColumn) const;
On 2016/12/19 23:36:44, szager1 wrote:
> It's a bit strange that the default value doesn't preserve the previous
> behavior.  Is this what you wanted?

This is exactly what I wanted. The previous behavior is weird and non-obvious. I
wanted the default behavior to be straight-forward, boring and without
surprises.

[szager1, 2016-12-20 09:37:07]

lgtm

[mstensho (USE GERRIT), 2016-12-20 09:38:47]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2016-12-20 09:38:56]

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-12-20 09:42:55]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1482226727016720, "parent_rev":
"91c3977f28b56efa2f60e99b316d19c9d0244ae8", "commit_rev":
"9ae54d48f8b8a1796d7f16e5b870e5dc17aeebd6"}

[commit-bot: I haz the power, 2016-12-20 09:43:20]

Description was changed from

==========
Make column snapping optional when translating to flow thread coordinates.

positionForPoint() wants this, but mapAncestorToLocal() requires that no
special behavior be applied.

While this CL doesn't fix bug 663062, it is a prerequisite for fixing it
without breaking existing tests (MulticolWithAbsPosNotContained in
MapCoordinatesTest unit test).

BUG=663062
==========

to

==========
Make column snapping optional when translating to flow thread coordinates.

positionForPoint() wants this, but mapAncestorToLocal() requires that no
special behavior be applied.

While this CL doesn't fix bug 663062, it is a prerequisite for fixing it
without breaking existing tests (MulticolWithAbsPosNotContained in
MapCoordinatesTest unit test).

BUG=663062

Review-Url: https://codereview.chromium.org/2590463002
==========

[commit-bot: I haz the power, 2016-12-20 09:43:22]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-12-20 09:45:42]

Description was changed from

==========
Make column snapping optional when translating to flow thread coordinates.

positionForPoint() wants this, but mapAncestorToLocal() requires that no
special behavior be applied.

While this CL doesn't fix bug 663062, it is a prerequisite for fixing it
without breaking existing tests (MulticolWithAbsPosNotContained in
MapCoordinatesTest unit test).

BUG=663062

Review-Url: https://codereview.chromium.org/2590463002
==========

to

==========
Make column snapping optional when translating to flow thread coordinates.

positionForPoint() wants this, but mapAncestorToLocal() requires that no
special behavior be applied.

While this CL doesn't fix bug 663062, it is a prerequisite for fixing it
without breaking existing tests (MulticolWithAbsPosNotContained in
MapCoordinatesTest unit test).

BUG=663062

Committed: https://crrev.com/90a520011b9e1f70355987371360540c922f4fc9
Cr-Commit-Position: refs/heads/master@{#439758}
==========

[commit-bot: I haz the power, 2016-12-20 09:45:43]

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