Ignore style changes on text node flowthread descendants.

Ignore style changes on text node flowthread descendants.

Text nodes inherit every property from their parent, including non-inheritable
ones! If the parent DOM node is the multicol container, and its 'position' goes
from 'static' to 'absolute', so will any text node child, as far as
ComputedStyle is concerned. If the multicol container only contains a text node
child, we'd incorrectly think that the multicol container started to only
contain an out-of-flow child, so that the column set could be removed.

Also don't notify the flowthread about descendant style changes if the
descendant in question is the flow thread (Calling flowThreadContainingBlock()
on the flow thread returns the flow thread). There's currently no way any harm
could be done if the flow tread were indeed treated as a descendant of itself
here, but it's just wrong and bad.

BUG=491409
R=dsinclair@chromium.org,jchaffraix@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196508

[dsinclair, 2015-05-25 13:42:48]

https://codereview.chromium.org/1149333003/diff/1/Source/core/layout/LayoutMu...
File Source/core/layout/LayoutMultiColumnFlowThread.cpp (right):

https://codereview.chromium.org/1149333003/diff/1/Source/core/layout/LayoutMu...
Source/core/layout/LayoutMultiColumnFlowThread.cpp:649: // Text nodes inherit
all properties from the parent node (including non-inheritable
Is the larger bug here that we inherit non-inheritable properties? Would the
correct fix to be to fix that?

[mstensho (USE GERRIT), 2015-05-25 18:25:13]

https://codereview.chromium.org/1149333003/diff/1/Source/core/layout/LayoutMu...
File Source/core/layout/LayoutMultiColumnFlowThread.cpp (right):

https://codereview.chromium.org/1149333003/diff/1/Source/core/layout/LayoutMu...
Source/core/layout/LayoutMultiColumnFlowThread.cpp:649: // Text nodes inherit
all properties from the parent node (including non-inheritable
On 2015/05/25 13:42:47, dsinclair wrote:
> Is the larger bug here that we inherit non-inheritable properties? Would the
> correct fix to be to fix that?

I've been thinking that doing something as crazy as text nodes inheriting all
properties from its parent served some important purpose, such as being able to
share the property data, to save memory. But maybe I'm wrong.

OK, let me try fixing that instead. There are some !isText() checks (or actually
!isTextOrSVGChild()) in LayoutObject::containingBlock() (and probably a bunch of
similar things throughout the code) related to this.

[dsinclair, 2015-05-25 18:27:42]

On 2015/05/25 at 18:25:13, mstensho wrote:
>
https://codereview.chromium.org/1149333003/diff/1/Source/core/layout/LayoutMu...
> File Source/core/layout/LayoutMultiColumnFlowThread.cpp (right):
> 
>
https://codereview.chromium.org/1149333003/diff/1/Source/core/layout/LayoutMu...
> Source/core/layout/LayoutMultiColumnFlowThread.cpp:649: // Text nodes inherit
all properties from the parent node (including non-inheritable
> On 2015/05/25 13:42:47, dsinclair wrote:
> > Is the larger bug here that we inherit non-inheritable properties? Would the
> > correct fix to be to fix that?
> 
> I've been thinking that doing something as crazy as text nodes inheriting all
properties from its parent served some important purpose, such as being able to
share the property data, to save memory. But maybe I'm wrong.
> 
> OK, let me try fixing that instead. There are some !isText() checks (or
actually !isTextOrSVGChild()) in LayoutObject::containingBlock() (and probably a
bunch of similar things throughout the code) related to this.


May want to ping rune@ or esprehn@ to see if the crazy is there for a reason
before sinking a lot of time into it.

[rune, 2015-05-26 09:47:31]

On 2015/05/25 18:27:42, dsinclair wrote:
> On 2015/05/25 at 18:25:13, mstensho wrote:
> >
>
https://codereview.chromium.org/1149333003/diff/1/Source/core/layout/LayoutMu...
> > File Source/core/layout/LayoutMultiColumnFlowThread.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1149333003/diff/1/Source/core/layout/LayoutMu...
> > Source/core/layout/LayoutMultiColumnFlowThread.cpp:649: // Text nodes
inherit
> all properties from the parent node (including non-inheritable
> > On 2015/05/25 13:42:47, dsinclair wrote:
> > > Is the larger bug here that we inherit non-inheritable properties? Would
the
> > > correct fix to be to fix that?
> > 
> > I've been thinking that doing something as crazy as text nodes inheriting
all
> properties from its parent served some important purpose, such as being able
to
> share the property data, to save memory. But maybe I'm wrong.
> > 
> > OK, let me try fixing that instead. There are some !isText() checks (or
> actually !isTextOrSVGChild()) in LayoutObject::containingBlock() (and probably
a
> bunch of similar things throughout the code) related to this.
> 
> 
> May want to ping rune@ or esprehn@ to see if the crazy is there for a reason
> before sinking a lot of time into it.

I don't know if any non-inherited properties from the parent is used by
LayoutText. If not, I don't know why we would need to copy the non-inherited
ones from the parent. I would have taken a shot at not copying the non-inherited
properties and see what the bots say.

It's possible ComputedStyle::visualInvalidationDiff should be short-circuited
for text nodes as well.

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

On 2015/05/25 18:25:13, mstensho wrote:
>
https://codereview.chromium.org/1149333003/diff/1/Source/core/layout/LayoutMu...
> File Source/core/layout/LayoutMultiColumnFlowThread.cpp (right):
> 
>
https://codereview.chromium.org/1149333003/diff/1/Source/core/layout/LayoutMu...
> Source/core/layout/LayoutMultiColumnFlowThread.cpp:649: // Text nodes inherit
> all properties from the parent node (including non-inheritable
> On 2015/05/25 13:42:47, dsinclair wrote:
> > Is the larger bug here that we inherit non-inheritable properties? Would the
> > correct fix to be to fix that?
> 
> I've been thinking that doing something as crazy as text nodes inheriting all
> properties from its parent served some important purpose, such as being able
to
> share the property data, to save memory. But maybe I'm wrong.
> 
> OK, let me try fixing that instead. There are some !isText() checks (or
actually
> !isTextOrSVGChild()) in LayoutObject::containingBlock() (and probably a bunch
of
> similar things throughout the code) related to this.

Here: https://codereview.chromium.org/1156143002/ - please let me know what you
think.

[mstensho (USE GERRIT), 2015-06-04 07:08:08]

On 2015/05/25 18:27:42, dsinclair wrote:
> On 2015/05/25 at 18:25:13, mstensho wrote:
> >
>
https://codereview.chromium.org/1149333003/diff/1/Source/core/layout/LayoutMu...
> > File Source/core/layout/LayoutMultiColumnFlowThread.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1149333003/diff/1/Source/core/layout/LayoutMu...
> > Source/core/layout/LayoutMultiColumnFlowThread.cpp:649: // Text nodes
inherit
> all properties from the parent node (including non-inheritable
> > On 2015/05/25 13:42:47, dsinclair wrote:
> > > Is the larger bug here that we inherit non-inheritable properties? Would
the
> > > correct fix to be to fix that?
> > 
> > I've been thinking that doing something as crazy as text nodes inheriting
all
> properties from its parent served some important purpose, such as being able
to
> share the property data, to save memory. But maybe I'm wrong.
> > 
> > OK, let me try fixing that instead. There are some !isText() checks (or
> actually !isTextOrSVGChild()) in LayoutObject::containingBlock() (and probably
a
> bunch of similar things throughout the code) related to this.
> 
> 
> May want to ping rune@ or esprehn@ to see if the crazy is there for a reason
> before sinking a lot of time into it.

Looks like we want to keep the craziness, since it saves memory. So can you
please review this patch then?

[dsinclair, 2015-06-04 13:55:00]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2015-06-04 13:55:01]

lgtm

[commit-bot: I haz the power, 2015-06-04 13:55:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149333003/1

[commit-bot: I haz the power, 2015-06-04 15:00:44]

Committed patchset #1 (id:1) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196508