[New Multicolumn] mapAbsoluteToLocalPoint() needs to convert to flow thread coordinates.

Source/core/layout/LayoutBoxModelObject.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2005 Allan Sandfeld Jensen (kde@carewolf.com)
  *           (C) 2005, 2006 Samuel Weinig (sam.weinig@gmail.com)
  * Copyright (C) 2005, 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
  * Copyright (C) 2010 Google Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ skipping 770 matching lines @@
     LayoutUnit y = paddingTop() + borderTop() + (verticalSpace / 2);
     return currentStyle.isHorizontalWritingMode() ? LayoutRect(x, y, caretWidth, height) : LayoutRect(y, x, height, caretWidth);
 }
 
 void LayoutBoxModelObject::mapAbsoluteToLocalPoint(MapCoordinatesFlags mode, TransformState& transformState) const
 {
     LayoutObject* o = container();
     if (!o)
         return;
 
-    if (o->isLayoutFlowThread())
-        transformState.move(o->columnOffset(LayoutPoint(transformState.mappedPoint())));
-
     o->mapAbsoluteToLocalPoint(mode, transformState);
 
     LayoutSize containerOffset = offsetFromContainer(o, LayoutPoint());
 
-    if (!style()->hasOutOfFlowPosition() && o->hasColumns()) {
+    if (o->isLayoutFlowThread()) {
+        // Descending into a flow thread. Convert to the local coordinate space, i.e. flow thread coordinates.
+        const LayoutFlowThread* flowThread = toLayoutFlowThread(o);
+        LayoutPoint visualPoint = LayoutPoint(transformState.mappedPoint());
+        transformState.move(visualPoint - flowThread->visualPointToFlowThreadPoint(visualPoint));
+        // |containerOffset| is also in visual coordinates. Convert to flow thread coordinates.
+        // TODO(mstensho): Wouldn't it be better add a parameter to instruct offsetFromContainer()

[Julien - ping for review, 2015/03/25 21:41:32]

What's preventing this change now? I was wondering also if we couldn't override
offsetFromContainer() (e.g. in LayoutFlowThreadMultiColumn) and pipe the calls
to the overridden function instead of an extra parameter to the function.

[mstensho (USE GERRIT), 2015/03/25 22:35:05]

offsetFromContainer() currently needs to perform the conversion from flow thread
coordinates to visual coordinates when walking up the tree, like when called
from mapLocalToContainer(), while it's bad to do it when walking down the tree,
which is what we do here.

I would prefer to sit back and think about it, rather than fixing it right away,
as part of this CL. Also: any function that takes more than 3 parameters is
insane, in my book. :) So I'm reluctant to add another parameter if there are
better ways. I'm starting to think that it's wrong to perform this conversion at
all in offsetFromContainer(), since it makes ugly assumptions about what the
caller uses the result for (and those callers that don't want it need to convert
it back). Better have the callers do this on their own, maybe?

[Julien - ping for review, 2015/03/26 15:38:56]

It seems reasonable to let the callers do it. I see that it's definitely not
trivial and better to be pushed into a follow-up change.

+        // to return flowthread coordinates in the first place? We're effectively performing two
+        // conversions here, when in fact none is needed.
+        containerOffset = toLayoutSize(flowThread->visualPointToFlowThreadPoint(toLayoutPoint(containerOffset)));
+    } else if (!style()->hasOutOfFlowPosition() && o->hasColumns()) {
         LayoutBlock* block = toLayoutBlock(o);
         LayoutPoint point(roundedLayoutPoint(transformState.mappedPoint()));
         point -= containerOffset;
         block->adjustForColumnRect(containerOffset, point);
     }
 
     bool preserve3D = mode & UseTransforms && (o->style()->preserves3D() || style()->preserves3D());
     if (mode & UseTransforms && shouldUseTransformFromContainer(o)) {
         TransformationMatrix t;
         getTransformFromContainer(o, containerOffset, t);
@@ skipping 72 matching lines @@
     ASSERT(!beforeChild || toBoxModelObject == beforeChild->parent());
     for (LayoutObject* child = startChild; child && child != endChild; ) {
         // Save our next sibling as moveChildTo will clear it.
         LayoutObject* nextSibling = child->nextSibling();
         moveChildTo(toBoxModelObject, child, beforeChild, fullRemoveInsert);
         child = nextSibling;
     }
 }
 
 } // namespace blink