Bug 72459 – Fix flaky SVG fonts tests

• Status: RESOLVED
• Resolution: WONTFIX
• Component: SVG

[Kenichi Ishibashi]

There are numerous flaky tests, for example:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=svg%2Fcustom%2Fsvg-fonts-fallback.xhtml%2Csvg%2Fcustom%2Fsvg-fonts-segmented.xhtml%2Csvg%2Ftext%2Ftext-overflow-ellipsis-svgfont.html&showExpectations=true

This entry is the master bug for fixing these tests.

[Kenichi Ishibashi]

Created attachment 115548 [details]
Patch

[Kenichi Ishibashi]

At first, I used event dispatching to notify whether the font are loaded. However, when I ran NRWT, I noticed this approach didn't work. If fonts are preliminary loaded and cached, WebKit immediately creates internal SimpleFontData objects before the document object is ready to dispatch events. In this case, there is no opportunity to dispatch font loaded events because at the time of the document is ready to dispatch events, font object are already created with cached data.

In this patch, I added Internals::isCustomFontLoaded(document, family). This returns true when the font object that corresponds with the given family name is already loaded. Each test wait until the function returns true in setInterval loop. I guess there is a better way to do that, but couldn't figure out.

[Collabora GTK+ EWS bot]

Comment on attachment 115548 [details]
Patch

Attachment 115548 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10510147

[Nikolas Zimmermann]

Comment on attachment 115548 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115548&action=review

Excellent work, thanks bashi for fixing this! I'd like to reduce the number of hacks springled around layout tests, and rather centralize/refactor the code, see below:

> Source/WebCore/css/CSSFontSelector.cpp:588
> +
> +    for (unsigned i = 0; i < familyFontFaces->size(); ++i) {
> +        CSSFontFace* face = familyFontFaces->at(i).get();
> +        if (!face->isLoaded())
> +            return false;
> +    }

size_t length = familyFontFaces->size();
for (size_t i = 0; i < length; ++i) {
    if (!familyFontFaces->at(i)->isLoaded())
        return false;
}

> Source/WebCore/css/CSSFontSelector.h:71
> +    bool isFontFaceLoaded(const String& familyName);

You should either avoid adding the familyName argument name here or add it in Internals.h as well in isCustomFontLoaded.

> LayoutTests/svg/W3C-SVG-1.1/fonts-elem-07-b.svg:123
> -            if (window.layoutTestController) {
> +            if (window.internals && window.layoutTestController) {
>                  layoutTestController.waitUntilDone();
>                  document.documentElement.offsetTop;
> -                setTimeout(function() { layoutTestController.notifyDone(); }, 100);
> +                setInterval(function() {
> +                    if (internals.isCustomFontLoaded(document, 'TestComic'))
> +                        layoutTestController.notifyDone();
> +                }, 0);

I'd really love to centralize this trick, in eg. svg/custom/resources/SVGFont-loading-events.js
How about you fold this into "waitForSVGFontsLoaded(['font1', 'font2'...." method that accepts a list of fonts to be waited for and a callback function that should be fired once it loaded.
If the callback is not provided layoutTestController.notifyDone() is called. This callback could be used in svg-fonts-in-text-controls.js for example to start the text metrics testing.

This way all tests using SVG Fonts could easily be tweaked to use your new approach, and if we ever fire events for this, we could modify our testing hooks in one place.
What do you think?

[Nikolas Zimmermann]

(In reply to comment #2)
> At first, I used event dispatching to notify whether the font are loaded. However, when I ran NRWT, I noticed this approach didn't work. If fonts are preliminary loaded and cached, WebKit immediately creates internal SimpleFontData objects before the document object is ready to dispatch events. In this case, there is no opportunity to dispatch font loaded events because at the time of the document is ready to dispatch events, font object are already created with cached data.
Good explaination.
 
> In this patch, I added Internals::isCustomFontLoaded(document, family). This returns true when the font object that corresponds with the given family name is already loaded. Each test wait until the function returns true in setInterval loop. I guess there is a better way to do that, but couldn't figure out.
Hm, you could of course pass in a callback function that you fire once the font is loaded - that either implies overhead for in the font loading code (need to notify callbacks, if a font is requested and present, or if it appears after a load, etc..), which is something we want to avoid as this is only used for testing.
So we could use a Timer in WebCore::Internals, which does the same as you do in script: poll the information. I'm not sure if there's any benefit in coding this manually.

If you nicely centralize the code to wait for SVG Fonts to be loaded in a script that can be used in LayoutTests/ we're happy as that's all what we want: reliable testing of SVG Fonts :-)

[Kenichi Ishibashi]

Created attachment 116886 [details]
Patch

[Kenichi Ishibashi]

Comment on attachment 115548 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115548&action=review

Sorry for late response. Revised the patch to address your comments/suggestions.

>> Source/WebCore/css/CSSFontSelector.cpp:588
>> +    }
> 
> size_t length = familyFontFaces->size();
> for (size_t i = 0; i < length; ++i) {
>     if (!familyFontFaces->at(i)->isLoaded())
>         return false;
> }

Done.

>> Source/WebCore/css/CSSFontSelector.h:71
>> +    bool isFontFaceLoaded(const String& familyName);
> 
> You should either avoid adding the familyName argument name here or add it in Internals.h as well in isCustomFontLoaded.

Added argument name in Internals.h.

>> LayoutTests/svg/W3C-SVG-1.1/fonts-elem-07-b.svg:123
>> +                }, 0);
> 
> I'd really love to centralize this trick, in eg. svg/custom/resources/SVGFont-loading-events.js
> How about you fold this into "waitForSVGFontsLoaded(['font1', 'font2'...." method that accepts a list of fonts to be waited for and a callback function that should be fired once it loaded.
> If the callback is not provided layoutTestController.notifyDone() is called. This callback could be used in svg-fonts-in-text-controls.js for example to start the text metrics testing.
> 
> This way all tests using SVG Fonts could easily be tweaked to use your new approach, and if we ever fire events for this, we could modify our testing hooks in one place.
> What do you think?

Done. Thank you for the suggestion!

[Kenichi Ishibashi]

(In reply to comment #5)
> Hm, you could of course pass in a callback function that you fire once the font is loaded - that either implies overhead for in the font loading code (need to notify callbacks, if a font is requested and present, or if it appears after a load, etc..), which is something we want to avoid as this is only used for testing.
> So we could use a Timer in WebCore::Internals, which does the same as you do in script: poll the information. I'm not sure if there's any benefit in coding this manually.

I agree with you that avoiding overhead just for testing. I tried to use a timer in WebCore::Internals, but couldn't find benefit. It just complicate the implementation so I decided to use a loop in scripts.

[WebKit Review Bot]

Comment on attachment 116886 [details]
Patch

Attachment 116886 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10678590

New failing tests:
svg/W3C-SVG-1.1/fonts-elem-07-b.svg
svg/custom/svg-fonts-without-missing-glyph.xhtml
svg/custom/svg-fonts-with-no-element-reference.html
svg/custom/svg-fonts-in-html.html

[Collabora GTK+ EWS bot]

Comment on attachment 116886 [details]
Patch

Attachment 116886 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10695045

[Ryosuke Niwa]

Comment on attachment 116886 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116886&action=review

> Source/WebCore/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the description.

> Source/WebCore/testing/Internals.cpp:613
> +    CSSStyleSelector* styleSelector = document->styleSelectorIfExists();
> +    if (!styleSelector)
> +        return false;
> +
> +    return styleSelector->fontSelector()->isFontFaceLoaded(familyName);

You probably need to add export symbols.

> LayoutTests/svg/custom/resources/SVGFont-loading-events.js:2
> +    if (window.internals && window.layoutTestController) {

Please do an early exit here.

[Kenichi Ishibashi]

Created attachment 117295 [details]
Try symbol export

[Kenichi Ishibashi]

Looks like http://trac.webkit.org/changeset/101858 fixed the flakiness. I'd like to close this bug as WONTFIX.