[webkit-reviews] review denied: [Bug 76684] Respects font fallback list while webfonts are loading : [Attachment 134710] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Kenichi Ishibashi
<bashi at chromium.org>'s request for review:
Bug 76684: Respects font fallback list while webfonts are loading
https://bugs.webkit.org/show_bug.cgi?id=76684

Attachment 134710: Patch
https://bugs.webkit.org/attachment.cgi?id=134710&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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).