441473 - implement user font set object

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

[John Daggett (:jtd)]

Implement user font set object, serves as container for use with @font-face rules.

- create by style system code, attached to PresShell(?)
- passed down to text run, font group
- resolves <font family> ==> <font entry object>
- initial lookup sparks the download of font data
- handles download of font data
- download completion causes a reflow(how? hmm...)
- also handles lookup using full font names (e.g. local(xxx))

Comment 1

[Damon Sicore (:damons)]

wanted1.9.1+, P1.

John, lemme know if these aren't actually supposed to be on the 1.9.1 list.  As I understand it, this item is key and is blocking other 1.9.1 work.  I'm doing the same for all the dependent bugs for 70132.  Is this correct?

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The user font set object should be attached to nsPresContext, so you don't have to work through the nsIPresShell interface.

To get 441469 moving, it just needs to support mapping font-family strings to user-font objects (a one-to-many mapping), where these user-font objects store the results of parsing @font-face rules. We should probably just implement it up to that point in 441469.

After that, we need to add an abstract gfxUserFontSet interface in gfx which the layout font set will implement. gfxUserFontSet needs some kind of Lookup method that returns an abstract gfxUserFontSpec which provides enough API to support font selection; a successful Lookup will trigger layout's download of font data. (Until the downloaded is done, gfxUserFontSpec will return "not supported" for every character.) gfxUserFontSet will have to be a parameter in gfxTextRunFactory::Params and used as part of the cache key for the textrun caches. It should probably also have a generation counter which is also part of the cache key, so that every time a font is added or removed, or a font download completes, we can get new results from the textrun caches.

When the download is done and gfxUserFontSpec starts returning "supported" for some characters, we'll actually have to prepare the font for use. I think for this we'll need an API on gfxUserFontSpec that takes a gfxFontStyle and returns a gfxFont. For downloaded TTFs, it would return a platform font; we'll need API, probably in gfxPlatform, to take a buffer of font data and return a font entry, plus a way to take a font entry and a gfxFontStyle and return the gfxFont. For downloaded TTFs we're then pretty much done since the existing code paths can handle these fonts for shaping, measuring and rendering.

For SVG fonts we need to push further and create a gfxUserFont object which supports text-to-glyph conversion and shaping, and rendering and measurement. But that's beyond the scope of this bug.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

One issue is how we're going to update the presentation when @font-face rules are added or removed and when fonts finish downloading. As a first cut we could just call RebuildAllStyleData whenever one of those things happens, but that's going to be pretty inefficient especially if several fonts finish downloading at different times.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm. That's pretty much the design we talked about at the work week. But I wonder if it makes more sense to do more in the style system so that the style system maps font family names to user fonts itself. That would have some major advantages... When a @font-face rule is added or removed, we can handle that as a font style change for the style contexts that reference that font name, instead of rebuilding the world. And when a font finishes downloading, we can handle that the same way. And we don't have to have the font set straddling gfx+layout and impacting the textrun caches etc. And, when a @font-face font appears first in the list of font families for an element (common case I guess), we can kick of the font load immediately from the style system instead of waiting for layout and font selection to happen --- which would give us a 0.5 second or so head start on the download.

A disadvantage of that approach is that we wouldn't be able to use downloaded fonts for final font fallback --- no big loss I think. Another disadvantage is that it means we have to do more work to move font-family-list parsing up out of gfx into layout --- probably a good thing in the end, though. I don't see any other disadvantages right now --- but I thought we discussed this at the work week so maybe there are some I've forgotten.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Until that's settled, we can still make progress on 441469 by attaching the font-set object to nsStyleSet and populating it with the @font-face data.

It would be good to get feedback on comment #4. For Vlad, Stuart and dbaron that'll be next week I guess.

Comment 6

[John Daggett (:jtd)]

(In reply to comment #1)
> As I understand it, this item is key and is blocking other 1.9.1 work.  I'm doing
> the same for all the dependent bugs for 70132.  Is this correct?

Yes, that's right

Comment 7

[John Daggett (:jtd)]

What exactly is the functionality of gfxUserFontSpec you imagine?

My thinking was:
  - lookup of <family name, style> in gfxUserFontSet triggers download of a given face
  - font miss occurs normally, fallback font used
  - download completion triggers style system update as you suggest

Are you imagining that we create some sort of pseudo-gfxFont with an empty cmap that gets pulled in as part of the font matching process and then swizzles itself around to be a real platform font after the download completes?  Sounds tricky.

Comment 8

[John Daggett (:jtd)]

> For SVG fonts we need to push further and create a gfxUserFont object which
> supports text-to-glyph conversion and shaping, and rendering and measurement.
> But that's beyond the scope of this bug.

One of the things that we'll have to work around is the way we do shaping now.  Within InitTextRun routines we typically match possibly multiple fonts against a string of text and then perform shaping and glyph extraction.  With user fonts that do their own shaping like SVG fonts we'll either need to break up the text runs or just overlay the results of SVG font shaping on top of text shaped with some default system font in place of the SVG font.  But that's beyond the scope here...

Comment 9

[John Daggett (:jtd)]

> I wonder if it makes more sense to do more in the style system so that
> the style system maps font family names to user fonts itself. That would
> have some major advantages... 

So adjust the font matching code in gfxFontGroup so that matching could be done independent of initializing a text run?  Doing font family name lookup is easy but the code for mapping style to face is probably better off kept in one place if at all possible.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #8)
> One of the things that we'll have to work around is the way we do shaping now. 
> Within InitTextRun routines we typically match possibly multiple fonts against
> a string of text and then perform shaping and glyph extraction.  With user
> fonts that do their own shaping like SVG fonts we'll either need to break up
> the text runs or just overlay the results of SVG font shaping on top of text
> shaped with some default system font in place of the SVG font.  But that's
> beyond the scope here...

We only shape one glyph-run at a time, shaping doesn't cross font boundaries on Linux and Windows. It might on Mac but that's an implementation detail we can change. I don't see a problem here, we use the platform shaper for native fonts and the SVG shaper (via APIs on the font) for SVG fonts.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #9)
> > I wonder if it makes more sense to do more in the style system so that
> > the style system maps font family names to user fonts itself. That would
> > have some major advantages... 
> 
> So adjust the font matching code in gfxFontGroup so that matching could be done
> independent of initializing a text run?

I don't think we need to change that.

> Doing font family name lookup is easy but the code for mapping style to face is
> probably better off kept in one place if at all possible.

Hmm. So this is the issue where someone wants to be able to use @font-face to add variant faces for system fonts (for particular weights, say)? Ugh.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, I seem to recall that being a problem. OK, so how about this: the style system expands each font-family name to a list of font-face-spec objects, one per @font-face rule, which expose the style and coverage data available via @font-face. When we have no @font-face rule for a family name, that translates to the equivalent of src:local(name). At this point we can also translate generic families like 'serif' to font-face-specs naming the local fonts set in preferences. The concatenation of those font-face-specs is what gets passed to initialize a gfxFontGroup. Then gfx can still do font matching and selection, and we still get the benefits of comment #4.

In that scenario I think we'd expand @font-face rules with multiple values for 'src' into multiple font-face-spec objects.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

A nice way to handle the "font not downloaded yet" issue would be to have three kinds of font-face-spec objects: 'local', 'not-downloaded-yet', and 'downloaded'. When gfx finds it necessary to look for glyphs in a not-downloaded-yet font-face-spec, that triggers the callback to layout to start loading, but otherwise gfx will just skip over not-downloaded-yet font-face-specs. Once the font has downloaded, the style system restyles elements so that not-downloaded-yet font-face-specs turn into 'downloaded', which triggers regeneration of textruns and reflows.

So then only 'downloaded' font-face-specs would support character coverage testing, and construction of gfxFont objects (either platform font objects or 'user' font objects, based on a particular font style and size).

Comment 14

[John Daggett (:jtd)]

(In reply to comment #11)
>> Doing font family name lookup is easy but the code for mapping style to face is
>> probably better off kept in one place if at all possible.
> 
> Hmm. So this is the issue where someone wants to be able to use @font-face to
> add variant faces for system fonts (for particular weights, say)? Ugh.

I just meant that you can have multiple @font-face rules that define a family with multiple weights/styles.  The code to select between different styles/weights is a tad tricky, especially when you factor in synthetic bolding/oblique'ing is involved, so it would be best if we could avoid duplicating it across platforms.

Yeah, I don't think we should try to allow adding variants to system fonts.  If an author defines 'Arial' that would effectively hide the system versions of Arial (modulo local() usage).  Safari allows an author to override the default font with something like:

  @font-face {
    font-family: Times;
    src: local(ArialMT);
  }

That's really specific to their environment, I don't see any need to imitate this for pref fonts.

Comment 15

[John Daggett (:jtd)]

(In reply to comment #12)
> The style system expands each font-family name to a list of
> font-face-spec objects, one per @font-face rule, which expose the style
> and coverage data available via @font-face. When we have no @font-face
> rule for a family name, that translates to the equivalent of
> src:local(name).

Right now, local(name) is defined to specify a unique face, not a family.  And the font a generic maps to is a function of the language, same for pref fonts.  I think using @font-face rules for these would get a bit hairy.

BTW, I trimmed the ability of authors to define generics via @font-face rules from the spec, I was trying to reduce the complexity of the @font-face definition as much as possible.  Let me know if you think that was one edit too many.

Comment 16

[Zack Weinberg (:zwol)]

Query, is the most recent edit of the spec visible at http://dev.w3.org/csswg/css3-fonts/ or should I be looking somewhere else?

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #15)
> Right now, local(name) is defined to specify a unique face, not a family.

I see. OK, so revised suggestion:

Construct gfxFontGroup with a list of font-family objects.
Each font-family object contains the family name plus a list of zero or more font-face-specs corresponding to the @font-face rules applying to that family, expanded so there's one 'src' per font-face-spec.
The font-face-specs come in three kinds 'local', 'not-downloaded-yet' and 'downloaded', as discussed before.

> And
> the font a generic maps to is a function of the language, same for pref fonts.

We can move mLangGroup from nsStyleVisiblity to nsStyleFont so that it's always available for font style resolution. This shouldn't be a problem.

Comment 18

[Zack Weinberg (:zwol)]

I've just switched the dependency relation with bug 441469 around; this bug now depends on that instead of vice versa.  This is because the CSS parser already has a data structure it fills out - CSS DOM objects.  So, my patches for that bug are just going to flesh out a CSSFontFaceRule object in the DOM and attach it to the style sheet.  This bug can then take it from there.  Please let me know if there is a problem with this approach.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #17)
> We can move mLangGroup from nsStyleVisiblity to nsStyleFont so that it's always
> available for font style resolution. This shouldn't be a problem.

For what it's worth, adding properties to nsStyleFont that aren't set by the 'font' shorthand means that we'll lose the optimization of caching in the rule tree when the 'font' shorthand is used.  That said, this already doesn't happen if the font size is in em or %, so this isn't a very serious concern.  And we probably also want this change for bug 416581.

Comment 20

[John Daggett (:jtd)]

(In reply to comment #18)
> I've just switched the dependency relation with bug 441469 around; this bug now
> depends on that instead of vice versa.  This is because the CSS parser already
> has a data structure it fills out - CSS DOM objects.

That's fine, thanks Zack.

Comment 21

[John Daggett (:jtd)]

Attachment: beginnings of a patch, v.0.1

Nothing functional yet, just the beginnings.

Within the style system, PresContext creates a user font shell when a font face rule is found.  This gets passed down into the gfxFontGroup (not quite sure how to do this correctly, code meanders into some gfx/src old Thebes code to create the font group so I'm using a god-awful hack).  When looking up fonts in the font group, names are matched to (1) user font set fonts, followed by (2) system fonts.

Next step is to add calls to AddFontFace into the style system code, then set up the callback to handle loading font data.  The code in Brad's patch has some code to do this in a hacky way, I'll probably start with that.  Then add in the code to create a platform font entry from the font data.

Comment 22

[John Daggett (:jtd)]

Note on patch, v.0.1: 

This is a patch on top of two other patches:

Bug 437356, patch v.0.5
Bug 441469, patch rev3

Comment 23

[John Daggett (:jtd)]

Attachment: work-in-progress patch, v.0.2, more details of the user font set mechanism

Flushed out the mechanism for the user font set object.  Next step is to write the loader routine (quick, dirty hack version), the platform code to create a platform font from data, and the font face name lookup routines for supporting local().  Then I need to go back into the style system and figure out how to map the CSSFontFace data onto the gfx data structures.  Then test...

Sequence of events to handle @font-face:

1. style system parses a font-face rule, add to user font set, creating one if needed (AddFontFace)
2. user font set passed down into font group constructor
3. font group fonts are looked up:
  a. lookup in user font set (FindFontEntry)
  b. otherwise, normal system font lookup
4. when a family name is found in FindFontEntry, loading starts(url) or local platform lookup occurs(local).  Loading is done using a callback into code in the style system (or somewhere else upstream from gfx).  Until loading completes, FindFontEntry passes back null.
5. when loading of a face completes, user font set object is called with data handle and creates a platform font (OnLoadComplete)
6. the return value from OnLoadComplete indicates that a new font was loaded.  Style system decides when/where/how to refresh styles
7. on refresh, font lookup occurs on the user font set and a freshly-minted platform font is provided.  Bolding/italics behaves exactly the same as native font families.

Based on the same two other patches, except for this patch I used Zack's rev4 patch (not sure it matters).

Comment 24

[John Daggett (:jtd)]

Attachment: work-in-progress patch, v.0.2, src local functional

src local functional now on mac, src url loading next.

Comment 25

[John Daggett (:jtd)]

Attachment: work-in-progress patch, v.0.3, src local functional

argh, better patch without dentritus...

This is a patch on top of two other patches:

Bug 437356, patch v.0.5
Bug 441469, patch rev4

Comment 26

[Zack Weinberg (:zwol)]

I have not read the entire code, but in skimming, I saw two things I wanted to comment on:

+    NSString *faceName = GetNSStringForString(aFontName);

Memory leak?  It doesn't look like the NSString object is ever deallocated.

+                    if (valueString.Equals(NS_LITERAL_STRING("opentype"))) {
+                        face.formatFlags |= gfxUserFontSet::FLAG_FORMAT_OPENTYPE;   
+                    } else if (valueString.Equals(NS_LITERAL_STRING("truetype"))) {
+                        face.formatFlags |= gfxUserFontSet::FLAG_FORMAT_TRUETYPE;   
(etc)

Do you think it would be better to treat these strings the same way CSS value keywords are treated, i.e. convert them to numeric constants inside the parser?  I could easily make that change in the 441469 patches.

Also, why'd you rearrange the lists in nsDOMClassInfo.cpp, and should I roll that into the 441469 patches?

Comment 27

[John Daggett (:jtd)]

(In reply to comment #26)

> Memory leak?  It doesn't look like the NSString object is ever deallocated.

This is a Cocoa-ism, the object is put into a retain buffer which is cleaned out periodically as part of the event cycle.  If you want to hold onto the object you increase it's retain count with [aFontName retain].  If you alloc an object yourself than you explicitly need to release it, [myStr release].

http://developer.apple.com/documentation/Cocoa/Conceptual/MemoryMgmt/Articles/mmPractical.html

> Do you think it would be better to treat these strings the same way CSS value
> keywords are treated, i.e. convert them to numeric constants inside the parser?
> I could easily make that change in the 441469 patches.

If you're checking for the an explicit list of supported formats, sure.  But either way is fine.

> Also, why'd you rearrange the lists in nsDOMClassInfo.cpp, and should I roll
> that into the 441469 patches?

Disregard this, I was just moving things around to avoid merge conflicts.  hg is a sensitive beast, seems to require lots of TLC...

Comment 28

[Zack Weinberg (:zwol)]

(In reply to comment #27)
> > Memory leak?
> This is a Cocoa-ism, (...)

Ah.  You can see I know next to nothing about modern Mac programming...

> > Do you think it would be better to treat these strings the same way CSS
> > value keywords are treated?
> 
> If you're checking for the an explicit list of supported formats, sure.  But
> either way is fine.

Well, on the one hand, it would be more efficient and probably easier for
you (I could even gin up the bitmask in the parser); on the other hand, it seems like the CSS parser shouldn't be aware of what formats happen to be
supported on a given platform.

Comment 29

[John Daggett (:jtd)]

Attachment: work-in-progress patch, v.0.4, src url almost functional

The mechanism for font loading functions and fires correctly and successfully creates a platform font on the mac.  Page redrawing not working yet, need to debug this.  Also, temp file creation fails when file exists (even with PR_TRUNCATE specified, grrr).

Also using nsPresContext::RebuildAllStyleData(NS_STYLE_HINT_REFLOW), getting warning:

WARNING: Unable to test style tree integrity -- no content node: file /builds/mozcentral/layout/base/nsCSSFrameConstructor.cpp, line 10060

Switched to nsPresContext::PostRebuildAllStyleDataEvent but am totally clueless at this point what the difference is.  More investigation needed.

This is a patch on top of two other patches:

Bug 437356, patch v.0.5
Bug 441469, patch rev4

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    nsIURI*           uri;           // uri if url !!!! this really needs to be a nsCOMPtr but this causes test code not to compile, ignore for now

If you don't want to include nsIURI.h here, just make this nsCOMPtr<nsISupports> and QI as necessary.

PostRebuildAllStyleDataEvent is asynchronous, RebuildAllStyleData is synchronous. But it looks to me like RebuildAllStyleData always gives that warning, we should probably remove the warning.

The patch looks pretty good. Not sure why page redrawing wouldn't work.

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #29)
> Also using nsPresContext::RebuildAllStyleData(NS_STYLE_HINT_REFLOW), getting
> warning:
> 
> WARNING: Unable to test style tree integrity -- no content node: file
> /builds/mozcentral/layout/base/nsCSSFrameConstructor.cpp, line 10060

Out of curiosity, what in the style data changes as the result of a font loading?  If the answer is nothing, then maybe all you need is a FrameNeedsReflow call on the root frame (with an eStyleChange hint)?  If something, it seems worth saying what at the callsite, in a comment.

I'm actually somewhat puzzled about why that assertion would be firing, though.  Does the root frame have a null mContent?

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes.
http://mxr.mozilla.org/seamonkey/source/layout/base/nsCSSFrameConstructor.cpp#4509

Comment 33

[John Daggett (:jtd)]

Attachment: patch, v.0.5, both src local, src url functional

Loads fonts from urls or via local reference.  This patch is Mac-only, I'm going to hook up the Windows-specific loader code tomorrow.  Ran through examples from:

  http://www.alistapart.com/articles/cssatten

For all the examples, the fonts load but the layout after the new fonts load seems to be based on the old layout, need to poke dbaron/roc to figure out what the right way to do the reflow is.  Word cache assertions fire because the generation value makes previously created words "unfindable" within RemoveWord.

This is a patch on top of two other patches:

Bug 437356, patch v.0.6
Bug 441469, patch rev7a

Comment 34

[John Daggett (:jtd)]

Attachment: screenshot, safari vs. patch v.0.5 rendering

Comment 35

[John Daggett (:jtd)]

Attachment: patch, v.0.6, fix up word cache problems, call FrameNeedsReflow

Fixes several problems:

- leaks caused by not releasing user font set obj
- word cache not getting cleaned out properly
- call FrameNeedsReflow on root frame

All examples on the A List Apart page now render roughly the same as Safari.

This is a patch on top of this patch:

Bug 441469, patch rev7a

Comment 36

[John Daggett (:jtd)]

One problem I'm seeing is a bazillion assertions related to safe scripts:

###!!! ASSERTION: How did we get here if it's not safe to run scripts?: 'nsContentUtils::IsSafeToRunScript()', file /builds/mozcentral/layout/base/nsPresShell.cpp, line 5570
###!!! ASSERTION: Someone forgot to block scripts: 'aIsSafeToFlush == nsContentUtils::IsSafeToRunScript()', file /builds/mozcentral/layout/base/nsPresShell.cpp, line 4515

Not sure how these changes would cause this other than calling FrameNeedsReflow.

Comment 37

[John Daggett (:jtd)]

Attachment: patch, v.0.7c, src url on windows, still debugging

implementation of MakePlatformFont on Windows, using TTLoadEmbeddedFont

Comment 38

[Akira Yoshihara]

(In reply to comment #36)
I was trying to do some testing on the patch, and found that it crashes firefox soon after the following assertion is thrown from the reftest.

> ###!!! ASSERTION: Someone forgot to block scripts: 'aIsSafeToFlush ==
> nsContentUtils::IsSafeToRunScript()', file
> /builds/mozcentral/layout/base/nsPresShell.cpp, line 4515

Crash is likely to occur when multiple test cases are tested. But I can't find out a particular test case that causes the crash, since the crash happens randomly.

Comment 39

[John Daggett (:jtd)]

Attachment: patch, v.0.8b, src url on windows functional

Downloading via src url on windows is now functional with this patch.  Need to implement src local on windows and do a lot more testing.

Comment 40

[John Daggett (:jtd)]

A few more notes regarding downloable fonts on Windows.  I tried originally using these API's:

Approach 1:

Take font data in a file and call CreateScalableFontResource to create a "font resource file" (so for a font.ttf you get a font.fot file which just acts as a proxy for the font data).  Then call AddFontResourceEx with the font resource file to load in the font.  The problem with this is that there's no control over the name that's loaded in, so a user downloading a font with the name 'Arial' will hide the system version across the process.  Unlike the Mac OS X where some sort of fontID uniquely identifies fonts, Windows API's like CreateFontIndirect rely on the name to reference and use fonts.  The font resource file is relatively small, we might be able to hack it to set a different name but that's dodgy.

Approach 2:

Use the font embeddeding library used to support EOT.  The advantage of this is that you can specify a new name when calling TTLoadEmbeddedFont so by picking a name that's different from fonts already on the system, you can avoid it polluting the general pool of fonts.  The downside is that TTLoadEmbeddedFont expects an EOT packaged font, so we need to create an EOT header on the fly.  The header is simple enough, it just contains some general info and name info.

However, the tricky part here is that the so-called EOT spec is missing some fairly important details.  There are 3 versions of the EOT header, tagged with version numbers 0x00010000, 0x00020001 and 0x00020002.  I initially tried to use the 0x00010000 format but the API refused to load various fonts, not precisely sure why.  My guess is that it doesn't actually accept simple, uncompressed fonts, it's made to load in fonts made with the WEFT tool which always compresses the font data (subsetting is user-specified, compression *always* occurs).  

The 0x00020001 format adds a root string field for tagging a font for a given site.  Fonts loaded with a header in this format with a null root string seem to load fine.  The WebKit code is doing the same thing, they must have gone down the same fun road.

So we have a way that works but it's not the prettiest piece of code in the world...

Comment 41

[John Daggett (:jtd)]

(In reply to comment #38)
> (In reply to comment #36)
> I was trying to do some testing on the patch, and found that it crashes firefox
> soon after the following assertion is thrown from the reftest.

Thanks Akira, I'm going to do some more testing over the weekend.  Hopefully I can track down what exactly is causing that assertion and/or crash.

Comment 42

[timeless]

Comment on attachment 333906 [details] [diff] [review]
patch, v.0.8b, src url on windows functional

> class gfxFontEntry {
...
>     PRPackedBool     mIsType1     : 1;
>+    PRPackedBool     mIsSVG       : 1;
>+    PRPackedBool     mIsUserFont  : 1;
>+    PRPackedBool     mIsProxy     : 1;
> 
>     PRUint16         mWeight;
>+    PRUint16         mStretch;
> 
>     PRPackedBool     mCmapInitialized;

this lonely packed bool is misplaced, please move him with his friends, and move the uints before the packed bools.

>+    // With downloadable fonts, the composition of the font group can change as fonts are downloaded
>+    // for each change in state of the user font set, the generation value is bumped to avoid picking up
>+    // previously created text runs in the text run word cache.  For font groups based on stylesheets
>+    // with no @font-face rule, this always returns 0.

/* comments, please. */


>     // Init this font group's font metrics. If there no bad fonts, you don't need to call this.
>     // But if there are one or more bad fonts which have bad underline offset,
>     // you should call this with the *first* bad font.

and here
>     void InitMetricsForBadFont(gfxFont* aBadFont);

like this:
>     /* If aResolveGeneric is true, then CSS/Gecko generic family names are
>      * replaced with preferred fonts.
>      *
>      * If aResolveFontName is true then fc() is called only for existing fonts
>      * and with actual font names.  If false then fc() is called with each
>      * family name in aFamilies (after resolving CSS/Gecko generic family names
>      * if aResolveGeneric).
>      */
>+    /*static*/ PRBool ForEachFontInternal(const nsAString& aFamilies,

>diff -Narp -U 8 backupmozcentral/gfx/thebes/public/gfxFontUtils.h mozcentral/gfx/thebes/public/gfxFontUtils.h
> class THEBES_API gfxFontUtils {
> public:
>-
>+    
please don't add trailing whitespace.
>     // for reading big-endian font data on either big or little-endian platforms

>+    // given a TrueType/OpenType data file, produce a EOT-format header for use with Windows T2Embed API
an EOT ...


>     virtual gfxFontGroup *CreateFontGroup(const nsAString& aFamilies,
>+                                          const gfxFontStyle *aStyle,
>+                                          gfxUserFontSet *aUserFontSet) = 0;
>+                                          
>+                                          
more trailing whitespace. but why two blank(ish!) lines?
>+    /**


>+struct gfxFontFaceSrc {
>+    PRBool            isLocal;       // url or local
>+    nsString          localName;     // full font name if local
>+    nsIURI*           uri;           // uri if url !!!! this really needs to be a nsCOMPtr but this causes test code not to compile, ignore for now
>+    PRUint32          formatFlags;   // format hint if url

this is a kinda strange ordering, are you trying to optimize anything here?



>+    class LoaderContext {
>+    public:
>+        LoaderContext(LoaderCallback aLoader)
>+            : mUserFontSet(nsnull), mLoaderProc(aLoader) { }

could you limit your things to one init per line? it makes adding other members easier on hg/cvs blame.

>+    // add in a font face
>+    // weight, stretch - 0 == unknown, [1, 9] otherwise
>+    // italic style = constants in gfxFont.h (e.g. FONT_STYLE_NORMAL)
>+    void AddFontFace(const nsAString& aFamilyName, const nsTArray<gfxFontFaceSrc>& aFontFaceSrcList, 
>+                     PRUint32 aWeight = 0, PRUint32 aStretch = 0, PRUint32 aItalicStyle = 0, gfxSparseBitSet *aUnicodeRanges = nsnull);

please wrap much earlier (personally, i recommend one arg per line).

i generally don't repeat my complaints, but i'll repeat this one once again (and not more):
>+    PRPackedBool              mIsLoading;
>+    nsTArray<gfxFontFaceSrc>  mSrcList;
>+    PRUint32                  mSrcIndex;       // index of the src item loading
>+    PRPackedBool              mItalicUnknown;  // base class only stores italic or not, need to handle "unknown"
packed bools need to placed in packs :)
>+    gfxMixedFontFamily*       mFamily;

>-            if (font->GetFontEntry()->FamilyEntry()->IsBadUnderlineFontFamily()) {
please use #if 0 above the block instead of this:
>+            if (0 && font->GetFontEntry()->FamilyEntry()->IsBadUnderlineFontFamily()) {  // jtdfix
your change alters blame which is not wanted.

>+    if (fs && (gfe = fs->FindFontEntry(aName, *fontStyle, needsBold))) {
>+        if (gfe && !gfe->mIsSVG) {
gfe can't be false here...

>+    // jtddebug
please use #ifdef DEBUG_jtd (it's ok to even have #define DEBUG_jtd 1 elsewhere in the file)
>+    if (mUserFontSet) {
>+        printf("InitTextRun fs: %p curGen: %d userSetGen: %d\n", mUserFontSet, mCurrGeneration, GetGeneration());
>+    }

>+gfxFontEntry::~gfxFontEntry() 
>+    if (mUserFontData)
>+        delete mUserFontData;
if isn't needed for delete.


>+struct AutoSwap_PRUint16 {
>+    operator PRUint32() { return NS_SWAP16(value); }
>+    PRUint16  value;

>+struct AutoSwap_PRInt16 {
>+    operator PRUint32() { return NS_SWAP16(value); }
>+    PRInt16  value;

i'm not sure what alignment you're aiming for, but it doesn't look good :(

>+struct SFNTHeader {
this is really too wide for my tastes, could you please consider sticking the comments on their own line, preceding the code they comment?
>+    AutoSwap_PRUint32    sfntVersion;            // Fixed, 0x00010000 for version 1.0.
...

way too long:
>+    AutoSwap_PRUint32    checkSumAdjustment;    // To compute: set it to 0, sum the entire font as ULONG, then store 0xB1B0AFBA - sum.
>+    AutoSwap_PRUint32    magicNumber;           // Set to 0x5F0F3CF5.
>+    AutoSwap_PRUint16    flags;
>+    AutoSwap_PRUint16    unitsPerEm;            // Valid range is from 16 to 16384. This value should be a power of 2 for fonts that have TrueType outlines.

if this comes from a spec, a link to the spec is appreciated:
>+    AutoSwap_PRUint64    created;               // Number of seconds since 12:00 midnight, January 1, 1904. 64-bit integer

>+// EOT handling -- needed for dealing with downloadable fonts on Windows
if it's this spec, it's too late :)
>+// EOT version 0x00010000
>+// based on http://www.w3.org/Submission/2008/SUBM-EOT-20080305/

>+        if ((PRUint32) nameRecord.platformID != NameRecord::PLATFORM_ID_MICROSOFT || 
>+                (PRUint32) nameRecord.encodingID != NameRecord::ENCODING_ID_MICROSOFT_UNICODEBMP || 

this feels like tabs, the (s around PRUint32 should line up.

>+        if (fe) {
>+            pe->mFamily->ReplaceFontEntry(pe, fe);
>+            IncrementGeneration();
>+            return PR_TRUE;
>+        } else {
remove |else {|, "else after return".
>+            // error occurred making platform font, remove this entry
>+            pe->mFamily->RemoveFontEntry(pe);
>+            return PR_FALSE;

>+    do {
>+        status = LoadNext(pe);
>+    } while (status != STATUS_LOADING && status != STATUS_END_OF_LIST);

having recently (2hrs?) encountered an infinite while loop, what protects against them?

(the loop involved a file iteration operation which could fail, and then the next attempt would try and fail the same iteration and it never made any progress).

>diff -Narp -U 8 backupmozcentral/gfx/thebes/src/gfxWindowsFonts.cpp mozcentral/gfx/thebes/src/gfxWindowsFonts.cpp

i'll read some more later. i hope you don't mind the comments.

Comment 43

[John Daggett (:jtd)]

(In reply to comment #42)
Thanks timeless.  I haven't done cleanup yet, but I will shortly.  As for coding conventions, I try in general to follow whatever the coding pattern is in the surrounding code (e.g. layout/style uses 2 spaces indent, gfx code 4 space).  So I think some of your suggestions may not match the overall coding patterns within gfx code.

Comment 44

[Akira Yoshihara]

(In reply to comment #41)

The crash seems to happen when you leave the page before the font downloading finishes.
The crash happens only when you applied the patch.
It is reproducible. The following URL is the test page which would cause the crash.
http://people.mozilla.org/~ctalbert/fontface/downloadable-font-crashtest.html
Would it be better if I open another bug for this?

Comment 45

[Akira Yoshihara]

I noticed a strange behavior that occurs only when @font-face is used.

When the font download finishes firefox renders again, using downloaded fonts. Then, some characters are rendered incorrectly.
So far, I found out that some Japanese characters are rendered incorrectly when the downloaded font does not contain Japanese characters in it. This problem occurs when the files are in Japanese encoding (Shift_JIS, EUC-jp). And it doesn't occur in UTF-8 encoding.

Every time this happens, message like the following shows up on the console.
InitTextRun fs: 0x16a2fac0 curGen: 3276801 userSetGen: 3276801

It seems that after the download, Chinese fonts are used to render Japanese characters, which makes them look strange.

Comment 46

[John Daggett (:jtd)]

(In reply to comment #45)
Thanks Akira.  Probably best to write these up as separate bugs for now.  In the set of steps to reproduce, be sure to list "Apply patch xx from bug 441473 and build" so that other folks don't waste time trying to reproduce the problem in a nightly build.  Add 441473 to the blocks list.

Comment 47

[Akira Yoshihara]

(In reply to comment #46)
> Probably best to write these up as separate bugs for now.
All right, so I opened 2 new bugs.

Bug 451426
Incorrect rendering of some Japanese characters when using downloadable fonts (@font-face)

Bug 451462
Firefox crashes when leaving the page before the font is downloaded (using @font-face rules)

Please be careful since the bug numbers might make you confused.

Comment 48

[John Daggett (:jtd)]

Attachment: stack trace for nested calls to PresShell::DoFlushPendingNotifications

The assertion occuring in comment 36 happens with the following stack trace:

Within PresShell::DoReflow, user font set code is initiating the load of a font with a call to NS_OpenURI.  This calls nsAppShell::OnProcessNextEvent which ends up calling PresShell::WillPaint, leading to PresShell::IsSafeToFlush getting called.

I'm guessing that I need to handle the OpenURI within a separate event, so that weird nested calls like this don't take place.

Comment 49

[John Daggett (:jtd)]

Attachment: patch, v.0.9, implement same-site origin restriction

This patch implements same-site origin restrictions on the underlying font to be downloaded.  This means that the origin of a font url must match the url of the underlying document.  Based on advice from Jonas I'm using the CheckMayLoad method of nsIPrincipal with the stylesheet's owning document node principal object.

The pref gfx.downloadable_fonts.same-site-origin.enabled can be used to disable this in debug builds, it's set to enabled by default.  Note that this restriction will break examples like the A List Apart article:

  http://www.alistapart.com/articles/cssatten

The examples in this article link against a stylesheet containing fonts on a different server.

Also, fixed the error noted in bug 451462.

Comment 50

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #48)
> Created an attachment (id=335673) [details]
> stack trace for nested calls to PresShell::DoFlushPendingNotifications
> 
> The assertion occuring in comment 36 happens with the following stack trace:
> 
> Within PresShell::DoReflow, user font set code is initiating the load of a font
> with a call to NS_OpenURI.  This calls nsAppShell::OnProcessNextEvent which
> ends up calling PresShell::WillPaint, leading to PresShell::IsSafeToFlush
> getting called.
> 
> I'm guessing that I need to handle the OpenURI within a separate event, so that
> weird nested calls like this don't take place.


You don't want to be using NS_OpenURI for remote resources.  The
comment above it says:
// Use this function with CAUTION. It creates a stream that blocks when you
// Read() from it and blocking the UI thread is a bad idea. If you don't want
// to implement a full blown asynchronous consumer (via nsIStreamListener) look
// at nsIStreamLoader instead.

You want to be loading stuff asynchronously.

I've actually never written any Necko consumers that do this, so I'm
not the right person to say how to do it.  bzbarsky is probably the
best person to bug, but the suggestions in that comment are probably
useful (particularly nsIStreamLoader, and maybe NS_NewStreamLoader,
but I'm just poking through headers).

Comment 51

[Akira Yoshihara]

(In reply to comment #49)
> Created an attachment (id=335684) [details]
> patch, v.0.9, implement same-site origin restriction

Same-site origin restriction seems to be working.

But unfortunately, now firefox hangs when I try to open a page using downloadable fonts.

These are the call stacks. I hope it will help.
https://wiki.mozilla.org/QA/Firefox3.1/FontFace_hung_attachment335684

Comment 52

[John Daggett (:jtd)]

Attachment: patch, v.0.9b, async loading, same-site origin check at load time

Reworked the way nsFontFaceLoader worked so that it now acts as nsIDownloadObserver object.  Needed to adjust the interface to nsIDownloader a bit to give more control over temp/cache file handling.  The advantages of this are:

- loading is now async
- uses disk cache so repeated loads will get pulled from that
- eliminates layout reentry assertions (comment 50)

Also reworked same-site origin checking so that it now occurs at load time rather than when parsing font-face rules.  Added code to handle checking when redirects occur.

Tested on mac, will test on windows tomorrow.

Comment 53

[John Daggett (:jtd)]

Attachment: patch, v.0.9c, windows local() support, bug fixes

Added support for src local() on Windows.

Example:

@font-face {
  font-family: Gentium;
  src: local("Gentium"), url(fonts/Gentium.ttf);
}

Unfortunately, using the Postscript name to search for a font face isn't supported on Windows, so on Windows local requires the full font face name (e.g. Arial Bold).  I'm going to have to fix up the spec to reflect this.

When a font load fails, the next src parameter was being ignored, that should be fixed now.

Comment 54

[John Daggett (:jtd)]

Attachment: patch, content changes, v.0.1, move nsSameOriginChecker into nsContentUtils

Moving the nsSameOriginChecker code out of nsXBLService into nsContentUtils so that it can be referenced from within code used to download font data.  This creates a dependency on some necko headers so I needed to add necko to the required list in a few places.

Comment 55

[John Daggett (:jtd)]

Attachment: patch, netwerk changes, v.0.1, allow control over temp/cache file in nsIDownloader

For use when downloading fonts I've made some additions to nsIDownloader to allow more control of temp/cache files.  

Specifically, my changes add a new init method so that a temp file prefix/suffix can be specified along with a boolean to control the automatic cleanup of temp files.  Depending upon the platform, the downloadable font code in some cases needs to keep the temp file around.  Same is true for cache files, so I've made the cache token a readonly attribute so that a reference can be added when the cache file needs to remain pinned beyond the lifetime of the downloader obj.

I've made the changes so that existing code will be unaffected other than the interface version bump.  I did make it so the code only calls srand once, rather than everytime a file is downloaded.  Without this change, when several files are downloaded at once the same seed is used and CreateUnique is forced to handle the name collision.

Comment 56

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 336956 [details] [diff] [review]
patch, content changes, v.0.1, move nsSameOriginChecker into nsContentUtils

Hmm.. it's unfortunate to have to add dependencies on necko like that :(

An alternative solution would be to add a function like

nsIInterfaceRequestor*
nsContentUtils::GetSameOriginChecker

That returns a nsSameOriginChecker. That way we could even reuse the same same-origin checker instance everywhere.

Comment 57

[John Daggett (:jtd)]

Attachment: patch, content changes, v.0.2, move nsSameOriginChecker into nsContentUtils without using necko hdrs

As per comment 56, use a static singleton method for accessing same-site origin checker obj.

Comment 58

[John Daggett (:jtd)]

Comment on attachment 336961 [details] [diff] [review]
patch, netwerk changes, v.0.1, allow control over temp/cache file in nsIDownloader

Email thread with biesi regarding changes to nsIDownloader:

biesi: Why don't you just pass in a file? That way the file won't get
biesi: deleted. Do you need any further behaviour changes?

jd: Because if a file is passed in, the download is not cached.  I want the
jd: "cached if you have it/temp if not" behavior.

biesi: What do you mean with "it is not cached"? The only difference is that
biesi: the data will be copied from the cache file to the specified file. The
biesi: data will still be read from the cache.

jd: Ok, fair enough, but then there's still a copy that occurs each time, 
jd: right?  Seems like it would be better to use the cache file directly if 
jd: at all possible.

biesi: True. So I don't see why you can't just keep the nsIDownloader object
biesi: alive... Because it seems like in your original patch you still have
biesi: to keep the cache token alive, so this should not make a difference.

jd: Yeah, that's another way.  I guess if you're not comfortable making changes
jd: to nsIDownloader I can experiment with doing it that way.

biesi: Yeah I don't really like those changes... it seems like a lot of
biesi: complications to the interface just to avoid the copy, and I don't
biesi: think that the copy is that expensive. Note that you are not
biesi: guaranteed to be able to use the cache entry anyway.

jd: You mean that sometimes it won't be in the disk cache even if the data was 
jd: downloaded before?  Or that the cache file returned wouldn't be valid in some cases?

biesi: If it is in the disk cache it might not be its own file. And if the
biesi: server sent the right (or the wrong?) caching headers, the entry will
biesi: only be in the memory cache. I'm not sure if we fixed this, but if the
biesi: server sent a gzipped reply, then hopefully you'd get a copy that was
biesi: ungzipped, instead of the cache file that is gzipped.

So I'm going to rework the code to use the existing version of nsDownloader.

Comment 59

[John Daggett (:jtd)]

Attachment: patch, v.0.9d, use unmodified nsDownloader

Instead of modifying nsIDownloader, cache the downloader obj until it's no longer needed to prevent obj from temp/cache file from being deleted/unpinned.  As a result, the font file will always be linked, since ATS requires a font file to have a .ttf extension.

This patch still includes the code in the contents subtree but I'll remove that once the patch is reviewed and checked in.

Comment 60

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 336992 [details] [diff] [review]
patch, content changes, v.0.2, move nsSameOriginChecker into nsContentUtils without using necko hdrs

Sweet!

Comment 61

[John Daggett (:jtd)]

patch, content changes, v.0.2 nsSameOriginChecker move pushed
changeset=9149972f78be

Comment 62

[Akira Yoshihara]

Some .otf fonts don't load on Windows.

I made a further investigation on this problem. Now I'm pretty sure that the problem occurs only when you are using the OpenType fonts based on PostScript(Type1) fonts. It does not happen on OpenType fonts based on TrueType fonts. I used a free tool (T2FAnalyzer) and found out that the problematic fonts didn't contain any TrueType related information. That would mean that these fonts are not TrueType based, but PostScript based. But actually, due to the limitation of the tool, I couldn't see if they really contain PostScript information.

The interesting thing is that when you use normal font-family rules, these fonts are correctly rendered on Windows' Firefox. It does not load only when you're using @font-face rules.

Comment 63

[John Daggett (:jtd)]

Attachment: patch, v.0.9f, cleanup of font loading code, logging

Reworked the user font set code that handles font loading so that it's more straightforward. Definitely might have introduced bugs by doing this however, this needs some stress testing.  Cleaned up passing of user font object through Thebes wrapper calls.  Added code to clean out temp files after a crash.

Also added new logging within the user font code and the font loader.  To use:

  export NSPR_LOG_FILE=userfonts.log
  export NSPR_LOG_MODULES=userfonts:5,fontdownloader:5

Comment 64

[Gabriele Best [:Gabri]]

John, can you make a test build on tryserver?
Thanks.

Comment 65

[Kevin Newman]

There has always been an issue with font names and selection in various apps on Windows when it comes to Postscript OpenType CFF fonts (.otf) vs. TrueType OpenType fonts (.ttf).

Windows seems to have a compatibility mode that attempts to squeeze the more robust font styles of OpenType into the 4 main categories that many GUIs use (regular, bold, italic, bold-italic).

That's actually a huge nuisance in applications like Flash CS3 (and browsers accessing system fonts with CSS) where you can't actually select some OpenType fonts easily without selecting one of the items listed, and then hitting bold to hope you get "Medium". Other applications though, like Photoshop CS3, InDesign CS3, etc. utilize (as far as GUI) a second drop down instead of the traditional bold and italic buttons (which CSS seems to attempt to duplicate in it's keywords).

It would be great if the various browser implementations (and Adobe) could get together and figure out how this should all work, crossplatform, so we don't end up with compatibility issues like we do with Flash CS3 (and other apps, where you often can't take an fla and move it from Mac to PC and vice versa if it uses a Postscript CFF OpenType font). This problem would be worse in a browser since that would be a runtime problem, not just an authoring problem (like it is with fla).

Just to further illustrate the issue mentioned above, a font like ClearviewText: 

http://www.terminaldesign.com/fonts/set/?ID=111

has many more than 2 styles, so in Windows it gets grouped by 4 fonts, so that for example, to get ClearviewText-Medium, you actually have to choose ClearviewText-Book and then make it bold using either the bold button in an app like Flash CS3, or by specifying font-style: bold in css. On Mac, you just choose "ClearviewText-Medium" as the font family/name and bold is ignored, or faux.

I would recommend doing it the Photoshop/Mac OS X way, even on Windows. 

Apologies if this is too vague or otherwise not useful. :-)

Comment 66

[Akira Yoshihara]

Attachment: archive of files needed to reproduce the font bug

(In reply to comment #63)
> Created an attachment (id=337626) [details]
> patch, v.0.9f, cleanup of font loading code, logging

After I applied the latest patch (v.0.9f), I found a new bug about rendering.
Once you load a font, and use the same font from another page, it is displayed in completely messed up way. Please see the screenshot. When you copy the messed up text and paste it into a text editor, it is displayed correctly. So it seems to be the graphics problem.

Steps to reproduce:
1. unzip the attached zip file
2. install verait.otf to your system (make sure you don't remove the font file from the directory)
3. open downloadable-font-conflictname.html
4. open downloadable-font-familywithseveral-ref.html
5. open downloadable-font-familywithseveral.html

Actual Results:
First sentence [Bitstream Vera Italic] is displayed in messed up way.

Expected Results:
First sentence [Bitstream Vera Italic] should be legible, just like the first sentence of downloadable-font-familywithseveral-ref.html

Comment 67

[John Daggett (:jtd)]

(In reply to comment #64)
> John, can you make a test build on tryserver?

Once I resolve the problem noted in comment 66, will do.

Comment 68

[John Daggett (:jtd)]

(In reply to comment #65)
> There has always been an issue with font names and selection in various apps on
> Windows when it comes to Postscript OpenType CFF fonts (.otf) vs. TrueType
> OpenType fonts (.ttf).

Kevin, this is the issue of style linking on Windows, it's not really related to this bug.  We've actually talked before about ways to work around this problem so I've created a separate bug for this, bug 454514.  Problems with OpenType CFF fonts (.otf) discussed in previous comments relate to problems using them as downloadable fonts which is tied to the weird and wacky Windows API's that handle font loading.

Comment 69

[John Daggett (:jtd)]

It appears that even though fonts are being loaded "privately", ATS tries to fire off notifications for each font loaded.  This causes the new temp cleanup code I added to fire and immediately delete the fonts that were loaded, hence all the rendering errors.

The ATS docs seem to indicate that using kATSOptionFlagsDoNotNotify will suppress this but that does seem to suppress them, so I'm going to set a flag and ignore the notification when the flag is set.

Also, using the nsIDownloader obj appears to fail every now and then with odd cache-related errors, so for now I'm going to just specify the temp file name and clean it up manually.  Hopefully I can fix this later so there's no extra copying.

I'll post a new patch and a try server build later tonight.

Comment 70

[John Daggett (:jtd)]

Attachment: patch, v.0.9h, fix up rendering problems

This fixes the problem caused by ATS notifications clearing out font files.

The ATS font loading API seems a tad flakey, every now and then it seems to load a font file fine but font API calls fail when using the ATSUFontID provided.  This seems to happen even when the font files are a binary match for previous font files that loaded and rendered correctly, so I'm guessing this is some weird ATS font cache bug.  I added code in the font entry constructor to immediately set an invalid flag when one of these errors occurs, so fallback will occur instead of corrupted rendering.

Steps to reproduce:

1. Load up one of the example pages from the A List Apart article
2. Keep hitting refresh, until the page is rendered with one of the fallback fonts (indicating an error loading a downloable font file)

I'm going to experiment with ways we can work around this (other load API calls? just do a reload with the same font file?).

Comment 71

[John Daggett (:jtd)]

Try server build (Mac/Win):

http://tinyurl.com/5b3ghd

Caveats:

- Windows: Postscript-style OTF fonts (i.e. CFF fonts) don't load
- Mac: Every now and then font load will fail and fallback font will show, investigating
- same-site origin restriction enabled by default (site A can't refer to fonts on site B)
- restriction can be disabled with gfx.downloadable_fonts.same-site-origin.enabled in about:config
- with restriction disabled, A List Apart article examples will load correctly:

  http://www.alistapart.com/articles/cssatten

- use of access control headers to allow cross-site usage not yet implemented
- Linux: not yet working, see bug 449356
- for now, fonts are stored in mozfontxxx.ttf files in system temp directory.  These may not get cleaned up if a crash occurs.
- unicode-range not yet implemented

If you run into problems, please note them here along with clear steps on how to reproduce.

Comment 72

[Dave Crossland]

Allow cross-site usage is important. 

The openfontlibrary.org website is going to be a central repository for "free software" fonts - that are redistributable and modifiable - and I expect there will be many "freeware font" (redistributable but non-modifiable) sites that also provide central repositories. 

Hundreds of millions of people publish on the web via servers they do not control at a file system level but do control the CSS of - eg, MySpace - and for these users it is essential that cross-site usage is possible. 

If a same-site origin restriction is enabled by default, this will mean font library websites will be much less useful, and all those web authors will not be able to publish to Firefox users as they ought to. 

When access control headers are implemented, will that mean the same-site origin restriction is DISabled by default, but can be Enabled with these headers?

Comment 73

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #72)
> Allow cross-site usage is important. 
> 
> The openfontlibrary.org website is going to be a central repository for "free
> software" fonts - that are redistributable and modifiable - and I expect there
> will be many "freeware font" (redistributable but non-modifiable) sites that
> also provide central repositories. 

These sites just need to enable Access-Controls on their servers to allow Firefox to use their fonts on any site.

> When access control headers are implemented, will that mean the same-site
> origin restriction is DISabled by default, but can be Enabled with these
> headers?

No. Access Controls can only grant additional rights; it can't impose additional restrictions.

So, a site can use Access Controls to tell Firefox that fonts on that site can be used by any other site (or by some set of other sites). That should be enough to satisfy openfontlibrary.org and whoever else.

Comment 74

[philippe (part-time)]

Running on OS X 10.5.4/Intel.

I went to R. Ishida's recent test page:
http://www.w3.org/International/tests/test-webfonts-1

* It seems to work better than the WebKit build in front of me :-)
* The Arabic tests (test 7 and 8) fail (in both browsers): the installed fonts are used instead.
I tried downloading/installing those fonts, but the browsers (both) didn't use them.

* I'm not sure about test 10, although it looks better than WebKit.

I played a bit with Japanese fonts (the nice Axis fonts) (on local dev server).
Those claim to be OpenType PostScript; however, if I add 'format("opentype")', Minefield won't load them; omitting the 'format' and it works OK.
(There is a free download sample version here - those are not complete, though:
http://www.typeproject.com/product/download.html)

Japanese fonts work pretty well, on Mac. So far :-)

Hmm, now I need to find some nice Jpn fonts that weight less than the 1.3Mb + for those I have here...

Comment 75

[Dave Crossland]

Rob, many thanks for this clarification, I hadn't seen the W3C access controls spec before today and clearly didn't totally understand it. Cheers!

Comment 76

[John Daggett (:jtd)]

Attachment: patch, v.0.9j, use ATS generations and clean up temp files on windows

Under Mac OS X, track the ATS generation to avoid unneeded updates after adding fonts.  On Windows, temp font files were not getting cleaned out.

Try server builds:
http://tinyurl.com/4ec8j8
(@font-face still on supported Mac/Win)

Comment 77

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 338274 [details] [diff] [review]
patch, v.0.9j, use ATS generations and clean up temp files on windows

You have an "xxx" comment in nsThebesFontMetrics::Init that looks like
it doesn't apply anymore.

I skipped the code inside gfx/thebes (which is most of the patch) on the
assumption that that's not the part you were asking my review on, and
that I probably wouldn't have a lot to add.

No need to #include gfxUserFontSet.h twice in nsPresContext.cpp.

In nsPresContext, could you make mUserFontSet a
nsRefPtr<gfxUserFontSet>?  Then you can avoid the manual reference
counting in SetUserFontSet and the changes to the constructor and
destructor.  It also seems like GetUserFontSet and SetUserFontSet could
be inline (unless #include-ing gfxUserFontSet.h in nsPresContext.h
triggers bad include dependencies, although it looks like it already
pulls in thebes headers).

In layout/style/Makefile.in, nsFontFaceLoader.h shouldn't be in the
EXPORTS list, since it's private to the module.  (Yes, the existing
EXPORTS list is too long, but that doesn't mean we should make it
worse.)  You shouldn't need to list it anywhere.

nsCSSRuleProcessor.cpp:

Not sure why you're #include-ing nsIDocument.h.

So these changes don't reflect that style sheets can be enabled and
disabled dynamically.  It looks like what you're doing is that once a
sheet is enabled, you're adding it to the user font set.  How you want
to solve this depends on things such as how expensive the gfxUserFontSet
objects are to keep around (e.g., in memory use), and whether there are
performance problems (e.g., cache loss) that would result from
destroying a gfxUserFontSet and then immediately building a new one that
was similar or identical.  It's a little bit tricky, since:
 * there are multiple nsCSSRuleProcessor objects, one for each level of
   the cascade (user-agent sheets, user sheets, author sheets)
 * each nsCSSRuleProcessor might cache some old RuleCascadeData objects
   in addition to the current one
 * some dynamic changes cause us to rebuild the nsCSSRuleProcessor for a
   cascade level; some dynamic changes cause us to clear and rebuild the
   RuleCascadeData; some cause us only to pick a different
   RuleCascadeData without necessarily rebuilding anything.
If you're interested, I could probably come up with slightly more
detailed suggestions if I had a better idea of what constraints you were
dealing with.

InsertFontFaceRule should probably begin with an NS_ASSERTION about the
type of the rule (to go along with the cast).

>+  PRUint32 unit, weight = NS_STYLE_FONT_WEIGHT_NORMAL, stretch = NS_STYLE_FONT_STRETCH_NORMAL, italicStyle = FONT_STYLE_NORMAL;

don't go past column 80

>+  val.GetStringValue(fontfamily);

You can't call GetStringValue without checking the unit first.

In InsertFontFaceRule, you could probably factor the code for weight,
stretch, and style into either a loop over the array:
  { eCSSFontDesc_Weight, &weight, NS_STYLE_FONT_WEIGHT_NORMAL },
  { eCSSFontDesc_Stretch, &stretch, NS_STYLE_FONT_STRETCH_NORMAL },
  { eCSSFontDesc_Style, &italicStyle, FONT_STYLE_NORMAL }
or into a function.

Should the case inside the handling of the src descriptor assert that it
doesn't hit the default case?

+          if (valueString.Equals(NS_LITERAL_STRING("opentype"))) {
+            face.formatFlags |= gfxUserFontSet::FLAG_FORMAT_OPENTYPE; 
+          } else if (valueString.Equals(NS_LITERAL_STRING("truetype"))) {
+            face.formatFlags |= gfxUserFontSet::FLAG_FORMAT_TRUETYPE; 
+          } else if (valueString.Equals(NS_LITERAL_STRING("truetype-aat"))) {
+            face.formatFlags |= gfxUserFontSet::FLAG_FORMAT_TRUETYPE_AAT; 
+          } else if (valueString.Equals(NS_LITERAL_STRING("embedded-opentype"))) {
+            face.formatFlags |= gfxUserFontSet::FLAG_FORMAT_EOT;   
+          } else if (valueString.Equals(NS_LITERAL_STRING("svg"))) {
+            face.formatFlags |= gfxUserFontSet::FLAG_FORMAT_SVG;   
+          }

This could be written using EqualsLiteral("...") rather than
Equals(NS_LITERAL_STRING("...")).  Though it could also be written as a
loop over an array of const struct { char
name[sizeof("embedded-opentype")]; PRInt32 flag; }, using EqualsASCII
(not EqualsLiteral).

You can also replace the string copy with a call to GetStringBufferValue
followed by use of case-insensitive comparison functions, instead of
GetStringValue followed by ToLowerCase.

+    nsPresContext *ctx = data->mPresContext;

Call it presContext (or pc, if you really want), not ctx (by
convention).

It might be cleaner (more encapsulated) to replace
nsPresContext::SetUserFontSet with either an aCreate boolean to
GetUserFontSet (which would mean it shouldn't be inline) or with a
second method to create a user font set if there isn't one already.
Creating it inside InsertRuleByWeight feels a bit odd.  You should
probably also null-check the creation of the loader context and the user
font set.

I think both of the changes to nsCSSRules.h should go away (not sure
what nsIPrincipal has to do with anything, and inserting trailing
whitespace is bad).

In your license headers for nsFontFaceLoader, the initial developer
should be the Mozilla Foundation, not Netscape (unless you've copied the
code from elsewhere), and "the original code" should probably be
nsFontFaceLoader (unless it's copied from elsewhere).

In nsFontFaceLoader.cpp, could you stick the same single-line comment
at the beginning (before any code) that you have in nsFontFaceLoader.h
so it shows up in
http://mxr.mozilla.org/mozilla-central/source/layout/style/

I'm somewhat disturbed that you didn't have to #include "prlog.h" in
nsFontFaceLoader.cpp, since including it in header files is somewhat bad
because of the macros (like FORCE_PR_LOG) that change its behavior.
Then again, I think I've complained about that elsewhere...

>+static PRLogModuleInfo *gFontDownloaderLog = PR_NewLogModule("fontdownloader");

I think the convention is that this should be in #ifdef PR_LOGGING,
since there's an option somewhere to turn off all NSPR logging even in
FORCE_PR_LOG files.

You might also want the FORCE_PR_LOG to be defined inside an #ifdef
MOZ_LOGGING.  But I'm getting lost at this point... maybe I should come
back to this point.

Instead of doing a do_GetService for the pref service, you could use
nsContentUtils::GetBoolPref, since it caches the pref service.  (Or,
even better, nsContentUtils::AddBoolPrefVarCache.)

>+      : mFontEntry(aFontToLoad), mFontURI(aFontURI), mLoaderContext(aContext)
eek, a tab character
(and also an 80th column violation on the previous line)

>+MakeTempFileName(nsCOMPtr<nsIFile>& tempFile)

We tend to avoid nsCOMPtr references.  Prefer either an nsIFile**
out parameter or an already_AddRefed<nsIFile> return value.

That said, we also have better (and more secure) ways of creating
temporary files.  See nsIFile.createUnique.  Use that instead.

That said, it looks like nsDownloader::OnStartRequest has code to create
a temporary file already.  Maybe you can just depend on *that* code and
not do any of it yourself?

(I'm not exactly an expert on any of that, though.)

>+nsresult nsFontFaceLoader::Init()
Maybe put the return type on its own line, like the rest of the methods
in this file?

>+  rv = NS_OpenURI(mDownloader, nsnull, mFontURI, nsnull, nsnull, sameOriginChecker);

I think you should not be using NS_OpenURI.  Using a blocking stream,
and potentially spinning a nested event loop, is, I think, a bad idea.
See comment 50.

(bzbarsky might be a better person to ask about what you should do here
both for this and for the temporary file stuff)

Throughout this file, the if (gFontDownloaderLog) sections should be in
an #ifdef PR_LOGGING (which is turned on by FORCE_PR_LOG, or something
like that).  PR_LOGGING is equivalent to PR_LOG not being a no-op.

OnDownloadComplete:

+    if (aStatus == NS_OK) {

You should use NS_SUCCEEDED() rather than tests against NS_OK.  (Twice
in this function.)

The user font set's OnLoadComplete method's third argument is an
nsresult, but you're passing PR_TRUE and PR_FALSE to it, which seems
wrong.  (They both translate to success (per NS_SUCCEEDED(), which tests
the high bit), and PR_FALSE == NS_OK, but I don't think that's what you
want.)  But the code inside OnLoadComplete tests:
>+    PRBool errorLoading = (aDownloadStatus != NS_OK);
which makes what you do end up working, but it makes no sense.

+      ps->StyleChangeReflow();

Might be worth having a comment that the reflow happens asynchronously.
(This means they can coalesce, which is good.)  (In the old days,
StyleChangeReflow was synchronous.  I had to look...)

>+  // -- presContext --> presShell --> document

Not sure what this is saying.

>+  nsIPresShell *ps = loaderCtx->mPresContext->PresShell();
>+  if (!ps)
>+    return PR_FALSE;

If a getter doesn't have "Get" in the name, you don't need to null-check
it -- it's guaranteed non-null.  (There are places where we haven't
removed "Get"s that should be removed from names to follow this
convention, but the other way around should be safe.)

>+  nsFontFaceLoader *loader = new nsFontFaceLoader(aFontToLoad, aFontURI, aContext);

You should make this an nsRefPtr, and remove the |delete loader| below,
for two reasons:
 (1) the Init method passes |this| to NS_NewDownloader which could
 AddRef and then, in case of failure, release
 (2) the delete loader call below could be deleting something somebody
 still has a pointer to; you shouldn't delete reference-counted objects
(I think I had a third, but I forgot it.)

You also might want to make the main flow of the function un-indented,
by changing the if (loader) block to be an
  if (!loader)
    return PR_FALSE;
and then having what's in the block, unindented.

>+NS_IMPL_ISUPPORTS1(nsFontFaceLoader, nsIDownloadObserver)

We tend to put that right after the constructor and destructor, and
before the other methods.

nsFontFaceLoader.h should use NS_DECL_NSIDOWNLOADOBSERVER rather than
writing out the OnDownloadComplete declaration.

Both nsFontFaceLoader.h and .cpp have a bunch of 80th-column violations;
I'd prefer not to have those in layout code.  Also, no need for blank
lines at the end of the files (but you should have a line terminator at
the end of the last line).

The pref name "gfx.downloadable_fonts.same-site-origin.enabled" seems a
bit confusing to me.  What do true and false do?  Is it possible to have
a name that makes it clearer what the pref does?  Maybe call it
"gfx.downloadable_fonts.enforce_same_site_origin"?

For both preferences, you should probably have code to observe dynamic
changes to the preferences, although it's not an absolute requirement.
(See comment about nsContentUtils::AddBoolPrefVarCache above, which
makes it easy to do for ones in content or layout.)

Comment 78

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, I didn't do a whole lot of thinking about the data structures I saw when I was reviewing.  There are a lot of data structures here; it might be nice to have a summary somewhere with brief descriptions of their purposes and lifetimes.  (Some of them have comments describing their purpose already.)

Comment 79

[Nicolas Spalinger]

The data structures of this object should also take into account the font metadata: we need that metadata to be parsed from the downloaded font file and then exposed to the user via a small status bar icon and corresponding dialog and/or an extra tab in Page Info (See earlier RFE thttps://bugzilla.mozilla.org/show_bug.cgi?id=131080 on this ).

There are various descriptive fields that can usefully be shown when present in the font: copyright, version, trademark, manufacturer, designer, description, vendor URL, designer URL, license, license URL. See the OT spec on the name table: http://partners.adobe.com/public/developer/opentype/index_name.html  Making the URL fields clickable would be ideal.

Creative Common's MozCC is a good example: http://wiki.creativecommons.org/MozCC

Then based on that information the browser can indicate to the user if the licensing of the corresponding font is legit or not: See the examples in the WIP spec: http://dev.w3.org/csswg/css3-fonts/#appendix0

The Open Font License already provides CC-style icons to represent its working model: http://scripts.sil.org/OFL#9ccf5052  more combinations could be created and shown depending on the font metadata.

Comment 80

[John Daggett (:jtd)]

(In reply to comment #77)
> >+  rv = NS_OpenURI(mDownloader, nsnull, mFontURI, nsnull, nsnull, sameOriginChecker);
> 
> I think you should not be using NS_OpenURI.  Using a blocking stream,
> and potentially spinning a nested event loop, is, I think, a bad idea.
> See comment 50.
> 
> (bzbarsky might be a better person to ask about what you should do here
> both for this and for the temporary file stuff)

I looked over NS_OpenURI and switched to using nsIDownloader based on the comment in nsNetUtil.h (the one you copied in comment 50):

// Use this function with CAUTION. It creates a stream that blocks when you
// Read() from it and blocking the UI thread is a bad idea. If you don't want
// to implement a full blown asynchronous consumer (via nsIStreamListener) look
// at nsIStreamLoader instead.

Since nsIDownloader is a subclass of nsIStreamListener I thought I was doing this async but I definitely might be wrong.  If this class isn't doing the download async than we probably also need to change the jar downloading code as this is using the same code (with the same call to NS_OpenURI).

The problem described in comment 48 doesn't occur anymore but that's by no means proof of anything.

bz, comments?

Comment 81

[John Daggett (:jtd)]

(In reply to comment #77)
> That said, it looks like nsDownloader::OnStartRequest has code to create
> a temporary file already.  Maybe you can just depend on *that* code and
> not do any of it yourself?
> 
> (I'm not exactly an expert on any of that, though.)

Yeah, but then I have to do some fancy dancing because of two things:

(1) the mac font api for loading font files demands a file with a .ttf extension
(2) to keep around the temp or cache file you need to keep the downloader object around

So it makes the code simpler to set up and manage just a file reference.  See comment 58 for more details related to this.

Comment 82

[Boris Zbarsky [:bzbarsky]]

Yeah, passing an nsIDownloader (and a context, null in this case, before the URI) to NS_OpenURI means you're calling the async version.  It's sad that they're named the same, but there we are.

Comment 83

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #82)
> Yeah, passing an nsIDownloader (and a context, null in this case, before the
> URI) to NS_OpenURI means you're calling the async version.  It's sad that
> they're named the same, but there we are.

OK, sounds good.  I didn't realize that there were different versions of NS_OpenURI and only some of them were evil.

Comment 84

[John Daggett (:jtd)]

Attachment: patch with weird header problems

For josh, causes a problem in widget/src/cocoa code, something to do with the ObjC error macros.

Comment 85

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #77)
> In nsPresContext, could you make mUserFontSet a
> nsRefPtr<gfxUserFontSet>?  Then you can avoid the manual reference
> counting in SetUserFontSet and the changes to the constructor and
> destructor.  It also seems like GetUserFontSet and SetUserFontSet could
> be inline (unless #include-ing gfxUserFontSet.h in nsPresContext.h
> triggers bad include dependencies, although it looks like it already
> pulls in thebes headers).

If this involves as much pain as it looks like it has, you're probably better off not doing it; I don't think it's critical that these methods be inline.

Comment 86

[John Daggett (:jtd)]

Yeah, I may end up doing that but the error I'm seeing is something *really* weird and makes me a little concerned about our ObjC exception handling code.  I just want to make sure I haven't uncovered some nasty lurking bug there.

Comment 87

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    PRPackedBool     mIsSVG       : 1;
+    PRPackedBool     mIsUserFont  : 1;
+    PRPackedBool     mIsProxy     : 1;
+    PRPackedBool     mIsValid     : 1;

Document these? It might be best to leave mIsSVG out until we actually support SVG fonts. In fact, I think here we shouldn't be talking about SVG, just "non-native font" (i.e. a font where shaping and rasterization are implemented by Gecko, not the platform).

 class THEBES_API gfxFontUtils {
 
 public:
-
+    

Don't add trailing whitespace.

+    PRBool                 isLocal;       // url or local

PRPackedBool. Doesn't make a real difference but PRBool in structs is bad form.

+    nsCOMPtr<nsIURI>       uri;           // uri if url !!!! this really needs to be a nsCOMPtr but this causes test code not to compile, ignore for now

Confusing comment since that *is* an nsCOMPtr
+    PRBool                 isLocal;       // url or local
+    nsString               localName;     // full font name if local
+    nsCOMPtr<nsIURI>       uri;           // uri if url !!!! this really needs to be a nsCOMPtr but this causes test code not to compile, ignore for now
+    PRUint32               formatFlags;   // format hint if url

Use "m" prefix please.

+    PRUint32               formatFlags;  // opentype, truetype, svg, etc. (if known)

What is the interpretation of this field? Give the enum in gfxUserFontSet a name and use it here, or move it somewhere it can be shared?

+    PRUint32               formatFlags;  // opentype, truetype, svg, etc. (if known)
+    nsCOMPtr<nsIFile>      fontFile;     // file containing font data
+    nsCOMPtr<nsISupports>  downloader;   // need to a ref to this to prevent file from being deleted

Use "m" prefix.

A lot of your lines are way longer than 80 characters.

Could we --- should we --- get rid of LoaderContext and just make it part of gfxUserFontSet, so that mLoaderProc is just a virtual method on gfxUserFontSet that the subclass overrides?

+    PRPackedBool                           mIsLoading;
+    nsTArray<gfxFontFaceSrc>               mSrcList;
+    PRUint32                               mSrcIndex;       // index of the src item loading
+    PRPackedBool                           mItalicUnknown;  // base class only stores italic or not, need to handle "unknown"
+    gfxMixedFontFamily*                    mFamily;

Put the PRPackedBools at the end with the PRUint32 just before them.

+    // counter used to make generation values unique per font set
+    static PRUint16 sFontSetCounter;

I don't get it ... we could easily create more than 2^16 gfxUserFontSets during the lifetime of the app.

+    gfxUserFontData* mUserFontData;

nsAutoPtr<gfxUserFontData>?

More to come ... I want to understand whether we need to download to a file on all platforms

Comment 88

[John Daggett (:jtd)]

(In reply to comment #87)
> +    // counter used to make generation values unique per font set
> +    static PRUint16 sFontSetCounter;
> 
> I don't get it ... we could easily create more than 2^16 gfxUserFontSets during
> the lifetime of the app.

The upper half-word of the user font set generation uses this value. The generation is used as part of the word cache key, this keeps text runs in the word cache per-document when user font sets are involved. So the question is really can you imagine a scenario under which a text run with a given generation is still in the word cache 65K+ user font sets later?  If so, we need to bump the size of the user font set generation to say, 64 bits.  I'll take a look again but that didn't seem like a remote possibility when I first thought about it.

Comment 89

[John Daggett (:jtd)]

Attachment: patch, v.0.9n, fix most of dbaron's issues

Still need to fix a few more of the problems noted in comment 77.  Will fix up these along and the things roc noted, except that handling the enabling/disabling of stylesheets is going to take a bit of thought.  This may be work better suited for a follow-on blocker bug, this patch is getting a tad large.

As for using nsRefPrt<gfxUserFontSet> in nsPresContext.h, that causes some wild-n-crazy Obj-C exception code compiler spoo when compiling mac widget code (which is otherwise completely unaffected by this patch). Josh is gonna take a look tomorrow.

Comment 90

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    bool useFontGroup = (fontGroup->GetUserFontSet() != nsnull);

PRBool (occurs more than once)

+            if (gUserFontsLog) {
+                nsCAutoString fontURI;
+                pe->mSrcList[pe->mSrcIndex].uri->GetSpec(fontURI);
+                
+                nsAutoString filePath;
+                aFontData.fontFile->GetPath(filePath);
+                
+                PR_LOG(gUserFontsLog, PR_LOG_DEBUG, ("userfonts (%p) [src %d] loaded uri: (%s) file: (%s) for (%s) gen: %8.8x\n", 
+                           this, pe->mSrcIndex, fontURI.get(), NS_ConvertUTF16toUTF8(filePath).get(), NS_ConvertUTF16toUTF8(pe->mFamily->Name()).get(), mGeneration));
+            }

Would be clearer if this was all #ifdef PR_LOGGING

It looks like we don't increment the generation counter when the user font set changes due to style rules being added or removed. Shouldn't we?

I'm not sure what causes textruns cached in nsTextFrame to be regenerated when a font has loaded. We don't actually change any style, so nsTextFrame::DidSetStyleContext doesn't kick in. How does this work?

It's not clear why nsFontFaceLoader *and* gfxPlatformMac both need to create a temporary .ttf file. I need to look closer at this.

Can you just explain, or better still document somewhere, why it's necessary on Windows to have a temporary file at all?

Comment 91

[John Daggett (:jtd)]

Attachment: patch, v.0.9o, updated to latest

Update to latest, fixed problem with italic handling

Try server builds:
http://tinyurl.com/475kyc

Comment 92

[Bill Gianopoulos [:WG9s]]

First the good news!

With this patch and all the other pending patches I can create a Windows build that scores 97/100 and has no X in the upper right.

Now the not-so-good news.

The build gets this error during linking:

c:\mozilla\mozilla2\gfx\thebes\src\gfxfontutils.cpp(1005) : warning C4715: 'gfxFontUtils::MakeEOTHeader' : not all control paths return a value

This function is supposed to return an nsresult, but in the normal case just falls off the bottom.

And then the bad news.

This patch does not work for Acid3 under Linux, either with my build or with the try server build.

Comment 93

[Zack Weinberg (:zwol)]

> This function is supposed to return an nsresult, but in the normal case just
> falls off the bottom.

Looks like there should be a "return NS_OK;" there.

Comment 94

[Gabriele Best [:Gabri]]

With the last patch, this text http://www.alistapart.com/d/cssatten/heid.html is not bolded anymore.

Comment 95

[Bill Gianopoulos [:WG9s]]

(In reply to comment #92)
> This patch does not work for Acid3 under Linux, either with my build or with
> the try server build.

Is this because of bug 449356?

Comment 96

[philippe (part-time)]

2 issues I noticed (all tested OS X 10.5.5 Intel):
* on the Acid 3 test, there remains a tiny red fuzzy line at the top of the square. I suspect it is due to font-smoothing on the Ahem font and not strictly related to @font-face. I've noticed before in test cases that WebKit and Opera render Ahem crisper than Gecko. This is probably OS X specific.

* 'ex' unit
The font specified via @font-face is ignored (fall-back font is used) when at least the following properties: border, padding, margin, text-shadow, width & height have values specified with ex units (ex. padding: 2ex; border: 1ex solid;). But font-size works correctly.
Oddly, it works correctly for Japanese font(s) for every property mentioned.

WebKit uses the specified font, but completely ignores the property value (uses the initial value).


(In reply to comment #94)
> With the last patch, this text http://www.alistapart.com/d/cssatten/heid.html
> is not bolded anymore.
Works fine here with the latest try-server build (OS X 10.5.5)

Comment 97

[Bill Gianopoulos [:WG9s]]

(In reply to comment #96)
> 2 issues I noticed (all tested OS X 10.5.5 Intel):
> * on the Acid 3 test, there remains a tiny red fuzzy line at the top of the
> square. I suspect it is due to font-smoothing on the Ahem font and not strictly
> related to @font-face. I've noticed before in test cases that WebKit and Opera
> render Ahem crisper than Gecko. This is probably OS X specific.

This is actually the issue this part of the test was designed to be testing.    The @font-face stuff was just a method to have the test work without requiring you to install the Ahem font (and of course ended up testing @font-face support at the same time).  The point of the test was to make sure the character aligned perfectly with the red box and completely obliterated it.

There should be one pixel or white between the red box and the black border.  If this is absent, then the red box is misaligned.  If it is present then the Ahem character is either positioned or sized incorrectly, which would still be a font issue, but probably irrespective of @font-face.

Comment 98

[Boris Zbarsky [:bzbarsky]]

The Ahem font-smoothing issue is known.  Basically, the test is making a bad assumption.  Webkit hardcodes the Ahem font to be treated differently from all other fonts just so they can "pass".  No idea what Opera does.

Comment 99

[Bill Gianopoulos [:WG9s]]

(In reply to comment #98)
> The Ahem font-smoothing issue is known.  Basically, the test is making a bad
> assumption.  Webkit hardcodes the Ahem font to be treated differently from all
> other fonts just so they can "pass".  No idea what Opera does.

Actually this was already fixed in the test, and supposedly webkit has taken the hardcoding out.  That is where the 1 pixel of white space between the red box and the black border came frrm.  any font smoothing is supposed to be confined to this 1 pixel region.

Comment 100

[Bill Gianopoulos [:WG9s]]

I should have included this link in my last comment.

http://ln.hixie.ch/?start=1206756775&count=1

Comment 101

[philippe (part-time)]

(In reply to comment #99)

Yes, WebKit is supposed to have removed their hack. I think they really did it.

I did mention the acid 3 issue here, because I'm not sure if the small reddish line I see on acid 3 is due to font-smoothing or possibly due to something else, related to @font-face (timing related to loading the font, causing some metrics to be off - e.g line-box sizing).
My eyes are not good enough to visually inspect 1px thin lines, even magnified with Pixie.

If the issue is font-smoothing, then it is _not_ a problem with @font-face perse and this particular discussion should move elsewhere. My local copy of Ahem equally shows slightly less crisp on Gecko then it does on WebKit and Opera, on the same machine, in tests unrelated to acid 3.

Comment 102

[philippe (part-time)]

Attachment: screenshot, red line in comment 96-100

For reference, the red line, with Pixie magnification

Comment 103

[Bill Gianopoulos [:WG9s]]

I would recommend developing a minimized testcase with only the border the red box and the Ahem character.  I would also download the Ahem font and make the testcase not be dependent on @font-face.  Get that to duplicate the issue and file a new bug and make that bug block the Acid3 meta-bug.  That way this won;t cloud the @font-face issue and permit working on the issue before the @font-face stuff lands.  I would do this myself, but I do not have access to a MAC.

Comment 104

[Bill Gianopoulos [:WG9s]]

(In reply to comment #91)
> Created an attachment (id=339414) [details]
> patch, v.0.9o, updated to latest

This patch no longer applies cleanly.

Comment 105

[John Daggett (:jtd)]

(In reply to comment #92)

> This patch does not work for Acid3 under Linux, either with my build or with
> the try server build.

The current patch only implements @font-face for Win/Mac.  Our Linux code requires some additional work to support user fonts (see bug 449356).

(In reply to comment #94)

> With the last patch, this text http://www.alistapart.com/d/cssatten/heid.html
> is not bolded anymore.

WFM with try-server build noted in comment 91, Win/Mac.  Renders same as Safari 3.1.  If you're still seeing the problem, could you post a screenshot along with platform info?  Not sure where you were expecting bold, the stylesheet (heid.css) doesn't seem to specify the font-weight anywhere.

Comment 106

[John Daggett (:jtd)]

(In reply to comment #77)
> So these changes don't reflect that style sheets can be enabled and
> disabled dynamically.  It looks like what you're doing is that once a
> sheet is enabled, you're adding it to the user font set.  How you want
> to solve this depends on things such as how expensive the gfxUserFontSet
> objects are to keep around (e.g., in memory use), and whether there are
> performance problems (e.g., cache loss) that would result from
> destroying a gfxUserFontSet and then immediately building a new one that
> was similar or identical.  It's a little bit tricky, since:
>  * there are multiple nsCSSRuleProcessor objects, one for each level of
>    the cascade (user-agent sheets, user sheets, author sheets)
>  * each nsCSSRuleProcessor might cache some old RuleCascadeData objects
>    in addition to the current one
>  * some dynamic changes cause us to rebuild the nsCSSRuleProcessor for a
>    cascade level; some dynamic changes cause us to clear and rebuild the
>    RuleCascadeData; some cause us only to pick a different
>    RuleCascadeData without necessarily rebuilding anything.
> If you're interested, I could probably come up with slightly more
> detailed suggestions if I had a better idea of what constraints you were
> dealing with.

This is an oversight on my part, I failed to consider this case.  It's a bit tricky to do, we'll need to set up an interface to enable/disable specific font faces.  Diabolical example: same family, some faces created in one sheet, other faces created in a separate sheet, disable stylesheet needs to only disable some faces not all.

I think this would be better to handle in a follow-on, blocking bug.

Comment 107

[John Daggett (:jtd)]

Attachment: patch, v.0.9p, more cleanup

Fixup merge with latest, more cleanup based on dbaron and roc comments.  Fixed up the MakeEOTHeader warning, now returns NS_OK correctly.

Comment 108

[Gabriele Best [:Gabri]]

Attachment: patch 0.9p vs Safari

I use WinXP with SP2.
Firefox font rendering vs Safari is a bit different.

Comment 109

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #106)
> This is an oversight on my part, I failed to consider this case.  It's a bit
> tricky to do, we'll need to set up an interface to enable/disable specific font
> faces.  Diabolical example: same family, some faces created in one sheet, other
> faces created in a separate sheet, disable stylesheet needs to only disable
> some faces not all.
> 
> I think this would be better to handle in a follow-on, blocking bug.

Having an interface to disable/enable faces doesn't feel right to me, given that the rest of the style system code works by not processing sheets that are disabled.  It would actually be somewhat tricky, since in the normal processing we simple don't handle sheets that are disabled; now we'd need to search them, and their @imports, for @font-face rules that were enabled before but now need to be disabled, while being careful not to disable something that's duplicated in an enabled style sheet.

You'll also definitely need to be able to handle adding faces dynamically if the author loads an additional style sheet after the document loads.


The simplest approach would be to just rebuild the gfxUserFontSet object from scratch, just like we do for a newly loaded page.  Is the performance or code complexity (on the gfx side) of doing this really that horrible?  (Note that these codepaths aren't tremendously performance-sensitive; they're ones where we're already redoing all CSS cascading and rule matching, which isn't trivial.)

Comment 110

[John Daggett (:jtd)]

(In reply to comment #109)
> The simplest approach would be to just rebuild the gfxUserFontSet object from
> scratch, just like we do for a newly loaded page.  Is the performance or code
> complexity (on the gfx side) of doing this really that horrible?  (Note that
> these codepaths aren't tremendously performance-sensitive; they're ones where
> we're already redoing all CSS cascading and rule matching, which isn't
> trivial.)

This sounds like the best approach for now.  Certain types of stylesheets will be affected by this more than others, specifically stylesheets that reference a huge per-site list of available fonts and only use a selected subset of those.  A new user font set means the hashtable of font data is going to be recreated from scratch.  The big performance hit will be in reloading and recreating the platform font but even that's not the end of the world, since the cache will probably have most of the font data cached locally.  It may also be a win to implement better caching of created fonts across pages, so that we don't reload a given font used across multiple pages.

Comment 111

[John Daggett (:jtd)]

(In reply to comment #108)
> I use WinXP with SP2.
> Firefox font rendering vs Safari is a bit different.

Webkit uses it's own, definitely-not-open-source font rendering library (CoreGraphics on Windows?).  That's why you see the difference.  If you download and install the fonts on the page locally, then view the page in FF3 without downloadable fonts enabled, you should see the same rendering differences.

Comment 112

[John Daggett (:jtd)]

Nice example page containing lots of .otf fonts:

  http://opentype.info/demo/webfontdemo.html

From Ralf Herrmann's blog:

  http://opentype.info/blog/

Renders nicely on Mac but on Windows none of the .otf fonts load.  Webkit loads them on both platforms.

Comment 113

[philippe (part-time)]

(In reply to comment #112)
 
>   http://opentype.info/demo/webfontdemo.html
... 
> Renders nicely on Mac but on Windows none of the .otf fonts load.  Webkit loads
> them on both platforms.

On my Mac(s) (10.5.5/intel) none of the opentype fonts rendered from Ralph's server.
I saved the file (+fonts) on my local dev server: same problem.

I then commented out the 'format("opentype")' part in the stylesheet and then the page rendered as intended.
I had noted a similar issue in comment 74 above about the Axis fonts (jpn).

Comment 114

[John Daggett (:jtd)]

(In reply to comment #113)
This should be fixed in the v.0.0p patch.  I just verified that this works on a similar configuration.  If you don't want to build it yourself, I just did a try server build:

  http://tinyurl.com/3gx6np

Let me know if that fails in your environment.

Comment 115

[Simon Montagu :smontagu]

Tests from the W3C internationalization working group: http://www.w3.org/blog/International/2008/09/23/new_tests_web_fonts

Comment 116

[philippe (part-time)]

(In reply to comment #114)
> (In reply to comment #113)
> This should be fixed in the v.0.0p patch.  I just verified that this works on a
> similar configuration.  If you don't want to build it yourself, I just did a
> try server build:
> 
>   http://tinyurl.com/3gx6np
> 
> Let me know if that fails in your environment.

Works perfectly now.

Comment 117

[John Daggett (:jtd)]

(In reply to comment #115)
> Tests from the W3C internationalization working group:
> http://www.w3.org/blog/International/2008/09/23/new_tests_web_fonts

Results (using try server build listed in comment 114).  Failure means "doesn't match the reference graphic", not necessarily "doesn't render correctly".  

1. Georgian pass
2. Armenian pass
3. Khmer 1 fail
4. Khmer 2 fail
5. Hindi 1 fail (font didn't load on Windows)
6. Hindi 2 fail (same)
7. Arabic 1 fail Mac, pass Windows
8. Arabic 2 fail Mac, pass Windows
9. Urdu fail Mac, pass Windows
10. Thai pass (although rendering on Mac looks a tad odd)
  Tibetan fail Mac, renders incorrectly on Windows
  Burmese fail Mac, pass Windows

Most of these use OpenType fonts so our Mac OS X renderings are going to fallback.

Khmer is a known issue (bug 294579).  And since all of the fonts used for complex scripts are OpenType fonts, failing on the Mac makes sense.  We do at least fall back to fonts that render correctly except as noted below.

These are bugs/problems:

* Khmer (all platforms)
* Hindi fonts don't load on Windows
* Thai Mac rendering of vowel markers(?) seem a bit high -- is this us or ATSUI?
* Tibetan on Windows doesn't fallback, so doesn't render correctly
* Burmese (Mac)

Comment 118

[Kevin Newman]

Using the build from comment #114 :

http://tinyurl.com/3gx6np
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b1pre) Gecko/20080924001604 Minefield/3.1b1pre

to look at page:
http://opentype.info/demo/webfontdemo.html

did _not_ work for me on Windows Vista Business x64.

Comment 119

[Daniel Bennett]

Also using the build at 
http://tinyurl.com/3gx6np
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b1pre) Gecko/20080924001604 Minefield/3.1b1pre
and two previous builds

I have been unable to see fonts downloaded at several sites including:
http://opentype.info/demo/webfontdemo.html
and
http://www.alistapart.com/d/cssatten/heid.html

I am using Vista Home Premium with 3GB with Q6600 processor.

I compared with Safari 3.1 on all pages to confirm.

Comment 120

[philippe (part-time)]

(In reply to comment #118)
and (In reply to comment #119)

* John explicitly mentions in comment 112 that
http://opentype.info/demo/webfontdemo.html
doesn't work on Windows.

* for the AlistApart website, you have to change the pref
gfx.downloadable_fonts.same-site-origin.enabled
in about:config
(the fonts are hosted on a different server/domain

Comment 121

[Kevin Newman]

* John mentions in Comment #114 that the linked build _should_ work, but it doesn't.

Comment 122

[John Daggett (:jtd)]

(In reply to comment #121)
> * John mentions in Comment #114 that the linked build _should_ work, but it
> doesn't.

Kevin, pages with .otf fonts work on Mac but not on Windows.  Philippe noted a problem when the format("opentype") hint was used, this has been fixed.

Sorry for not being clearer.

Comment 123

[John Daggett (:jtd)]

(In reply to comment #120)
> * for the AlistApart website, you have to change the pref
> gfx.downloadable_fonts.same-site-origin.enabled
> in about:config
> (the fonts are hosted on a different server/domain

Based on review comments, this was changed to:

gfx.downloadable_fonts.enforce_same_site_origin

Enable that and the A List Apart examples should work, both Mac and Win.  Linux, not yet.

Comment 124

[John Daggett (:jtd)]

Oops, substitute "Disable" for "Enable" in the comment above.

Comment 125

[John Daggett (:jtd)]

(In reply to comment #90)

> I'm not sure what causes textruns cached in nsTextFrame to be
> regenerated when a font has loaded. We don't actually change any
> style, so nsTextFrame::DidSetStyleContext doesn't kick in. How does
> this work?

When the font download completes and a platform font is successfully constructed, PresShell::StyleChangeReflow gets called.  This in turn causes existing text runs to get flushed.  Here's the stack with associated code:

#0  nsTextFrame::ClearTextRun (this=0xe13724) at /builds/mozcentral/layout/generic/nsTextFrameThebes.cpp:3567
#1  0x114cefc5 in nsTextFrame::MarkIntrinsicWidthsDirty (this=0xe13724) at /builds/mozcentral/layout/generic/nsTextFrameThebes.cpp:5267
#2  0x11400fec in PresShell::FrameNeedsReflow (this=0xcf6a00, aFrame=0xe11168, aIntrinsicDirty=eStyleChange, aBitToAdd=1024) at /builds/mozcentral/layout/base/nsPresShell.cpp:3248
#3  0x113fb2e2 in PresShell::StyleChangeReflow (this=0xcf6a00) at /builds/mozcentral/layout/base/nsPresShell.cpp:2989
#4  0x1157b73b in nsFontFaceLoader::OnDownloadComplete (this=0x18b47360, aDownloader=0x1c5762d0, aRequest=0x18b475e0, aContext=0x0, aStatus=


PresShell::StyleChangeReflow()

  return FrameNeedsReflow(rootFrame, eStyleChange, NS_FRAME_IS_DIRTY);
  

PresShell::FrameNeedsReflow

  if (aIntrinsicDirty == eStyleChange) {
    // Mark all descendants dirty (using an nsVoidArray stack rather than
    // recursion).
    nsVoidArray stack;
    stack.AppendElement(aFrame);

    while (stack.Count() != 0) {
      nsIFrame *f =
        static_cast<nsIFrame*>(stack.FastElementAt(stack.Count() - 1));
      stack.RemoveElementAt(stack.Count() - 1);

      PRInt32 childListIndex = 0;
      nsIAtom *childListName;
      do {
        childListName = f->GetAdditionalChildListName(childListIndex++);
        for (nsIFrame *kid = f->GetFirstChild(childListName); kid;
             kid = kid->GetNextSibling()) {
          kid->MarkIntrinsicWidthsDirty();
          stack.AppendElement(kid);
        }
      } while (childListName);
    }
  }
  

nsTextFrame::MarkIntrinsicWidthsDirty

  ClearTextRun();
  nsFrame::MarkIntrinsicWidthsDirty();


> Can you just explain, or better still document somewhere, why it's
> necessary on Windows to have a temporary file at all?

At this point, a file isn't needed, it's dumped as soon as the font is loaded.  But I'd prefer to keep this as is for now, until we figure out how to get OTF font (Postscript-style CFF OpenType fonts) loading correctly under Windows.

Comment 126

[John Daggett (:jtd)]

Attachment: patch, v.0.9q, cleanup logging

Patch for review, cleans up various things based on dbaron, roc comments.

Remaining issues:

* rebuild user font set object when stylesheet enable/disable occurs
* .otf fonts don't load under Windows (possibly an API limitation?)
* better temp file name scheme (nsIFile::CreateUnique?)
* rather than full reflow, selectively update based on which text 
  runs are affected by user font set changes

At this point we don't have a DOM API for adding/removing @font-face rules (bug 443978) so I'm going to skip handling that for now.

hg repository containing changes:

  http://hg.mozilla.org/users/jdaggett_mozilla.com/downloadablefonts/

Comment 127

[Daniel Bennett]

Thanks. I had saw the config note and toggled it. After that point the A List Apart examples worked perfectly. Is there a list of test pages with all the variations that need to be tested? If not should we put some together?

Daniel

Comment 128

[John Daggett (:jtd)]

Latest try server build:

  http://people.mozilla.com/~jdaggett/webfontslatest

This will redirect to the latest try server build and to the latest nightly once this is checked in.

> Is there a list of test pages with all the
> variations that need to be tested? If not should we put some together?

There isn't.  One of the summer interns, Akira, worked on a bunch of reftests, they're attached to bug xxx.  We still need to work out the legalities of checking in specific fonts, there are several bugs pending on individual fonts.

Any more tests you can come up with would be a great help.

Comment 129

[philippe (part-time)]

A test case for the issue with 'ex' units I mentioned in comment 96:
http://l-c-n.com/index-test.html

http://l-c-n.com/ uses basically the same stylesheet, but using 'em' units
(there is one difference between the 2: the real page use 'graublau', the test page use 'diavlo', which is really different from 'Helvetica Neue', the fall-back font)

---
PS - the PrinceXml test cases linked in bug 70132 show an issue with absolute positioning and -moz-column. That is unrelated to @font-face and is filed as bug 454749.
(http://www.princexml.com/howcome/2008/webfonts/)

Comment 130

[John Daggett (:jtd)]

Did some fiddling around with .otf font support on Windows today.  Poking around on the Microsoft support site, I noticed the KB below stating that .otf fonts are not supported, even in the latest version of Microsoft Office:

  KB 908475 You cannot embed an Adobe OpenType font in a document in an Office program
  http://support.microsoft.com/kb/908475

The curious thing is that you can save documents in Word that reference .otf fonts, it's obviously embedding the font (the size changes dramatically) but the fonts fail to display when opened on a machine without the underlying font.  For .ttf fonts, the embedded fonts render fine.  

Webkit also recently switched to supporting GDI font handling and in their latest Windows builds, they also don't appear to support .otf fonts:

  https://bugs.webkit.org/show_bug.cgi?id=21127

Note: with Safari 3.1.2, downloaded .otf fonts render just fine.

Same problem for Chrome.

I suspect this may be a limitation of the TTLoadEmbeddedFont API, I'm going to fiddle around a bit more with that.

Comment 131

[Robert O'Callahan (:roc) (email my personal email if necessary)]

IMHO we should not worry about OTF on Windows right now, but focus on getting the current patch landed and delve into OTF issues later.

Comment 132

[Pedro Sland]

(In reply to comment #131)
> IMHO we should not worry about OTF on Windows right now, but focus on getting
> the current patch landed and delve into OTF issues later.

I personally do not agree. Most users run windows (and the linux version is also broken at the moment so it is not as if it is win that is holding things back). Partial support isn't much use to anyone (why I moved from IE) so until a workaround is found there is little point (in my opinion) putting this patch into the release until the OTF issues are fixed.

Comment 133

[Bill Gianopoulos [:WG9s]]

(In reply to comment #132)
> (In reply to comment #131)
> > IMHO we should not worry about OTF on Windows right now, but focus on getting
> > the current patch landed and delve into OTF issues later.
> 
> I personally do not agree. Most users run windows (and the linux version is
> also broken at the moment so it is not as if it is win that is holding things
> back). Partial support isn't much use to anyone (why I moved from IE) so until
> a workaround is found there is little point (in my opinion) putting this patch
> into the release until the OTF issues are fixed.

Actually, Roc's approach does make sense.

1. Clean this up and get it landed and mark this bug as fixed.

2. Open a new bug on @font-face not working under Windows with OTF fonts.

3. Open a new bug on @font-face not working under Linux.  Make that bug dependent on bug 449356? and make it block bug 410460 (the Acid3 tracking bug)

Comment 134

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+++ mozcentral/gfx/thebes/public/gfxFontTest.h	2008-09-22 10:28:39.000000000 +0900
@@ -39,16 +39,17 @@
 #define GFX_FONT_TEST_H
 
 #include "nsString.h"
 #include "nsTArray.h"
 
 #include "cairo.h"
 
 #include "gfxFont.h"
+#include "gfxUserFontSet.h"

So should we #include gfxUserFontSet.h in gfxFont.h?

+// after download info needed to initialize platform font
+// platform-specific code is responsible for cleaning out font data and
+// managing cache token, since file may need to persist during use of
+// font depending upon semantics of platform font API
+// lifetime: time during which font is downloaded and active

This comment is hard to read because it's unclear whether the second line follows on from the first line or not. Add punctuation and/or grammar :-)

+                                const gfxFontStyle& aFontStyle, PRBool& needsBold);

aNeedsBold

+    mGeneration = ((PRUint32) sFontSetCounter) << 16;

Constructor cast

+    // format hint flags, union of all possible formats
+    PRUint32               mFormatFlags;

Please say where these flags are defined (or define them here).

+    // owned by user font set obj, deleted within destructor
+    LoaderContext*  mLoaderContext;

nsAutoPtr?

+        else if (metrics.ntmFlags & (NTM_PS_OPENTYPE))
+        else if (metrics.ntmFlags & (NTM_TT_OPENTYPE))

Parens around those symbols are unnecessary

-#define DUMP_TEXT_RUNS
+//#define DUMP_TEXT_RUNS

Trunk has this commented out, so I'm not sure what your diff is against.

+    SanitizeMetrics(&mMetrics, (PRBool) (GetFontEntry()->mIsBadUnderlineFont));

Surely you don't need that PRBool cast

+
 gfxAtsuiFontGroup::gfxAtsuiFontGroup(const nsAString& families,
-                                     const gfxFontStyle *aStyle)
-    : gfxFontGroup(families, aStyle)
+                                     const gfxFontStyle *aStyle,
+                                     gfxUserFontSet *aUserFontSet)
+    : gfxFontGroup(families, aStyle, aUserFontSet)
 {
     ForEachFont(FindATSUFont, this);
+    

Lose the extra blank lines

+    if (fs && (gfe = fs->FindFontEntry(aName, *fontStyle, needsBold))) {
+        // assume for now platform font if not SVG
+        fe = static_cast<MacOSFontEntry*> (gfe);

So here you're assuming that SVG fonts are not implemented? Is that wise, or should we have some extra check here?

In gfxFont.h:
+    gfxUserFontSet* mUserFontSet;

Could this be nsRefPtr<gfxUserFontSet> if we #included gfxUserFontSet.h here?

Why do we need to store mUserFontSetGeneration in gfxTextRun? Is it to validate the textruns in the textrun cache?

+    PRBool isExists, isReadable;
+    nsresult rv = localFile->Exists(&isExists);
+    if (NS_FAILED(rv))
+        return nsnull;
+        
+    if (!isExists)
+        return nsnull;
+    rv = localFile->IsReadable(&isReadable);
+    if (NS_FAILED(rv) || !isReadable)
+        return nsnull;

Do we need these checks? Can't we just try to open the file and return null if that fails?

+//   PRUint8  name[size];  // array of UTF-16 chars, english version of name record string

Shouldn't that be PRUnichar instead of PRUint8?

+// later versions of the EOT format add in fields between the fullName data and the font data

The rootString is already in that area so can you clarify the comment?

+gfxFontUtils::MakeEOTHeader(nsIFile *aFontData, PRUint8 *aHeader, PRUint32& aHeaderLen, PRBool& aIsCFF)

Prefer pointers for these out parameters.

+    PRUint32 fontDataSize = (PRUint32) (fileInfo.size);

Constructor cast

Should this Windows EOT code be put in its own file? Would be a bit cleaner to have a file full of EOT-only stuff that's built only on Windows.

+    offset = PR_Seek64(fd, (PROffset64)headOffset, PR_SEEK_SET);

Constructor cast

Probably worth defining a helper function that does a PR_Seek + PR_Read

Lots of (PRUint32) casts that should be constructor-style casts

+struct NameRecordData {
+    PRUint32  offset;
+    PRUint32  length;
+};

Explain what this is for.

+    NameRecordData names[EOTFixedHeader::EOT_NUM_NAMES];
+    PRBool familyFound = PR_FALSE, styleFound = PR_FALSE, versionFound = PR_FALSE, fullFound = PR_FALSE;

Seems like we could avoid these booleans and just preinitialize the lengths in 'names' to zero, then check for names of nonzero length to see if we found the right parts.

Having gfxFontUtils::MakeEOTHeader fill a fixed-size buffer and just abort if 2K isn't big enough seems a bit slack :-). Why not use an nsTArray<PRUint8> and use AppendElements() to extend it whenever you need more space?

Currently still going through MakeEOTHeader ... goodnight for now :-)

Comment 135

[Kevin Newman]

There is an alternative function on MSDN that at least meantions OTF file outlines: http://msdn.microsoft.com/en-us/library/ms536754(VS.85).aspx

Comment 136

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That function basically converts a TTF/OTF file into an EOT file, so it's not what we need here.

John, I'd make an EOT out of a Postscript OTF font and see if IE can use it. If it can't, I'd say we've got a platform bug here and just move on for now.

Comment 137

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Older comments not yet addressed:

Could we --- should we --- get rid of LoaderContext and just make it part of
gfxUserFontSet, so that mLoaderProc is just a virtual method on gfxUserFontSet
that the subclass overrides?

+    gfxUserFontData* mUserFontData;

nsAutoPtr<gfxUserFontData>?

Carrying on with new comments:

+    // append rootStringSize == 0
+    *eotEnd++ = 0;
+    *eotEnd++ = 0;
+    eotLength += 2;

You're missing a buffer length check here, but better to just go with nsTArray. With the array you won't need eotEnd/eotLength either, they'll be implicit in the array length.

+    for (i = 0; i < 10; i++)
+        eotHeader->panose[i] = os2Data.panose[i];

eotHeader->panose = os2Data.panose;

+        strOffset = nameOffset + (PRUint32) nameHeader.stringOffset + nameoff;
+        *((PRUint16*) eotEnd) = (PRUint16) strLen;
+        CopySwapUTF16((PRUint16*) eotEnd, (PRUint16*) eotEnd, (strLen >> 1));  // length is number of UTF-16 chars, not bytes

You use a lot of C-style casts. You shouldn't be using any.

+    mItalic = (aItalicStyle == FONT_STYLE_ITALIC || aItalicStyle == FONT_STYLE_ITALIC);

What did you want to write here? OBLIQUE or something?

Seems CleanupTempFonts will delete the font files for running profiles if the user launches a new profile. Does that matter? If ATSU can handle the font files being deleted (which it should, if it has them open), then we should remove the file after calling ATSFontActivateFromFileSpecification.

+            PRBool exists;
+            if (!NS_FAILED(mFontFile->Exists(&exists)) && exists) {
+                mFontFile->Remove(PR_FALSE);
+            }

Why not just Remove and ignore any error?

+    NSString *path = GetNSStringForString(filePath);

Do we leak this string along various error paths?

+    MacOSUserFontData *userFontData = new MacOSUserFontData(containerRef, 
+                                                            aFontData->mFontFile, 
+                                                            aFontData->mDownloader);

Need a null check here?

+                           (PRUint32) (aProxyEntry->mItalic ? 
+                                       FONT_STYLE_ITALIC : 
+                                       FONT_STYLE_NORMAL), 
+                           (gfxUserFontData *)userFontData)

Some C-style casts that should be constructor cast and static_cast

Please document why in gfxTextRunWordCache we have to use the fontgroup in the key --- I assume it's because the fontgroup's first font can change.

+    nsAutoString key(aFamilyName);
+    ToLowerCase(key);

What spec says that matching should be lowercase? A better approach would be to create an nsCaseInsensitiveStringHashKey so that this logic goes into the hashtable.

+gfxProxyFontEntry::gfxProxyFontEntry(const nsTArray<gfxFontFaceSrc>& aFontFaceSrcList, 
+             PRUint32 aWeight, 
+             PRUint32 aStretch, 
+             PRUint32 aItalicStyle, 
+             gfxSparseBitSet *aUnicodeRanges)

Add some kind of TODO comment to use aUnicodeRanges here.

+    mItalic = (aItalicStyle & 
+                 (FONT_STYLE_ITALIC | FONT_STYLE_OBLIQUE) ? PR_TRUE : PR_FALSE);

Use != 0 instead of PR_TRUE : PR_FALSE. Basically every use of PR_TRUE : PR_FALSE is wrong/ugly.

+    mGeneration = ((PRUint32) sFontSetCounter) << 16;

constructor cast

+    PRUint32 status;
+
+    status = LoadNext(pe);

Give the STATUS_ enum a name and use that type for 'status' and LoadNext.

The gfxUserFontSet generation counter can wrap around in 16 bits. This seems easily reachable especially for a malicious page.

IsFormatSupported should be a virtual method in gfxPlatform. Then you don't need Mac #ifdefs and platforms that don't support font downloading can just return PR_FALSE for everything.

+    aLogFont->lfItalic         = (aItalic ? TRUE : FALSE);

aItalic != 0

Your temporary file names are predictable. This could be a security hole if the downloader doesn't use the equivalent of O_EXCL to create the file. This needs to be checked.

Up to gfxWindowsFonts.cpp

Comment 138

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    PRBool              mFound;

PRPackedBool

+    nsString            mFullName;
+    nsString            mFamilyName;
+    HDC                 mDC;
+    gfxFontEntry        *mFontEntry;

Why the big indent?

+    FullFontNameSearch *data = (FullFontNameSearch*) userArg;

static_cast (at least 2 occurrences)

+        HDC hdc = GetDC(nsnull);
+        SetGraphicsMode(hdc, GM_ADVANCED);
+        data->mDC = hdc;

Why not check if mDC is already non-null and reuse it? And move the Release to LookupLocalFont.

+    HMODULE fontlib = LoadLibrary("t2embed.dll");

Can we avoid leaking the library?

+    HANDLE                 mFontRef;

Crazy indent

+class EOTFontStreamReader {

Comment what this class is for.

+        : mFontFile(aFontFile), mFd(nsnull), mOpenError(PR_FALSE), mInHeader(PR_TRUE), mHeaderOffset(0), mEOTHeader(aEOTHeader), mEOTHeaderLen(aEOTHeaderLen)

Crazy line length

+        nsresult rv = mFontFile->Exists(&isExists);
+        if (NS_FAILED(rv))
+            return PR_FALSE;
+            
+        if (!isExists)
+            return PR_FALSE;
+        rv = mFontFile->IsReadable(&isReadable);
+        if (NS_FAILED(rv) || !isReadable)
+            return PR_FALSE;
+
+        rv = mFontFile->OpenNSPRFileDesc(PR_RDONLY, 0, &mFd);
+        if (NS_FAILED(rv) || !mFd)
+            return PR_FALSE;

Why not just try to open the file? You'll error out if you can't read it.

+        (gfxWindowsFontType) (isCFF ? GFX_FONT_TYPE_PS_OPENTYPE : GFX_FONT_TYPE_TRUETYPE) /*type*/, 
+        (PRUint32) (aProxyEntry->mItalic ? FONT_STYLE_ITALIC : FONT_STYLE_NORMAL), w, winUserFontData);

Casts

Comment 139

[Robert O'Callahan (:roc) (email my personal email if necessary)]

John, what about tests here? Seems like it should be simple enough to at least write an Acid3-style reftest. It would be good to have a version of Ahem in our reftest directory anyway to use for reftesting other stuff.

Comment 140

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+      nsFontFaceLoaderContext *loaderCtx = new nsFontFaceLoaderContext(presContext);

Null check

+          if (valueString.LowerCaseEqualsASCII("opentype")) {

Are these keywords really supposed to be case-insensitive? That seems a little strange.

+        srcArray.AppendElement(face);

Would be a little more efficient to start with AppendElements(1) and fill in the face directly into the array.

Does it really make sense to mix all the format flags together? Seems like that could create confusion further down the stack.

+  if (!gEnforceSameSiteOrigin)
+    return PR_TRUE;

Seems like if this pref is false, we even allow an http:// page to load a file:// font. That's really bad and I think should be flat-out prohibited. But I can never remember the correct check for that, so get advice from bz or someone else.

+  return !NS_FAILED(rv);

return NS_SUCCEEDED(rv)

+  if (NS_FAILED(rv)) {
+    return PR_FALSE;
+  }
+
+  return PR_TRUE;

return NS_SUCCEDED(rv)

How do we cancel the download(s) if the prescontext goes away? We should probably also pause them if the presshell is frozen (i.e. goes into bfcache).

It looks to me like user fonts are only enabled for SVG and nsTextFrame here. I think this is a good thing, but we need to stay aware of it.

Phew! I'm finally done. I probably missed stuff but overall I think we're in pretty good shape.

Comment 141

[Zack Weinberg (:zwol)]

(In reply to comment #140)

> +          if (valueString.LowerCaseEqualsASCII("opentype")) {
> 
> Are these keywords really supposed to be case-insensitive? That seems a little
> strange.

Unless something explicitly says otherwise, one would presume the general principle from CSS2.1 holds:

4.1.3 Characters and case

The following rules always hold:

    * All CSS style sheets are case-insensitive, except for parts that are not under the control of CSS.

Comment 142

[Zack Weinberg (:zwol)]

> It looks to me like user fonts are only enabled for SVG and nsTextFrame here. I
> think this is a good thing, but we need to stay aware of it.

MathML is another place people will want it to work.

Comment 143

[Bill Gianopoulos [:WG9s]]

(In reply to comment #126)
> Created an attachment (id=340306) [details]
> patch, v.0.9q, cleanup logging

Unfortunately, this no longer applies cleanly.

Comment 144

[Bill Gianopoulos [:WG9s]]

I should have probably mentioned that it was the gfxP{angoFonts.cpp patches that I was not able to at all ascertain what to do with.  For my own builds, I just left out the gfxPangoFonts parts in the windows builds and left this patch out entirely in the Linux builds, since there are more patches required to make this work under Linux anyway.

Comment 145

[John Daggett (:jtd)]

From Simon Daniels, Microsoft:

> Do you know if the font embedding API's under Windows support CFF
> OpenType fonts?

Yes we consider this a bug in t2embed.dll - historically (pre-OpenType)
it rejected fonts without a glyf table. The fix is easy, but was not
fixed in Vista due to concerns over the number of machines without CFF
rasterizers.

[Note: Windows 7 bug 257987]

Comment 146

[John Daggett (:jtd)]

> +    if (fs && (gfe = fs->FindFontEntry(aName, *fontStyle, needsBold))) {
> +        // assume for now platform font if not SVG
> +        fe = static_cast<MacOSFontEntry*> (gfe);
> 
> So here you're assuming that SVG fonts are not implemented? Is that wise, or
> should we have some extra check here?

Based on one of your earlier review comments, I removed the flag to
indicate whether a font entry was a SVG font entry or not.  When we
implement SVG fonts, this code will be changed, I don't think additional
checks matter right now.

> Why do we need to store mUserFontSetGeneration in gfxTextRun? Is it to validate
> the textruns in the textrun cache?

Yes, it is used to calculate the word cache key.  The key is used both
when inserting into and when removing from the word cache, so it needs
to be consistent.

> Could we --- should we --- get rid of LoaderContext and just make it
> part of gfxUserFontSet, so that mLoaderProc is just a virtual method
> on gfxUserFontSet that the subclass overrides?

We can't call anything in layout code from within gfx code, hence the
need for any calls to nsPresContext etc. to be handled within layout
code.

Comment 147

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> > Could we --- should we --- get rid of LoaderContext and just make it
> > part of gfxUserFontSet, so that mLoaderProc is just a virtual method
> > on gfxUserFontSet that the subclass overrides?
> 
> We can't call anything in layout code from within gfx code, hence the
> need for any calls to nsPresContext etc. to be handled within layout
> code.

But we could subclass gfxUserFontSet from within layout and instead of mLoaderProc, have a pure virtual method in gfxUserFontSet that is overridden in the layout subclass.

Comment 148

[John Daggett (:jtd)]

>  #include "cairo.h"
> 
>  #include "gfxFont.h"
> +#include "gfxUserFontSet.h"
> 
> So should we #include gfxUserFontSet.h in gfxFont.h?

> In gfxFont.h:
> +    gfxUserFontSet* mUserFontSet;
> 
> Could this be nsRefPtr<gfxUserFontSet> if we #included gfxUserFontSet.h here?

> +    gfxUserFontData* mUserFontData;
> 
> nsAutoPtr<gfxUserFontData>?

We can't include gfxUserFontSet.h in gfxFont.h because
gfxMixedFontFamily and gfxProxyFontEntry inherit from classes in
gfxFont.h.

> +//   PRUint8  name[size];  // array of UTF-16 chars, english version of name
> record string
> 
> Shouldn't that be PRUnichar instead of PRUint8?

Nope, that's the way the EOT file layout document is written, size is in
bytes.  I'll add a comment to clarify that.

> Should this Windows EOT code be put in its own file? Would be a bit cleaner to
> have a file full of EOT-only stuff that's built only on Windows.

Since much of this code deals with dissecting TrueType tables, I'd like
to keep it with other code that handles TrueType tables, that'll lead to
better reuse.

> +    NSString *path = GetNSStringForString(filePath);
> 
> Do we leak this string along various error paths?

This is a NSString not a nsString, it's automatically cleaned up as a part of the Cocoa-maintained auto-retain pool.  See comment 27.

> The gfxUserFontSet generation counter can wrap around in 16 bits. This seems
> easily reachable especially for a malicious page.

I don't quite understand the concern here.  The lower bits will rollover
but the upper word will remain specific to *one* given user font set, so
I don't see what additional capabilities this gives an attacker.  There
is no way for the generation counter to rollover into the generation
count belonging to a different document.

> +    aLogFont->lfItalic         = (aItalic ? TRUE : FALSE);
> 
> aItalic != 0

So is aItalic != 0 guaranteed to be the same as TRUE?  The LOGFONT
structure serves as an input to a Windows API which is specified as
"lfItalic: Specifies an italic font if set to TRUE. "  Probably doesn't
matter if it's TRUE or simply non-zero but...

> Does it really make sense to mix all the format flags together? Seems
> like that could create confusion further down the stack.

The format flags are intended as a quick way of rejecting fonts before they are downloaded.  In almost all cases only one would ever be specified but there are cases such as format("opentype","truetype-aat") that indicate a font contains *both* AAT tables and OpenType tables and so can be used on different platforms.  Fonts with just format("truetype-aat") could be skipped on Windows machines.

Comment 149

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> > The gfxUserFontSet generation counter can wrap around in 16 bits. This seems
> > easily reachable especially for a malicious page.
> 
> I don't quite understand the concern here.  The lower bits will rollover
> but the upper word will remain specific to *one* given user font set, so
> I don't see what additional capabilities this gives an attacker.  There
> is no way for the generation counter to rollover into the generation
> count belonging to a different document.

That's true, but I'm worried about the consequences of a rollover in the current document. If there are no consequences, we don't need a generation counter...

Anyway, I see what you're trying to do now. Why not just have a single global generation counter that increments any time *any* font set changes? Then we won't likely get a rollover and generation counter values will be unique across documents (except for 0, which is only for empty gfxUserFontSets so collisions at 0 don't matter).

> > +    aLogFont->lfItalic         = (aItalic ? TRUE : FALSE);
> > 
> > aItalic != 0
> 
> So is aItalic != 0 guaranteed to be the same as TRUE? 

It's guaranteed to be 1, and I believe TRUE must be 1. If (1 != 0) != TRUE then I don't think Windows would have survived the 80s :-).

> > Does it really make sense to mix all the format flags together? Seems
> > like that could create confusion further down the stack.
> 
> The format flags are intended as a quick way of rejecting fonts before they
> are downloaded.  In almost all cases only one would ever be specified but
> there are cases such as format("opentype","truetype-aat") that indicate a
> font contains *both* AAT tables and OpenType tables and so can be used on
> different platforms.  Fonts with just format("truetype-aat") could be skipped
> on Windows machines.

OK.

Comment 150

[John Daggett (:jtd)]

> +  if (!gEnforceSameSiteOrigin)
> +    return PR_TRUE;
> 
> Seems like if this pref is false, we even allow an http:// page to
> load a file:// font. That's really bad and I think should be flat-out
> prohibited. But I can never remember the correct check for that, so
> get advice from bz or someone else.

Makes sense. Not sure how to do this though.  

bz?

Comment 151

[John Daggett (:jtd)]

> +    for (i = 0; i < 10; i++)
> +        eotHeader->panose[i] = os2Data.panose[i];
> 
> eotHeader->panose = os2Data.panose;

/builds/mozcentral/gfx/thebes/src/gfxFontUtils.cpp:1010: error: ISO C++ forbids assignment of arrays

> Seems CleanupTempFonts will delete the font files for running profiles
> if the user launches a new profile. Does that matter? If ATSU can
> handle the font files being deleted (which it should, if it has them
> open), then we should remove the file after calling
> ATSFontActivateFromFileSpecification.

Yeah, I think I should disable this for now and rethink how to do this without trashing fonts used by other concurrently running profiles.  ATS actually needs the font file to be kept around, it will behave very strangely without them.

> +    nsAutoString key(aFamilyName); 
> +    ToLowerCase(key);
> 
> What spec says that matching should be lowercase? A better approach
> would be to create an nsCaseInsensitiveStringHashKey so that this
> logic goes into the hashtable.

The CSS 2.1 font spec doesn't say specifically one way or another but our code has always assumed case-insensitive matching of font family names, same for other browsers.  In other words, arial, ARIAL, aRiAl will all match successfully (this isn't specific to downloadable fonts).

> Your temporary file names are predictable. This could be a security
> hole if the downloader doesn't use the equivalent of O_EXCL to create
> the file. This needs to be checked.

nsDownloader ends up using the default Init method of nsFileOutputStream which only sets the PR_WRONLY | PR_CREATE_FILE | PR_TRUNCATE open flags, PR_EXCL is not set.  Adding code to use nsIFile::CreateUnique, I think that should eliminate the hole you're thinking about. 

> +    HMODULE fontlib = LoadLibrary("t2embed.dll");
> 
> Can we avoid leaking the library?

This is just weak linking, references to this library are kept until process teardown occurs.  At that point, is it really necessary to explicitly unload this DLL?

> How do we cancel the download(s) if the prescontext goes away? We
> should probably also pause them if the presshell is frozen (i.e. goes
> into bfcache).

There's no mechanism here to explicitly cancel a download.  Where should that be hooked in?

> It looks to me like user fonts are only enabled for SVG and
> nsTextFrame here. I think this is a good thing, but we need to stay
> aware of it.

Hmmm, seems like this is enabled for any path using text runs.  Are there other paths you're thinking that would somehow *not* end up using this mechanism?  Or that would not be refreshed correctly after the font download as it stands now?

> Anyway, I see what you're trying to do now. Why not just have a single
> global generation counter that increments any time *any* font set
> changes? Then we won't likely get a rollover and generation counter
> values will be unique across documents (except for 0, which is only
> for empty gfxUserFontSets so collisions at 0 don't matter).

Making it global makes it possible, although highly unlikely, for generations from two different documents to collide.  That makes me much more concerned than two generations for the same document colliding.  I rather just increase the size of the generation value to 64 bits.

Comment 152

[Boris Zbarsky [:bzbarsky]]

What you probably want to be doing are the following pre-load checks:

1)  CheckLoadURI check.
2)  CheckMayLoad check, as needed.
3)  Content policy check.

We have some places that assume that 2 implies 1, and that's even sort of true for now.  In which case you want to do 1 instead of 2 if you don't need same-origin enforcement.

Comment 153

[Igor Velkov]

There need to remember about webmasters - morons.
User should have an options to comletely disable this option for some sites/domains/uri, and command (hotkey, menu) to swich loaded fonts back to default for the current page.

Comment 154

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #151)
> > +    for (i = 0; i < 10; i++)
> > +        eotHeader->panose[i] = os2Data.panose[i];
> > 
> > eotHeader->panose = os2Data.panose;
> 
> /builds/mozcentral/gfx/thebes/src/gfxFontUtils.cpp:1010: error: ISO C++
> forbids assignment of arrays

Huh. That seems stupid, since you can wrap the array in a struct and assign that. OK, use memcpy here instead.

> > +    nsAutoString key(aFamilyName); 
> > +    ToLowerCase(key);
> > 
> > What spec says that matching should be lowercase? A better approach
> > would be to create an nsCaseInsensitiveStringHashKey so that this
> > logic goes into the hashtable.
> 
> The CSS 2.1 font spec doesn't say specifically one way or another but our code
> has always assumed case-insensitive matching of font family names, same for
> other browsers.  In other words, arial, ARIAL, aRiAl will all match
> successfully (this isn't specific to downloadable fonts).

OK. I'll let this go for now but please file a bug to add nsCaseInsensitiveStringHashkey and use it here.

> > Your temporary file names are predictable. This could be a security
> > hole if the downloader doesn't use the equivalent of O_EXCL to create
> > the file. This needs to be checked.
> 
> nsDownloader ends up using the default Init method of nsFileOutputStream which
> only sets the PR_WRONLY | PR_CREATE_FILE | PR_TRUNCATE open flags, PR_EXCL is
> not set.  Adding code to use nsIFile::CreateUnique, I think that should
> eliminate the hole you're thinking about.

Yeah, it should.

> > +    HMODULE fontlib = LoadLibrary("t2embed.dll");
> > 
> > Can we avoid leaking the library?
> 
> This is just weak linking, references to this library are kept until process
> teardown occurs.  At that point, is it really necessary to explicitly unload
> this DLL?

I suppose not.

> > How do we cancel the download(s) if the prescontext goes away? We
> > should probably also pause them if the presshell is frozen (i.e. goes
> > into bfcache).
> 
> There's no mechanism here to explicitly cancel a download.  Where should that
> be hooked in?

PresShell::Destroy to cancel the downloads. If we want to suspend/resume font downloads for pages in the bfcache --- which I think we do, although I think we could defer it to a future patch --- PresShell::Freeze and PresShell::Thaw.

> Hmmm, seems like this is enabled for any path using text runs.  Are there
> other paths you're thinking that would somehow *not* end up using this
> mechanism?

Yes, nsIRenderingContext::DrawString and friends --- used in a few places, but most importantly XUL's nsTextBoxFrame. So certain kinds of XUL text won't get user fonts. I think that's fine. At some point we'll change those users over to use nsTextFrame and user fonts will be enabled for XUL "for free".

> > Anyway, I see what you're trying to do now. Why not just have a single
> > global generation counter that increments any time *any* font set
> > changes? Then we won't likely get a rollover and generation counter
> > values will be unique across documents (except for 0, which is only
> > for empty gfxUserFontSets so collisions at 0 don't matter).
> 
> Making it global makes it possible, although highly unlikely, for generations
> from two different documents to collide.

If something can cause 2^32 font downloads to happen, then I'm sure it could also cause 2^16 page loads to happen, so your existing scheme doesn't work either.

> That makes me much more concerned
> than two generations for the same document colliding.  I rather just increase
> the size of the generation value to 64 bits.

Sounds like the best plan. But use a global counter, because it's simpler than having a bitfield split.

Comment 155

[John Daggett (:jtd)]

Attachment: patch, v.1.0, changes based on roc review

Lots of minor changes based on roc's review comments.

* use nsTArray<PRUint8> when making the EOT font header on Windows
* font set generation a simple 64-bit counter
* use CreateUnique when making temp filenames

I'm going to deal with the CheckLoadURI/content policy check in the bug to implement access-control-header support.

Vlad, I've put you down for the sr.  Stuart has looked at this once already, if he has time feel free to push the sr to him instead.

Comment 156

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #155)
> I'm going to deal with the CheckLoadURI/content policy check in the bug to
> implement access-control-header support.

I think that's not a good idea. If we land it like that for beta1, it means we're adding a giant security hole in beta1.

Comment 157

[John Daggett (:jtd)]

(In reply to comment #156)
> (In reply to comment #155)
> > I'm going to deal with the CheckLoadURI/content policy check in the bug to
> > implement access-control-header support.
> 
> I think that's not a good idea. If we land it like that for beta1, it means
> we're adding a giant security hole in beta1.

Ok, I'll look at this some more but note that this only applies when a user manually disables the same-site origin restriction.

Comment 158

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oh! That's true. I guess we can postpone it then.

Comment 159

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Just remove CleanupTempFonts and the commented-out code that calls it.

+    if (aHeader->Length() < sizeof(EOTFixedHeader)) {
+        if (!aHeader->AppendElements(sizeof(EOTFixedHeader) - aHeader->Length()))
+            return NS_ERROR_OUT_OF_MEMORY;

This isn't necessary. You know (and can assert) that aHeader->Length() is zero on entry to this function. So just AppendElements(sizeof(EOTFixedHeader)).

+    if (aHeader->Length() < sizeof(EOTFixedHeader) + eotVariableLength) {
+        if (!aHeader->AppendElements(sizeof(EOTFixedHeader) 
+                                     + eotVariableLength - aHeader->Length()))
+            return NS_ERROR_OUT_OF_MEMORY;

Similar here. Although here you're actually appending too many elements, as it stands, since you don't need to append another sizeof(EOTFixedHeader) elements.

+    // append null root string size
+    *eotEnd++ = 0;
+    *eotEnd++ = 0;

Here, assert that eotEnd == aHeader->Elements() + aHeader->Length() (i.e. that the length computation was correct).

+    //for (i = 0; i < 10; i++)
+    //    eotHeader->panose[i] = os2Data.panose[i];

Just remove this.

+    err = ATSFontActivateFromFileSpecification(&spec, 
+                                               kPrivateATSFontContextPrivate, 
+                                               kATSFontFormatUnspecified,
+                                               NULL, 
+                                               kATSOptionFlagsDoNotNotify, 
+                                               &containerRef);

Seems like containerRef should be released along some error paths in this function.

+    aLogFont->lfItalic         = aItalic != 0;

Sorry, I misled you, this should just be lfItalic = aItalic.

+        PRBool isExists, isReadable;

These variables are no longer used.

+      srcArray.AppendElements(1);
+      gfxFontFaceSrc& face = srcArray[faceIndex++];

You don't need faceIndex. Just write
  gfxFontFaceSrc *face = srcArray.AppendElements(1);
and check face != nsnull for OOM, by the way.

Comment 160

[John Daggett (:jtd)]

Follow-on blocker bugs:

Bug 457821 P1 need to rebuild user font set objects when style sheets are disabled/enabled or rules are modified/added/removed 
Bug 458169 P1 implement downloadable font support on Linux
Bug 458160 P2 Can't use .otf fonts via @font-face on Windows 
Bug 457825 P3 support use of access control headers to allow cross-site downloable fonts

Other related:

Bug 458176 P3 rebuild text runs more selectively when loading downloadable font
Bug 452870 Implement tests for @font-face rules

Comment 161

[[e-mail]]

Attachment: VC7.1 bustage fix

Comment 162

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: uses time and LoadLibraryW instead of time32 and LoadLibrary

this broke the windows ce build in two places

Comment 163

[John Daggett (:jtd)]

(In reply to comment #162)
> Created an attachment (id=343061) [details]
> uses time and LoadLibraryW instead of time32 and LoadLibrary
> 
> this broke the windows ce build in two places

Brad, Doug already set up another bug for this, bug 458256.  I'm going to move the patch over there.

Comment 164

[John Daggett (:jtd)]

checked in VC 7.1 bustage fix, thanks Neil!

Comment 165

[Doug Turner (:dougt)]

Comment on attachment 343061 [details] [diff] [review]
uses time and LoadLibraryW instead of time32 and LoadLibrary

This patch is no longer needed.

the LoadLibraryW landed as part of 458256.

The calls to time() has also been removed.

Comment 166

[Asif Hisam]

is eot support added to the trunk?
I just tested with 3.6a1pre and eot fonts are not working.

Comment 167

[John Daggett (:jtd)]

(In reply to comment #166)
> is eot support added to the trunk?
> I just tested with 3.6a1pre and eot fonts are not working.

EOT fonts are not supported and there is no intention at this time to include support for this IE-specific technology.  This bug is related to direct TrueType/OpenType font linking.

Comment 168

[Asif Hisam]

okay.
We have eot font extension which is working fine with firefox 2. But it is not working with Firefox 3.
It seems that AddFontMemResourceEx api is not able to enumerate the font to firefox 3. 
I tested with AddFontResourceEx with FR_PRIVATE flag and it is able enumerate font to firefox 3.

How we can get AddFontMemResourceEx to work with firefox 3?



(In reply to comment #167)
> (In reply to comment #166)
> > is eot support added to the trunk?
> > I just tested with 3.6a1pre and eot fonts are not working.
> 
> EOT fonts are not supported and there is no intention at this time to include
> support for this IE-specific technology.  This bug is related to direct
> TrueType/OpenType font linking.