Untangle multicol coordinate space conversion from offsetFromContainer().

Untangle multicol coordinate space conversion from offsetFromContainer().

The various offsetFromContainer() implementations used to convert from flow
thread coordinates to visual coordinates if the container was a flow thread.
That works when mapping a position relatively to some ancestor, but not when
mapping a position relatively to some descendant. Put differently: It works
fine when walking upwards in a tree, but not so fine when walking it downwards
(we need the opposite operation in that case; convert from visual to flow
thread coordinates). That was the reason for some mess in mapAncestorToLocal(),
since we had to cancel out the shenanigans carried out by
offsetFromContainer().

So, instead, perform this flowthread-to-visual coordinate space conversion
where we need it, and don't cause trouble for those who don't need it.

No behavior changes intended. This is also why we're keeping this coordinate
space conversion in CaretBase for now, even if it's wrong (see bug 596070).
Simply removing that *now* wouldn't fix the bug anyway, just alter it (probably
for the better, but who knows -- still buggy). A proper fix will land shortly.

BUG=568492
R=leviw@chromium.org

Committed: https://crrev.com/4daee13be8513a6130f1111e7cd4d573329a70c5
Cr-Commit-Position: refs/heads/master@{#382153}

[mstensho (USE GERRIT), 2016-03-18 20:51:40]

Levi fast again? :)

https://codereview.chromium.org/1820483002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/CaretBase.cpp (right):

https://codereview.chromium.org/1820483002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/CaretBase.cpp:90: LayoutObject*
containerObject = (LayoutObject*)containerItem;
Pretty sure I'm violating the API encapsulation like a real h4x0r here. :) The
other option would be to add a temporary method to the LayoutItem API, but that
seems silly, since the patch that removes this code is more or less ready to be
filed already.

[leviw_travelin_and_unemployed, 2016-03-18 22:27:01]

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

[leviw_travelin_and_unemployed, 2016-03-18 22:27:02]

LGTM except for the C-style but C++-override-inducing cast ;)

As for fast again, I ended up getting a sheriff rotation sprung on me
surprisingly which is slowing me down, but otherwise back to normal! Did ya miss
me?

https://codereview.chromium.org/1820483002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/CaretBase.cpp (right):

https://codereview.chromium.org/1820483002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/CaretBase.cpp:90: LayoutObject*
containerObject = (LayoutObject*)containerItem;
On 2016/03/18 at 20:51:40, mstensho wrote:
> Pretty sure I'm violating the API encapsulation like a real h4x0r here. :) The
other option would be to add a temporary method to the LayoutItem API, but that
seems silly, since the patch that removes this code is more or less ready to be
filed already.

Really you should be adding that method to the API here. It's not so bad :)

[mstensho (USE GERRIT), 2016-03-18 22:45:23]

https://codereview.chromium.org/1820483002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/CaretBase.cpp (right):

https://codereview.chromium.org/1820483002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/CaretBase.cpp:90: LayoutObject*
containerObject = (LayoutObject*)containerItem;
On 2016/03/18 22:27:02, leviw wrote:
> On 2016/03/18 at 20:51:40, mstensho wrote:
> > Pretty sure I'm violating the API encapsulation like a real h4x0r here. :)
The
> other option would be to add a temporary method to the LayoutItem API, but
that
> seems silly, since the patch that removes this code is more or less ready to
be
> filed already.
> 
> Really you should be adding that method to the API here. It's not so bad :)

Done.

[mstensho (USE GERRIT), 2016-03-18 22:45:34]

The CQ bit was checked by mstensho@opera.com

[mstensho (USE GERRIT), 2016-03-18 22:45:35]

The patchset sent to the CQ was uploaded after l-g-t-m from leviw@chromium.org
Link to the patchset: https://codereview.chromium.org/1820483002/#ps20001
(title: "Address code review comment.")

[commit-bot: I haz the power, 2016-03-18 22:46:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1820483002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1820483002/20001

[commit-bot: I haz the power, 2016-03-19 00:46:55]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-19 00:50:26]

Description was changed from

==========
Untangle multicol coordinate space conversion from offsetFromContainer().

The various offsetFromContainer() implementations used to convert from flow
thread coordinates to visual coordinates if the container was a flow thread.
That works when mapping a position relatively to some ancestor, but not when
mapping a position relatively to some descendant. Put differently: It works
fine when walking upwards in a tree, but not so fine when walking it downwards
(we need the opposite operation in that case; convert from visual to flow
thread coordinates). That was the reason for some mess in mapAncestorToLocal(),
since we had to cancel out the shenanigans carried out by
offsetFromContainer().

So, instead, perform this flowthread-to-visual coordinate space conversion
where we need it, and don't cause trouble for those who don't need it.

No behavior changes intended. This is also why we're keeping this coordinate
space conversion in CaretBase for now, even if it's wrong (see bug 596070).
Simply removing that *now* wouldn't fix the bug anyway, just alter it (probably
for the better, but who knows -- still buggy). A proper fix will land shortly.

BUG=568492
R=leviw@chromium.org
==========

to

==========
Untangle multicol coordinate space conversion from offsetFromContainer().

The various offsetFromContainer() implementations used to convert from flow
thread coordinates to visual coordinates if the container was a flow thread.
That works when mapping a position relatively to some ancestor, but not when
mapping a position relatively to some descendant. Put differently: It works
fine when walking upwards in a tree, but not so fine when walking it downwards
(we need the opposite operation in that case; convert from visual to flow
thread coordinates). That was the reason for some mess in mapAncestorToLocal(),
since we had to cancel out the shenanigans carried out by
offsetFromContainer().

So, instead, perform this flowthread-to-visual coordinate space conversion
where we need it, and don't cause trouble for those who don't need it.

No behavior changes intended. This is also why we're keeping this coordinate
space conversion in CaretBase for now, even if it's wrong (see bug 596070).
Simply removing that *now* wouldn't fix the bug anyway, just alter it (probably
for the better, but who knows -- still buggy). A proper fix will land shortly.

BUG=568492
R=leviw@chromium.org

Committed: https://crrev.com/4daee13be8513a6130f1111e7cd4d573329a70c5
Cr-Commit-Position: refs/heads/master@{#382153}
==========

[commit-bot: I haz the power, 2016-03-19 00:50:27]

Patchset 2 (id:??) landed as
https://crrev.com/4daee13be8513a6130f1111e7cd4d573329a70c5
Cr-Commit-Position: refs/heads/master@{#382153}