[webkit-reviews] review denied: [Bug 72459] Fix flaky SVG fonts tests : [Attachment 115548] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Kenichi Ishibashi
<bashi at chromium.org>'s request for review:
Bug 72459: Fix flaky SVG fonts tests
https://bugs.webkit.org/show_bug.cgi?id=72459

Attachment 115548: Patch
https://bugs.webkit.org/attachment.cgi?id=115548&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
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?