Clear baseLayoutStyle when the font selector version has increased.

Clear baseLayoutStyle when the font selector version has increased.

FontFaceCache has a version() member which is increased when we add fonts
to the cache. It is used to detect if new fonts have been added to the
cache after a LayoutStyle was calculated. Different version() numbers will
make Font comparison fail and trigger relayout with new font data.

When the font added to the cache is new to the style system, it is
accompanied by a full document recalc. If we add a stylesheet with an
@font-face rule, the @font-face will cause an addition to the font cache,
and a document style recalc. So, if we have a "font-family:webfont,serif"
where "webfont" does not match anything, we lay out with the serif "font".
If we then add a stylesheet with an @font-face for "webfont", we need to
re-layout with that new font even if the computed style for font-family
doesn't change. That is triggered by the version number incremented by the
addition of "webfont".

This is all well until we add or remove a stylesheet which causes an
analyzed stylesheet update where we get a Reconstruct. The StyleResolver
will be recreated and font faces will be re-added, hence increasing the
version number of the font cache. The analyzed stylesheet update is smart
enough to avoid a full document recalc if it figures out that we did not
really add or remove any @font-face rules, just re-adding the ones we had.

We will then end up with a computed Font that was created for a different
font cache version, but we figured out we didn't need to recompute the
style or relayout since the font cache didn't really change with the
increased version.

If we are animating an element while such a reconstruct happen, we end up
having a baseLayoutStyle for the animation which is based on an old font
cache version. The LayoutStyle comparison ASSERT for baseLayoutStyle in
ElementAnimations will trigger because the Font comparison will fail,
since the version number for a newly created LayoutStyle Font will be
different.

To avoid that, we throw away the baseLayoutStyle after such a reconstruct
happens, in which case the stored version in the FontFallbackList will not
be in sync with the one stored in the FontFaceCache.

The real bug here is that the version tagging/increment here is imprecise
and not correlated with needsStyleRecalc. Removing an empty stylesheet
like in the added test-case will cause any element to be re-layout the
next time some other computed style change happen on that element as we
detect that the version number is out of sync. I have reported
crbug.com/471079 for that.

There is a generation() in FontCache which is similar to version(), but
increasing that will always trigger a full document recalc in
StyleEngine::fontsNeedUpdate.

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

[eae, 2015-03-26 23:26:32]

LGTM! This is great, love the detailed CL description and test!

[esprehn, 2015-03-26 23:28:16]

Good find!

https://codereview.chromium.org/1038193002/diff/1/Source/core/animation/Eleme...
File Source/core/animation/ElementAnimations.cpp (right):

https://codereview.chromium.org/1038193002/diff/1/Source/core/animation/Eleme...
Source/core/animation/ElementAnimations.cpp:99: setAnimationStyleChange(false);
This bit may be read in a bunch of places, this would mean it's invalid any time
before this method is called.

We should just make isAnimationStyleChange check check the bit:

isAnimationStyleChange() {
   return ... && (!m_baseLayoutStyle ||
m_baseLayoutStyle->font().isFallbackValid());
}

I think this also requires a TODO/FIXME that explains why we'd need to check
font explicitly since that's pretty weird.

https://codereview.chromium.org/1038193002/diff/1/Source/platform/fonts/Font.h
File Source/platform/fonts/Font.h (right):

https://codereview.chromium.org/1038193002/diff/1/Source/platform/fonts/Font....
Source/platform/fonts/Font.h:171: bool isFallbackValid() const { return
!m_fontFallbackList || m_fontFallbackList->isValid(); }
Ditto, does this need to be inline?

https://codereview.chromium.org/1038193002/diff/1/Source/platform/fonts/FontF...
File Source/platform/fonts/FontFallbackList.h (right):

https://codereview.chromium.org/1038193002/diff/1/Source/platform/fonts/FontF...
Source/platform/fonts/FontFallbackList.h:67: bool isValid() const { return
m_fontSelector ? m_fontSelector->version() == m_fontSelectorVersion :
m_fontSelectorVersion == 0; }
Can we move this out of line instead?

[rune, 2015-03-27 07:14:22]

https://codereview.chromium.org/1038193002/diff/1/Source/core/animation/Eleme...
File Source/core/animation/ElementAnimations.cpp (right):

https://codereview.chromium.org/1038193002/diff/1/Source/core/animation/Eleme...
Source/core/animation/ElementAnimations.cpp:99: setAnimationStyleChange(false);
On 2015/03/26 23:28:15, esprehn wrote:
> This bit may be read in a bunch of places, this would mean it's invalid any
time
> before this method is called.
> 
> We should just make isAnimationStyleChange check check the bit:
> 
> isAnimationStyleChange() {
>    return ... && (!m_baseLayoutStyle ||
> m_baseLayoutStyle->font().isFallbackValid());
> }

Yep. Doing this made the patch a lot simpler.

Done.

> I think this also requires a TODO/FIXME that explains why we'd need to check
> font explicitly since that's pretty weird.

Done.

https://codereview.chromium.org/1038193002/diff/1/Source/platform/fonts/Font.h
File Source/platform/fonts/Font.h (right):

https://codereview.chromium.org/1038193002/diff/1/Source/platform/fonts/Font....
Source/platform/fonts/Font.h:171: bool isFallbackValid() const { return
!m_fontFallbackList || m_fontFallbackList->isValid(); }
On 2015/03/26 23:28:15, esprehn wrote:
> Ditto, does this need to be inline?

Done.

https://codereview.chromium.org/1038193002/diff/1/Source/platform/fonts/FontF...
File Source/platform/fonts/FontFallbackList.h (right):

https://codereview.chromium.org/1038193002/diff/1/Source/platform/fonts/FontF...
Source/platform/fonts/FontFallbackList.h:67: bool isValid() const { return
m_fontSelector ? m_fontSelector->version() == m_fontSelectorVersion :
m_fontSelectorVersion == 0; }
On 2015/03/26 23:28:15, esprehn wrote:
> Can we move this out of line instead?

Done.

[dstockwell, 2015-03-27 07:36:11]

On 2015/03/27 at 07:14:22, rune wrote:
>
https://codereview.chromium.org/1038193002/diff/1/Source/core/animation/Eleme...
> File Source/core/animation/ElementAnimations.cpp (right):
> 
>
https://codereview.chromium.org/1038193002/diff/1/Source/core/animation/Eleme...
> Source/core/animation/ElementAnimations.cpp:99:
setAnimationStyleChange(false);
> On 2015/03/26 23:28:15, esprehn wrote:
> > This bit may be read in a bunch of places, this would mean it's invalid any
time
> > before this method is called.
> > 
> > We should just make isAnimationStyleChange check check the bit:
> > 
> > isAnimationStyleChange() {
> >    return ... && (!m_baseLayoutStyle ||
> > m_baseLayoutStyle->font().isFallbackValid());
> > }
> 
> Yep. Doing this made the patch a lot simpler.

Great, lgtm

[rune, 2015-03-27 07:58:02]

The CQ bit was checked by rune@opera.com

[rune, 2015-03-27 07:58:03]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/1038193002/#ps20001
(title: "Addressed review issues")

[commit-bot: I haz the power, 2015-03-27 07:58:14]

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

[commit-bot: I haz the power, 2015-03-27 11:26:43]

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