Bug 76684 - Respects font fallback list while webfonts are loading
: Respects font fallback list while webfonts are loading
Status: RESOLVED FIXED
: WebKit
Text
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
: http://crbug.com/104233
:
: 82725
:
  Show dependency treegraph
 
Reported: 2012-01-19 20:46 PST by
Modified: 2012-04-03 02:08 PST (History)


Attachments
Patch (7.19 KB, patch)
2012-01-19 20:55 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch (7.84 KB, patch)
2012-01-23 16:05 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch (7.98 KB, patch)
2012-01-24 16:23 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch (5.91 KB, patch)
2012-01-25 18:17 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Rebased to ToT (5.87 KB, patch)
2012-03-01 00:09 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch (10.58 KB, patch)
2012-03-14 16:07 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (4.12 KB, patch)
2012-03-28 17:12 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch (7.27 KB, patch)
2012-03-29 18:47 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch (3.62 KB, patch)
2012-03-30 01:54 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Use perl instead of python (3.53 KB, patch)
2012-03-30 02:23 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (3.37 KB, patch)
2012-04-03 00:30 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-01-19 20:46:37 PST
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.
------- Comment #1 From 2012-01-19 20:55:51 PST -------
Created an attachment (id=123250) [details]
Patch
------- Comment #2 From 2012-01-20 10:34:21 PST -------
This last resort font <http://unicode.org/policies/lastresortfont_eula.html>? I haven't seen that happen on any Web sites.
------- Comment #3 From 2012-01-22 16:18:26 PST -------
(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.
------- Comment #4 From 2012-01-22 16:22:50 PST -------
I'm not sure whether we should display the text at all while font is loading.
------- Comment #5 From 2012-01-22 16:27:43 PST -------
(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 #6 From 2012-01-23 14:02:02 PST -------
(From update of attachment 123250 [details])
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() ?
------- Comment #7 From 2012-01-23 16:05:23 PST -------
Created an attachment (id=123645) [details]
Patch
------- Comment #8 From 2012-01-23 16:06:30 PST -------
(From update of attachment 123250 [details])
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 #9 From 2012-01-24 15:17:34 PST -------
(From update of attachment 123645 [details])
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?
------- Comment #10 From 2012-01-24 16:23:23 PST -------
Created an attachment (id=123836) [details]
Patch
------- Comment #11 From 2012-01-24 16:29:59 PST -------
(From update of attachment 123645 [details])
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.
------- Comment #12 From 2012-01-25 11:06:35 PST -------
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.
------- Comment #13 From 2012-01-25 18:17:24 PST -------
Created an attachment (id=124051) [details]
Patch
------- Comment #14 From 2012-01-25 18:18:52 PST -------
(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.
------- Comment #15 From 2012-01-30 15:41:05 PST -------
Hi mitz,

Could you please take another look?
------- Comment #16 From 2012-02-02 06:55:07 PST -------
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?
------- Comment #17 From 2012-03-01 00:09:59 PST -------
Created an attachment (id=129650) [details]
Rebased to ToT
------- Comment #18 From 2012-03-01 00:12:54 PST -------
(In reply to comment #17)
> Created an attachment (id=129650) [details] [details]
> Rebased to ToT

Hi mitz,

Could you please take a look? We want to fix the issue.
------- Comment #19 From 2012-03-02 10:02:14 PST -------
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 #20 From 2012-03-14 14:30:56 PST -------
(From update of attachment 129650 [details])
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.
------- Comment #21 From 2012-03-14 16:07:18 PST -------
Created an attachment (id=131947) [details]
Patch
------- Comment #22 From 2012-03-14 16:11:17 PST -------
(From update of attachment 129650 [details])
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.
------- Comment #23 From 2012-03-20 21:41:28 PST -------
Hi Dimitri,

Could you please take another look?
------- Comment #24 From 2012-03-27 17:14:48 PST -------
(In reply to comment #23)
> Hi Dimitri,
> 
> Could you please take another look?

Ping Dimitri?
------- Comment #25 From 2012-03-28 09:48:12 PST -------
(From update of attachment 131947 [details])
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.
------- Comment #26 From 2012-03-28 17:12:04 PST -------
Created an attachment (id=134453) [details]
Patch for landing
------- Comment #27 From 2012-03-28 17:12:43 PST -------
(From update of attachment 131947 [details])
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 #28 From 2012-03-28 18:41:19 PST -------
(From update of attachment 134453 [details])
Clearing flags on attachment: 134453

Committed r112489: <http://trac.webkit.org/changeset/112489>
------- Comment #29 From 2012-03-28 18:41:25 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #30 From 2012-03-29 11:28:16 PST -------
(In reply to comment #29)
> All reviewed patches have been landed.  Closing bug.

Did the layout test not get landed?
------- Comment #31 From 2012-03-29 11:29:58 PST -------
(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.
------- Comment #32 From 2012-03-29 11:52:29 PST -------
FYI: It looks like the layout tests got dropped in your final patch. Please commit them when you get a chance.
------- Comment #33 From 2012-03-29 17:54:56 PST -------
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?
------- Comment #34 From 2012-03-29 18:47:51 PST -------
Reopening to attach new patch.
------- Comment #35 From 2012-03-29 18:47:57 PST -------
Created an attachment (id=134710) [details]
Patch
------- Comment #36 From 2012-03-29 18:49:17 PST -------
(In reply to comment #35)
> Created an attachment (id=134710) [details] [details]
> Patch

I'd like to wait Tony's comment for adding Ahem.ttf.
------- Comment #37 From 2012-03-30 00:06:06 PST -------
(From update of attachment 134710 [details])
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).
------- Comment #38 From 2012-03-30 01:54:43 PST -------
Created an attachment (id=134748) [details]
Patch
------- Comment #39 From 2012-03-30 02:23:09 PST -------
Created an attachment (id=134751) [details]
Use perl instead of python
------- Comment #40 From 2012-03-30 02:24:20 PST -------
(From update of attachment 134710 [details])
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 #41 From 2012-03-30 10:37:20 PST -------
(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
------- Comment #42 From 2012-04-01 17:25:39 PST -------
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] [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
------- Comment #43 From 2012-04-03 00:30:11 PST -------
Created an attachment (id=135284) [details]
Patch for landing
------- Comment #44 From 2012-04-03 02:08:17 PST -------
(From update of attachment 135284 [details])
Clearing flags on attachment: 135284

Committed r112999: <http://trac.webkit.org/changeset/112999>
------- Comment #45 From 2012-04-03 02:08:24 PST -------
All reviewed patches have been landed.  Closing bug.