Last Comment Bug 700057 - Japanese WOFF with TrueType outline no longer display EUDC (Gaiji) characters
: Japanese WOFF with TrueType outline no longer display EUDC (Gaiji) characters
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: mozilla11
Assigned To: Masatoshi Kimura [:emk]
:
:
Mentors:
http://musashi.or.tv/wofftest/woff_li...
Depends on:
Blocks: 660088
  Show dependency treegraph
 
Reported: 2011-11-05 13:45 PDT by Masatoshi Kimura [:emk]
Modified: 2011-12-19 08:39 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (3.19 KB, patch)
2011-11-06 04:40 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Create artificial names if English names not found in the name table (4.89 KB, patch)
2011-12-02 21:22 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Create artificial names if English names not found in the name table (6.21 KB, patch)
2011-12-14 05:27 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Create artificial names if English names not found in the name table (6.27 KB, patch)
2011-12-14 08:26 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Create artificial names if English names not found in the name table (8.42 KB, patch)
2011-12-16 06:31 PST, Masatoshi Kimura [:emk]
jfkthame: review+
Details | Diff | Splinter Review
hg qdiff -b to make the review easier (6.48 KB, patch)
2011-12-16 06:32 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review

Description Masatoshi Kimura [:emk] 2011-11-05 13:45:59 PDT
Steps to reproduce:
1. Turn off DirectWrite if enabled.
2. Open the URL.

Actual result:
Hexbox is displayed.

Expected result:
Circled 「外」 should be displayed.
Comment 1 Masatoshi Kimura [:emk] 2011-11-05 13:46:52 PDT
On Firefox 8 or later, the following message is displayed on Error Console.

Error: downloadable font: not usable by platform (font-family: "myFont" style:normal weight:normal stretch:normal src index:0)
source: http://musashi.or.tv/wofftest/woff_linkgaijittf.woff
Source Code:
@font-face {   font-family: "myFont";   src: url("woff_linkgaijittf.woff") format("woff"); }
Comment 2 Masatoshi Kimura [:emk] 2011-11-05 13:48:23 PDT
2011-06-17-03-mozilla-central/ works http://hg.mozilla.org/mozilla-central/rev/306bdc7101c3
2011-06-18-03-mozilla-central/ broken http://hg.mozilla.org/mozilla-central/rev/0830b8ed9f02
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=306bdc7101c3&tochange=0830b8ed9f02

Bug 660088 is the most likely to be the culprit.
Comment 3 Masatoshi Kimura [:emk] 2011-11-05 14:04:00 PDT
When I reverted OTS to r72, the problem disappeared. (r73 does roughly corresponds to our bug 660088.) So something seems to go wrong in 'name' table handling.
Comment 4 Masatoshi Kimura [:emk] 2011-11-05 22:18:29 PDT
Strangely, Firefox fails to display EUDC even if gfx.downloadable_fonts.sanitize sets to false.
Comment 5 Masatoshi Kimura [:emk] 2011-11-05 22:57:07 PDT
The font was rejected by MakeEOTHeader because it contains only Japanese (languageID = 0x411) name records. Before bug 660088, it did not cause a problem because OTS created artificial name records and ignored original one.
Comment 6 Masatoshi Kimura [:emk] 2011-11-06 04:40:19 PST
Created attachment 572285 [details] [diff] [review]
patch
Comment 7 Masatoshi Kimura [:emk] 2011-11-06 04:41:30 PST
Try Server result:
https://tbpl.mozilla.org/?tree=Try&rev=58456b8b31bc
Comment 8 Jonathan Kew (:jfkthame) 2011-12-01 23:14:02 PST
(In reply to Masatoshi Kimura [:emk] from comment #5)
> The font was rejected by MakeEOTHeader because it contains only Japanese
> (languageID = 0x411) name records. Before bug 660088, it did not cause a
> problem because OTS created artificial name records and ignored original one.

This implies that before we had OTS at all, the font would also have failed, right? So basically, the original font has always failed except for an interval where we had OTS completely replacing the 'name' table with an artificial one, which "accidentally" worked around the problem.

Although I sympathise with the desire to make this font work, I'm not entirely comfortable with the patch here. The EOT specification[1] explicitly states - in the description of every one of the name fields - that it contains the _English language_ name strings. While putting Japanese names in there may work in practice, it doesn't seem right to ignore the spec in this way. Also, if a font contained names in multiple non-English languages - say, zh-CN, zh-HK and ja-JA but not en-US - it would still be rejected, AFAICS, as the code wouldn't know which one to pick.

So I'd suggest an alternative approach: MakeEOTHeader should look only for the en-US names, and if it doesn't find them, it should supply artificial English names such as "Unknown font name" instead of using a different language. (Basically, something like what OTS used to do.) I think this should be no trickier to code - perhaps simpler, even - and comes closer to following the EOT spec for the header.

[1] http://www.w3.org/Submission/EOT/
Comment 9 Masatoshi Kimura [:emk] 2011-12-02 21:22:09 PST
Created attachment 578809 [details] [diff] [review]
Create artificial names if English names not found in the name table

Oh, I didn't notice EOT spec mandates English names. Thanks to pointing out.
I think it's better to work around the problem somehow because it's an implementation detail that we create EOT header to activate WOFF on Windows. Web pages don't know how we handle WOFF internally. We even reject EOT fonts from Web pages.
I've employed your suggestion.
Comment 10 Jonathan Kew (:jfkthame) 2011-12-12 10:08:17 PST
Comment on attachment 578809 [details] [diff] [review]
Create artificial names if English names not found in the name table

Review of attachment 578809 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFontUtils.cpp
@@ +2120,5 @@
>      
>      // -- iterate through name records, look for specific name ids with
>      //    matching platform/encoding/etc. and store offset/lengths
> +    static const PRUnichar dummyNameStorage[] = L"Unknown font nameRegular";
> +    NameRecordData names[EOTFixedHeader::EOT_NUM_NAMES] = { 0, 34, 34, 14, 0, 0, 0, 34 };

This seems rather cryptic to me - I'd be happier if we had a couple of named string constants for the family and style name, and then copy the names and set up the header fields just as though we're copying them from the real font. That may be fractionally less efficient than having these predefined arrays and then just copying them wholesale, but I think the code clarity is more important here.

Also, please use NS_LITERAL_NAMED_STRING rather than the L"" prefix; I think it's better to follow the usual Gecko convention here even though this particular function is within a Windows-only #ifdef block.

@@ +2177,5 @@
> +        if (needNames != ((1 << EOTFixedHeader::EOT_FAMILY_NAME_INDEX) | 
> +                          (1 << EOTFixedHeader::EOT_STYLE_NAME_INDEX) | 
> +                          (1 << EOTFixedHeader::EOT_FULL_NAME_INDEX) | 
> +                          (1 << EOTFixedHeader::EOT_VERSION_NAME_INDEX))) {
> +            return NS_ERROR_FAILURE;

If I'm understanding this correctly, it'll reject a font that has _some_ English names but doesn't have all of them; the substitution of "artificial" names will only happen if they're _all_ missing. Wouldn't it be feasible to substitute the fake names on an individual basis instead? So, for example, if a font has only a Japanese family name, but the English style name "Regular" or "Plain" is present - things like this can happen! - we'd provide "Unknown" for the family name but keep the existing style name, rather than reject the font.
Comment 11 Masatoshi Kimura [:emk] 2011-12-14 05:27:13 PST
Created attachment 581600 [details] [diff] [review]
Create artificial names if English names not found in the name table
Comment 12 Masatoshi Kimura [:emk] 2011-12-14 08:26:17 PST
Created attachment 581648 [details] [diff] [review]
Create artificial names if English names not found in the name table

Ah, the artificial name length shouldn't overwrite the real one.
Comment 13 Jonathan Kew (:jfkthame) 2011-12-15 07:48:57 PST
Comment on attachment 581648 [details] [diff] [review]
Create artificial names if English names not found in the name table

Review of attachment 581648 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry to go around again on this - I was trying to get my head around the logic of finding names/substituting/rejecting, and finding it a bit confusing. I think it can be made a bit simpler and easier to understand. Please see if the suggestions below make sense to you.....

::: gfx/thebes/gfxFontUtils.cpp
@@ +2133,5 @@
>      PRUint32 needNames = (1 << EOTFixedHeader::EOT_FAMILY_NAME_INDEX) | 
>                           (1 << EOTFixedHeader::EOT_STYLE_NAME_INDEX) | 
>                           (1 << EOTFixedHeader::EOT_FULL_NAME_INDEX) | 
>                           (1 << EOTFixedHeader::EOT_VERSION_NAME_INDEX);
> +    PRUint32 nonEnglishNames = needNames;

I think you don't really need this at all now. Any names in the needNames set for which we don't find an English string can just use the dummyNames entry instead, without caring whether we actually saw non-English names in the font. (If the font's names didn't use the MS platform and encoding, for example, the loop below wouldn't notice them at all.)

@@ +2177,5 @@
> +            if (needNames & (1 << index)) {
> +                names[index].length = dummyNames[index].Length() * sizeof(PRUnichar);
> +            }
> +            nonEnglishNames &= ~(1 << index);
> +        }

And with nonEnglishNames gone, there's no need for the else{} block here. Just use length==0 as a signal to read the dummy record instead of the names[] entry later.

(That will need to be taken into account when computing the size of the variable portion of the header, and when calculating strLen for the name copy.)

@@ +2187,2 @@
>          return NS_ERROR_FAILURE;
>      }

Then this test isn't needed; we don't care if we didn't find the names, because we'll use the dummy ones.

@@ +2216,2 @@
>  
>          strOffset = nameOffset + nameStringsBase + nameoff;

Validating nameoff/namelen and then calculating strOffset only needs to be done if the name was actually found, not if we're going to use the dummy entry, so that could be moved inside the if() below...

@@ +2216,5 @@
>  
>          strOffset = nameOffset + nameStringsBase + nameoff;
>  
>          // output 2-byte str size
>          strLen = namelen & (~1);  // UTF-16 string len must be even

And for dummy entries, strLen will come directly from the 2*Length() of the dummy name, instead of from names[i].length.
Comment 14 Masatoshi Kimura [:emk] 2011-12-16 06:31:03 PST
Created attachment 582255 [details] [diff] [review]
Create artificial names if English names not found in the name table
Comment 15 Masatoshi Kimura [:emk] 2011-12-16 06:32:48 PST
Created attachment 582256 [details] [diff] [review]
hg qdiff -b to make the review easier

The patch becomes much simpler than before. Thanks for the advice!
Comment 16 Jonathan Kew (:jfkthame) 2011-12-16 09:27:21 PST
Comment on attachment 582255 [details] [diff] [review]
Create artificial names if English names not found in the name table

Looks good to me, thanks.
Comment 17 Masatoshi Kimura [:emk] 2011-12-18 21:35:20 PST
https://tbpl.mozilla.org/?tree=Try&rev=ec57aab1fc2b
Comment 19 Marco Bonardo [::mak] 2011-12-19 08:39:18 PST
https://hg.mozilla.org/mozilla-central/rev/7ed48b4ab9d1

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