[webkit-reviews] review denied: [Bug 76684] Respects font fallback list during webfonts are loading : [Attachment 129650] Rebased to ToT

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Kenichi Ishibashi
<bashi at chromium.org>'s request for review:
Bug 76684: Respects font fallback list during webfonts are loading
https://bugs.webkit.org/show_bug.cgi?id=76684

Attachment 129650: Rebased to ToT
https://bugs.webkit.org/attachment.cgi?id=129650&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
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.