Make column snapping optional when translating to flow thread coordinates.

third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.h

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef MultiColumnFragmentainerGroup_h
 #define MultiColumnFragmentainerGroup_h
 
 #include "core/layout/LayoutMultiColumnFlowThread.h"
 #include "wtf/Allocator.h"
 
@@ skipping 61 matching lines @@
     return m_logicalBottomInFlowThread - m_logicalTopInFlowThread;
   }
 
   void resetColumnHeight();
   bool recalculateColumnHeight(LayoutMultiColumnSet&);
 
   LayoutSize flowThreadTranslationAtOffset(LayoutUnit,
                                            LayoutBox::PageBoundaryRule,
                                            CoordinateSpaceConversion) const;
   LayoutUnit columnLogicalTopForOffset(LayoutUnit offsetInFlowThread) const;
+
+  // If SnapToColumnPolicy is SnapToColumn, visualPointToFlowThreadPoint() won't
+  // return points that lie outside the bounds of the columns: Before converting
+  // to a flow thread position, if the block direction coordinate is outside the
+  // column, snap to the bounds of the column, and reset the inline direction
+  // coordinate to the start position in the column. The effect of this is that
+  // if the block position is before the column rectangle, we'll get to the
+  // beginning of this column, while if the block position is after the column
+  // rectangle, we'll get to the beginning of the next column. This is behavior
+  // that positionForPoint() depends on.
+  enum SnapToColumnPolicy { DontSnapToColumn, SnapToColumn };

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

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.

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

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

   LayoutPoint visualPointToFlowThreadPoint(
-      const LayoutPoint& visualPoint) const;
+      const LayoutPoint& visualPoint,
+      SnapToColumnPolicy = DontSnapToColumn) const;

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

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]

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.

+
   LayoutRect fragmentsBoundingBox(
       const LayoutRect& boundingBoxInFlowThread) const;
 
   LayoutRect flowThreadPortionRectAt(unsigned columnIndex) const;
   LayoutRect flowThreadPortionOverflowRectAt(unsigned columnIndex) const;
 
   // Get the first and the last column intersecting the specified block range.
   // Note that |logicalBottomInFlowThread| is an exclusive endpoint.
   void columnIntervalForBlockRangeInFlowThread(
       LayoutUnit logicalTopInFlowThread,
@@ skipping 92 matching lines @@
 
  private:
   LayoutMultiColumnSet& m_columnSet;
 
   Vector<MultiColumnFragmentainerGroup, 1> m_groups;
 };
 
 }  // namespace blink
 
 #endif  // MultiColumnFragmentainerGroup_h