*** NOT FOR LANDING *** Text nodes should only inherit inheritable properties.

*** NOT FOR LANDING *** Text nodes should only inherit inheritable properties.

Trying this instead of adding more workarounds [1] for over-styled text nodes.

[1] https://codereview.chromium.org/1149333003/

[mstensho (USE GERRIT), 2015-05-29 13:13:41]

mstensho@opera.com changed reviewers:
+ dsinclair@chromium.org, esprehn@chromium.org, jchaffraix@chromium.org,
leviw@chromium.org, rune@opera.com

[mstensho (USE GERRIT), 2015-05-29 13:13:42]

There are still some issues to solve here:

Need to fix https://code.google.com/p/chromium/issues/detail?id=492785 and
https://code.google.com/p/chromium/issues/detail?id=492602 first.

There's also one failing table test. It fails because text nodes no longer
inherit vertical-align (it's not supposed to be inherited by default).

With the current solution we also inherit some bidi and SVG stuff that's not
supposed to be inherited. I don't like that. Not sure how much work it would be
to fix it, though.

Anyway, now at least we have a good overview over what it entails code-wise to
only inherit inheritable properties on text nodes. I'd like some feedback before
continuing. I think it generally makes the code look healthier, but I'm
wondering if there would be performance or memory implications by doing this.

https://codereview.chromium.org/1156143002/diff/140001/Source/core/style/Comp...
File Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1156143002/diff/140001/Source/core/style/Comp...
Source/core/style/ComputedStyle.cpp:101:
newStyle->inheritUnicodeBidiFrom(parentStyle);
Would be great to get rid of inheritance of non-inheritable bidi stuff and SVG
here, but that would require additional code changes. If we cannot get rid of
it, createWithInheritableProperties() isn't a good name.

[leviw_travelin_and_unemployed, 2015-05-29 17:44:52]

https://codereview.chromium.org/1156143002/diff/160001/Source/core/dom/Layout...
File Source/core/dom/LayoutTreeBuilder.cpp (right):

https://codereview.chromium.org/1156143002/diff/160001/Source/core/dom/Layout...
Source/core/dom/LayoutTreeBuilder.cpp:152: RefPtr<ComputedStyle> style =
ComputedStyle::createWithInheritableProperties(parentStyle);
Have you ran the perf tests? I wonder if this allocation will matter.

[Julien - ping for review, 2015-06-01 19:13:48]

The change looks like a great simplification.

Like Levi, I would expect this code to be performance sensitive so it would be
nice to have some data on that.

https://codereview.chromium.org/1156143002/diff/160001/Source/core/layout/Lay...
File Source/core/layout/LayoutText.cpp (right):

https://codereview.chromium.org/1156143002/diff/160001/Source/core/layout/Lay...
Source/core/layout/LayoutText.cpp:712: if (isCombineText()) {
We were only creating LayoutCombineText nodes if style()->hasTextCombine(). What
changed?

[mstensho (USE GERRIT), 2015-06-01 19:31:37]

So far, testing seems to show that some performance tests are 5-10% slower,
while some other tests are faster. Not sure how much I can trust that; results
fluctuate quite a lot. I'm also worried about memory usage, since we now create
more ComputedStyle objects than we used to.

Yeah, I like what the code looks like after the changes too, :) but maybe
whoever decided to use the same properties on text nodes as on their parents had
exactly this in mind: save memory, increase performance, at the cost of some
pitfalls in the code. I lost the trail somewhere in 2002 (there was talk about a
KDE branch, I think), last time I tried to dig through the revision history.

https://codereview.chromium.org/1156143002/diff/160001/Source/core/layout/Lay...
File Source/core/layout/LayoutText.cpp (right):

https://codereview.chromium.org/1156143002/diff/160001/Source/core/layout/Lay...
Source/core/layout/LayoutText.cpp:712: if (isCombineText()) {
On 2015/06/01 19:13:47, Julien Chaffraix - PST wrote:
> We were only creating LayoutCombineText nodes if style()->hasTextCombine().
What
> changed?

-webkit-combine-text isn't an inherited property.

[esprehn, 2015-06-02 22:54:08]

We intentionally don't malloc LayoutStyle objects for text nodes since that
means higher memory usage. For many pages this doubles the number of
LayoutStyles we create. I don't think this change is an improvement.

Also why do so many tests change?

[mstensho (USE GERRIT), 2015-06-03 10:40:28]

On 2015/06/02 22:54:08, esprehn wrote:
> We intentionally don't malloc LayoutStyle objects for text nodes since that
> means higher memory usage. For many pages this doubles the number of
> LayoutStyles we create. I don't think this change is an improvement.

It cleans up the code, but yeah, this WILL increase memory usage. So I guess I
should drop this then, and live with the hacks (and I have to add one more; see
https://codereview.chromium.org/1149333003/ ).

> Also why do so many tests change?

Text nodes would no longer have z-index, so some DRT results would change.

[mstensho (USE GERRIT), 2015-06-04 18:09:35]

Dropping this