Bug 72459

Created attachment 115548 [details] Patch

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.

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

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?

(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 :-)

Created attachment 116886 [details] Patch

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!

(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.

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

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

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.

Created attachment 117295 [details] Try symbol export

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