During webfonts are loading, WebKit uses the last resort fonts for layout. I think we should respect generic font family names (if they exists) for the font fallback mechanism, not just using the last resort fonts. This way, we can provide a workaround to detect webfonts are fully loaded. See http://crbug.com/104233 for more details.
Created attachment 123250 [details] Patch
This last resort font <http://unicode.org/policies/lastresortfont_eula.html>? I haven't seen that happen on any Web sites.
(In reply to comment #2) > This last resort font <http://unicode.org/policies/lastresortfont_eula.html>? I haven't seen that happen on any Web sites. No. I use that term to describe the fonts which are returned by getNonRetainedLastResortFallbackFont(). These fonts are differ among ports. For example, mac port returns "Times" or "Lucida Grande". Sorry for using misleading term.
I'm not sure whether we should display the text at all while font is loading.
(In reply to comment #4) > I'm not sure whether we should display the text at all while font is loading. This patch don't display the text during font loading (IMHO, the text should be displayed, though). It tries to find fonts based on generic font family names (if they exist on "font-family") for layout.
Comment on attachment 123250 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123250&action=review > Source/WebCore/css/CSSFontFaceSource.cpp:40 > These extra dependencies are really sad. Also, there is fontDataForGenericFamily in CSSFontSelector.cpp. So my feeling is that getGenericFallbackFontData should be a part of CSSFontSelector, not CSSFontFaceSource, something like CSSFontSelector::getGetFallbackFontData() ?
Created attachment 123645 [details] Patch
Comment on attachment 123250 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123250&action=review Thank you for review! >> Source/WebCore/css/CSSFontFaceSource.cpp:40 >> > > These extra dependencies are really sad. Also, there is fontDataForGenericFamily in CSSFontSelector.cpp. > So my feeling is that getGenericFallbackFontData should be a part of CSSFontSelector, not CSSFontFaceSource, something like CSSFontSelector::getGetFallbackFontData() ? I agree. Removed these dependencies and moved the function to CSSFontSelector as CSSFontSelector::getGenericFallbackFontData().
Comment on attachment 123645 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123645&action=review > Source/WebCore/css/CSSFontSelector.cpp:623 > +{ CSSFontSelector.cpp already has fontDataForGenericFamily() which looks almost same as this method. What is the difference between these two?
Created attachment 123836 [details] Patch
Comment on attachment 123645 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123645&action=review >> Source/WebCore/css/CSSFontSelector.cpp:623 >> +{ > > CSSFontSelector.cpp already has fontDataForGenericFamily() which looks almost same as this method. What is the difference between these two? The only difference is fontDataForGenericFamily() uses the given font family name to select an appropriate font data, while this method selects a font data based on FontDescription's generic family. Revised the patch to use fontDataForGenericFamily() in this method to share the logic.
Why go with a generic family? Wouldn’t it be better to just use the rest of the fallback list (which may include a generic family, and terminates in the “last resort” font)? I imagine this could be done by having the temporary font we supply be one that has no coverage, so everything falls back through the rest of the list. I guess the vertical metrics would still need to come from somewhere, possibly the next item on the list. This would have the advantage that in the case the font eventually fails to load, and the fallback is used, the layout wouldn’t change at all.
Created attachment 124051 [details] Patch
(In reply to comment #12) > Why go with a generic family? Wouldn’t it be better to just use the rest of the fallback list (which may include a generic family, and terminates in the “last resort” font)? I imagine this could be done by having the temporary font we supply be one that has no coverage, so everything falls back through the rest of the list. I guess the vertical metrics would still need to come from somewhere, possibly the next item on the list. This would have the advantage that in the case the font eventually fails to load, and the fallback is used, the layout wouldn’t change at all. The solution is what I was looking for. I couldn't figure out how I can use the rest of the fallback list. Thank you so much for your suggestion.
Hi mitz, Could you please take another look?
I'd appreciate if someone could review the patch. I pinged not only on the bugzilla but also by sending an email. What else can I do?
Created attachment 129650 [details] Rebased to ToT
(In reply to comment #17) > Created an attachment (id=129650) [details] > Rebased to ToT Hi mitz, Could you please take a look? We want to fix the issue.
This issue is one that affects the Google Docs suite of apps and they're very keen to get fixed. Any help would be appreciated!
Comment on attachment 129650 [details] Rebased to ToT View in context: https://bugs.webkit.org/attachment.cgi?id=129650&action=review > Source/WebCore/ChangeLog:6 > + For layout, use the rest of the fallback list during webfonts are loading. during -> while > Source/WebCore/ChangeLog:7 > + This is done by returning the temporary font that has no coverage. Can you explain this a bit? What's a "coverage"? > Source/WebCore/css/CSSSegmentedFontFace.cpp:119 > + // If the faceFontData is loading, add a range which has no coverage > + // so that using the rest of fallback list for layout. > + if (faceFontData->isLoading()) > + newFontData->appendRange(FontDataRange(0, 0, faceFontData)); > else { > - for (unsigned j = 0; j < numRanges; ++j) > - newFontData->appendRange(FontDataRange(ranges[j].from(), ranges[j].to(), faceFontData)); > + const Vector<CSSFontFace::UnicodeRange>& ranges = m_fontFaces[i]->ranges(); > + unsigned numRanges = ranges.size(); > + if (!numRanges) > + newFontData->appendRange(FontDataRange(0, 0x7FFFFFFF, faceFontData)); > + else { > + for (unsigned j = 0; j < numRanges; ++j) > + newFontData->appendRange(FontDataRange(ranges[j].from(), ranges[j].to(), faceFontData)); > + } can this be made into a static helper with a good descriptive name? You may not need a comment then. > LayoutTests/fast/text/fallback-font-while-loading.html:21 > + style.innerText = '@font-face { font-family: Ahem; src: url(../../resources/Ahem.ttf?' + Date.now() + '); }'; Is this really the best way to ensure that we have not cached a file? Don't we have some tools in loader-related tests? > LayoutTests/fast/text/fallback-font-while-loading.html:22 > + document.getElementsByTagName('head')[0].appendChild(style); document.head is simpler.
Created attachment 131947 [details] Patch
Comment on attachment 129650 [details] Rebased to ToT View in context: https://bugs.webkit.org/attachment.cgi?id=129650&action=review Thank you so much for review. >> Source/WebCore/ChangeLog:6 >> + For layout, use the rest of the fallback list during webfonts are loading. > > during -> while Done. (I've just noticed I should have also fixed the title. Will fix next patch) >> Source/WebCore/ChangeLog:7 >> + This is done by returning the temporary font that has no coverage. > > Can you explain this a bit? What's a "coverage"? Done. >> Source/WebCore/css/CSSSegmentedFontFace.cpp:119 >> + } > > can this be made into a static helper with a good descriptive name? You may not need a comment then. Done. >> LayoutTests/fast/text/fallback-font-while-loading.html:21 >> + style.innerText = '@font-face { font-family: Ahem; src: url(../../resources/Ahem.ttf?' + Date.now() + '); }'; > > Is this really the best way to ensure that we have not cached a file? Don't we have some tools in loader-related tests? We don't have webfont loader-related tests. I revised the test to use cgi. >> LayoutTests/fast/text/fallback-font-while-loading.html:22 >> + document.getElementsByTagName('head')[0].appendChild(style); > > document.head is simpler. Done.
Hi Dimitri, Could you please take another look?
(In reply to comment #23) > Hi Dimitri, > > Could you please take another look? Ping Dimitri?
Comment on attachment 131947 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131947&action=review ok. In the future, please don't wait a week if you don't get a response from a reviewer. In this particular case, you have my email, and heck, you even have my IM. We all get a ton of mail -- it's likely that some will fall through cracks. Be persistent :) > Source/WebCore/css/CSSSegmentedFontFace.cpp:93 > + if (!numRanges) > + newFontData->appendRange(FontDataRange(0, 0x7FFFFFFF, faceFontData)); Can do early return here.
Created attachment 134453 [details] Patch for landing
Comment on attachment 131947 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131947&action=review I see. Thank you for review! >> Source/WebCore/css/CSSSegmentedFontFace.cpp:93 >> + newFontData->appendRange(FontDataRange(0, 0x7FFFFFFF, faceFontData)); > > Can do early return here. Done.
Comment on attachment 134453 [details] Patch for landing Clearing flags on attachment: 134453 Committed r112489: <http://trac.webkit.org/changeset/112489>
All reviewed patches have been landed. Closing bug.
(In reply to comment #29) > All reviewed patches have been landed. Closing bug. Did the layout test not get landed?
(In reply to comment #30) > (In reply to comment #29) > > All reviewed patches have been landed. Closing bug. > > Did the layout test not get landed? Also, Ahem.ttf is already in LayoutTests/resources/Ahem.ttf, so we don't need to check in another copy.
FYI: It looks like the layout tests got dropped in your final patch. Please commit them when you get a chance.
Thank you for the notice guys! I'll create a patch to add the test. > Also, Ahem.ttf is already in LayoutTests/resources/Ahem.ttf, so we don't need to check in another copy. The test will be served via local http server so I think we can't use LayoutTests/resources/Ahem.ttf from the test. No?
Reopening to attach new patch.
Created attachment 134710 [details] Patch
(In reply to comment #35) > Created an attachment (id=134710) [details] > Patch I'd like to wait Tony's comment for adding Ahem.ttf.
Comment on attachment 134710 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134710&action=review getahem.cgi needs a better name - nothing in it tells the reader that it's going to do it slowly. I'd suggest following an existing pattern for a slow loading test (find LayoutTests/http/tests/ -name *slow* | grep -v svn), and using a language that one of those tests already uses, not python. r- because there is no need to add another copy of Ahem. > LayoutTests/ChangeLog:3 > + Respects font fallback list while webfonts are loading This is grammatically incorrect. Did you mean to say something like "Fallback fonts should be used while a web font is being loaded"? > LayoutTests/http/tests/webfont/fallback-font-while-loading.html:15 > + layoutTestController.overridePreference('WebKitSerifFont', 'Arial'); Is this needed because you know that this string has different width when using Arial and when using Ahem, but aren't sure about default serif font? That's worth explaining in a comment. > LayoutTests/http/tests/webfont/fallback-font-while-loading.html:37 > +addTextWithWebfont(); Why does this element need to be added dynamically? > LayoutTests/http/tests/webfont/getahem.cgi:7 > +font_data = open("../resources/Ahem.ttf").read() This loads a local file already. You can avoid adding Ahem.ttf by using a relative path to an existing one ("../../../resources/Ahem.ttf" if I counted the dots correctly).
Created attachment 134748 [details] Patch
Created attachment 134751 [details] Use perl instead of python
Comment on attachment 134710 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134710&action=review Thank you for review! >> LayoutTests/ChangeLog:3 >> + Respects font fallback list while webfonts are loading > > This is grammatically incorrect. Did you mean to say something like "Fallback fonts should be used while a web font is being loaded"? Thanks. Fixed. >> LayoutTests/http/tests/webfont/fallback-font-while-loading.html:15 >> + layoutTestController.overridePreference('WebKitSerifFont', 'Arial'); > > Is this needed because you know that this string has different width when using Arial and when using Ahem, but aren't sure about default serif font? That's worth explaining in a comment. Done. >> LayoutTests/http/tests/webfont/fallback-font-while-loading.html:37 >> +addTextWithWebfont(); > > Why does this element need to be added dynamically? I just used the way that the original repro used, but there is no need to do it. Revised to use tags. >> LayoutTests/http/tests/webfont/getahem.cgi:7 >> +font_data = open("../resources/Ahem.ttf").read() > > This loads a local file already. You can avoid adding Ahem.ttf by using a relative path to an existing one ("../../../resources/Ahem.ttf" if I counted the dots correctly). You are right. Done.
Comment on attachment 134751 [details] Use perl instead of python View in context: https://bugs.webkit.org/attachment.cgi?id=134751&action=review > LayoutTests/http/tests/webfont/fallback-font-while-loading.html:23 > + // Set Arial as serif to make sure that the string has different width when using Ahem and when using serif. > + layoutTestController.overridePreference('WebKitSerifFont', 'Arial'); Nit: could you just use arial directly as the font-family instead of serif? > LayoutTests/http/tests/webfont/fallback-font-while-loading.html:37 > +//addTextWithWebfont(); Please delete
Thank you for review. r112489 was rolled out so I'll revise the patch with original change later. (In reply to comment #41) > (From update of attachment 134751 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=134751&action=review > > > LayoutTests/http/tests/webfont/fallback-font-while-loading.html:23 > > + // Set Arial as serif to make sure that the string has different width when using Ahem and when using serif. > > + layoutTestController.overridePreference('WebKitSerifFont', 'Arial'); > > Nit: could you just use arial directly as the font-family instead of serif? > > > LayoutTests/http/tests/webfont/fallback-font-while-loading.html:37 > > +//addTextWithWebfont(); > > Please delete
Created attachment 135284 [details] Patch for landing
Comment on attachment 135284 [details] Patch for landing Clearing flags on attachment: 135284 Committed r112999: <http://trac.webkit.org/changeset/112999>