Don't return false for RenderStyle::operator== while fonts load.

Don't return false for RenderStyle::operator== while fonts load.

This caused an assert for baseRenderStyle in ActiveAnimations to hit. When
a font finishes loading, we trigger a full recalc of Document. That recalc
relies on getting a visual diff to trigger re-layout and paint and was
triggered by the Font::operator==. Instead, we remove the false return from
that operator and explicitly check if font loading status changed in
RenderStyle::diffNeedsFullLayoutAndPaintInvalidation().

R=eae@chromium.org
BUG=433765
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186572

[rune, 2014-12-04 12:54:17]

https://codereview.chromium.org/754743004/diff/1/Source/core/rendering/style/...
File Source/core/rendering/style/RenderStyle.cpp (right):

https://codereview.chromium.org/754743004/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/RenderStyle.cpp:512: return true;
This catches the case going from loading to not loading. It would be safer to do
!= instead.

[eae, 2014-12-04 19:31:58]

Seems like a good approach, judging by the failing tests though it looks like
we've missed at least one edge case :(

[rune, 2014-12-04 20:56:57]

On 2014/12/04 at 19:31:58, eae wrote:
> Seems like a good approach, judging by the failing tests though it looks like
we've missed at least one edge case :(

Yes. Will look into it.

[rune, 2014-12-04 23:29:16]

On 2014/12/04 at 20:56:57, rune wrote:
> On 2014/12/04 at 19:31:58, eae wrote:
> > Seems like a good approach, judging by the failing tests though it looks
like we've missed at least one edge case :(
> 
> Yes. Will look into it.

Two of the tests are relying on a Font comparison in HarfBuzzShaper[1] to return
false when a font is loading:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[eae, 2014-12-04 23:30:38]

On 2014/12/04 23:29:16, rune wrote:
> On 2014/12/04 at 20:56:57, rune wrote:
> > On 2014/12/04 at 19:31:58, eae wrote:
> > > Seems like a good approach, judging by the failing tests though it looks
> like we've missed at least one edge case :(
> > 
> > Yes. Will look into it.
> 
> Two of the tests are relying on a Font comparison in HarfBuzzShaper[1] to
return
> false when a font is loading:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Right, we probably want to change that to check if it is loading.

[rune, 2014-12-04 23:42:03]

On 2014/12/04 at 23:30:38, eae wrote:
> On 2014/12/04 23:29:16, rune wrote:
> > On 2014/12/04 at 20:56:57, rune wrote:
> > > On 2014/12/04 at 19:31:58, eae wrote:
> > > > Seems like a good approach, judging by the failing tests though it looks
> > like we've missed at least one edge case :(
> > > 
> > > Yes. Will look into it.
> > 
> > Two of the tests are relying on a Font comparison in HarfBuzzShaper[1] to
return
> > false when a font is loading:
> > 
> >
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Right, we probably want to change that to check if it is loading.

You mean in addition to Font comparison like this?:

... cachedResults->font == *m_font && !cachedResults->font.loadingCustomFonts()
&& !m_font->loadingCustomFonts() ...

[eae, 2014-12-04 23:48:23]

On 2014/12/04 23:42:03, rune wrote:
> On 2014/12/04 at 23:30:38, eae wrote:
> > On 2014/12/04 23:29:16, rune wrote:
> > > On 2014/12/04 at 20:56:57, rune wrote:
> > > > On 2014/12/04 at 19:31:58, eae wrote:
> > > > > Seems like a good approach, judging by the failing tests though it
looks
> > > like we've missed at least one edge case :(
> > > > 
> > > > Yes. Will look into it.
> > > 
> > > Two of the tests are relying on a Font comparison in HarfBuzzShaper[1] to
> return
> > > false when a font is loading:
> > > 
> > >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > Right, we probably want to change that to check if it is loading.
> 
> You mean in addition to Font comparison like this?:
> 
> ... cachedResults->font == *m_font &&
!cachedResults->font.loadingCustomFonts()
> && !m_font->loadingCustomFonts() ...

Yeah, I think that would do the trick.

[rune, 2014-12-05 00:21:39]

Triggered new try with Harfbuzz change.

https://codereview.chromium.org/754743004/diff/1/Source/core/rendering/style/...
File Source/core/rendering/style/RenderStyle.cpp (right):

https://codereview.chromium.org/754743004/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/RenderStyle.cpp:512: return true;
On 2014/12/04 at 12:54:17, rune wrote:
> This catches the case going from loading to not loading. It would be safer to
do != instead.

Done.

[eae, 2014-12-05 00:23:20]

On 2014/12/05 00:21:39, rune wrote:
> Triggered new try with Harfbuzz change.
> 
>
https://codereview.chromium.org/754743004/diff/1/Source/core/rendering/style/...
> File Source/core/rendering/style/RenderStyle.cpp (right):
> 
>
https://codereview.chromium.org/754743004/diff/1/Source/core/rendering/style/...
> Source/core/rendering/style/RenderStyle.cpp:512: return true;
> On 2014/12/04 at 12:54:17, rune wrote:
> > This catches the case going from loading to not loading. It would be safer
to
> do != instead.
> 
> Done.

LGTM assuming the bots like it.

[rune, 2014-12-05 06:03:59]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-12-05 06:04:29]

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

[commit-bot: I haz the power, 2014-12-05 06:56:29]

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