467669 - Need chrome-accessible API for getting list of font faces used by content

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, enhancement)

Description

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

Use case #1: Right now there is no way to determine which font(s) are actually used by a piece of text (i.e. after font selection/fallback), other than by looking at the rendering and guessing which font it is, or by turning on PR_LOGGING or otherwise digging into the guts of gfx. We should allow DOMI, Firebug and other tools to get this information. It would be useful for Web developers and Mozilla developers.

Use case #2: It would also be useful if Page Info displayed, alongside other media, a list of all downloadable fonts used in the document. Web authors and font vendors would like this.

I propose an API extension to inDOMUtils, something like
  nsIDOMFontFaceList getUsedFontFaces(nsIDOMRange aRange);
which returns the set of font faces used by content in the given range. This covers both of the above use cases efficiently. You can drill down to the font used for each character, either easily and slowly by calling getUsedFontFaces once per character, or more efficiently by starting off with a large range and subdividing it whenever more than one font is returned.

nsIDOMFontFaceList would be a non-live collection of nsIDOMFontFace objects. I'm not sure exactly what API those objects should offer, but I think at least

  readonly attribute nsIDOMCSSFontFaceRule rule; // null if no associated rule
  readonly attribute DOMString URI; // null if not a downloaded font, i.e. local
  readonly attribute DOMString format; // as per http://www.w3.org/TR/css3-webfonts/#referencing
  readonly attribute DOMString name; // full font name as obtained from the font resource

Comment 1

[John J. Barton]

Firebug really likes live-edit: will the info in nsIDOMFontFaceList be enough to allow Firebug to track down writable stuff that controls the font face? (Sorry my CSS knowledge ;-)

Comment 2

[John Daggett (:jtd)]

(In reply to comment #1)
> Firebug really likes live-edit: will the info in nsIDOMFontFaceList be
> enough to allow Firebug to track down writable stuff that controls the
> font face?

The list of matched font faces is a byproduct of the font matching
process, which involves not only CSS properties but also preference
settings and locally available fonts.  

text ==> [CSS font properties, pref fonts, system fonts] ==> matched font list

The resulting font list is really a read-only list.  Firebug could
naturally rebuild this list when changes are made to CSS font properties.

Comment 3

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

There are only two ways Web authors can control which faces get used for a given text node: the CSS 'font-family' property, and CSS @font-face rules which define new family names referring to specific downloaded or local font files.

So if you are displaying the font-faces used for a particular DOM text node, then you could link to the @font-face CSS rule for that face, if there is one. If the font is a downloaded font, you could allow the user to edit the URI and the format, and propagate those changes back to the @font-face CSS rule (or possibly create a new rule that matches the text node somehow).

If the user edits the name, it gets tricky. They probably intend to choose a different font. One quick and dirty way to handle it would be to remove the existing name from the CSS font-family property for this text node (either creating a new CSS rule that applies to this node, or finding an existing CSS rule that's setting font-family for this node), and then add the new name to the front of the CSS font-family property. But if that font isn't in the system, or it maps to a @font-face rule that refers to a font resource whose internal name is different to the name given in the @font-face rule, then the font name you get back won't reflect what the user asked for.

Maybe it would help if we had something like
  readonly attribute DOMString CSSFamilyName;
giving the family name specified in the CSS font-family property that triggered the use of this font face. That could be null, though, if the font was selected from font preferences or font fallback, so we had to use a font that wasn't mentioned in the CSS font-family property. Probably still worth having though.

Comment 4

[John J. Barton]

(In reply to comment #2)
...
> The list of matched font faces is a byproduct of the font matching
> process, which involves not only CSS properties but also preference
> settings and locally available fonts.  
> 
> text ==> [CSS font properties, pref fonts, system fonts] ==> matched font list

So I think Firebug would want to simulate this picture, so the developer could see "oh the pref font caused arial to be used here", not just "arial got used here"

Comment 5

[John Daggett (:jtd)]

> readonly attribute nsIDOMCSSFontFaceRule rule; // null if no associated rule
> readonly attribute DOMString URI; // null if not a downloaded font, i.e. local
> readonly attribute DOMString format; // as per http://www.w3.org/TR/css3-webfonts/#referencing
> readonly attribute DOMString name; // full font name as obtained from the font resource

Couple minor points.  I'm assuming the URI field would contain the URI of the font that was actually downloaded, so if the src descriptor contained several url's, the URI would match the one that was actually used.  I think we'll need to add formats to the list for formats not supported as a downloadable font, Windows bitmap/vector formats for example.

Comment 6

[John Daggett (:jtd)]

> So I think Firebug would want to simulate this picture, so the
> developer could see "oh the pref font caused arial to be used here",
> not just "arial got used here"

Not quite sure what you mean by "simulate" here but if you just mean to
provide feedback as to how a specific face was chosen, something like
"CSS font list", "pref list", "system fallback", I'm sure that's
definitely possible.

Operations like the editing of URI's would simply modify the underlying
CSS rule, I don't think this API needs to take that into account.

Comment 7

[John Daggett (:jtd)]

> Maybe it would help if we had something like
>   readonly attribute DOMString CSSFamilyName;
> giving the family name specified in the CSS font-family property that
> triggered the use of this font face. That could be null, though, if
> the font was selected from font preferences or font fallback, so we
> had to use a font that wasn't mentioned in the CSS font-family
> property. Probably still worth having though.

Yeah, I think having the family name is useful.  For downloaded fonts the full font name will need to come from the font itself and this name may not match the family name at all:

  @font-face {
    font-family: Headline;
    src: url(Vollkorn.otf);
  }

BTW, I'm assuming we're discussing a 1.9.x feature, not a 1.9.1 one, right?

Comment 8

[John Daggett (:jtd)]

(In reply to comment #3)
> There are only two ways Web authors can control which faces get used
> for a given text node: the CSS 'font-family' property, and CSS
> @font-face rules which define new family names referring to specific
> downloaded or local font files.

Actually, all of the CSS font properties are involved in choosing the
face, not just the font-family property.

Might also be nice to indicate synthetic faces, fake italics and bolding.

Comment 9

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

(In reply to comment #5)
> Couple minor points.  I'm assuming the URI field would contain the URI of the
> font that was actually downloaded, so if the src descriptor contained several
> url's, the URI would match the one that was actually used.

Right.

> I think we'll need
> to add formats to the list for formats not supported as a downloadable font,
> Windows bitmap/vector formats for example.

Yep.

(In reply to comment #6)
> > So I think Firebug would want to simulate this picture, so the
> > developer could see "oh the pref font caused arial to be used here",
> > not just "arial got used here"
> 
> Not quite sure what you mean by "simulate" here but if you just mean to
> provide feedback as to how a specific face was chosen, something like
> "CSS font list", "pref list", "system fallback", I'm sure that's
> definitely possible.

Yeah, we should do that. We'll need another string attribute, I guess? Perhaps
  readonly attribute DOMString selectionReason;
taking the values "font-family", "prefs", "fallback" or so.

> Operations like the editing of URI's would simply modify the underlying
> CSS rule, I don't think this API needs to take that into account.

Yeah.

(In reply to comment #7)
> Yeah, I think having the family name is useful.  For downloaded fonts the full
> font name will need to come from the font itself and this name may not match
> the family name at all:

Right, that's why we need to provide both.

> BTW, I'm assuming we're discussing a 1.9.x feature, not a 1.9.1 one, right?

Absolutely!

(In reply to comment #8)
> (In reply to comment #3)
> > There are only two ways Web authors can control which faces get used
> > for a given text node: the CSS 'font-family' property, and CSS
> > @font-face rules which define new family names referring to specific
> > downloaded or local font files.
> 
> Actually, all of the CSS font properties are involved in choosing the
> face, not just the font-family property.

Oh, right. Still, those other ways are a lot more indirect.

> Might also be nice to indicate synthetic faces, fake italics and bolding.

Yeah. Additional boolean properties on the face object?

Comment 10

[Karl Tomlinson (:karlt)]

I'm wondering what kind of implementation is imagined here.

For many of these properties it is sufficient to examine the fonts that have
been selected for normal layout.

> (In reply to comment #6)
> > > So I think Firebug would want to simulate this picture, so the
> > > developer could see "oh the pref font caused arial to be used here",
> > > not just "arial got used here"
> > 
> > Not quite sure what you mean by "simulate" here but if you just mean to
> > provide feedback as to how a specific face was chosen, something like
> > "CSS font list", "pref list", "system fallback", I'm sure that's
> > definitely possible.
> 
> Yeah, we should do that.

This would require a different implementation, something like: perform another
pass of font-selection, but this time get different information.

> > Might also be nice to indicate synthetic faces, fake italics and bolding.
> 
> Yeah. Additional boolean properties on the face object?

If we want to future proof the API, then should some sort of arbitrary
name/value list be presented?

Other, often not so important, properties, are font_matrix (including scale) and
font_options (hinting, antialias) properties.

Comment 11

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

(In reply to comment #10)
> I'm wondering what kind of implementation is imagined here.
> 
> For many of these properties it is sufficient to examine the fonts that have
> been selected for normal layout.

I think that's the idea...

> > (In reply to comment #6)
> > > > So I think Firebug would want to simulate this picture, so the
> > > > developer could see "oh the pref font caused arial to be used here",
> > > > not just "arial got used here"
> > > 
> > > Not quite sure what you mean by "simulate" here but if you just mean to
> > > provide feedback as to how a specific face was chosen, something like
> > > "CSS font list", "pref list", "system fallback", I'm sure that's
> > > definitely possible.
> > 
> > Yeah, we should do that.
> 
> This would require a different implementation, something like: perform another
> pass of font-selection, but this time get different information.

I think we could gather that information at font selection time and stuff it into some bits stolen from GlyphRun::mCharacterOffset.

> > > Might also be nice to indicate synthetic faces, fake italics and bolding.
> > 
> > Yeah. Additional boolean properties on the face object?
> 
> If we want to future proof the API, then should some sort of arbitrary
> name/value list be presented?

I'd prefer to avoid that.

> Other, often not so important, properties, are font_matrix (including scale)
> and
> font_options (hinting, antialias) properties.

I don't think Web authors would need those, at least, not in the near future. If we want to extend the DOM object with additional properties we can do that later.

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

As Jonathan Kew mentioned here: 
http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/ded9b1ccec4eb7f4?hl=en#

Natural extension of this bug would be support for WOFF fonts (Firefox 3.6). This format allows to associate meta-data (vendor, credits, description, copyright, ...) and so, if there is also a way how to access it (using nsIDOMFontFace object?), tools like Firebug could display all the info to the user.

Related Firebug report here:
http://code.google.com/p/fbug/issues/detail?id=3071

Honza

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: WIP - get list of font faces actually used within a DOM range

This patch is a start at something along the lines of comment #0. So far, it successfully collects a list of font-face objects, but they only provide a font name attribute, not any of the other proposed attributes.

Would appreciate any feedback as to whether this looks like it's heading in the right direction....

Comment 14

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

nsFontFaceList::mFontFaces should be a hashtable. An important use case for this is to get the font faces used in an entire document, so AddFontsFromTextRun needs to be super fast. (E.g. for "page info" to show a list of the downloadable fonts used.)

GetFontFacesForFrames is a bit confusing. From the way you use it, it looks like it needs to return the faces for all the text under a given content node, including anonymous content. What you have now has some bugs, for example it doesn't iterate over all continuations of aFrame, but it needs to. And it does iterate over all siblings of aFrame, but it shouldn't. It also checks aFrame->IsLeaf() which only asks whether DOM children of the element should have frames created for them; for example <input> returns true for IsLeaf() but we should check the fonts used for the anonymous frames inside <input>. It also fails to descend into abs-pos and float children.

I think basically GetFontFacesForFrames should require that aFrame is the first frame in the continuation chain and then look like this:
  if aFrame is a text frame, call GetFontFacesForText and return
  for aFrame and all of its continuations:
    for all the in-flow (null child-list) children of the frame:
      if the child is a placeholder frame, call GetFontFacesForFrames for the out-of-flow frame
      otherwise, if the child is first in its continuation chain, call GetFontFacesForFrames on it

I suppose you should also traverse the nsGkAtoms::popupList to get the fonts used in combobox dropdowns and XUL menus/panels.

In GetFontFacesForText instead of using -1,-1 as the magic values I would use 0 and PR_INT32_MAX.

I think GetUsedFontFaces only needs to flush style (which also flushes frame construction), not layout.

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: part 1 - get list of fonts used in a range

This is the basic support for exposing a list of the fonts used in a range of the document. Following patches will add implementations of more of the nsIDOMFontFace attributes.

Comment 16

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - find the @font-face rule responsible for a font

Comment 17

[Jonathan Kew [:jfkthame]]

Attachment: part 3 - track how we ended up selecting the given font

This works fine on Windows and OS X, but the Linux code (in gfxPangoFontGroup::FindFontForChar) is just a quick hack to make it build and run - it does not properly track whether fonts come from CSS, prefs, etc. Karl, is this something you could look at? I'm sure you have a much better handle on the Pango/FontConfig stuff with patterns and fontsets etc than I do....

Comment 18

[Jonathan Kew [:jfkthame]]

Attachment: part 4 - implement a bunch more attributes for user fonts

Comment 19

[Jonathan Kew [:jfkthame]]

Attachment: part 5 - provide access to the WOFF metadata block

Comment 20

[Jonathan Kew [:jfkthame]]

Attachment: part 6 - return the real font name rather than our internal identifier

The gfxFontEntry's name field is not necessarily the real name that we want to expose to users, so read it from the font's 'name' table instead.

Note that for fonts that have been through the OTS sanitizer, this still doesn't give the desired result, because OTS currently discards the original 'name' table and provides a standard "canned" version with fixed strings instead. So we need to enhance OTS's support for this table in order to preserve the proper names.

Comment 21

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

Comment on attachment 534739 [details] [diff] [review]
part 1 - get list of fonts used in a range

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

The rest looks good.

::: content/base/src/nsRange.cpp
@@ +2248,5 @@
> +  }
> +
> +  // Hold strong pointers across the flush
> +  nsCOMPtr<nsIDOMNode> startContainer = do_QueryInterface(mStartParent);
> +  nsCOMPtr<nsIDOMNode> endContainer = do_QueryInterface(mEndParent);

Is this necessary? The caller should be holding a reference to the nsRange, which is holding onto its start/end.

@@ +2255,5 @@
> +  if (!mStartParent->IsInDoc()) {
> +    return NS_ERROR_UNEXPECTED;
> +  }
> +
> +  mStartParent->GetCurrentDoc()->FlushPendingNotifications(Flush_Style);

I would just make this flush OwnerDoc() and not bother checking IsInDoc above. If the element is in a document, that document must be OwnerDoc.

@@ +2263,5 @@
> +    return NS_ERROR_UNEXPECTED;
> +  }
> +
> +  nsRefPtr<nsFontFaceList> fontFaceList = new nsFontFaceList();
> +  NS_ENSURE_TRUE(fontFaceList, NS_ERROR_OUT_OF_MEMORY);

OOM check not needed.

::: layout/base/nsLayoutUtils.cpp
@@ +4091,5 @@
> +                                     aFontFaceList);
> +          NS_ENSURE_SUCCESS(rv, rv);
> +          continue;
> +        }
> +        if (!child->GetPrevContinuation()) {

I'd just do if (child->GetPrevContinuation() continue; at the top of the loop body. Then you can do if (placeholder) child = out-of-flow-frame and have a single call to GetFontFacesForFrames.

@@ +4139,5 @@
> +    NS_ENSURE_TRUE(textRun, NS_ERROR_OUT_OF_MEMORY);
> +
> +    aFontFaceList->AddFontsFromTextRun(textRun,
> +                                       fstart - f->GetContentOffset(),
> +                                       fend - fstart);

You need to do some funky stuff here with the gfxSkipCharsIterator to map content offsets to post-whitespace-compression/text-transform textrun offsets. Need tests :-)

::: layout/inspector/public/nsIDOMFontFace.idl
@@ +9,5 @@
> +  //   0x01 = CSS
> +  //   0x02 = prefs
> +  //   0x04 = fallback
> +  // (note that the same font may have been found in multiple ways)
> +  readonly attribute unsigned long fontMatchType;

Let's use three boolean attributes here.

@@ +13,5 @@
> +  readonly attribute unsigned long fontMatchType;
> +
> +  // available for all fonts
> +  readonly attribute DOMString name; // full font name as obtained from the font resource
> +  readonly attribute DOMString CSSFamilyName; // family name as used in CSS font-family

Better specify what this is if it wasn't obtained via CSS ... looks like "(no family)", but it might be better to just return the empty string. Localizers would want to localize the placeholder anyway.

You need to clarify that this is not (necessarily) the name that was used to refer to the font in the CSS font-family property(ies) that triggered use of this font. It's simply a name that *can* be used in CSS to refer to this font.

@@ +16,5 @@
> +  readonly attribute DOMString name; // full font name as obtained from the font resource
> +  readonly attribute DOMString CSSFamilyName; // family name as used in CSS font-family
> +
> +  // meaningful for fontMatchType & 0x01 (CSS) only
> +  readonly attribute long CSSFamilyIndex; // index in the font-family list, or -1 for pref/fallback fonts

This is the index for some font-family property, but we're not specifying which one. It looks like you use the font-family property for the first frame that uses this font as you traverse the frame tree. So to use this value, you really need to provide the style property as well in the form of a nsIDOMCSSStyleDeclaration. Or alternatively you could return the DOM node you found (possibly an anonymous node) and the caller could use inDOMUtils::GetCSSStyleRules() and scan the rules for the first style rule setting the font-family property.

Also, if you only return one property/index pair, the caller won't be able to find all references to the font (subdividing the content range won't really work because you can't find all anonymous content that way). So the ideal thing to return here would be a list of pairs of (nsIDOMCSSStyleDeclaration, index), or (nsIDOMNode, index). Listing all DOM nodes that use the font is probably not a good idea for a range covering a large page, so a list of (nsIDOMCSSStyleDeclaration, index) sounds good.

If we use a list, we don't need a boolean to indicate whether CSS rules selected the font, because that would be true if and only if the list is nonempty.

One use-case that's not addressed is the task of finding some or all text in the page that uses a given font. People can subdivide the page DOM themselves and make repeated calls to getUsedFontFaces but if that turns out to be impractical we may need to extend the API here. I guess that can wait.

Comment 22

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

Comment on attachment 534740 [details] [diff] [review]
part 2 - find the @font-face rule responsible for a font

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

r+ with that.

::: layout/style/nsFontFaceLoader.cpp
@@ +693,5 @@
> +{
> +  for (PRUint32 i = 0; i < mRules.Length(); ++i) {
> +    if (mRules[i].mFontEntry == aFontEntry) {
> +      NS_ADDREF(mRules[i].mContainer.mRule);
> +      return mRules[i].mContainer.mRule;

If you're returning an addrefed value, you should be returning an already_AddRefed. However I would just return a non-addrefed value here.

Comment 23

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

Comment on attachment 534743 [details] [diff] [review]
part 4 - implement a bunch more attributes for user fonts

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

::: gfx/thebes/gfxUserFontSet.h
@@ +85,4 @@
>      virtual ~gfxUserFontData() { }
> +
> +    PRUint32          mSrcIndex;
> +    nsString          mSrcText;  // text of URI or local() name for the source used

how about we store an nsCOMPtr<nsIURI> and an separate string for the local name, so we can lazily get the string from the URI without having to copy in StoreUserFontData?

::: layout/inspector/src/nsFontFace.cpp
@@ +125,5 @@
>  {
> +  if (mFontEntry->IsUserFont() && !mFontEntry->IsLocalUserFont()) {
> +    NS_ASSERTION(mFontEntry->mUserFontData, "missing userFontData");
> +    aURI = mFontEntry->mUserFontData->mSrcText;
> +  }

truncate aURI in the else case

@@ +136,5 @@
>  {
> +  if (mFontEntry->IsLocalUserFont()) {
> +    NS_ASSERTION(mFontEntry->mUserFontData, "missing userFontData");
> +    aLocalName = mFontEntry->mUserFontData->mSrcText;
> +  }

truncate aLocalName in the else case

Comment 24

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

Comment on attachment 534744 [details] [diff] [review]
part 5 - provide access to the WOFF metadata block

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

::: gfx/thebes/gfxUserFontSet.cpp
@@ +446,5 @@
> +                    memcpy(metadata.Elements(),
> +                           aFontData + metaOffset, metaCompLen);
> +                }
> +            }
> +        }

Move this medata extraction out to a helper function.

::: layout/inspector/src/nsFontFace.cpp
@@ +196,5 @@
> +                       (const Bytef *)(userFontData->mMetadata.Elements()),
> +                       userFontData->mMetadata.Length()) == Z_OK &&
> +            destLen == userFontData->mMetaOrigLen)
> +        {
> +          // FIXME: what if the original metadata isn't utf8-encoded?

The WOFF spec should definitely require the metadata to be UTF8!

Comment 25

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

Comment on attachment 534745 [details] [diff] [review]
part 6 - return the real font name rather than our internal identifier

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

Why are we storing mRealName in the userData as well as being able to get it from the gfxFontEntry?

Comment 26

[John Daggett (:jtd)]

Comment on attachment 534745 [details] [diff] [review]
part 6 - return the real font name rather than our internal identifier

+    // the "real" name of the face, if available from the font resource
+    // (may be expensive); returns Name() if nothing better is available
+    virtual nsString RealName();

I think "RealName" is a poor way to describe this, "FaceName" would be
more appropriate.  Basically, it's the name that describes the face
(e.g. "Arial Bold").  The complexity here is dealing with user fonts
since the name table is stripped out by the sanitizer.

+nsString
+gfxFontEntry::RealName()
+{
+    FallibleTArray<PRUint8> nameTable;
+    nsresult rv = GetFontTable(TRUETYPE_TAG('n','a','m','e'), nameTable);
+    if (NS_SUCCEEDED(rv)) {
+        nsAutoString name;
+        rv = gfxFontUtils::GetFullNameFromTable(nameTable, name);
+        if (NS_SUCCEEDED(rv)) {
+            return name;
+        }
+    }
+    return Name();
+}

Does this need to be the default?  Seems like the default should be
Name() and allow that to be overriden for the special cases (i.e. user
fonts).  Is there any reason the fullname should be required?


+    // read the fullname from the sfnt data (used to save the original name
+    // prior to renaming the font for installation)
+    static nsresult
+    GetFullNameFromSFNT(const PRUint8* aFontData, PRUint32 aLength,
+                        nsAString& aFullName);
+
+    // helper to get fullname from name table
+    static nsresult
+    GetFullNameFromTable(FallibleTArray<PRUint8>& aNameTable,
+                         nsAString& aFullName);

These functions implicitly require validation prior to use, they
assume the table directory is sane.  I think the ReadNames routine
should be safe but with this patch we are exposing ourselves to evil
name tables, since we're passing in the raw font data to ReadNames. 
(The sanitizer doesn't validate this data, it chucks it and
synthesizes a generic one).

Comment 27

[Jonathan Kew [:jfkthame]]

(In reply to comment #26)
> +nsString
> +gfxFontEntry::RealName()
> +{
> +    FallibleTArray<PRUint8> nameTable;
> +    nsresult rv = GetFontTable(TRUETYPE_TAG('n','a','m','e'), nameTable);
> +    if (NS_SUCCEEDED(rv)) {
> +        nsAutoString name;
> +        rv = gfxFontUtils::GetFullNameFromTable(nameTable, name);
> +        if (NS_SUCCEEDED(rv)) {
> +            return name;
> +        }
> +    }
> +    return Name();
> +}
> 
> Does this need to be the default?  Seems like the default should be
> Name() and allow that to be overriden for the special cases (i.e. user
> fonts).  Is there any reason the fullname should be required?

User fonts aren't the only issue. In many cases, the "name" we use for the gfxFontEntry is not the actual font name as found in the font resource, but some other identifier. (E.g., on Linux, it's the font filename; even OS X and Windows font entries don't necessarily have the same Name() for the same actual font.) To consistently provide the user with the real name of the font, it seemed simplest to read it from the font itself by default.

> These functions implicitly require validation prior to use, they
> assume the table directory is sane.  I think the ReadNames routine
> should be safe but with this patch we are exposing ourselves to evil
> name tables, since we're passing in the raw font data to ReadNames. 
> (The sanitizer doesn't validate this data, it chucks it and
> synthesizes a generic one).

See comment #20; we're not exposing ourselves to unvalidated font data, but we need a further patch - to validate and preserve the name table in OTS - before this will actually give us the right names.

Comment 28

[Jonathan Kew [:jfkthame]]

(In reply to comment #21)

> @@ +13,5 @@
> > +  readonly attribute unsigned long fontMatchType;
> > +
> > +  // available for all fonts
> > +  readonly attribute DOMString name; // full font name as obtained from the font resource
> > +  readonly attribute DOMString CSSFamilyName; // family name as used in CSS font-family
> 
> Better specify what this is if it wasn't obtained via CSS ... looks like
> "(no family)", but it might be better to just return the empty string.
> Localizers would want to localize the placeholder anyway.

Actually, I want to ensure that we always return a correct family name, but that requires a further patch for the Linux case as our font entries there aren't generally members of a gfxFontFamily.

> You need to clarify that this is not (necessarily) the name that was used to
> refer to the font in the CSS font-family property(ies) that triggered use of
> this font. It's simply a name that *can* be used in CSS to refer to this
> font.

True. (I'd have liked to be able to return the name that was actually used in the CSS, but I don't see any way to retrieve that without adding extra overhead to the font-selection process, and I don't want to do that just for the sake of this API.)

> 
> @@ +16,5 @@
> > +  readonly attribute DOMString name; // full font name as obtained from the font resource
> > +  readonly attribute DOMString CSSFamilyName; // family name as used in CSS font-family
> > +
> > +  // meaningful for fontMatchType & 0x01 (CSS) only
> > +  readonly attribute long CSSFamilyIndex; // index in the font-family list, or -1 for pref/fallback fonts
> 
> This is the index for some font-family property, but we're not specifying
> which one. It looks like you use the font-family property for the first
> frame that uses this font as you traverse the frame tree. 

Yes. (That's somewhat arbitrary, but seemed easiest to understand.) My assumption was that someone wanting to actually use this would need to make a targeted query for a single node/frame, in order to have a context where this is meaningful.

Alternatively, we might not want to expose this at all - I don't know if there's any compelling use case. It just seemed like something we could (cheaply) provide, and that might sometimes be of interest.

> So to use this
> value, you really need to provide the style property as well in the form of
> a nsIDOMCSSStyleDeclaration. Or alternatively you could return the DOM node
> you found (possibly an anonymous node) and the caller could use
> inDOMUtils::GetCSSStyleRules() and scan the rules for the first style rule
> setting the font-family property.

Tracking this every time we do font selection just so that we can return it in the (occasional) case where someone actually queries for the fonts seems like it might be more overhead than is justified by the use cases. I've been aiming to provide as much potentially useful information as we reasonably can retrieve _without_ adding any significant time or memory cost to the overall process. (There's the added field in the GlyphRun to track how the font was matched, and I was hesitant about that; if it shows up as significant, we may want to reconsider just how much we care about exposing this.)

Comment 29

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

(In reply to comment #28)
> (I'd have liked to be able to return the name that was actually used
> in the CSS, but I don't see any way to retrieve that without adding extra
> overhead to the font-selection process, and I don't want to do that just for
> the sake of this API.)

Anyway, it's not unique right? A font could be selected using both "Times New Roman" and "serif" on different elements, for example.

> > This is the index for some font-family property, but we're not specifying
> > which one. It looks like you use the font-family property for the first
> > frame that uses this font as you traverse the frame tree. 
> 
> Yes. (That's somewhat arbitrary, but seemed easiest to understand.) My
> assumption was that someone wanting to actually use this would need to make
> a targeted query for a single node/frame, in order to have a context where
> this is meaningful.
> 
> Alternatively, we might not want to expose this at all - I don't know if
> there's any compelling use case. It just seemed like something we could
> (cheaply) provide, and that might sometimes be of interest.

It seems useful for the use-case of "show me the style rules that cause us to use this font".

> > So to use this
> > value, you really need to provide the style property as well in the form of
> > a nsIDOMCSSStyleDeclaration. Or alternatively you could return the DOM node
> > you found (possibly an anonymous node) and the caller could use
> > inDOMUtils::GetCSSStyleRules() and scan the rules for the first style rule
> > setting the font-family property.
> 
> Tracking this every time we do font selection just so that we can return it
> in the (occasional) case where someone actually queries for the fonts seems
> like it might be more overhead than is justified by the use cases. I've been
> aiming to provide as much potentially useful information as we reasonably
> can retrieve _without_ adding any significant time or memory cost to the
> overall process. (There's the added field in the GlyphRun to track how the
> font was matched, and I was hesitant about that; if it shows up as
> significant, we may want to reconsider just how much we care about exposing
> this.)

I don't think you need to modify font selection to get this. As you walk the frame tree looking for fonts, for each text frame that uses this font you can examine the style rules applying to it and determine which style rule is supplying font-family to the frame. If a glyph run for the textframe uses the font in question, and the font for that glyph run was chosen from the CSS family, then you know the rule that was used to choose it.

Hmm, there is a problem with the glyph run matching data though. A single glyph run could span element boundaries and be selected by CSS in one element and as fallback in another!

Comment 30

[Jonathan Kew [:jfkthame]]

(In reply to comment #29)
> Hmm, there is a problem with the glyph run matching data though. A single
> glyph run could span element boundaries and be selected by CSS in one
> element and as fallback in another!

I think the modifications to AddGlyphRun in patch 3 mean that in this case we'll now record two separate glyph runs instead of merging them. Though perhaps that's not desirable, and we should instead OR together the match types and merge the runs in this case.

Comment 31

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

Brilliant! So you're all set.

Comment 32

[Karl Tomlinson (:karlt)]

Comment on attachment 534742 [details] [diff] [review]
part 3 - track how we ended up selecting the given font

I don't know whether this has been considered but something to bear in mind is
that glyph runs are not the only users of font matching info.  It is quite
possible that the line-height and baseline are determined by the first font in
the fontgroup, but all the glyphs come from other fonts.

(In reply to comment #17)
> This works fine on Windows and OS X, but the Linux code (in
> gfxPangoFontGroup::FindFontForChar) is just a quick hack to make it build
> and run - it does not properly track whether fonts come from CSS, prefs,
> etc. Karl, is this something you could look at? I'm sure you have a much
> better handle on the Pango/FontConfig stuff with patterns and fontsets etc
> than I do....

My first thought was that this is too hard for gfxPangoFonts, but having
looked at the implementation for other platforms, I realize this doesn't do
what I thought you were trying to do anyway.

This is more an indication of what happened in our font-matching algorithm
than any mapping back to CSS.  In that respect, I think the gfxPangoFonts
implementation is fine.

However, the name "kCSSFontFamily" is not a good representation of what it
means.  Something like kFontGroup would be more appropriate.

> const nsString& gfxFontEntry::FamilyName() const
> {
>-    NS_ASSERTION(mFamily, "gfxFontEntry is not a member of a family");
>-    return mFamily->Name();
>+    if (mFamily) {
>+        return mFamily->Name();

Not something new here, but the mapping from gfxFontEntry to family is not
unique.  There may be two different but overlapping sets of faces, where each
set contains all faces having one family name.

>@@ -2332,17 +2336,19 @@ gfxFontGroup::MakeSpaceTextRun(const Par

>         // Short-circuit for size-0 fonts, as Windows and ATSUI can't handle
>         // them, and always create at least size 1 fonts, i.e. they still
>         // render something for size 0 fonts.
>-        textRun->AddGlyphRun(font, 0);
>+        textRun->AddGlyphRun(font,
>+                             gfxTextRange::MatchInfo(gfxTextRange::kCSSFontFamily, 0),
>+                             0, PR_FALSE);

We don't know that the CSS-specified font exists.
kFontGroup would make sense here.

>+        if (matchedFont) {

>+        } else {
>+            aTextRun->AddGlyphRun(mainFont,
>+                                  gfxTextRange::MatchInfo(gfxTextRange::kCSSFontFamily, 0),
>+                                  runStart, (matchedLength > 0));
>+        }

Something similar here.
Here we even know that no font supports this character.

>     // 1. check fonts in the font group
>     for (PRUint32 i = 0; i < FontListLength(); i++) {
>         nsRefPtr<gfxFont> font = GetFontAt(i);
>-        if (font->HasCharacter(aCh))
>+        if (font->HasCharacter(aCh)) {
>+            aMatchInfo.mType = gfxTextRange::kCSSFontFamily;
>+            aMatchInfo.mIndex = i;

Again, here.
We don't know that GetFontAt(i) comes from a CSS-specified family.

Hasn't the font list already been truncated to remove non-existing fonts?
That would mean the index doesn't directly relate to CSS.
ISTR that there are already some pref fonts in this list for "serif", etc.
(including when no generic was explicitly provided), and a fallback font is used if necessary.

>+    gfxTextRange::MatchInfo matchInfo;
> 
>     for (PRUint32 i = 0; i < len; i++) {

It would be nice to declare matchInfo inside the loop to make it clear that
the info is not carried from one iteration to the next.  Perhaps that would
mean rethinking the initialization in the default constructor.

>             gfxTextRange r(0,1);
>             r.font = font;
>+            r.matchInfo = matchInfo;

>                 gfxTextRange r(origI, i+1);
>                 r.font = font;
>+                r.matchInfo = matchInfo;

I wonder why font and matchInfo are not passed to the gfxTextRange constructor.

>@@ -4226,18 +4256,18 @@ gfxTextRun::SetSpaceGlyph(gfxFont *aFont

>+    AddGlyphRun(aFont, gfxTextRange::MatchInfo(gfxTextRange::kCSSFontFamily, 0),
>+                aCharIndex, PR_FALSE);

Another GetFontAt(0) situation, I assume.  Do any callers pass anything else?
Should the aFont parameter to SetSpaceGlyph be removed and replaced with
GetFontGroup()->GetFontAt(0)?

>     virtual already_AddRefed<gfxFont>
>         FindFontForChar(PRUint32 ch, PRUint32 prevCh, PRInt32 aRunScript,
>-                        gfxFont *aPrevMatchedFont);
>+                        gfxFont *aPrevMatchedFont,
>+                        gfxTextRange::MatchInfo& aMatchInfo);

Usually we pass out-parameters by pointer.  It makes it clearer that the
parameter in the function is not a local copy of the variable, and that the
caller's variable can be altered.

>+    if (!(mMatchInfo.mType & gfxTextRange::kCSSFontFamily) &&
>+         (aMatchInfo.mType & gfxTextRange::kCSSFontFamily))

I don't find the odd alignment here helps understanding.
I first thought the ! applied to the whole expression.

Sounds like the awkwardness of a single index but possibly multiple match
types has already been discussed.

Comment 33

[Jonathan Kew [:jfkthame]]

(In reply to comment #25)
> Comment on attachment 534745 [details] [diff] [review] [review]
> part 6 - return the real font name rather than our internal identifier

> Why are we storing mRealName in the userData as well as being able to get it
> from the gfxFontEntry?

There are two separate issues here that need to be addressed for nsIDOMFontFace to be able to return the proper name for presentation purposes:

(1) For installed fonts: On some platforms, the "name" that we use as an identifier in the gfxFontEntry subclass is not really the "font name" as found in the font resource, but something else that's more convenient/efficient for our internal use. E.g. on Linux, we use the pathname of the font file; with DWrite, we construct a "fake" fullname from the family and style names that DW gives us, to save having to actually read the name table - but this often differs from the name that's actually present in the font resource. So this is why gfxFontEntry::RealName() is provided, and reads the 'name' table (falling back to our internal "name" if this fails - e.g. non-sfnt fonts).

We might want to specialize this further in some subclasses - e.g. with Freetype on Linux we could use FT APIs to get the name, regardless of the font type.

Solving this issue here, rather than requiring the gfxFontEntry to always have the proper font name, means that we only pay the cost of manually reading the 'name' table in the (rare, non-perf-critical) case of nsIDOMFontFace queries, not for every font we use. (We could cache the real name in the font entry once we've read it, if we think it's likely to be wanted many times, but I don't think it's worth it.)

(2) For user fonts: In some cases, we replace the 'name' table in the font as part of our platform font activation process. This means that gfxFontEntry::RealName() cannot retrieve the desired font name from the font tables. So for user fonts, extracting the proper name and attaching it to the gfxUserFontData record means that we still have a way to return the true internal name of the font resource from nsIDOMFontFace, even after it's been munged during activation on the platform.

The number of user font entries present at any given time is normally fairly small, and the cost of extracting the name is minor when considered as part of the overall process of downloading, validating and activating the resource.

Comment 34

[Jonathan Kew [:jfkthame]]

(In reply to comment #32)
> Comment on attachment 534742 [details] [diff] [review] [review]
> >+    gfxTextRange::MatchInfo matchInfo;
> > 
> >     for (PRUint32 i = 0; i < len; i++) {
> 
> It would be nice to declare matchInfo inside the loop to make it clear that
> the info is not carried from one iteration to the next.

Actually, it _is_ deliberately carried over: in the case where FindFontForChar returns the previously-matched font, we want to preserve the existing value of matchInfo rather than reinitialize it.

> >             gfxTextRange r(0,1);
> >             r.font = font;
> >+            r.matchInfo = matchInfo;
> 
> >                 gfxTextRange r(origI, i+1);
> >                 r.font = font;
> >+                r.matchInfo = matchInfo;
> 
> I wonder why font and matchInfo are not passed to the gfxTextRange
> constructor.

Re font: I'm not sure why... "that's just how it was". Re matchInfo: I didn't add that to the constructor because there's also a usage of gfxTextRange (in gfxDWriteShaper) that doesn't care about font or matchInfo, only the extent of the range. But I think the more correct thing to do is to eliminate the use of gfxTextRange there, it's not really needed. So I've updated the patch accordingly.

> >@@ -4226,18 +4256,18 @@ gfxTextRun::SetSpaceGlyph(gfxFont *aFont
> 
> >+    AddGlyphRun(aFont, gfxTextRange::MatchInfo(gfxTextRange::kCSSFontFamily, 0),
> >+                aCharIndex, PR_FALSE);
> 
> Another GetFontAt(0) situation, I assume.  Do any callers pass anything else?

Not at present, I believe.

> Should the aFont parameter to SetSpaceGlyph be removed and replaced with
> GetFontGroup()->GetFontAt(0)?

I think we could; but on the other hand, there's a case where we may eventually want to call it with a different font parameter. Currently, it looks like we always use the first font in the fontGroup for the inter-word spaces in a text run, and I think we should consider changing that: if the words on both sides of a space both resolved to the same (non-first) font, then ISTM that it would be better for the inter-word space to also be drawn from that font. To support that, we'd need the aFont parameter here.

Anyhow, that's for a separate bug, if anything.

Comment 35

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

(In reply to comment #33)

OK.

Comment 36

[Jonathan Kew [:jfkthame]]

Attachment: part 1 v2 - get list of fonts used in a range

Comment 37

[Jonathan Kew [:jfkthame]]

Attachment: part 2 v2 - find the @font-face rule responsible for a font

Carrying forward r=roc

Comment 38

[Jonathan Kew [:jfkthame]]

Attachment: part 3 v2 - track how we ended up selecting the given font

Updated following comments from roc & karlt.

Comment 39

[Jonathan Kew [:jfkthame]]

Attachment: part 4 v2 - implement a bunch more attributes for user fonts

Comment 40

[Jonathan Kew [:jfkthame]]

Attachment: part 5 v2 - provide access to the WOFF metadata block

Carrying forward r=roc.

Comment 41

[Jonathan Kew [:jfkthame]]

Attachment: part 6 v2 - return the real font name rather than our internal identifier

Comment 42

[Jonathan Kew [:jfkthame]]

Attachment: part 7 - chrome mochitest to exercise the font-inspector API

(This is big because it includes a copy of a Gentium font in woff form for test purposes.)

Comment 43

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

Comment on attachment 537359 [details] [diff] [review]
part 1 v2 - get list of fonts used in a range

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

::: layout/base/nsLayoutUtils.cpp
@@ +4119,5 @@
> +      }
> +      if (fend > aEndOffset) {
> +        fend = aEndOffset;
> +      }
> +    }

Simpler to write
  PRInt32 fstart = NS_MAX(offset, aStartOffset);
  PRInt32 fend = NS_MIN(curr->GetContentEnd(), aEndOffset);
  if (fstart == fend)
    continue;

Comment 44

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

Comment on attachment 537361 [details] [diff] [review]
part 3 v2 - track how we ended up selecting the given font

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

::: layout/inspector/src/nsFontFace.cpp
@@ +65,5 @@
>  NS_IMETHODIMP
>  nsFontFace::GetFromFontGroup(PRBool * aFromFontGroup)
>  {
> +  *aFromFontGroup =
> +    (mMatchType & gfxTextRange::kFontGroup) ? PR_TRUE : PR_FALSE;

Use != 0 instead of  ? :. Same for the other two occurrences.

Comment 45

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

Comment on attachment 537362 [details] [diff] [review]
part 4 v2 - implement a bunch more attributes for user fonts

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

Comment 46

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

Comment on attachment 537364 [details] [diff] [review]
part 6 v2 - return the real font name rather than our internal identifier

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

::: gfx/thebes/gfxFontUtils.cpp
@@ +1451,5 @@
> +    const SFNTHeader *sfntHeader =
> +        reinterpret_cast<const SFNTHeader*>(aFontData);
> +    const TableDirEntry *dirEntry =
> +        reinterpret_cast<const TableDirEntry*>(aFontData + sizeof(SFNTHeader));
> +    PRUint32 numTables = sfntHeader->numTables;

Don't we need a length check before we read this field?

@@ +1454,5 @@
> +        reinterpret_cast<const TableDirEntry*>(aFontData + sizeof(SFNTHeader));
> +    PRUint32 numTables = sfntHeader->numTables;
> +    PRBool foundName = PR_FALSE;
> +    for (PRUint32 i = 0; i < numTables; i++, dirEntry++) {
> +        if (dirEntry->tag == TRUETYPE_TAG('n','a','m','e')) {

How about length check(s) to make sure these reads are OK?

@@ +1468,5 @@
> +    PRUint32 len = dirEntry->length;
> +    if (!nameTable.SetLength(len)) {
> +        return NS_ERROR_OUT_OF_MEMORY;
> +    }
> +    memcpy(nameTable.Elements(), aFontData + dirEntry->offset, len);

Some length check needs to cover this too.

Comment 47

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

Comment on attachment 537359 [details] [diff] [review]
part 1 v2 - get list of fonts used in a range

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

::: content/base/src/nsRange.cpp
@@ +2249,5 @@
> +
> +  // Flush out layout so our frames are up to date.
> +  nsIDocument* doc = mStartParent->GetOwnerDoc();
> +  NS_ENSURE_TRUE(doc, NS_ERROR_UNEXPECTED);
> +  doc->FlushPendingNotifications(Flush_Style);

Flush_Frames is probably better than Flush_Style here, even though they're currently the same value.

Comment 48

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

Comment on attachment 537366 [details] [diff] [review]
part 7 - chrome mochitest to exercise the font-inspector API

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

You don't seem to be testing fromFontGroup/fromLanguagePres/fromSystemFallback ... nor CSSFamilyName.

::: layout/inspector/tests/chrome/test_bug467669.xul
@@ +34,5 @@
> +  fonts = domUtils.getUsedFontFaces(rng);
> +  ok(fonts.length == 1, "expected 1 font for simple Latin text, got " + fonts.length);
> +  f = fonts.item(0);
> +  ok(!f.rule, "No @font-face rule expected");
> +  ok(f.srcIndex == -1, "srcIndex should be -1, got " + f.srcIndex);

Instead of ok(a == b), use is(). then you won't need the explicit "expected, ..., got ..." in your tests.

Comment 49

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

Comment on attachment 537366 [details] [diff] [review]
part 7 - chrome mochitest to exercise the font-inspector API

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

You don't seem to be testing fromFontGroup/fromLanguagePres/fromSystemFallback ... nor CSSFamilyName.

::: layout/inspector/tests/chrome/test_bug467669.xul
@@ +114,5 @@
> +
> +  SimpleTest.finish();
> +}
> +
> +function report(e, f) {

Actually modifying the test DOM feels a bit dodgy. What's this reporting actually needed for? Maybe a better way to report results would be to use dump(e.id + " fonts: " + ...), or even ok(true, e.id + " fonts: " + ...).

Comment 50

[Jonathan Kew [:jfkthame]]

(In reply to comment #46)
> Comment on attachment 537364 [details] [diff] [review] [review]
> part 6 v2 - return the real font name rather than our internal identifier
> 
> Review of attachment 537364 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxFontUtils.cpp
> @@ +1451,5 @@
> > +    const SFNTHeader *sfntHeader =
> > +        reinterpret_cast<const SFNTHeader*>(aFontData);
> > +    const TableDirEntry *dirEntry =
> > +        reinterpret_cast<const TableDirEntry*>(aFontData + sizeof(SFNTHeader));
> > +    PRUint32 numTables = sfntHeader->numTables;
> 
> Don't we need a length check before we read this field?
> 
> @@ +1454,5 @@
> > +        reinterpret_cast<const TableDirEntry*>(aFontData + sizeof(SFNTHeader));
> > +    PRUint32 numTables = sfntHeader->numTables;
> > +    PRBool foundName = PR_FALSE;
> > +    for (PRUint32 i = 0; i < numTables; i++, dirEntry++) {
> > +        if (dirEntry->tag == TRUETYPE_TAG('n','a','m','e')) {
> 
> How about length check(s) to make sure these reads are OK?
> 
> @@ +1468,5 @@
> > +    PRUint32 len = dirEntry->length;
> > +    if (!nameTable.SetLength(len)) {
> > +        return NS_ERROR_OUT_OF_MEMORY;
> > +    }
> > +    memcpy(nameTable.Elements(), aFontData + dirEntry->offset, len);
> 
> Some length check needs to cover this too.

I don't mind adding checks, but they're not strictly needed because this function is called only after the font file has been through the OTS sanitizer (or, if the sanitizer is disabled, after we've called ValidateSFNTHeaders, which also checks all these values).

Comment 51

[Jonathan Kew [:jfkthame]]

(In reply to comment #48)
> You don't seem to be testing
> fromFontGroup/fromLanguagePres/fromSystemFallback ... 

It'll be hard to test these in a platform-independent way. On Linux, the gfxPangoFonts code doesn't support them anyway, as the use of fontconfig patterns means that the whole font-matching structure is quite different.

We could at least do a Win & Mac test that covers them, I guess.

> nor CSSFamilyName.

Oh yes - definitely should do something with that.

Comment 52

[Jonathan Kew [:jfkthame]]

(In reply to comment #49)
> Actually modifying the test DOM feels a bit dodgy. 

If it breaks anything, presumably that'd be a bug we should fix! :)

> What's this reporting
> actually needed for? 

It's not needed at all, it was just handy when running the test manually to be able to confirm what fonts were being found.

Comment 53

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

(In reply to comment #50)
> I don't mind adding checks, but they're not strictly needed because this
> function is called only after the font file has been through the OTS
> sanitizer (or, if the sanitizer is disabled, after we've called
> ValidateSFNTHeaders, which also checks all these values).

OK, but please clearly comment along the flow of control that such validation must have happened.

(In reply to comment #51)
> We could at least do a Win & Mac test that covers them, I guess.

That'd be good.

(In reply to comment #52)
> It's not needed at all, it was just handy when running the test manually to
> be able to confirm what fonts were being found.

OK. I would convert them to commented-out 'dump' calls.

Comment 54

[Jonathan Kew [:jfkthame]]

Attachment: part 6 v3 - return the real font name rather than our internal identifier

Added sanity-checks in GetFullNameFromSFNT, although in principle they should be redundant; also added comments indicating where validation happens before this is called.

Comment 55

[Jonathan Kew [:jfkthame]]

Comment on attachment 537359 [details] [diff] [review]
part 1 v2 - get list of fonts used in a range

Requesting sr? as this introduces new APIs to query an nsIDOMNSRange for the fonts actually used to render, and nsIDOMFontFace and nsIDOMFontFaceList for the results of this query.

Comment 56

[Jonathan Kew [:jfkthame]]

Attachment: part 7 v2 - chrome mochitest to exercise the font-inspector API

Tidied up, and added tests for CSS family name, and for fromFontGroup/fromLanguagePrefs properties on Mac & Win. (It's difficult to reliably predict what characters will end up using the fromSystemFallback path, as this depends on details of the installed fonts as well as the browser font prefs.)

Comment 57

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

Comment on attachment 537359 [details] [diff] [review]
part 1 v2 - get list of fonts used in a range

I believe dbaron is still travelling

Comment 58

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

Comment on attachment 537572 [details] [diff] [review]
part 6 v3 - return the real font name rather than our internal identifier

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

Comment 59

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

Comment on attachment 537648 [details] [diff] [review]
part 7 v2 - chrome mochitest to exercise the font-inspector API

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

Comment 60

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

Is it really not possible for the match flags to be implemented on Linux? Even if we can't tell prefs vs system fallback apart, being able to tell the difference between CSS fontgroup and fallback would be nice.

Comment 61

[Karl Tomlinson (:karlt)]

(In reply to comment #60)
> Is it really not possible for the match flags to be implemented on Linux?
> Even if we can't tell prefs vs system fallback apart, being able to tell the
> difference between CSS fontgroup and fallback would be nice.

Fontgroup is not necessarily CSS, on any platform.  It may include prefs or system fallback (UI font or similar).  I expect it is hard to come up with something on Linux, but it does not seem worth the effort, especially given the difference is weakly defined even on other platforms.

Comment 62

[Karl Tomlinson (:karlt)]

An option to consider might be to make the distinction kBaseFonts/kScriptFallback/kSystemFallback or similar to distinguish whether the base fonts (usually including a pref font based on the HTML language or locale) or whether fonts for some other script are used (where the script is determined from the characters).
The difference between kBaseFonts and kScriptFallback can easily be implemented in gfxPangoFonts, but kSystemFallback would be much harder.

Does the kBaseFonts/kScriptFallback make sense on other platforms?
(There is a similar distinction there as I remember it, but I haven't studied carefully recently.)

Comment 63

[Jonathan Kew [:jfkthame]]

Attachment: WIP: simple addon to show fonts in selected text

This is a bare-bones font inspector add-on, mostly to check that it can be done. :) Adds a "Show Fonts Used" item to the Firefox content window contextual menu; this just displays an alert listing the names of the fonts used in the selected range of text.

Once we land this bug, we could make something like this available through AMO.

Comment 64

[Boris Zbarsky [:bzbarsky]]

So before I dig into the API details, is there a reason we're putting the range API on nsIDOMNSRange, where it's visible to web content, instead of nsIRange?

Comment 65

[Jonathan Kew [:jfkthame]]

(In reply to comment #64)
> So before I dig into the API details, is there a reason we're putting the
> range API on nsIDOMNSRange, where it's visible to web content, instead of
> nsIRange?

Not that I can see... it's really just an internal utility for layout/inspector/src/inDOMUtils.cpp to use. So we can easily declare it on nsIRange instead, I think.

Comment 66

[Jonathan Kew [:jfkthame]]

Attachment: part 1 v3 - get list of fonts used in a range

Like this, you mean?

Comment 67

[Boris Zbarsky [:bzbarsky]]

>+  // Flush out layout so our frames are up to date.

Do you mean to flush frames or layout?  If it's the former, change the comment accordingly; if it's the latter, you need Flush_Layout.  Given that you use offsets, you probably want Flush_Layout.

>+         continue;
>+       } else if (node == endContainer) {

No need for else after continue.

>+        if (child->GetType() == nsGkAtoms::placeholderFrame) {
>+          nsPlaceholderFrame* placeholder =
>+            static_cast<nsPlaceholderFrame*>(child);
>+          child = placeholder->GetOutOfFlowFrame();
>+        }

 child = nsPlaceholderFrame::GetRealFrameFor(child);

>+    aFrame = aFrame->GetNextContinuation();

What about special siblings?

Also, this will walk a frame multiple times if the parent has continuations that contain continuations for the frame.  I'd think we want to walk the continuation chain only on the initial entrypoint and then just recurse down our child lists.  That way we don't have to walk our textframe continuations either, when called from GetFontFacesForFrames.

Comment 68

[Boris Zbarsky [:bzbarsky]]

One other thing.  File a followup on making that iterator return nsINode?

Comment 69

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

(In reply to comment #67)
> Do you mean to flush frames or layout?  If it's the former, change the
> comment accordingly; if it's the latter, you need Flush_Layout.  Given that
> you use offsets, you probably want Flush_Layout.

Actually I don't think it matters if we use a stale layout, because that shouldn't affect font selection. It doesn't really matter though, it probably makes sense to flush layout just to be safe.

> >+    aFrame = aFrame->GetNextContinuation();
> 
> What about special siblings?

Good catch, we do need to walk the special-sibling list...

Comment 70

[Jonathan Kew [:jfkthame]]

Attachment: part 1 v4 - get list of fonts used in a range - updated

So is it sufficient to use GetNextContinuationOrSpecialSibling(aFrame) in place of aFrame->GetNextContinuation() in the loop, or do we need to do more than that? I'm not really familiar with all this frame-tree stuff...

Comment 71

[Boris Zbarsky [:bzbarsky]]

> So is it sufficient to use GetNextContinuationOrSpecialSibling(aFrame)

Yes.

Comment 72

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 538288 [details] [diff] [review]
part 1 v4 - get list of fonts used in a range - updated

sr=me

Comment 73

[Jonathan Kew [:jfkthame]]

Attachment: part 4.1 - fix Mac font backend to mark local user fonts properly

It turned out during testing on OS X that the Mac font-list code doesn't correctly set the font entry flags for user fonts with src:local(...). That causes nsFontFace::GetLocalName() to fail for such fonts.

Comment 74

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

Comment on attachment 539173 [details] [diff] [review]
part 4.1 - fix Mac font backend to mark local user fonts properly

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

Comment 75

[Jonathan Kew [:jfkthame]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/592b0c82089d (pt 1)
http://hg.mozilla.org/integration/mozilla-inbound/rev/8c602c722bca (pt 2)
http://hg.mozilla.org/integration/mozilla-inbound/rev/05c732013acb (pt 3)
http://hg.mozilla.org/integration/mozilla-inbound/rev/52b41635185e (pt 4)
http://hg.mozilla.org/integration/mozilla-inbound/rev/66fd359aa53c (pt 4.1)
http://hg.mozilla.org/integration/mozilla-inbound/rev/e843b8a89772 (pt 5)
http://hg.mozilla.org/integration/mozilla-inbound/rev/a83c312c64ab (pt 6)
http://hg.mozilla.org/integration/mozilla-inbound/rev/0a90750a887d (pt 7)

Comment 76

[Jonathan Kew [:jfkthame]]

This was backed out from -incoming (thanks, dbaron & ehsan) due to bustage on windows & android.

http://hg.mozilla.org/integration/mozilla-inbound/rev/ed3cf3967b69

Comment 77

[Jonathan Kew [:jfkthame]]

Relanded on m-i with appropriate bustage fixes (and a clean try run):

http://hg.mozilla.org/integration/mozilla-inbound/rev/08a6a9f91d6e
http://hg.mozilla.org/integration/mozilla-inbound/rev/14eb4f38aff8
http://hg.mozilla.org/integration/mozilla-inbound/rev/60365868c2c1
http://hg.mozilla.org/integration/mozilla-inbound/rev/cd6ab4bf765e
http://hg.mozilla.org/integration/mozilla-inbound/rev/db0de7fffb38
http://hg.mozilla.org/integration/mozilla-inbound/rev/d78e452a44d3
http://hg.mozilla.org/integration/mozilla-inbound/rev/7f9f323a7313
http://hg.mozilla.org/integration/mozilla-inbound/rev/dea94e351c69

Comment 78

[Mounir Lamouri (:mounir)]

Merged to m-c:
http://hg.mozilla.org/mozilla-central/rev/08a6a9f91d6e
http://hg.mozilla.org/mozilla-central/rev/14eb4f38aff8
http://hg.mozilla.org/mozilla-central/rev/60365868c2c1
http://hg.mozilla.org/mozilla-central/rev/cd6ab4bf765e
http://hg.mozilla.org/mozilla-central/rev/db0de7fffb38
http://hg.mozilla.org/mozilla-central/rev/d78e452a44d3
http://hg.mozilla.org/mozilla-central/rev/7f9f323a7313
http://hg.mozilla.org/mozilla-central/rev/dea94e351c69

Comment 80

[Jan Honza Odvarko [:Honza] (always need-info? me)]

In Firebug we would like to display meta data coming from a *.woff file. Is it now possible to parse the file and get the meta-data?

Imagine that Firebug net panel could parse corresponding response for a *.woff file and display nice meta-data information (instead of just a binary response). Would something like that be possible, given that this bug is fixed? Or should I report a new bug?

Here is the original Firebug issue report with a screenshot how the feature would look like from user perspective.
http://code.google.com/p/fbug/issues/detail?id=3071

Honza

Comment 81

[Jonathan Kew [:jfkthame]]

You should be able to do this now: the "metadata" attribute of nsIDOMFontFace will contain the WOFF metadata as a string, which you can parse and display (if it's present - not every file provides it).

The fontinfo extension (https://addons.mozilla.org/en-US/firefox/addon/fontinfo/) does this in a fairly simplistic way (it doesn't include full language handling, to choose and display the most appropriate version of localized metadata), but you could refer to that as a starting point.

Comment 82

[Jonathan Kew [:jfkthame]]

(Though nsIDOMFontFace won't help you get WOFF metadata, etc, directly from the network response, if that's specifically what you want to do. The expectation here is that you start with a DOM range of interest - whether an entire document, a single character, or something in between - query it to get its nsIDOMFontFace(s), and then examine the face's attributes. We don't have APIs to help you directly examine the binary blob coming from the network.)

Comment 83

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Jonathan Kew from comment #82)
> (Though nsIDOMFontFace won't help you get WOFF metadata, etc, directly from
> the network response, if that's specifically what you want to do.
Yep, that's exactly what I wanted.

> The
> expectation here is that you start with a DOM range of interest - whether an
> entire document, a single character, or something in between - query it to
> get its nsIDOMFontFace(s), and then examine the face's attributes.
I see

> We don't
> have APIs to help you directly examine the binary blob coming from the
> network.)
Is there a plan to have such API?
I guess it could be a matter of just exposing an existing API to JS, no?

Honza

Comment 84

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

It would probably be easier to just parse the data yourself. Finding the right table in the file is not hard.

Comment 85

[Sebastian Zartner [:sebo]]

Maybe not hard, but also not that nice and error-prone, since you're just getting the info indirectly.

Comment 86

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

Someone only has to write it once and package it in a library. And that library can work on any browser.

Creating a new browser API just for this seems overkill. It would be slightly tricky to reuse our existing code.

Comment 87

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #84)
> It would probably be easier to just parse the data yourself. Finding the
> right table in the file is not hard.
Is there any doc I could read in order to understand the structure?
Honza

Comment 88

[Jonathan Kew [:jfkthame]]

(In reply to Jan Honza Odvarko from comment #87)
> Is there any doc I could read in order to understand the structure?

http://dev.w3.org/webfonts/WOFF/spec/

Comment 89

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/inIDOMUtils
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIDOMFontFaceList
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIDOMFontFace

And already mentioned on Firefox 7 for developers.