Merge LayoutInline::mapAbsoluteToLocalPoint() into LayoutObject.

Merge LayoutInline::mapAbsoluteToLocalPoint() into LayoutObject.

Avoids code duplication and maybe also makes the LayoutObject implementation
more correct. It was typically only LayoutText objects that used the
LayoutObject implementation of mapAbsoluteToLocalPoint() prior to this change.

Some extra care was needed when calling style() members, since text nodes just
copy their parent's computed style instead of just inheriting the inheritable
ones. Apart from that, it's basically just about replacing the LayoutObject
implementation with that of LayoutInline.

R=leviw@chromium.org,ojan@chromium.org

[mstensho (USE GERRIT), 2015-06-16 16:05:01]

Trying to get rid of some duplicated code.

Next up: getting rid of the LayoutBox implementation of this method

[mstensho (USE GERRIT), 2015-06-16 20:19:07]

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

[ojan, 2015-06-19 02:21:09]

Sorry it took me so long to respond here. It's hard for me to have confidence
this is a correct change. What's the goal here? The code doesn't actually seem
simpler afterwards.

Is the goal to make it so that we move in the direction of not having
LayoutTexts inherit style from their parents? I was thinking, what if we
actually didn't have style() on LayoutText and instead exposed a TextStyle that
wraps the ComputedStyle and only exposes the things that LayoutText's need. This
is similar in spirit to https://codereview.chromium.org/1164933006.

https://codereview.chromium.org/1181953005/diff/1/Source/core/layout/LayoutOb...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1181953005/diff/1/Source/core/layout/LayoutOb...
Source/core/layout/LayoutObject.cpp:2045: if (!isText() &&
style()->hasInFlowPosition() && hasLayer())
Nit: I believe a LayoutText can't have a layer, so can you instead put an
ASSERT(!isText()) inside here?

https://codereview.chromium.org/1181953005/diff/1/Source/core/layout/LayoutOb...
Source/core/layout/LayoutObject.cpp:2066: bool preserve3D = mode & UseTransforms
&& (o->style()->preserves3D() || (!isText() && style()->preserves3D()));
I don't think this is right. Even if your parent preserves3D, I don't think the
LayoutText wants to.

[mstensho (USE GERRIT), 2015-06-19 09:52:42]

On 2015/06/19 02:21:09, ojan wrote:
> Sorry it took me so long to respond here. It's hard for me to have confidence
> this is a correct change. What's the goal here? The code doesn't actually seem
> simpler afterwards.

Well, now LayoutObject::mapAbsoluteToLocalPoint() is about as heavy as
LayoutInline::mapAbsoluteToLocalPoint() used to be. But at least we reduced the
number of implementations by one. :)

> Is the goal to make it so that we move in the direction of not having
> LayoutTexts inherit style from their parents?

Oh, no I have given up on that for now. I just want less (potentially poorly)
duplicated code. The plan is to merge the LayoutBox implementation in here as
well. Afterwards, I'd like to do a similar unification of the various
offsetFromContainer() methods, to get rid of duplicated code.

I've always found this area of the code very confusing to work with. Snippets of
almost identical code everywhere!

The ultimate goal is to get rid of a multicol kludge in
LayoutBoxModelObject::mapAbsoluteToLocalPoint(). Admittedly, I could do that
without this rather big cleanup.

> I was thinking, what if we
> actually didn't have style() on LayoutText and instead exposed a TextStyle
that
> wraps the ComputedStyle and only exposes the things that LayoutText's need.
This
> is similar in spirit to https://codereview.chromium.org/1164933006.

That sounds nice! But as I've explained now, that's orthogonal to my work here.

I've just uploaded https://codereview.chromium.org/1198433002/ , so that you can
see the big picture, with all the changes that I intend to make.

I haven't yet addressed any of the two code review issues you raised, BTW. Will
get to that if we agree to go ahead with these changes at all.

https://codereview.chromium.org/1181953005/diff/1/Source/core/layout/LayoutOb...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1181953005/diff/1/Source/core/layout/LayoutOb...
Source/core/layout/LayoutObject.cpp:2045: if (!isText() &&
style()->hasInFlowPosition() && hasLayer())
On 2015/06/19 02:21:09, ojan wrote:
> Nit: I believe a LayoutText can't have a layer, so can you instead put an
> ASSERT(!isText()) inside here?

Indeed. I can remove it. I put it there, so that we wouldn't check (the
non-inherited) style()->hasInFlowPosition() on text nodes. But yeah, it's an
unnecessary check here, since we require a layer too, which text may never have.

https://codereview.chromium.org/1181953005/diff/1/Source/core/layout/LayoutOb...
Source/core/layout/LayoutObject.cpp:2066: bool preserve3D = mode & UseTransforms
&& (o->style()->preserves3D() || (!isText() && style()->preserves3D()));
On 2015/06/19 02:21:09, ojan wrote:
> I don't think this is right. Even if your parent preserves3D, I don't think
the
> LayoutText wants to.

First of all: this was an unintended change from my side.

So, if a parent preserves3D, a LayoutInline should too, while a LayoutText
shouldn't? I have no idea, but that does sound inconsistent. But OK, to err on
the safe side, I'd better add a broader !isText() check.

[mstensho (USE GERRIT), 2015-07-28 09:46:11]

Review request timed out. :) I'll close it. If I ever want to make another
attempt at cleaning this up, I'll file a new CL.

[ojan, 2015-09-15 17:10:13]

On 2015/07/28 at 09:46:11, mstensho wrote:
> Review request timed out. :) I'll close it. If I ever want to make another
attempt at cleaning this up, I'll file a new CL.

Sigh. Getting caught up on email backlog. So sorry that I dropped the ball on
this. Levi was going to walk me through this code and we never got around to
scheduling it. If you decide you want to tackle this again, let me know and me,
you and levi can sync up.

[mstensho (USE GERRIT), 2015-12-10 21:23:06]

On 2015/09/15 17:10:13, ojan wrote:
> On 2015/07/28 at 09:46:11, mstensho wrote:
> > Review request timed out. :) I'll close it. If I ever want to make another
> attempt at cleaning this up, I'll file a new CL.
> 
> Sigh. Getting caught up on email backlog. So sorry that I dropped the ball on
> this. Levi was going to walk me through this code and we never got around to
> scheduling it. If you decide you want to tackle this again, let me know and
me,
> you and levi can sync up.

Rebooted: https://codereview.chromium.org/1516003003/