Clear need for animation style for forced styleForElement.

Clear need for animation style for forced styleForElement.

When style recalc for an element is triggered by a StyleRecalcChange from
the ancestors, we need to clear the m_animationStyleChange flag on
ElementAnimations if set since the baseLayoutStyle cannot be used. This
is normally done from Element::recalcStyle just before recalcOwnStyle is
called.

Document::inheritHtmlAndBodyElementStyles does styleForElement outside of
the recalcStyle machinery and didn't clear this flag which caused an
assert for baseLayoutStyle comparison.

In particular, the case was that while an animation was running on body,
attempting to load an @font-face would trigger a version increase in the
font cache and a SubtreeStyleChange for the document, while we still tried
to use the baseLayoutStyle for the animating body because we ignored the
Force passed to inheritHtmlAndBodyElementStyles.

Comparing the baseLayoutStyle with the LayoutStyle created from scratch
would cause comparison to fail, due to a Font comparison fail because of
the increased font cache version number, which in turn caused the assert
to trigger.

R=esprehn@chromium.org,dstockwell@chromium.org
BUG=459661
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192241

[rune, 2015-03-17 08:50:17]

PTAL

[dstockwell, 2015-03-18 11:05:54]

lgtm

https://codereview.chromium.org/1000413003/diff/1/LayoutTests/animations/base...
File LayoutTests/animations/base-render-style-body-crash.html (right):

https://codereview.chromium.org/1000413003/diff/1/LayoutTests/animations/base...
LayoutTests/animations/base-render-style-body-crash.html:3: body {
-webkit-transition: background-color 1s }
just 'transition'

https://codereview.chromium.org/1000413003/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1000413003/diff/1/Source/core/dom/Document.cp...
Source/core/dom/Document.cpp:1621: bodyStyle = body->layoutStyle();
It's confusing to place this in the else branch, is it worth it?

https://codereview.chromium.org/1000413003/diff/1/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1000413003/diff/1/Source/core/dom/Element.cpp...
Source/core/dom/Element.cpp:3375: if (ElementAnimations* elementAnimations =
elementRareData()->elementAnimations())
there's an elementAnimations() getter on this

[esprehn, 2015-03-18 11:41:01]

https://codereview.chromium.org/1000413003/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1000413003/diff/1/Source/core/dom/Document.cp...
Source/core/dom/Document.cpp:1606:
documentElement()->clearAnimationStyleChange();
This seems okay as a short term hack, but long term I think we want to refactor
the Element::recalcStyle and Element::recalcOwnStyle so we can compute the style
of the documentElement and the body and then abort and then just call
->layoutStyle() here. Calling into styleForElement() directly like this is the
real bug.

https://codereview.chromium.org/1000413003/diff/1/Source/core/dom/Document.cp...
Source/core/dom/Document.cpp:1621: bodyStyle = body->layoutStyle();
On 2015/03/18 at 11:05:54, dstockwell wrote:
> It's confusing to place this in the else branch, is it worth it?

+1

https://codereview.chromium.org/1000413003/diff/1/Source/core/dom/Document.cp...
Source/core/dom/Document.cpp:1623: if (!bodyStyle || body->needsStyleRecalc() ||
didRecalcDocumentElement)
Why do you need to recalc the body if we recalced the document?

[esprehn, 2015-03-18 11:41:38]

https://codereview.chromium.org/1000413003/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1000413003/diff/1/Source/core/dom/Document.cp...
Source/core/dom/Document.cpp:1623: if (!bodyStyle || body->needsStyleRecalc() ||
didRecalcDocumentElement)
On 2015/03/18 11:41:01, esprehn wrote:
> Why do you need to recalc the body if we recalced the document?

document element*

[rune, 2015-03-18 14:53:03]

https://codereview.chromium.org/1000413003/diff/1/LayoutTests/animations/base...
File LayoutTests/animations/base-render-style-body-crash.html (right):

https://codereview.chromium.org/1000413003/diff/1/LayoutTests/animations/base...
LayoutTests/animations/base-render-style-body-crash.html:3: body {
-webkit-transition: background-color 1s }
On 2015/03/18 11:05:54, dstockwell wrote:
> just 'transition'

Done.

https://codereview.chromium.org/1000413003/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1000413003/diff/1/Source/core/dom/Document.cp...
Source/core/dom/Document.cpp:1606:
documentElement()->clearAnimationStyleChange();
On 2015/03/18 11:41:01, esprehn wrote:
> This seems okay as a short term hack, but long term I think we want to
refactor
> the Element::recalcStyle and Element::recalcOwnStyle so we can compute the
style
> of the documentElement and the body and then abort and then just call
> ->layoutStyle() here. Calling into styleForElement() directly like this is the
> real bug.

Acknowledged.

https://codereview.chromium.org/1000413003/diff/1/Source/core/dom/Document.cp...
Source/core/dom/Document.cpp:1621: bodyStyle = body->layoutStyle();
On 2015/03/18 11:41:01, esprehn wrote:
> On 2015/03/18 at 11:05:54, dstockwell wrote:
> > It's confusing to place this in the else branch, is it worth it?
> 
> +1

Done.

https://codereview.chromium.org/1000413003/diff/1/Source/core/dom/Document.cp...
Source/core/dom/Document.cpp:1623: if (!bodyStyle || body->needsStyleRecalc() ||
didRecalcDocumentElement)
On 2015/03/18 11:41:37, esprehn wrote:
> On 2015/03/18 11:41:01, esprehn wrote:
> > Why do you need to recalc the body if we recalced the document?
> 
> document element*

If documentElement style is changed, it may affect the body style through
inheritance.

https://codereview.chromium.org/1000413003/diff/1/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1000413003/diff/1/Source/core/dom/Element.cpp...
Source/core/dom/Element.cpp:3375: if (ElementAnimations* elementAnimations =
elementRareData()->elementAnimations())
On 2015/03/18 11:05:54, dstockwell wrote:
> there's an elementAnimations() getter on this

Done.

[rune, 2015-03-19 08:10:15]

esprehn@ wdyt?

[esprehn, 2015-03-19 10:40:58]

Lgtm w/ not fixed.

https://codereview.chromium.org/1000413003/diff/20001/Source/core/dom/Element.h
File Source/core/dom/Element.h (right):

https://codereview.chromium.org/1000413003/diff/20001/Source/core/dom/Element...
Source/core/dom/Element.h:316: void setAnimationStyleChange(bool) const;
Setters should not be const.

[rune, 2015-03-19 10:48:10]

On 2015/03/19 10:40:58, esprehn wrote:
> Lgtm w/ not fixed.
> 
>
https://codereview.chromium.org/1000413003/diff/20001/Source/core/dom/Element.h
> File Source/core/dom/Element.h (right):
> 
>
https://codereview.chromium.org/1000413003/diff/20001/Source/core/dom/Element...
> Source/core/dom/Element.h:316: void setAnimationStyleChange(bool) const;
> Setters should not be const.

So, should I stop using elementAnimations() accessor that dstockwell requested,
or add a private non-const elementAnimations() accessor?

[esprehn, 2015-03-19 10:52:14]

Nit*

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[rune, 2015-03-20 09:19:22]

The CQ bit was checked by rune@opera.com

[rune, 2015-03-20 09:19:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
dstockwell@chromium.org, esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1000413003/#ps40001
(title: "Review issue: removed constness")

[commit-bot: I haz the power, 2015-03-20 09:19:37]

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

[commit-bot: I haz the power, 2015-03-20 10:55:55]

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