If CachedFont::ensureCustomFontData() fails, WebKit simply ignore the webfont and use fallback fonts without any error messages. This behavior often confuses page authors and end users. I think it would be nice to show an error message when it fails. Chromium uses font sanitizer for webfonts to prevent activating malicious fonts, so showing such kind of error message would help page authors to find problems if they use invalid fonts. What do you think? I'll upload a proposed patch.
Created attachment 142405 [details] proposed patch
Not commenting on the idea, I think that the message could use some improvement. First, it should say "cannot", not "failed". And it would be helpful to at least tell which font could not be activated.
Sorry for late response (I was absent last two weeks). (In reply to comment #2) > Not commenting on the idea, I think that the message could use some improvement. First, it should say "cannot", not "failed". Will do. > And it would be helpful to at least tell which font could not be activated. CSSFontFaceSource::m_string contains the URI of the font and it is passed to addConsoleMessage() so we can see which font could not be activated.
Created attachment 145533 [details] Patch
Comment on attachment 145533 [details] Patch Attachment 145533 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12899201 New failing tests: fast/images/style-access-during-imageChanged-crash.html fast/text/custom-font-data-crash2.html fast/text/custom-font-data-crash.html fast/loader/font-face-empty.html fast/loader/goto-anchor-infinite-layout.html http/tests/css/font-face-src-loading.html fast/css/font-face-data-uri-invalid.html
Created attachment 145559 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 145533 [details] Patch As the EWS pointed out this will create lot of failures.
Created attachment 145676 [details] Rebaselined affected test expectations
Comment on attachment 145676 [details] Rebaselined affected test expectations This is completely wrong. What is the point of this debug output? Why you need that? I don't think we want to pollute the console with this especially in release. Also it is really wrong to update the expected files. Your Changelog fails to explain why you do this.
(In reply to comment #9) > (From update of attachment 145676 [details]) > This is completely wrong. What is the point of this debug output? Why you need that? I don't think we want to pollute the console with this especially in release. Also it is really wrong to update the expected files. Your Changelog fails to explain why you do this. The message isn't a debug message. See the description of the motivation. From web page authors and users point of view, failing activating webfonts almost the same as failing loading webfonts, and we show a error message when we fail to load webfonts. Firefox shows an error message if it fails to activate webfont. Attaching a screenshot. I checked all rebaselined tests try to load invalid fonts or URI.
Created attachment 145681 [details] Screenshot of Firefox
(In reply to comment #11) > (In reply to comment #9) > > (From update of attachment 145676 [details] [details]) > > This is completely wrong. What is the point of this debug output? Why you need that? I don't think we want to pollute the console with this especially in release. Also it is really wrong to update the expected files. Your Changelog fails to explain why you do this. > > The message isn't a debug message. See the description of the motivation. From web page authors and users point of view, failing activating webfonts almost the same as failing loading webfonts, and we show a error message when we fail to load webfonts. Firefox shows an error message if it fails to activate webfont. Attaching a screenshot. I checked all rebaselined tests try to load invalid fonts or URI. Nevermind then I went directly through the review mode, you should definitively put that in the Changelog (sometimes people don't read the bug history) otherwise It's hard to understand what you are trying to achieve (and also important for the history inside the repo). Still the debug output is not as complete as FF it seems. > CSSFontFaceSource::m_string contains the URI of the font and it is passed to addConsoleMessage() so we can see which font could not be activated. Well according to the modification of the expected files it doesn't work as I can't see which font couldn't be loaded but I maybe missing something here. I'm still confused on why you need to modify the expected files of the tests, read why these tests fails to load the web font (is that even intended on all tests??)?. Do you guarantee that they pass on other port than Chromium, like the console message will be printed. I will test on Qt tomorrow. I'm not a font expert, I just stumbled across your patch so maybe someone with a better knowledge can review that.
(In reply to comment #13) > Nevermind then I went directly through the review mode, you should definitively put that in the Changelog (sometimes people don't read the bug history) otherwise It's hard to understand what you are trying to achieve (and also important for the history inside the repo). Still the debug output is not as complete as FF it seems. You are right. I'll explain the intent of the patch on the ChangeLog. To make the error message more precise, we have to modify platform-dependent code like FontCustomPlatformData, that sounds over-engineering to me for this purpose. > I'm still confused on why you need to modify the expected files of the tests, read why these tests fails to load the web font (is that even intended on all tests??)?. Do you guarantee that they pass on other port than Chromium, like the console message will be printed. I will test on Qt tomorrow. I'm not a font expert, I just stumbled across your patch so maybe someone with a better knowledge can review that. Most of these tests check whether WebKit doesn't crash when @font-face contains invalid URIs. http/tests/css/font-face-src-loading.html loads an empty webfont, and the purpose of the test is independent of whether the font is activated or not. I don't have other port environment, so I would like to see EWS results.
Created attachment 145686 [details] Patch
Comment on attachment 145686 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145686&action=review > Source/WebCore/css/CSSFontFaceSource.cpp:167 > + fontSelector->document()->addConsoleMessage(NetworkMessageSource, LogMessageType, ErrorMessageLevel, "Cannot activate the font.", m_string); I've never seen the use of the word "activate" in this context. Perhaps something like "Cannot use the font" might be more user friendly? > Source/WebCore/css/CSSFontFaceSource.cpp:172 > fontDescription.textOrientation(), fontDescription.widthVariant(), fontDescription.renderingMode()), true, false)); I see a really bad indentation here :(
Created attachment 157575 [details] Patch
Comment on attachment 145686 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145686&action=review Thank you for review! >> Source/WebCore/css/CSSFontFaceSource.cpp:167 >> + fontSelector->document()->addConsoleMessage(NetworkMessageSource, LogMessageType, ErrorMessageLevel, "Cannot activate the font.", m_string); > > I've never seen the use of the word "activate" in this context. Perhaps something like "Cannot use the font" might be more user friendly? Done. >> Source/WebCore/css/CSSFontFaceSource.cpp:172 >> fontDescription.textOrientation(), fontDescription.widthVariant(), fontDescription.renderingMode()), true, false)); > > I see a really bad indentation here :( I just concatenated the lines.
Comment on attachment 157575 [details] Patch Attachment 157575 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13476020 New failing tests: http/tests/css/link-css-disabled-value-with-slow-loading-sheet-in-error.html
Created attachment 157587 [details] Archive of layout-test-results from gce-cr-linux-06 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-06 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 157575 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157575&action=review > Source/WebCore/css/CSSFontFaceSource.cpp:168 > + fontSelector->document()->addConsoleMessage(NetworkMessageSource, LogMessageType, ErrorMessageLevel, "Cannot use the font.", m_string); Seems like a good idea, but generally seems like too vague a message. This would be much better with a message that cites the URL for the font.