Implement parsing of @font-face rules within the style system. Spec: http://dev.w3.org/csswg/css3-fonts/

Note: old version of a potential patch attached to main @font-face bug, bug 70132

wanted1.9.1+, P1.

I went through css3-fonts and pulled a grammar out of it. It's underspecified in quite a few places, contradicts itself in others, and seems to have a couple of pieces missing; so I'd appreciate it if folks would read over this and check it, especially the #comments. I have about 1/4 of an implementation. font-face-rule: '@font-face' '{' <font-description> '}' font-description: <font-descriptor>+ font-descriptor: <font-family-desc> | <font-src-desc> | <font-style-desc> | <font-weight-desc> | <font-stretch-desc> | <font-variant-desc> | <font-size-desc> # font-variant-desc and font-size-desc are missing from the spec, # but logically should be allowed font-family-desc: 'font-family' ':' <family-name> ';' # spec says "Trimmed out multiple values" - possible compatibility issue # spec wonders whether to allow a trailing !important to override a # system font font-src-desc: 'src' ':' <font-src-val> [ ',' <font-src-val> ]* ';' font-src-val: <font-face-uri> | <font-face-name> font-face-uri: <uri> [ 'format(' <string> [ ',' <string> ]* ')' ] # example XXVIII uses unquoted identifiers, but is marked as obsolete font-face-name: 'local(' ( <string> | <ident> ) ')' # spec does not actually nail down what goes inside the # parentheses; examples suggest an <ident>, but a <string> would # allow more complex names as may be necessary font-style-desc: 'font-style' ':' <font-style-val> [ ',' <font-style-val> ]* ';' font-style-val: 'normal' | 'italic' | 'oblique' # 4.4 says "a comma-separated list is permitted" for this but no # other font characteristics; text in 4.1 suggests all should take # comma-sep lists; text should exclude 'inherit' from this and other # value lists (done in grammar) font-weight-desc: 'font-weight' ':' <font-weight-val> [ ',' <font-weight-val> ]* ';' font-weight-val: 'normal' | 'bold' | '100' | '200' | '300' | '400' | '500' | '600' | '700' | '800' | '900' font-stretch-desc: 'font-stretch' ':' <font-stretch-val> [ ',' <font-stretch-val> ]* ';' font-stretch-val: 'normal' | 'wider' | 'narrower' | 'ultra-condensed' | 'extra-condensed' | 'condensed' | 'semi-condensed' | 'semi-expanded' | 'expanded' | 'extra-expanded' | 'ultra-expanded' # apparently missing descriptors: font-variant-desc: 'font-variant' ':' <font-variant-val> [ ',' <font-variant-val> ]* ';' font-variant-val: 'normal' | 'small-caps' font-size-desc: 'font-size' ':' <length> [ ',' <length> ]* ';' # all other values for the font-size property make no sense

(In reply to comment #3) > I went through css3-fonts and pulled a grammar out of it. It's > underspecified in quite a few places, contradicts itself in others, > and seems to have a couple of pieces missing Yeah, totally my fault I'm afraid. ;) To some extent we're doing this backwards because the CSS2 spec was a monolith and never implemented. Safari implements a set of features that roughly matches this spec, Opera I haven't tested. > font-family-desc: 'font-family' ':' <family-name> ';' > # spec says "Trimmed out multiple values" - possible compatibility issue Safari only implements single family names. > # spec wonders whether to allow a trailing !important to override a > # system font An issue here is whether an @font-face family name overrides a font family name on your system. If an author specifies @font-face { family-name: Arial; src: local(Times); } > font-face-uri: <uri> [ 'format(' <string> [ ',' <string> ]* ')' ] > # example XXVIII uses unquoted identifiers, but is marked as obsolete Yeah, there was a comment by the Opera guy working on @font-face about this: http://lists.w3.org/Archives/Public/www-style/2008Jun/0199.html Seems like quotes shouldn't be necessary here. > font-face-name: 'local(' ( <string> | <ident> ) ')' > # spec does not actually nail down what goes inside the > # parentheses; examples suggest an <ident>, but a <string> would > # allow more complex names as may be necessary We should allow quotes but not require them. The larger problem is how to specify *what* the meaning of a full font name is. I think the most we can specify here is "a name that uniquely identifies a font face". Note that this is *not* the same as a font family name. > font-style-desc: 'font-style' ':' <font-style-val> [ ',' <font-style-val> ]* > ';' > font-style-val: 'normal' | 'italic' | 'oblique' > # 4.4 says "a comma-separated list is permitted" for this but no > # other font characteristics; text in 4.1 suggests all should take > # comma-sep lists; text should exclude 'inherit' from this and other > # value lists (done in grammar) Argh, editor error here. I'll clean this up today. None of these font descriptors should be multi-valued. And, yeah, there's no cascade here, so inherit doesn't make sense. > # apparently missing descriptors Nope, I cleaned these out intentionally. No implementation that I can find uses font-variant properties when matching font faces. Before AAT and OpenType the only way to use variants was to create a separate font, now these are incorporated into the main font as a feature variation. And font-size is only needed for bitmap font matching, with outline fonts you don't match different faces for different sizes. Were there other problems you see?

(In reply to comment #4) > > Safari only implements single family names. Ok, so no compatibility concern, good. Abstractly, is there a *use* for multiple family names? Also, why is font-family a descriptor rather than being written @font-face <family-name> { ... } ? (Besides compatibility.) > An issue here is whether an @font-face family name overrides a font > family name on your system. Right, I saw that text in the spec. It seems a little weird to me to have @font-face not override system fonts. It's in some sense more specific than the system defaults. > > font-face-uri: <uri> [ 'format(' <string> [ ',' <string> ]* ')' ] > > # example XXVIII uses unquoted identifiers, but is marked as obsolete > > Yeah, there was a comment by the Opera guy working on @font-face about this: > > http://lists.w3.org/Archives/Public/www-style/2008Jun/0199.html > > Seems like quotes shouldn't be necessary here. So we should go with <string>|<ident> here, as for font-face-name? I certainly wouldn't object. > > font-face-name: 'local(' ( <string> | <ident> ) ')' > > # spec does not actually nail down what goes inside the > > # parentheses; examples suggest an <ident>, but a <string> would > > # allow more complex names as may be necessary > > We should allow quotes but not require them. The larger problem is how to > specify *what* the meaning of a full font name is. Well, the parser doesn't care what the string means. All the examples use <ident>, but I think we want to permit <string>, lest it turn out that <ident> doesn't cover some names that people want to use, and then we have another mess like url() requiring special cases in the lexer. > > # 4.4 says "a comma-separated list is permitted" for this but no > > # other font characteristics; text in 4.1 suggests all should take > > # comma-sep lists > > Argh, editor error here. I'll clean this up today. None of these font > descriptors should be multi-valued. That makes my life way easier, but I'm still going to query it: it sounded from the text like one might need multi-valued descriptors to cover things like multiple-weights-in-one-file fonts. Also, does/should src: still take multiple values? > > # apparently missing descriptors > > Nope, I cleaned these out intentionally. No implementation that I > can find uses font-variant properties when matching font faces. > Before AAT and OpenType the only way to use variants was to create a > separate font, now these are incorporated into the main font as a > feature variation. I'm not sure I follow. Just because you *can* combine the variant fonts into the main font doesn't mean you *have* to, right? What if someone wants to use both variants of their fancy schmancy header font that they got back in the 90s as a pair of TrueType files, normal and small-caps? > And font-size is only needed for bitmap font > matching, with outline fonts you don't match different faces for > different sizes. > Were there other problems you see? Well, I seem to have totally forgotten to include the unicode-range stuff; it would be nice to have a formal grammar for <urange> written by someone (not me) who knows what it's supposed to be. Also, while I personally support the future-compatible permission to use values up to U+7FFFFFFF, aren't people insisting that no, really, we're never going to go past U+10FFFF? (I don't buy it, me.) And, since this is not actually a <ruleset> even though it looks like one, could you check which of the CSS2.1 error recovery rules are relevant? In particular, should an empty descriptor @font-face { ...; ; ...; } be silently ignored as it is for <ruleset>, how much should one skip on seeing an unrecognized descriptor or a non-<ident> where a descriptor name should be, and is it ok to leave off the semicolon on the last descriptor before the close brace? (Personally I would favor making this as much like <ruleset> as possible both for implementation convenience and because people are going to assume it works the same way.)

Ok, I've checked in a new version of the spec with cleaned up language for font descriptors and unicode-range.

Fonts that contain multiple fonts such as TrueType collections are all-or-nothing deals. The use of font-weight, font-style, etc. is intended to obviate the need to download specific faces by allowing the style system to make assumptions about the characteristics of each face. But with a collection of faces, you'll need to download the whole thing, it's much better to keep the implementation of this simple. So please omit the parsing of mulitple values.

(In reply to comment #5) > Also, does/should src: still take multiple values? Yes.

As for error handling, what you describe makes sense. The CSS font spec, from version 1 onwards, says that font-variant is used as part of the font *matching* procedure. But that only makes sense in the context of a face that is a "small-caps face", a font family that includes a separate face for just small-caps. This is extremely rare and no really necessary since smart font technologies like OpenType or AAT allow you to include small-cap glyphs as alternate forms. To implement matching of small-cap faces is hard, you'd probably need to parse the style name which are typically localized so you would need to recognize n variations of "small caps" where n is the number of localizations contained in the font name table. With some fonts that's not a small number. So I'd really like to keep obsolete features that no one implements out of future implementations. ;)

I haven't finished it, but I've got enough code that I would appreciate someone looking at it and telling me whether I'm completely barking up the wrong tree here. So here's a partial patch. It should not be expected to compile - if nothing else, I haven't added entries to the translation lists for the new diagnostics, and more importantly, the new nsCSSFontFaceRule class doesn't exist. But there is parsing for the basic syntax and for all the descriptors that share syntax with regular properties.

ok, here's a patch which is almost complete. It compiles, and it should parse @font-face rules, but it doesn't actually do anything with the data. There are a whole bunch of stub methods on nsCSSFontFaceRule that need implementing. I'm stopping here because I am really not sure I did nsCSSFontFaceRule right and it looks like I now need to implement another XPCOM class, a specialization of nsIDOMCSSStyleDeclaration that actually holds the data from the @font-face. Or else I need to back up a ways and treat @font-face descriptors as regular properties so I can use regular nsCSSDeclaration (or whichever class is actually used by a regular style rule, I'm finding this corner of the class hierarchy *very* confusing). Anyway, requesting review at this stage of the parser code and the general approach to the data structure, and for boneheaded mistakes in the XPCOM goo. I note that a firefox-bin built with this patch crashes on startup, but the crash is somewhere in the profile code; I think it's unrelated breakage (earlier, an unmodified tree would *not* be persuaded to use the debug profile).

for the record, I have already noticed the missing newline at the end of css.properties, and it is fixed in my tree.

A few comments from skimming: The nsCSSValue.h change requires changing all the (aUnit <= eCSSUnit_Attr) tests in nsCSSValue.{h,cpp}. You can't change nsDOMClassInfoID.h without changing two other lists that need to be symmetric with it. And you're also supposed to add new entries to the end. (I think. Although a bunch of people haven't.) The big question is how similar we want the storage of descriptors/values to be to the storage of properties/values. Do we want to reuse nsCSSDataBlock.{h,cpp} ? that plus nsCSSDeclaration? Do we want the descriptors to have their own enumerated ID space or tack them on to the end of the property IDs? It's hard for me to have a good idea of how hard each of the options here would be without trying them...

(In reply to comment #13) > The nsCSSValue.h change requires changing all the (aUnit <= eCSSUnit_Attr) > tests in nsCSSValue.{h,cpp}. Is it ok if instead I put my new pseudo-units in between eCSSUnit_String and eCSSUnit_Attr (so eCSSUnit_Attr now has value 13)? > You can't change nsDOMClassInfoID.h without changing two other lists that need > to be symmetric with it. And you're also supposed to add new entries to the > end. (I think. Although a bunch of people haven't.) You're talking about the sClassInfoData array and the long series of DOM_CLASSINFO_MAP blocks in nsDOMClassInfo::Init, both in nsDomClassInfo.cpp, right? I had indeed missed those. And also the comments in nsDOMClassInfoID.h saying that yes, one is supposed to add new entries to the end. All these are fixed in my tree now. I think, based on comparison with how other CSS classes are done in there, that I can get away with just listing nsIDOMCSSFontFaceRule in the DOM_CLASSINFO_MAP block, yes? Unlike the QueryInterface definition? ... with luck, fixing this will make the crash on startup that I was just scratching my head over go away. I was pretty sure I had done *something* wrong with the XPCOM goo. > The big question is how similar we want the storage of descriptors/values > to be to the storage of properties/values. Do we want to reuse > nsCSSDataBlock.{h,cpp} ? that plus nsCSSDeclaration? Do we want the > descriptors to have their own enumerated ID space or tack them on to the > end of the property IDs? It's hard for me to have a good idea of how > hard each of the options here would be without trying them... Alas, this is just what I was hoping you had some thoughts on. Right now, making up a custom nsIDOMCSSStyleDeclaration implementation for the .style member of a nsIDOMCSSFontFaceRule is looking like a major headache. All the parser code is already written assuming we are not using nsCSSDataBlock nor nsCSSDeclaration for this, and changing that would also be a headache, but maybe it would come out less ugly in the end...

(In reply to comment #14) > (In reply to comment #13) > > The nsCSSValue.h change requires changing all the (aUnit <= eCSSUnit_Attr) > > tests in nsCSSValue.{h,cpp}. > > Is it ok if instead I put my new pseudo-units in between eCSSUnit_String and > eCSSUnit_Attr (so eCSSUnit_Attr now has value 13)? I think I'd prefer just changing the 5 or so occurrences and using an order that makes more sense (most to least generic). > > The big question is how similar we want the storage of descriptors/values > > to be to the storage of properties/values. Do we want to reuse > > nsCSSDataBlock.{h,cpp} ? that plus nsCSSDeclaration? Do we want the > > descriptors to have their own enumerated ID space or tack them on to the > > end of the property IDs? It's hard for me to have a good idea of how > > hard each of the options here would be without trying them... > > Alas, this is just what I was hoping you had some thoughts on. Right now, > making up a custom nsIDOMCSSStyleDeclaration implementation for the .style > member of a nsIDOMCSSFontFaceRule is looking like a major headache. All > the parser code is already written assuming we are not using nsCSSDataBlock > nor nsCSSDeclaration for this, and changing that would also be a headache, > but maybe it would come out less ugly in the end... Well, if you were using nsCSSDeclaration and nsCSSDataBlock, you'd get to that by compressing from some other data structure. Now that I think about it more, there's little point to using nsCSSDataBlock but not nsCSSDeclaration since the point of sharing the code would be to share the use of nsDOMCSSDeclaration and DOMCSSDeclarationImpl. I suspect it's probably easier to just do an additional implementation of nsIDOMCSSStyleDeclaration. (Maybe more code, but more isolated and less hairy.) I'm curious if Boris has an opinion here, though.

(In reply to comment #15) > I think I'd prefer just changing the 5 or so occurrences and using an order > that makes more sense (most to least generic). Done. As long as I was touching all of them I added a helper predicate. > I suspect it's probably easier to just do an additional implementation of > nsIDOMCSSStyleDeclaration. (Maybe more code, but more isolated and less > hairy.) Am trying to do this now. Incidentally, fixing nsDOMClassInfo did make the crash on startup go away.

Zack, do you have an updated patch with the changes you noted in comment #16? Thanks!

Here it is. The data structures still aren't complete, I need to write an implementation of nsIDOMCSSDeclaration that stores descriptors rather than properties. Hope to have that by the end of the day.

Creating a separate impl of nsIDOMCSSDeclaration makes sense to me, for what it's worth. We expect property descriptors to be pretty dense (in the sense that most will be specified), right, unlike the sparseness that nsCSSDeclaration has going on and that it's optimized for...

Here's a version of the patch which is complete enough that it should be possible to move forward with 441473. Things that should work: - Parsing of all @font-face descriptors except unicode-range. - The DOM objects created from an @font-face rule, and all their read-only methods and attributes (except the one that needs the nonexistent nsIDOMCSSValue, just like for regular style properties) - Internal code can get a nsCSSFontFaceRule object and use its GetDesc() method to retrieve nsCSSValue objects representing the descriptor values. Things that I know don't work: - DOM mutation; just haven't written the code, it's straightforward. - unicode-range: there's code for this already but it doesn't work because the lexer doesn't tokenize "U+03A0-03FF" the way it expects. I think we need a special lexer mode for these constructs. John Daggett: I need to know whether the GetDesc() interface is adequate to your needs for the user font set object. For all descriptors that correspond to regular CSS properties, the nsCSSValue objects you get out have the same content (however, some values legitimate for properties will never occur for descriptors); for src: and unicode-range: I had to make something up, and the clearest documentation for that should be the comments above SerializeFontSrc and SerializeUnicodeRange in nsCSSRules.cpp. Everyone: if you see something I did wrong with XPCOM, please tell me.

(In reply to comment #20) Not quite sure yet but I should note that layout/style system code can call into gfx but not the other way around. So code that uses GetDesc would essentially be parsing the src format again?

(In reply to comment #21) > Not quite sure yet but I should note that layout/style system code can call > into gfx but not the other way around. I'm not sure what that means for this code. I had imagined that your code would notice the addition of a CSSFontFaceRule object to the stylesheet DOM tree, grab it, and use GetDesc to translate to the user font set stuff. > So code that uses GetDesc would essentially be parsing the src format again? Not really. GetDesc() gives you a nsCSSValue object - that object may hold a NS_STYLE_* constant (font-weight, font-stretch, font-style), or a plain string (font-family), or an array with structure (src, unicode-range). In the array case you have to do a little interpretation, but I would compare it to walking over an existing parse tree rather than reparsing from scratch. I'm deliberately not explaining the array properties because I want to know if the comments in the code are sufficient, and if not, fix them.

I have spun off unicode-range: and DOM mutation as bug 449376 and bug 449378, respectively; I don't believe either should block this bug. The next revision of the patch for this bug will have the busted unicode-range: parser replaced with a stub; the DOM mutator methods were already stubs.

Gah, I meant bug 443976 and bug 443978.

... and then I fumblefinger and deassign the bug. Will correct with next patch upload, since it doesn't seem to be possible to reassign-to-self-and-change-to-ASSIGNED in one go otherwise.

Revised final patch. I think this is landable. unicode-range: and DOM mutation are now in their own bugs; the busted unicode-range: code has been replaced with a stub.

+ // XXX ParseFontSrc and ParseFontRanges are to-implement. ParseFontSrc seems implemented to me, am I missing something?

Oops, yes, that was a note to self and is now obsolete. I've deleted it in my tree.

So, some pretty general comments before a detailed review: I think a bunch of the functions that have switch statements for all the font descriptors could be simplified greatly if we had an array (indexed by the descriptor enum) of something like: struct FontDescriptorInfo { CSSFontDescriptorBlock::*nsCSSValue mMember; const char *mName; }; you could use this to reduce duplication in the following methods (of CSSFontDescriptorBlock / CSSFontFaceRule): GetCssText, RemoveProperty, GetLength, Item, List, SetDesc, GetDesc. Use "rv" as the name for nsresult return values (not err), but not other out parameters (use aResult). (Both types of fixes needed.) In method implementations, be consistent about putting the return type and the method name on separate lines. (This is local style (mostly), and makes it easy to search for method implementations by searching for the qualified name at beginning of line.) In nsCSSParser, I think you ought to consolidate the error reporting at the caller of ParseFontDescriptor rather than having long error handling sections inside each case. I'm also not sure it's worth having specific error messages inside the descriptor parsers; we haven't done that for property parsers except for particular commonly-hit issues. What's eCSSFontDesc_UNKNOWN used for? Why is it needed? Should the copy-constructor of CSSFontFaceRule be private, given Clone? If not, comments should distinguish them. I'm not sure whether it's ok for CSSFontDescriptorBlock to use the same classinfo as another class. (If you're able to, it might be worth posting a new patch that addresses some of these, particularly the ones requiring bigger changes, as I start looking at the details. I may interleave doing that with some other reviews that I have...)

(In reply to comment #29) > > I think a bunch of the functions that have switch statements for all > the font descriptors could be simplified greatly if we had an array > (indexed by the descriptor enum) [...] Ooh, good idea. > Use "rv" as the name for nsresult return values (not err), but not > other out parameters (use aResult). (Both types of fixes needed.) Ok. > In method implementations, be consistent about putting the return > type and the method name on separate lines. (This is local style > (mostly), and makes it easy to search for method implementations by > searching for the qualified name at beginning of line.) That's my default style as well; I think I might have put both on one line in a few places where it looked like that was the file style. Will correct. > In nsCSSParser, I think you ought to consolidate the error reporting > at the caller of ParseFontDescriptor rather than having long error > handling sections inside each case. I'm also not sure it's worth > having specific error messages inside the descriptor parsers; we > haven't done that for property parsers except for particular > commonly-hit issues. I'll see what I can do there. I'm not sure what you mean to suggest as an alternative to specific error messages, though. Just return PR_FALSE from the descriptor parser and print something generic at the point where we call SkipDeclaration and loop, maybe? > What's eCSSFontDesc_UNKNOWN used for? Why is it needed? If someone writes a descriptor name that's not in the spec, like @font-face { font-magic: 42; } that'll show up as eCSSFontDesc_UNKNOWN in ParseFontDescriptor, and we then do the usual dance with silently ignoring other vendors' private descriptors or diagnosing otherwise. > Should the copy-constructor of CSSFontFaceRule be private, given Clone? If > not, comments should distinguish them. I have no idea whether it should or not! Both the copy-constructor and Clone were cut and pasted from, er, whatever's immediately above CSSFontFaceRule in that file. > I'm not sure whether it's ok for CSSFontDescriptorBlock to use the same > classinfo as another class. That's more copy and paste. It seemed like it ought to work, since both classes are implementing nsIDOMCSSStyleDeclaration. There's a caveat, though: CSSFontDescriptorBlock does *not* implement nsIDOMCSS2Properties or nsIDOMNSCSS2Properties either as declared or in QueryInterface, but the classinfo it's sharing with regular CSSStyleDeclaration says it does. I think that ought to, in the worst case, result in a weird JS exception when QueryInterface fails, but I haven't actually tried it. > (If you're able to, it might be worth posting a new patch that > addresses some of these, particularly the ones requiring bigger > changes, as I start looking at the details. I may interleave doing > that with some other reviews that I have...) I'll put that near the top of my list, but I probably won't get to it till tomorrow morning; right now I'm working on some other bugs.

revised patch incorporating some of the changes suggested in comment 29. Copy constructor for CSSFontFaceRule and classinfo for CSSFontDescriptorBlock are still the same.

Comment on attachment 328792 [details] [diff] [review] (rev 5) dbaron's suggested cleanups >diff --git a/dom/locales/en-US/chrome/layout/css.properties b/dom/locales/en-US/chrome/layout/css.properties >@@ -127,3 +127,4 @@ PEInaccessibleProperty2=Cannot specify v > PEInaccessibleProperty2=Cannot specify value for internal property. > PECommentEOF=end of comment > SEUnterminatedString=Found unclosed string '%1$S'. >+PEUnknownFontDesc=Unknown @font-face property '%1$S'. s/property/descriptor/ >diff --git a/dom/public/nsDOMClassInfoID.h b/dom/public/nsDOMClassInfoID.h >@@ -433,6 +433,9 @@ enum nsDOMClassInfoID { > eDOMClassInfo_HTMLAudioElement_id, > #endif > >+ // @font-face in CSS >+ eDOMClassInfo_CSSFontFaceRule_id, >+ > // This one better be the last one in this list > eDOMClassInfoIDCount > }; You had some problems merging with the video landing; some of your additions are before the MOZ_MEDIA ifdef chunk and some are after. Given that MOZ_MEDIA is off by default (I think, and it doesn't matter much since it's pretty new), I think you should make all three DOM class info insertions be right before the MOZ_MEDIA block. (If you don't fix that, you'll make things crash when MOZ_MEDIA is enabled due to the order mismatch.) Given that there are already two separate classinfo declarations for CSSStyleDeclaration and ComputedCSSStyleDeclaration, I'm reasonably confident you need a third for your new implementation of nsIDOMCSSStyleDeclaration -- especially since it implements fewer interfaces. I'm not exactly sure what macros to use there. (I'm also not sure why only one of the two existing ones uses ARRAY_SCRIPTABLE_FLAGS.)

(In reply to comment #32) > >+PEUnknownFontDesc=Unknown @font-face property '%1$S'. > > s/property/descriptor/ That was intentional; all other diagnostics relating to @font-face share existing strings and therefore talk about properties; it would be way too confusing for just one to refer to descriptors. I went this way instead of duplicating all the strings needed because it's less work for the translators and I figured your average web programmer isn't going to know or care that *these* things-which-look-exactly-like-properties have a different name in the spec. > You had some problems merging with the video landing; some of your additions > are before the MOZ_MEDIA ifdef chunk and some are after. Given that MOZ_MEDIA > is off by default (I think, and it doesn't matter much since it's pretty new), > I think you should make all three DOM class info insertions be right before the > MOZ_MEDIA block. Will fix. > Given that there are already two separate classinfo declarations for > CSSStyleDeclaration and ComputedCSSStyleDeclaration, I'm reasonably confident > you need a third for your new implementation of nsIDOMCSSStyleDeclaration -- > especially since it implements fewer interfaces. I'm not exactly sure what > macros to use there. (I'm also not sure why only one of the two existing ones > uses ARRAY_SCRIPTABLE_FLAGS.) Ok, I'll look into it.

Comment on attachment 328792 [details] [diff] [review] (rev 5) dbaron's suggested cleanups >diff --git a/layout/style/nsCSSParser.cpp b/layout/style/nsCSSParser.cpp >+ PRBool ParseFontFormats(nsresult& aErrorCode, nsTArray<nsCSSValue>& values); Maybe call this ParseFontSrcFormat to make it clear it's parsing a part of the src descriptor. >+// font-face-rule: '@font-face' '{' font-description '}' >+// font-description: font-descriptor+ >+// font-descriptor: font-family-desc >+// | font-style-desc >+// | font-weight-desc >+// | font-stretch-desc >+// | font-src-desc >+// | unicode-range-desc >+// >+// All font-*-desc rules follow the pattern s/rules/descriptors/ (or if you meant a term for the units of grammar, "productions", which is not a CSS term) >+// Each of them, except the last two, bears some resemblance to a How about "All but the last two descriptors resemble a" >+// property/value pair in regular rules, but the values are >+// restricted. I'd drop "in regular rules". Property/value pairs only show up there (and they're called "rule sets", not "regular rules"). >+PRBool >+CSSParserImpl::ParseFontFaceRule(nsresult& aErrorCode, RuleAppendFunc aAppendFunc, void* aData) >+ nsRefPtr<nsCSSFontFaceRule> rule(new nsCSSFontFaceRule()); Check for out-of-memory: if (!rule) { aErrorCode = NS_ERROR_OUT_OF_MEMORY; return PR_FALSE; } >+ if (!GetToken(aErrorCode, PR_TRUE)) { >+ REPORT_UNEXPECTED_EOF(PEFontFaceEOF); >+ break; >+ } >+ if (mToken.IsSymbol('}')) { // done! >+ UngetToken(); >+ break; >+ } >+ >+ // ignore extra semicolons >+ if (mToken.IsSymbol(';')) >+ continue; I'd actually prefer if you moved this into ParseFontDescriptor, since it's a bit odd for a complicated parsing function to start by assuming that a token is already gotten for it but it needs to look at that token. (You can just s/break/return PR_FALSE/ and s/continue/return PR_TRUE/.) >+PRBool >+CSSParserImpl::ParseFontDescriptor(nsresult& aErrorCode, >+ nsCSSFontFaceRule* aRule) >+{ >+ if (eCSSToken_Ident != mToken.mType) { >+ REPORT_UNEXPECTED_TOKEN(PEParseDeclarationDeclExpected); I'd actually add a new error message here (with "expected descriptor" rather than "expected declaration"). >+ if (!ParseFontDescriptorValue(aErrorCode, descID, value)) { >+ const PRUnichar *params[] = { >+ descName.get() >+ }; >+ REPORT_UNEXPECTED_P(PEPropertyParsingError, params); >+ return PR_FALSE; >+ } Use 2-space indent inside this block. >-PRBool CSSParserImpl::ParseURL(nsresult& aErrorCode, nsCSSValue& aValue) [up to here; taking a break for a bit]

(In reply to comment #33) > (In reply to comment #32) > > >+PEUnknownFontDesc=Unknown @font-face property '%1$S'. > > > > s/property/descriptor/ > > That was intentional; all other diagnostics relating to @font-face share > existing strings and therefore talk about properties; it would be way too > confusing for just one to refer to descriptors. I went this way instead of > duplicating all the strings needed because it's less work for the translators > and I figured your average web programmer isn't going to know or care that > *these* things-which-look-exactly-like-properties have a different name in the > spec. I'm against using incorrect terminology. Omitting the terminology entirely is fine, so any other relevant error messages should be changed to say things like "while parsing value of font-family" rather than "while parsing value of font-family property". I actually might make the above error "Unexpected descriptor '%1$S' inside @font-face".

Comment on attachment 328792 [details] [diff] [review] (rev 5) dbaron's suggested cleanups >+PRBool >+CSSParserImpl::ParseURL(nsresult& aErrorCode, nsCSSValue& aValue) >+ // Note: relies on ExpectSymbol() and everything under it not >+ // clobbering tk->mIdent. >+ if (!ExpectSymbol(aErrorCode, ')', PR_TRUE)) >+ return PR_FALSE; >+ >+ return ProcessURL(aErrorCode, aValue, tk->mIdent); You shouldn't rely on tk->mIdent not being clobbered, since there are two easy ways around it. Pick 1 of the following: 1) reverse the calls to ExpectSymbol and ProcessURL. 2) Do "nsString url = tk->mIdent" before the ExpectSymbol call, and pass url through. Strings use copy-on-write reference-counted buffers, so this won't do any extra copying. (Even though it's possible that tk->mIdent, which is an nsAutoString (a string with a stack buffer), is using its stack buffer, ProcessURL forces the creation of a malloc'd buffer anyway. >+PRBool >+CSSParserImpl::ParseFontDescriptorValue(nsresult& aErrorCode, >+ nsCSSFontDesc aDescID, >+ nsCSSValue& aValue) >+{ >+ switch (aDescID) { The structure of this switch seems a little odd: given the choice between: 1) returning from inside the switch in both success or failure 2) assigning a value inside the switch and only returning at the end 3) returning from inside the switch on failure and returning at the end on success 4) (3), but the other way around I'd probably have picked (1) or (2), not (3). It's ok as is, though, but also ok to change. >+ case eCSSFontDesc_Weight: >+ if (!ParseFontWeight(aErrorCode, aValue) || >+ aValue.GetUnit() == eCSSUnit_Inherit || You also need to reject eCSSUnit_Initial. (inherit and initial are accepted together by ParseVariant, etc.) >@@ -6165,6 +6341,110 @@ PRBool CSSParserImpl::ParseFamily(nsresu > return PR_TRUE; > } > >+// src: ( uri-src | local-src ) (',' ( uri-src | local-src ) )* >+// uri-src: uri [ 'format(' string ( ',' string )* ')' ] >+// local-src: 'local(' ( string | ident ) ')' >+ >+PRBool >+CSSParserImpl::ParseFontSrc(nsresult& aErrorCode, nsCSSValue& aValue) >+{ >+ // could we maybe turn nsCSSValue::Array into nsTArray<nsCSSValue>? >+ nsTArray<nsCSSValue> values; >+ nsCSSValue cur; >+ for (;;) { >+ if (!GetToken(aErrorCode, PR_TRUE)) >+ break; >+ if (mToken.mType == eCSSToken_String) { >+ // spec does not explicitly permit a bare string instead of a url(), >+ // but everywhere else in CSS, they're ok, so... Um, the only place I can think of that does that is @import. It's not true of the values of background-image, list-style-image, etc., and for values of 'content' strings and URLs are different. You shouldn't extend the syntax this way. (Or are you doing it to match other implementations?) >+ } else if (mToken.mType == eCSSToken_Function && >+ mToken.mIdent.LowerCaseEqualsLiteral("local")) { >+ if (!ExpectSymbol(aErrorCode, '(', PR_FALSE)) >+ return PR_FALSE; This ExpectSymbol call is guaranteed not to fail, since you have a function token. If you want, you can assert, but don't check at runtime. >+ if (!GetToken(aErrorCode, PR_TRUE)) >+ return PR_FALSE; You might want to comment that the spec is unclear on whether whitespace is allowed, so we're allowing it. >+ if (mToken.mType != eCSSToken_Ident && >+ mToken.mType != eCSSToken_String) >+ return PR_FALSE; Both http://www.w3.org/TR/1998/REC-CSS2-19980512/fonts.html#value-def-font-face-name and http://www.w3.org/TR/2002/WD-css3-webfonts-20020802/#referencing are pretty clear that only string is allowed here. You can also merge these two ifs into one, separated by ||. (And the same thing in some other places too, if you want.) >+ cur.SetStringValue(mToken.mIdent, eCSSUnit_Local_Font); >+ values.AppendElement(cur); >+ if (!ExpectSymbol(aErrorCode, ')', PR_TRUE)) >+ return PR_FALSE; Same comment here about whitespace. >+PRBool >+CSSParserImpl::ParseFontFormats(nsresult& aErrorCode, >+ nsTArray<nsCSSValue> & values) >+{ >+ nsCSSValue cur; Move this down to where it's first used (in fact, you can use the string constructor instead of SetStringValue). >+ if (!GetToken(aErrorCode, PR_TRUE)) >+ return PR_TRUE; // EOF harmless here >+ if (mToken.mType != eCSSToken_Function || >+ !mToken.mIdent.LowerCaseEqualsLiteral("format")) { >+ UngetToken(); >+ return PR_TRUE; >+ } >+ if (!ExpectSymbol(aErrorCode, '(', PR_FALSE)) >+ return PR_FALSE; This ExpectSymbol also can't fail. (At some point we should probably make the function tokens just include the '('.) >+PRBool >+CSSParserImpl::ParseFontRanges(nsresult& aErrorCode, nsCSSValue& aValue) >+{ >+ // not currently implemented (bug 443976); do not throw errors >+ SkipDeclaration(aErrorCode, PR_TRUE); >+ return PR_TRUE; Everything else that's in the spec but we don't implement is an error; we should probably return PR_FALSE here for consistency with other things we don't implement.

It looks like you can drop the whole ParseURL/ProcessURL split, though, given my later comment.

Comment on attachment 328792 [details] [diff] [review] (rev 5) dbaron's suggested cleanups >diff --git a/layout/style/nsCSSProps.cpp b/layout/style/nsCSSProps.cpp Some of this seems a little like overkill for managing a list of 6 names, but I suppose the symmetry with properties is a good thing, so it's probably all for the best. >+nsCSSFontDesc >+nsCSSProps::LookupFontDesc(const nsAString& aFontDesc) >+{ >+ NS_ASSERTION(gFontDescTable, "no lookup table, needs addref"); >+ return nsCSSFontDesc(gFontDescTable->Lookup(aFontDesc)); >+} >+ >+ Just one blank line here, though. >diff --git a/layout/style/nsCSSRules.cpp b/layout/style/nsCSSRules.cpp >+// Many of the descriptors can be serialized with this function. >+static void >+SerializeEnumProp(nsCSSProperty aProperty, nsCSSValue const & aValue, >+ nsAString & aResult NS_OUTPARAM) >+{ >+ if (aValue.GetUnit() == eCSSUnit_Enumerated) >+ CopyASCIItoUTF16(nsCSSProps::LookupPropertyValue(aProperty, >+ aValue.GetIntValue()), >+ aResult); >+ else if (aValue.GetUnit() == eCSSUnit_Normal) >+ aResult.AssignLiteral("normal"); >+ else >+ aResult.Truncate(); >+} I don't think this is needed; it looks like it just implements a subset of what nsCSSDeclaration::AppendCSSValueToString does. You should use that instead. >+static void >+SerializeFontSrc(nsCSSValue const & src, nsAString & aResult NS_OUTPARAM) Could you put the const before the typename like we normally do? >+{ >+ aResult.Truncate(); >+ if (src.GetUnit() != eCSSUnit_Array) >+ return; Can you assert about what types are allowed? Is it just _Array and _Null? >+ nsCSSValue::Array const & sources = *src.GetArrayValue(); Same here about the const. >+ PRUint32 i = 0; >+ nsAutoString formats; >+ >+ while (i < sources.Count()) { >+ if (sources[i].GetUnit() == eCSSUnit_URL) { Stick to 2-space indent rather than jumping to 4-space mid-function. >+ aResult.AppendLiteral("url("); >+ aResult.Append(sources[i].GetOriginalURLValue()); >+ aResult.Append(')'); I think you should serialize to the quoted URL syntax rather than unquoted. >+ } else if (sources[i].GetUnit() == eCSSUnit_Local_Font) { >+ aResult.AppendLiteral("local(\""); >+ aResult.Append(sources[i].GetStringBufferValue()); >+ aResult.AppendLiteral("\")"); You should use nsStyleUtil::EscapeCSSString for both of these. >+ i++; >+ formats.Truncate(); May as well declare formats inside the loop. (I think Truncate will make it drop any buffer it has anyway.) It might also be preferable to separate the items inside format() by ", " rather than just ",". >+// A unicode-range: descriptor is represented as an array of >+// integers, to be interpreted as a sequence of pairs: min max min max ... >+// It is in no particular order. (Possibly it should be sorted and >+// overlaps consolidated, but right now we don't do that.) >+static void >+SerializeUnicodeRange(nsCSSValue const & aValue, >+ nsAString & aResult NS_OUTPARAM) >+{ Should this move into your separate unicode-range patch? It would be easier to review there. >+class CSSFontDescriptorBlock : public nsIDOMCSSStyleDeclaration >+{ >+public: >+ NS_DECL_ISUPPORTS >+ NS_DECL_NSIDOMCSSSTYLEDECLARATION >+ >+ CSSFontDescriptorBlock() {} >+ virtual ~CSSFontDescriptorBlock() {} Destructor may as well be private and non-virtual, since there are no derived classes. >+ friend class nsCSSFontFaceRule; Why is the friend declaration needed? Could it be avoided? >+// QueryInterface implementation for CSSFontDescriptorBlock >+NS_INTERFACE_MAP_BEGIN(CSSFontDescriptorBlock) >+ NS_INTERFACE_MAP_ENTRY(nsIDOMCSSStyleDeclaration) >+ NS_INTERFACE_MAP_ENTRY(nsISupports) >+ NS_INTERFACE_MAP_ENTRY_CONTENT_CLASSINFO(CSSStyleDeclaration) As I said before, this needs separate classinfo. >+NS_INTERFACE_MAP_END >+// Mapping from nsCSSFontDesc codes to CSSFontDescriptorBlock fields. >+// Keep this in sync with enum nsCSSFontDesc in nsCSSProperty.h. The reference in the other direction looks like it has an out-of-date name. >+// helper for string GetPropertyValue and RemovePropertyValue >+nsresult >+CSSFontDescriptorBlock::GetPropertyValue(nsCSSFontDesc aFontDescID, >+ nsAString & aResult NS_OUTPARAM) >+{ >+ NS_ENSURE_ARG_RANGE(aFontDescID, eCSSFontDesc_UNKNOWN, eCSSFontDesc_COUNT); Why are UNKNOWN and COUNT allowed inputs? >+ nsCSSValue const & val = this->*CSSFontDescriptorBlock::Fields[aFontDescID]; Could you put the const before the type? >+ case eCSSFontDesc_Family: >+ if (val.GetUnit() == eCSSUnit_String) >+ aResult.Assign(val.GetStringBufferValue()); >+ return NS_OK; >+ >+ case eCSSFontDesc_Style: >+ SerializeEnumProp(eCSSProperty_font_style, val, aResult); >+ return NS_OK; >+ >+ case eCSSFontDesc_Weight: >+ if (val.GetUnit() == eCSSUnit_Integer) >+ CopyASCIItoUTF16(nsPrintfCString("%d", val.GetIntValue()), aResult); >+ else >+ SerializeEnumProp(eCSSProperty_font_weight, val, aResult); >+ return NS_OK; >+ >+ case eCSSFontDesc_Stretch: >+ SerializeEnumProp(eCSSProperty_font_stretch, val, aResult); >+ return NS_OK; These can all be replaced by appropriate calls to nsCSSSDeclaration::AppendCSSValueToString, as I said above. >+ case eCSSFontDesc_UNKNOWN: >+ case eCSSFontDesc_COUNT: >+ ; >+ } >+ NS_NOTREACHED("CSSFontDescriptorBlock::GetPropertyValue: " >+ "out-of-range value got to the switch"); Why not assert this at the beginning of the function instead? >+ return NS_ERROR_INVALID_ARG; >+}

Comment on attachment 328792 [details] [diff] [review] (rev 5) dbaron's suggested cleanups >+// attribute DOMString cssText; >+NS_IMETHODIMP >+CSSFontDescriptorBlock::GetCssText(nsAString & aCssText) >+{ >+ nsAutoString descStr; >+ >+ aCssText.Truncate(); >+ for (int id = eCSSFontDesc_UNKNOWN + 1; >+ id < eCSSFontDesc_COUNT; >+ id++) { Maybe make |id| an nsCSSFontDesc. The loop condition is a little uglier, but it's better for later. >+ if ((this->*CSSFontDescriptorBlock::Fields[id]).GetUnit() >+ != eCSSUnit_Null && maybe indent the != an extra 2 spaces? >+ GetPropertyValue((nsCSSFontDesc)id, descStr) == NS_OK && Use NS_SUCCEEDED() rather than direct comparison to NS_OK. >+ descStr.Length() > 0) { Why the check for the length? Perhaps an assertion? >+// DOMString removeProperty (in DOMString propertyName) raises (DOMException); >+NS_IMETHODIMP >+CSSFontDescriptorBlock::RemoveProperty(const nsAString & propertyName, >+ nsAString & aResult NS_OUTPARAM) >+{ >+ nsCSSFontDesc descID = nsCSSProps::LookupFontDesc(propertyName); >+ nsresult rv = GetPropertyValue(descID, aResult); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ if (descID != eCSSFontDesc_UNKNOWN) Why are you checking descID here, *after* the GetPropertyValue call? Seems like it's better to check it before. >+ { >+ NS_ASSERTION(descID > eCSSFontDesc_UNKNOWN && >+ descID < eCSSFontDesc_COUNT, >+ "LookupFontDesc returned value out of range"); >+ >+ (this->*CSSFontDescriptorBlock::Fields[descID]).Reset(); >+ } Local style is not to indent the {} more than the if. >+// readonly attribute unsigned long length; >+NS_IMETHODIMP >+CSSFontDescriptorBlock::GetLength(PRUint32 *aLength) >+{ >+ PRUint32 len = 0; >+ for (int id = eCSSFontDesc_UNKNOWN + 1; >+ id < eCSSFontDesc_COUNT; >+ id++) Same thing about the type of |id|. (Maybe even have a macro to do it, given that there are still a few more below?) >+// DOMString item (in unsigned long index); >+NS_IMETHODIMP >+CSSFontDescriptorBlock::Item(PRUint32 index, nsAString & aResult NS_OUTPARAM) >+{ >+ PRInt32 nset = -1; >+ int id; >+ for (id = eCSSFontDesc_UNKNOWN + 1; >+ id < eCSSFontDesc_COUNT; >+ id++) >+ if ((this->*CSSFontDescriptorBlock::Fields[id]).GetUnit() >+ != eCSSUnit_Null) { >+ nset++; >+ if (nset == (PRInt32)index) >+ break; >+ } >+ if (id < eCSSFontDesc_COUNT) >+ aResult.AssignASCII(nsCSSProps::GetStringValue((nsCSSFontDesc)id).get()); >+ else >+ aResult.Truncate(); Rather than an if-else after the loop, why not put the then-part inside the loop (with an early return) and just have the Truncate after the loop? >+NS_IMETHODIMP >+CSSFontDescriptorBlock::GetParentRule(nsIDOMCSSRule** aParentRule) >+{ >+ NS_ENSURE_ARG_POINTER(aParentRule); Out-params should be guaranteed non-null and shouldn't be checked at runtime. Assert if you want.

Comment on attachment 328792 [details] [diff] [review] (rev 5) dbaron's suggested cleanups >+// ------------------------------------------- >+// CSSFontDescriptorBlock (part of nsICSSFontFaceRule) >+// I'm not sure the "part of nsICSSFontFaceRule" makes sense. Maybe "used by nsCSSFontFaceRule"? (note the removal of the "I") >+// ------------------------------------------- >+// nsICSSFontFaceRule >+// Should this say nsCSSFontFaceRule instead? >+nsCSSFontFaceRule::nsCSSFontFaceRule() >+ : nsCSSRule(), mDescriptors(new CSSFontDescriptorBlock()) >+{ >+ mDescriptors->mRule = this; >+} Not worried about out-of-memory? It would be nice to preserve the invariant that a font-face rule always has mDescriptors. They could really even be allocated together (i.e., have the mDescriptors be a member object rather than pointer) and the descriptors forwards AddRef and Release to its owning rule. (Giving them the same XPCOM identity would be bad, though, since it would add a DOM API that we don't want to add.) >+nsCSSFontFaceRule::nsCSSFontFaceRule(const nsCSSFontFaceRule& aCopy) >+ : nsCSSRule(aCopy), >+ mDescriptors(new CSSFontDescriptorBlock(*aCopy.mDescriptors)) >+{ >+} Seems like you need to set mDescriptors->mRule here too. >+// QueryInterface implementation for nsCSSFontFaceRule >+NS_INTERFACE_MAP_BEGIN(nsCSSFontFaceRule) >+ NS_INTERFACE_MAP_ENTRY(nsICSSRule) >+ NS_INTERFACE_MAP_ENTRY(nsIStyleRule) >+ NS_INTERFACE_MAP_ENTRY(nsIDOMCSSFontFaceRule) >+ NS_INTERFACE_MAP_ENTRY(nsIDOMCSSRule) >+ NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDOMCSSFontFaceRule) I'd make this go through nsCSSRule like the others, rather than nsIDOMCSSFontFaceRule. (Tradition says to go with the first base class.) >+NS_IMETHODIMP >+nsCSSFontFaceRule::GetType(PRInt32& aType) const >+{ >+ aType = nsICSSRule::FONT_FACE_RULE; >+ return NS_OK; >+} >+ >+// ??? why both this and the above >+NS_IMETHODIMP >+nsCSSFontFaceRule::GetType(PRUint16* aType) >+{ >+ *aType = nsIDOMCSSRule::FONT_FACE_RULE; >+ return NS_OK; >+} They're on different interfaces. Note the different prefixes on the constants. >+class nsCSSFontFaceRule : public nsCSSRule, >+ nsCOMPtr<CSSFontDescriptorBlock> mDescriptors; This should probably be nsRefPtr rather than nsCOMPtr, since nsCOMPtr is for COM interfaces. (It compiles since CSSFontDescriptorBlock only implements one interface, but you should still probably use nsRefPtr.) That said, this will probably change based on my comments above. >diff --git a/layout/style/nsCSSValue.cpp b/layout/style/nsCSSValue.cpp >diff --git a/layout/style/nsCSSValue.h b/layout/style/nsCSSValue.h >+inline PRBool >+UnitValueIsString(nsCSSUnit aUnit) >+{ >+ return aUnit >= eCSSUnit_String && aUnit <= eCSSUnit_Font_Format; >+} Maybe this should be a static method in nsCSSValue, just to avoid polluting the global scope? In the test: There's a bunch of indentation done by tabs (but pretty randomly, like you switched editors). You should use spaces throughout. "ok(0, ...)" should probably be "ok(false, ...)" (multiple occurrences) why not "function doTests()" rather than "var doTests = function()"? I wouldn't bother with setting display.innerHTML. It's not clear to me why you need timeouts; I think the whole thing should work synchronously. + try { + while(sheet.cssRules.length > 0) + sheet.deleteRule(0); + sheet.insertRule(testset[curTest].rule, 0); + display.innerHTML = testset[curTest].rule; + } catch (e if e instanceof DOMException) { + ok(e.code == DOMException.SYNTAX_ERR + && !('d' in testset[curTest]), + testset[curTest].rule + " syntax error thrown", e); + } catch (e) { + ok(0, testset[curTest].rule, "During prep: " + e); + } Going back a little: + // nothing else is set + for (var i = 0; i < s.length; i++) { + ok(s.item(i) in d, testset[curTest].rule, + "Unexpected item #" + i + ": " + d); + } Instead, maybe just check the count against the number of items in d? (The "Unexpected item" text seems like an odd thing to see when looking at the list of passing tests, too.) + if (sheet.cssRules.length == 0) { + is(sheet.cssRules.length, 0, + testset[curTest].rule + " rule count (0)"); + } else { + is(sheet.cssRules.length, 1, + testset[curTest].rule + " rule count (1 non-fontface)"); + isnot(sheet.cssRules[0].type, 5 /*FONT_FACE_RULE*/, + testset[curTest].rule + " rule type (1 non-fontface)"); + } Maybe you should make the if here test whether rule starts with "@", so that you're testing a little more (rather than confirming what you already know about sheet.cssRules.length)? + // round-tripping of cssText + if(n && !testset[curTest].noncanonical) + is(sheet.cssRules[0].cssText.replace(/[ \n]+/g, " "), + testset[curTest].rule, + testset[curTest].rule + " rule text"); This is a somewhat strong test; I'd at least comment that it may well be ok if it's intentionally changed in the future. Also, space between "if" and "(". Looks pretty good overall, but I'd like to look at the revised version once you fix these issues.

I'm working through revision now. If I don't call something out here, assume I've taken care of it. > I'm not sure why only one of [CSSStyleDeclaration and > ComputedCSSStyleDeclaration] uses ARRAY_SCRIPTABLE_FLAGS. I have not been able to figure out what ARRAY_SCRIPTABLE_FLAGS *means* and therefore I have not been able to resolve this question. I'm using DOM_DEFAULT_SCRIPTABLE_FLAGS for the new classinfo for @font-face StyleDeclarations, to be safe. > I'd actually prefer if you moved this into ParseFontDescriptor, since it's > a bit odd for a complicated parsing function to start by assuming that > a token is already gotten for it but it needs to look at that token. No can do; it will cause a syntactically correct @font-face to throw a parse error when it hits the close brace. There's already parser functions that do things like this, e.g. ParseGroupRule. > >+ // spec does not explicitly permit a bare string instead of a url(), > >+ // but everywhere else in CSS, they're ok, so... > Um, the only place I can think of that does that is @import. Dropped, and I collapsed ProcessURL back into ParseURL, but I did not revert to the original ParseURL. > This ExpectSymbol call is guaranteed not to fail, since you have a function > token. If you want, you can assert, but don't check at runtime. All other code that I can find that parses function tokens, checks the return value of ExpectSymbol. For now, I have not changed this. > Both > http://www.w3.org/TR/1998/REC-CSS2-19980512/fonts.html#value-def-font-face-name > and http://www.w3.org/TR/2002/WD-css3-webfonts-20020802/#referencing > are pretty clear that only string is allowed [in a local(...)]. http://dev.w3.org/csswg/css3-fonts/#src disagrees. See example X in that section, the paragraph immediately above that example, and the discussion in comment 3/comment 4 to this bug. > Why is the friend declaration needed? Could it be avoided? It's needed so the nsCSSFontFaceRule destructor can zap mDescriptors->mRule. I could not think of any better way to do that. It also lets SetDesc() and GetDesc() access member variables directly, but they could forward to CSSFontDescriptorBlock functions instead. > Why are UNKNOWN and COUNT allowed inputs [to the GetPropertyValue overload > that takes a nsCSSFontDesc enumerator]? COUNT isn't; NS_ENSURE_ARG_RANGE is min <= arg < max. UNKNOWN is allowed because that lets string GetPropertyValue just blindly hand the result of nsCSSProps::LookupFontDesc to this function. > Why not assert this at the beginning of the function instead? This way it triggers on hypothetical future descriptors that got left out of the switch. > Maybe make |id| a nsCSSFontDesc ++ cannot be applied to a variable of enumerated type. (I had it that way to begin with.) I suppose I could do "id = nsCSSFontDesc(int(id) + 1)" but I think it's clearer the way it is, overall. > Why the check for the length? Perhaps an assertion? In testing, GetPropertyValue could return an empty string in cases where the unit wasn't null. I don't remember exactly how you got that, and it may not be true anymore. > It would be nice to preserve the invariant that a font-face rule always has > descriptors. They could really even be allocated together [...] I agree, but I'm going to go back and do that as a separate change after I finish with all your other comments. >>+ NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDOMCSSFontFaceRule) >I'd make this go through nsCSSRule like the others Just to make sure, you're talking only about the line of code I quoted, right? >>+ // ??? why both this and the above > They're on different interfaces. Note the different [argument types] I saw that, but I don't really understand how we came to have two different interfaces with methods on them that have identical semantics and signature except for the size of the out parameter... > [The mDescriptors field] should probably be nsRefPtr rather than nsCOMPtr, > since nsCOMPtr is for COM interfaces I don't get it; isn't CSSFontDescriptorBlock a COM interface? (stopping here for the evening)

Zach ARRAY_SCRIPTABLE_FLAGS means that you can implement a GetProperty callback and an Enumerate callback in the classinfo helper. The whole thing lets you implement [n] indexing. For example, try doing document.body.style[0] on a page on which a <body> has inline style. Technically, the ECMAScript bindings for CSSStyleDeclaration at http://www.w3.org/TR/2000/REC-DOM-Level-2-Style-20001113/ecma-script-binding.html say: item(index) This method returns a String. The index parameter is of type Number. Note: This object can also be dereferenced using square bracket notation (e.g. obj[1]). Dereferencing with an integer index is equivalent to invoking the item method with that index. So our computed style (which doesn't do this) is buggy.

(In reply to comment #41) > > Both > > http://www.w3.org/TR/1998/REC-CSS2-19980512/fonts.html#value-def-font-face-name > > and http://www.w3.org/TR/2002/WD-css3-webfonts-20020802/#referencing > > are pretty clear that only string is allowed [in a local(...)]. > > http://dev.w3.org/csswg/css3-fonts/#src disagrees. See example X in that > section, the paragraph immediately above that example, and the discussion in > comment 3/comment 4 to this bug. Then you need to handle the name being multiple tokens as well, unless the draft says clearly that unquoted font names inside local must be CSS identifiers. (Consider, e.g., font names that start with numbers or symbols, or that have spaces in the middle.) > COUNT isn't; NS_ENSURE_ARG_RANGE is min <= arg < max. UNKNOWN is allowed > because that lets string GetPropertyValue just blindly hand the result of > nsCSSProps::LookupFontDesc to this function. http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsDebug.h#247 looks like min <= arg <= max to me. > > Maybe make |id| a nsCSSFontDesc > > ++ cannot be applied to a variable of enumerated type. (I had it that way to > begin with.) I suppose I could do "id = nsCSSFontDesc(int(id) + 1)" but I > think it's clearer the way it is, overall. I think I'd prefer it with the +1 and cast; you don't need the int cast, though. > > Why the check for the length? Perhaps an assertion? > > In testing, GetPropertyValue could return an empty string in cases where the > unit wasn't null. I don't remember exactly how you got that, and it may not be > true anymore. I think it should be an assertion; that shouldn't happen, and if it is, we should find out why. > >>+ NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDOMCSSFontFaceRule) > >I'd make this go through nsCSSRule like the others > > Just to make sure, you're talking only about the line of code I quoted, right? yes. > >>+ // ??? why both this and the above > > They're on different interfaces. Note the different [argument types] > > I saw that, but I don't really understand how we came to have two different > interfaces with methods on them that have identical semantics and signature > except for the size of the out parameter... And the set of constants available; one set is frozen by the DOM spec, the other has constants appropriate for our custom rules. > > [The mDescriptors field] should probably be nsRefPtr rather than nsCOMPtr, > > since nsCOMPtr is for COM interfaces > > I don't get it; isn't CSSFontDescriptorBlock a COM interface? A COM interface: * must have only pure virtual functions (no data members, no other functions) * must inherit either from nsISupports or one other COM interface (no multiple inheritance) * should be defined in IDL, although some older ones aren't

(In reply to comment #42) > Zach ARRAY_SCRIPTABLE_FLAGS means that you can implement a GetProperty > callback and an Enumerate callback in the classinfo helper. > The whole thing lets you implement [n] indexing. I said completely the wrong thing originally. I understood that ARRAY_SCRIPTABLE_FLAGS means this object is to be [n] indexable, and requires that *something* implements GetProperty and Enumerate methods. What I didn't understand yesterday is what requirements that places on which C++ classes. After spending about an hour this morning digging through xpconnect/ and the nsDOMClassInfo.cpp helper classes, I think nsCSSStyleDeclSH implements GetProperty and Enumerate in terms of the underlying object's Item and GetLength methods, but I'm not 100% sure. (It would help if I could find any documentation at all for nsDOMClassInfo ... all the XPCOM documentation seems to center on classes whose entire behavior is specified in XPIDL.) A closely related thing I'm still not sure about: CSSStyleDeclaration gets a DOM_CLASSINFO_MAP_BEGIN block, ComputedCSSStyleDeclaration gets a DOM_CLASSINFO_MAP_BEGIN_NO_CLASS_IF block. This seems to mean that the latter has "no class interface". I don't know what it means to have no class interface, whether one of them is declared wrong, or which one I should use for CSSFontFaceStyleDecl.

(In reply to comment #43) > (In reply to comment #41) > > > only string is allowed [in a local(...)]. > > > > http://dev.w3.org/csswg/css3-fonts/#src disagrees. See example X > > in that section, the paragraph immediately above that example, and > > the discussion in comment 3/comment 4 to this bug. > > Then you need to handle the name being multiple tokens as well, > unless the draft says clearly that unquoted font names inside local > must be CSS identifiers. The draft does not give a formal grammar for local(). I would prefer to have it specify local-font-name : 'local(' ( IDENT | STRING ) ')' than have to worry about unquoted multiple-token sequences, and from the above discussion, I think jdaggett agrees. > http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsDebug.h#247 > looks like min <= arg <= max to me. Bleah, I misread it. Will fix. > I think I'd prefer it with the +1 and cast; you don't need the int > cast, though. Ok. > > In testing, GetPropertyValue could return an empty string in cases > > where the unit wasn't null. > > I think it should be an assertion; that shouldn't happen, and if it > is, we should find out why. Ok. > > I don't get it; isn't CSSFontDescriptorBlock a COM interface? > > A COM interface: > * must have only pure virtual functions (no data members, no other > functions) > * must inherit either from nsISupports or one other COM interface > (no multiple inheritance) > * should be defined in IDL, although some older ones aren't So as a rule of thumb, nsCOMPtr is only for the classes whose names are nsIsomething, and one should use nsRefPtr for a direct pointer to a COM implementation class, or in general anything whose memory is managed with AddRef/Release but isn't a nsI class, correct?

> I think nsCSSStyleDeclSH implements GetProperty and Enumerate in terms of the > underlying object's Item and GetLength methods Sounds right to me. I can't recall what the NO_CLASS_IF thing means. jst told me once, but I forgot... he or peterv would know.

(In reply to comment #45) > The draft does not give a formal grammar for local(). I would prefer > to have it specify > > local-font-name : 'local(' ( IDENT | STRING ) ')' > > than have to worry about unquoted multiple-token sequences, and from > the above discussion, I think jdaggett agrees. I think allowing unquoted values only if they're identifiers could be very confusing for authors. > So as a rule of thumb, nsCOMPtr is only for the classes whose names > are nsIsomething, and one should use nsRefPtr for a direct pointer to > a COM implementation class, or in general anything whose memory is > managed with AddRef/Release but isn't a nsI class, correct? with s/nsISomething/XPCOM interfaces/, yes. nsIDNService isn't an XPCOM interface (but it implements nsIIDNService, which is). (Not to be confused, of course, with nsDNSService and nsIDNSService, which are for "DNS", not "IDN".)

(In reply to comment #47) > I think allowing unquoted values only if they're identifiers could be very > confusing for authors. You're probably right. The attribute-match syntax already has something like this ("attribute values must be identifiers or strings") but that doesn't necessarily mean we should make more places like that. Honestly, I think my second choice is to allow only strings. Let's find out what jdaggett thinks, though. > > So as a rule of thumb, nsCOMPtr is only for the classes whose names > > are nsIsomething, and one should use nsRefPtr for a direct pointer to > > a COM implementation class, or in general anything whose memory is > > managed with AddRef/Release but isn't a nsI class, correct? > > with s/nsISomething/XPCOM interfaces/, yes. Hm, I wonder if that means nsCSSMediaRule's nsCOMPtr<nsMediaList> field is wrong, too. Also, can you comment on RefPtr versus AutoPtr?

(In reply to comment #48) > Hm, I wonder if that means nsCSSMediaRule's nsCOMPtr<nsMediaList> field is > wrong, too. Yes. > Also, can you comment on RefPtr versus AutoPtr? nsAutoPtr calls delete, not AddRef()/Release().

(continuing through the review ...) > Maybe [UnitValueIsString] should be a static method in nsCSSValue After looking at it again I realized it makes more sense as an instance method, exactly parallel to the existing IsLengthUnit, IsTimeUnit, etc. I also took the liberty of stripping unnecessary casts and parentheses from all of those methods. > It's not clear to me why you need timeouts [in the test] I switched it over to a synchronous loop and it works. I was just assuming that since that other CSS parser test I wrote needed timeouts, so would this one. The main point of the "nothing else is set" test is to exercise item(), and I'd rather not throw that away. I don't see *anything* from ok() in the passed-test logs, although I have confirmed that they are getting run. I don't understand what you're suggesting I do with the "this ill-formed input didn't result in a @font-face rule object" logic. I don't want to throw away the .cssText round-trip either, because that's the only thing that verifies that .cssText works at all. I've added commentary.

Here's a revision with all the changes I've made to date. I'm now going to look into embedding the CSSFontDescriptorBlock in the nsCSSFontFaceRule.

A couple of design questions have come up; I'd like advice. From the perspective of the external DOM, there are supposed to be two objects: the font-face-rule, and its associated style-declaration. Internally, we know that they are always allocated at the same time and become garbage at the same time, so we'd like to combine them into one allocation. I see two ways to do that: either we can embed one of the objects in the other, or we can have an (entirely hidden) implementation class that derives from both interfaces. In concrete C++ terms it's the difference between class nsCSSFontFaceStyleDecl : public nsIDOMCSSStyleDeclaration { // a bunch of nsCSSValue members }; class nsCSSFontFaceRule : public nsIDOMCSSFontFaceRule { ... nsCSSFontFaceStyleDecl mStyle; }; and class CSSFontFaceImpl : public nsIDOMCSSFontFaceRule, public nsIDOMCSSStyleDeclaration { // a bunch of nsCSSValue members }; [footnote: other base classes omitted for clarity] The major advantage of two separate classes is that the two DOM interfaces in question have conflicting method names; the implementation class would have to know which interface GetCssText() was called through, for instance. I'm not sure that's even possible. The major advantage of a single implementation class for both interfaces is that you can go from the style declaration to the encapsulating rule without either embedding a back-pointer in the style declaration or playing dirty tricks with offsetof(). (This is needed for GetParentRule() and for reference counting.) Whichever way we go on that, another question is whether nsCSSFontFaceRule needs its own IID. It implements two methods beyond nsIDOMCSSFontFaceRule; neither of them needs to be scriptable, but both do need to be accessible from internal code. It's not a problem for the parser, which knows what it allocated, but jdaggett's code in bug 441473 is currently doing a static_cast to get from nsIDOMCSSFontFaceRule to nsCSSFontFaceRule so it can call GetDesc(). Maybe that's fine, I dunno.

(In reply to comment #52) > The major advantage of a single implementation class for both interfaces is > that you can go from the style declaration to the encapsulating rule without > either embedding a back-pointer in the style declaration or playing dirty > tricks with offsetof(). (This is needed for GetParentRule() and for reference > counting.) The problem with a single class is that the DOM glue would expose that they're the same object because they'd QueryInterface to each other. (For what it's worth, I do the offsetof tricks in this case in DOMCSSDeclarationImpl::DomRule, in nsCSSStyleRule.cpp.) > Whichever way we go on that, another question is whether nsCSSFontFaceRule > needs its own IID. It implements two methods beyond nsIDOMCSSFontFaceRule; > neither of them needs to be scriptable, but both do need to be accessible from > internal code. It's not a problem for the parser, which knows what it > allocated, but jdaggett's code in bug 441473 is currently doing a static_cast > to get from nsIDOMCSSFontFaceRule to nsCSSFontFaceRule so it can call > GetDesc(). Maybe that's fine, I dunno. It's probably OK as is. And in general IIDs are for interfaces, not classes.

This revision merges the rule object and the declaration object, and also corrects a bit of the DOM classinfo as recommended by jst. I think this is ready for re-review now.

The patch needed a little tweaking after bug 325064 landed - mainly to the test case.

> nsCSSMediaRule's nsCOMPtr<nsMediaList> field Predates the existence of nsRefPtr. It was OK by the rules we had in place then (for lack of better ones), but wouldn't be OK in new code.

Zack, It looks like your patch doesn't trim out quoted font family names, the value returned by fontFace->GetDesc(eCSSFontDesc_Family, val) is *different* for the two @font-face rules below: @font-face { font-family: Bongo Two; src: url(xxx); } @font-face { font-family: "Bongo Two"; src: url(yyy); } I can resolve this in my InsertFontFaceRule code for bug 441473 but I'm not quite sure that's really the right place.

For now, it might be worth using nsFont::EnumerateFamilies since that's what the font-family parsing code expects to be used on the string it outputs. (Bug 280443 is filed on fixing that mess.) I should double-check that only one family is allowed when it's a descriptor, though.

Or the parsing code for the descriptor could use nsFont::EnumerateFamilies -- that might actually make more sense.

I'm confused by comments 58 and 59 - are they a response to comment 57? Who should be using nsFont::EnumerateFamilies, me (in the parser) or jdaggett (in InsertFontFaceRule)? Regarding comment 57, is the difference just the quotation marks? The CSS parser in general tries to preserve a distinction between quoted and unquoted literals, I imagine for round-tripping (editor, Firebug, etc). so I would say it's correct to make InsertFontFaceRule deal with it. My code is just blindly calling ParseFamily(). There's a bug there, 'cos in @font-face it should throw a parse error if there's more than one family-name, or a generic-family instead of a family-name. Will fix with next patch revision.

(In reply to comment #60) > I'm confused by comments 58 and 59 - are they a response to comment 57? Yes. > Who > should be using nsFont::EnumerateFamilies, me (in the parser) or jdaggett (in > InsertFontFaceRule)? I think you -- you can use it to ensure that you only get one family -- and then you'll have the name in a slightly more normalized form.

(Quoted versus unquoted don't have different semantics for the descriptor, so you may as well always serialize to quoted. For the property, they are different, since "serif" is a font name while serif is a generic.)

Ok, will do.

Comment on attachment 330499 [details] [diff] [review] (rev 7a) refresh because of bug 325064 landing Going through the things I commented on in my previous review: -PEPropertyParsingError=Error in parsing value for property '%1$S'. -PEExpectEndProperty=Expected end of value for property but found '%1$S'. +PEPropertyParsingError=Error in parsing value for '%1$S'. +PEExpectEndProperty=Expected end of value but found '%1$S'. You should rename the keys so that localizers know to re-translate them. (If you can't think of anything better, add a 2 to the end of the name.) This will require changing nsCSSParser.cpp to match. In ParseFontDescriptorValue, could you write the returns as: return (expression) rather than if (!expression) return PR_FALSE; return PR_TRUE; ? This will require inverting the logic inside expression... I'm still not happy about what you're allowing inside local(). I'm ok with implementing either spec -- either the old one, which only allows strings inside, or the new one, in which you have to allow arbitrary sequences of tokens, not just a single identifier. + // strings. We allow spaces around the value for consistency + // with other function constructs. Could you explicitly say that the spec is unclear? nsCSSFontFaceStyleDecl::GetPropertyValue needs to return NS_OK for unicode range, since it should act like it does for unknown descriptors, and its caller (which implements a DOM API) returns NS_OK for unknown descriptors. It would be wrong to have only the unimplemented descriptors throw and the unknown descriptors return the empty string. class CSSFontDescriptorBlock has been renamed, but there's still one forward declaration of it, which should go. Could you make |id| an nsCSSFontDesc in GetLength too, to match the other methods? In nsCSSFontFaceStyleDecl::Item, don't indent the braces more than the if, and do put braces around the contents of the for. In nsCSSValue, I'd actually call IsStringUnit UnitHasStringValue. The Is*Unit functions really have useful semantics to the caller; this one doesn't, since it's just about storage. + if (sheet.cssRules.length == 0) { + is(sheet.cssRules.length, 0, + testset[curTest].rule + " rule count (0)"); + } else { + is(sheet.cssRules.length, 1, + testset[curTest].rule + " rule count (1 non-fontface)"); + isnot(sheet.cssRules[0].type, 5 /*FONT_FACE_RULE*/, + testset[curTest].rule + " rule type (1 non-fontface)"); + } You should make the if here test whether rule starts with "@", so that you're testing a little more (rather than confirming what you already know about sheet.cssRules.length)? Now going through the difference between the patches: Could you move the definition of ContaininingRule() before the QueryInterface implementation and the AddRef and Release implementations right after QueryInterface, so all the nsISupports stuff is together? + if (mDecl.GetPropertyValue(id, descStr) != NS_OK) This should use NS_SUCCEEDED(), not a check against NS_OK. + inline nsCSSFontFaceRule* ContainingRule(); + inline const nsCSSFontFaceRule* ContainingRule() const; Even though they're protected, it's probably better to put the definitions in the header file (but after the definition of nsCSSFontFaceRule), just to avoid the potential of linker errors (e.g., if we use them from an inline function in the future). It looks like you changed all the tests that test unquoted url(), e.g. url(/fonts/Mouse), to use quoted url(), e.g., url(\"/fonts/Mouse\"). You should probably at least have one test for the former (although it would need a noncanonical:true). With those things fixed, r+sr=dbaron.

oh, and also fixing the thing about forbidding multiple font families, per the discussion in comment 57 through comment 63.

(In reply to comment #64) I've done all that you asked except: > I'm still not happy about what you're allowing inside local(). I'm ok > with implementing either spec -- either the old one, which only allows > strings inside, or the new one, in which you have to allow arbitrary > sequences of tokens, not just a single identifier. After a bit more thought I made local() accept the same syntax as font-family: within a @font-face; that is, a single <family-name>. This isn't an *arbitrary* sequence of tokens, but it does accept a sequence of unquoted identifiers with whitespace in between. I think this is most likely to be what the committee meant to specify (similar language is used for both) although, alas, I'm not sure we actually implement CSS2.1's <family-name> correctly! as there isn't a formal grammar for *that*, either. > + if (sheet.cssRules.length == 0) { > + is(sheet.cssRules.length, 0, > + testset[curTest].rule + " rule count (0)"); > + } else { > + is(sheet.cssRules.length, 1, > + testset[curTest].rule + " rule count (1 non-fontface)"); > + isnot(sheet.cssRules[0].type, 5 /*FONT_FACE_RULE*/, > + testset[curTest].rule + " rule type (1 non-fontface)"); > + } > > You should make the if here test whether rule starts with "@", so that > you're testing a little more (rather than confirming what you already know > about sheet.cssRules.length)? I still don't understand what change you want here. The current code is written a little abstrusely because is() doesn't support alternatives, but the intent is to test "if the test is marked as syntactically incorrect, then evaluating the style sheet produced either no rule at all, or a single rule which is not a @font-face rule". The single rule does *not* necessarily start with '@', e.g. the second test case is { rule: "font-face { }" }, which produces a plain old style rule that matches the (nonstandard) tag "<font-face>".

Here's the revised patch.

Of course no sooner do I upload rev 8 than I realize I forgot to add test cases for the new sanity checks on font-family: and src:local(). This slight revision changes only the tests.

(In reply to comment #66) > > + if (sheet.cssRules.length == 0) { > > + is(sheet.cssRules.length, 0, > > + testset[curTest].rule + " rule count (0)"); > > + } else { > > + is(sheet.cssRules.length, 1, > > + testset[curTest].rule + " rule count (1 non-fontface)"); > > + isnot(sheet.cssRules[0].type, 5 /*FONT_FACE_RULE*/, > > + testset[curTest].rule + " rule type (1 non-fontface)"); > > + } > > > > You should make the if here test whether rule starts with "@", so that > > you're testing a little more (rather than confirming what you already know > > about sheet.cssRules.length)? > > I still don't understand what change you want here. The current code is > written a little abstrusely because is() doesn't support alternatives, but the > intent is to test "if the test is marked as syntactically incorrect, then > evaluating the style sheet produced either no rule at all, or a single rule > which is not a @font-face rule". The single rule does *not* necessarily start > with '@', e.g. the second test case is > > { rule: "font-face { }" }, > > which produces a plain old style rule that matches the (nonstandard) tag > "<font-face>". I meant to change the first line from: if (sheet.cssRules.length == 0) { to if (testset[curTest].rule.charAt(0) == "@") {

(In reply to comment #69) > > I meant to change the first line from: > if (sheet.cssRules.length == 0) { > to > if (testset[curTest].rule.charAt(0) == "@") { That doesn't test the same thing, though. There aren't currently any such test cases, but consider @media print { @font-face; } which should generate an empty @media rule...?

Since jdaggett is waiting for this stuff, I went ahead and had bz check rev 8a in. It looks like it's surviving the tinderboxes. I hope we can deal with any remaining issues in follow-up patches.

I've backed this out due to the Tp regressions showing on talos (some 10% on Windows). http://graphs.mozilla.org/#show=911655,911694,912148&sel=1218054635,1218310800

I'm really skeptical that this patch would have resulted in *any* change to Tp on Windows, since the only thing it will do is parse rules which would otherwise be ignored. I really doubt any of the pages tested in the Talos pagesets use downloadable fonts. Did the Tp value improve once this was pulled out?

Sorry, it does look like I was mistaken about this being the cause. I can probably reland this in the morning.

Relanded: http://hg.mozilla.org/index.cgi/mozilla-central/rev/ccc930630562