Stop explicit inheritance if LayoutStyle doesn't change.

Stop explicit inheritance if LayoutStyle doesn't change.

Propagation of explicitly inherited properties down the tree is not
necessary when LayoutStyle does not change, so start comparing LayoutStyle
before checking the explicitInheritance flag.

The LayoutStyle comparison should not include the explicitInheritance flag
as that typically results in a propagation diff caused by that flag being
unset on new LayoutStyle instances before any children get their styles
calculated.

The assumption is that it is not relevant in other uses of
LayoutStyle::operator==, either. In particular, also the comparison with
baseRenderStyle in the ASSERT in ActiveAnimations::updateBaseLayoutStyle.

The explicitInheritance flag is also skipped for copyNonInheritedFrom.
The reason is that it's only used for sharing non-inherited properties
between elements which don't have the same children, hence should not
share that flag either.

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

[rune, 2015-02-13 00:09:28]

rune@opera.com changed reviewers:
+ jchaffraix@chromium.org

[Timothy Loh, 2015-02-13 00:51:19]

lgtm

[esprehn, 2015-02-13 02:29:12]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org, timloh@chromium.org

[esprehn, 2015-02-13 02:29:13]

https://codereview.chromium.org/921843004/diff/1/Source/core/layout/style/Lay...
File Source/core/layout/style/LayoutStyle.cpp (left):

https://codereview.chromium.org/921843004/diff/1/Source/core/layout/style/Lay...
Source/core/layout/style/LayoutStyle.cpp:254:
noninherited_flags.explicitInheritance =
other.noninherited_flags.explicitInheritance;
Why is it okay not to copy this bit? This seems to break the contract of what
copyNonInheritedFrom does?

https://codereview.chromium.org/921843004/diff/1/Source/core/layout/style/Lay...
File Source/core/layout/style/LayoutStyle.h (left):

https://codereview.chromium.org/921843004/diff/1/Source/core/layout/style/Lay...
Source/core/layout/style/LayoutStyle.h:230: && explicitInheritance ==
other.explicitInheritance
This doesn't seem right, why is it okay to return == for a style where the
explicitInheritance bit doesn't match?

[rune, 2015-02-13 10:41:49]

On 2015/02/13 at 02:29:13, esprehn wrote:
>
https://codereview.chromium.org/921843004/diff/1/Source/core/layout/style/Lay...
> File Source/core/layout/style/LayoutStyle.cpp (left):
> 
>
https://codereview.chromium.org/921843004/diff/1/Source/core/layout/style/Lay...
> Source/core/layout/style/LayoutStyle.cpp:254:
noninherited_flags.explicitInheritance =
other.noninherited_flags.explicitInheritance;
> Why is it okay not to copy this bit? This seems to break the contract of what
copyNonInheritedFrom does?

It's a flag saying something about the children's style. copyNonInheritedFrom
copy these flags between LayoutStyles for elements which obviously don't share
the same children. Either, we shouldn't have a matched-properties-cache match
when this flag is set, or we should not copy it.

As opposed to unique(), which may indicate that the computed styles have been
tampered with after the matched properties have been applied,
explicitInheritance does not cause the resulting computed style, given a set of
matched properties, to be different. So it's OK to do a copyNonInheritedFrom the
cached style.

I agree this is messy, and it stems from the fact that we store a few flags on
LayoutStyle that is sensitive to the element context. Those flags should
probably be moved to a struct separate from NonInheritedFlags as long as it
doesn't increase the class size.

WDYT?

>
https://codereview.chromium.org/921843004/diff/1/Source/core/layout/style/Lay...
> File Source/core/layout/style/LayoutStyle.h (left):
> 
>
https://codereview.chromium.org/921843004/diff/1/Source/core/layout/style/Lay...
> Source/core/layout/style/LayoutStyle.h:230: && explicitInheritance ==
other.explicitInheritance
> This doesn't seem right, why is it okay to return == for a style where the
explicitInheritance bit doesn't match?

Because it says something about the LayoutStyle for children, not the
LayoutStyle itself. The explicitInheritance flag can only be correctly set by
children recalculating their LayoutStyles.

[rune, 2015-02-18 07:29:46]

On 2015/02/13 at 10:41:49, rune wrote:
> On 2015/02/13 at 02:29:13, esprehn wrote:
> >
https://codereview.chromium.org/921843004/diff/1/Source/core/layout/style/Lay...
> > File Source/core/layout/style/LayoutStyle.cpp (left):
> > 
> >
https://codereview.chromium.org/921843004/diff/1/Source/core/layout/style/Lay...
> > Source/core/layout/style/LayoutStyle.cpp:254:
noninherited_flags.explicitInheritance =
other.noninherited_flags.explicitInheritance;
> > Why is it okay not to copy this bit? This seems to break the contract of
what copyNonInheritedFrom does?
> 
> It's a flag saying something about the children's style. copyNonInheritedFrom
copy these flags between LayoutStyles for elements which obviously don't share
the same children. Either, we shouldn't have a matched-properties-cache match
when this flag is set, or we should not copy it.
> 
> As opposed to unique(), which may indicate that the computed styles have been
tampered with after the matched properties have been applied,
explicitInheritance does not cause the resulting computed style, given a set of
matched properties, to be different. So it's OK to do a copyNonInheritedFrom the
cached style.
> 
> I agree this is messy, and it stems from the fact that we store a few flags on
LayoutStyle that is sensitive to the element context. Those flags should
probably be moved to a struct separate from NonInheritedFlags as long as it
doesn't increase the class size.
> 
> WDYT?

@esprehn: ping.

[rune, 2015-02-18 12:01:15]

New patchsets have been uploaded after l-g-t-m from timloh@chromium.org

[rune, 2015-02-18 12:08:45]

I have gone through all the non-inherited-flags and, in the cases where they're
left out of copyNonInheritedFrom or operator==, documented why. For
completeness, the skipped flags are listed commented to show that they're
intentionally skipped.

I've renamed copyNonInheritedFrom -> copyNonInheritedFromCached to make its
purpose clearer.

A few more flags are skipped during comparison and a test added to show why.

Hope this makes things better/clearer.

[rune, 2015-02-23 10:03:21]

@esprehn, @timloh: wdyt about the latest patch set?

[rune, 2015-02-26 14:37:28]

On 2015/02/23 10:03:21, rune wrote:
> @esprehn, @timloh: wdyt about the latest patch set?

ping

[esprehn, 2015-02-27 00:54:56]

lgtm, thanks for doing the diligence and documenting why this is correct. I
really appreciate the work you put into your changes. :)

[rune, 2015-02-27 08:38:34]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2015-02-27 08:39:39]

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

[commit-bot: I haz the power, 2015-02-27 10:01:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-27 10:01:57]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/4...)

[rune, 2015-02-27 11:34:38]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2015-02-27 11:35:33]

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

[commit-bot: I haz the power, 2015-02-27 12:24:00]

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