Closed Bug 549861 (font-variant) Opened 14 years ago Closed 11 years ago

implement parsing of font feature properties

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: jtd, Assigned: jtd)

References

(Depends on 1 open bug, Blocks 2 open bugs, )

Details

(Keywords: dev-doc-needed, Whiteboard: [evang-wanted][DocArea=CSS])

Attachments

(19 files, 40 obsolete files)

3.54 KB, text/html
Details
40.14 KB, image/png
Details
1.15 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
14.81 KB, patch
jtd
: review+
Details | Diff | Splinter Review
61.30 KB, patch
jtd
: review+
Details | Diff | Splinter Review
24.97 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
22.17 KB, patch
jtd
: review+
Details | Diff | Splinter Review
24.38 KB, patch
jtd
: review+
Details | Diff | Splinter Review
46.84 KB, patch
jtd
: review+
Details | Diff | Splinter Review
51.78 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
15.87 KB, patch
jtd
: review+
Details | Diff | Splinter Review
29.13 KB, patch
jtd
: review+
Details | Diff | Splinter Review
5.06 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
6.53 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
27.99 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
9.20 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
17.53 KB, patch
jtd
: review+
Details | Diff | Splinter Review
25.37 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
190.19 KB, patch
khuey
: review+
Details | Diff | Splinter Review
Font feature support has now been incorporated into the editor's draft of the CSS3 Fonts spec:

  http://dev.w3.org/csswg/css3-fonts/#font-rend-props

As a first pass, I'm going to set up a patch to parse the properties in section 6 in -moz- form and pass them down to gfx within an expanded version of gfxFontStyle.  These can then be accessed by the harfbuzz code that Jonathan is working on.

This is an extension of the experimental -moz-font-feature-opentype work done for bug 511339.  Harfbuzz work is done on bug 449292.
Whiteboard: [evang-wanted]
Blocks: css-fonts-3
Alias: font-variant
This implements parsing of "simple" font-variant-xxx subproperties.  It doesn't implement font-variant-alternates as that requires support for the @font-feature-values rule.  Style system stores the property data within additional fields in nsFont.  Next step is to implement gfx changes to translate the data in those fields into a list of feature settings and merge those with the existing feature settings before handing the feature list to harfbuzz.
Note that the attached patch is using the latest editor's draft of the spec, not the WD of March 2011:

http://dev.w3.org/csswg/css3-fonts
Full parsing support for -moz-font-variant-xxx subproperties other than -moz-font-variant-alternates.  Property data is pushed down to fields in nsFont.
Attachment #560320 - Attachment is obsolete: true
Changes in gfx to support font-variant subproperties.  This is done by changing how the gfxFontFeature array is constructed within the gfxFontStyle object.  A method of nsFont adds specific feature settings based on the variantXXX fields and then appends the font feature settings (from -moz-font-feature-settings).  Within the feature list of gfxFontStyle it's possible to have multiple instances of the same feature (e.g. liga=1, liga=0, liga=1).  These are resolved when combined with per-font feature defaults in harfbuzz shaping code.

Seems to work, haven't really kicked the tires very much.

Tryserver submission, should be done in a few hours:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jdaggett@mozilla.com-e0b39f0f34b7
Add in support for -moz-font-alternates for the simple values that don't take feature values: contextual, no-contextual, historical-forms, and ruby.  The other properties require support for the @font-feature-values rule to define font-specific selector values.  Also fixed a reftest crash.
Attachment #561958 - Attachment is obsolete: true
Attachment #561981 - Attachment is obsolete: true
This patch adds support for parsing and referencing data in @font-feature-values rules.  These rules define simple numeric constants for use with specific font families.  Spec reference:

http://dev.w3.org/csswg/css3-fonts/#font-feature-values

The simple description is that this is to avoid have numeric selectors that only apply to a single font in a property value:

  font-variant-alternates: swash(3);

Instead this will be written as:

  @font-feature-values MyFont {
    @swash flowing 3;
  }

  font-variant-alternates: swash(flowing);

The use of swash in this case will only apply when the font in use is MyFont, if fallback occurs the default glyph will be used.

This is still just a work-in-progress patch, I need to go through and improve error handling, write tests, and implement things like parsing of values set via DOM access methods.
Black text represents default glyphs, blue text the selected alternates.
Attached image screenshot of testcase
Since the testcase uses a few fonts that aren't universally available, I've attached a screenshot of how the testcase renders with parts 1-3 applied.
Next steps:

- go back and consolidate the parsing of the various font-variant-xxx subproperties
- implement DOM setting/getting with associated parsing
- tests, both for the parsing and for rendering
- implement -moz- shorthand for font-variant that parses all the values of the font-variant-xxx subproperties
- performance tuning (keep this code out of the main text rendering path as much as possible)
Attachment #566754 - Attachment is obsolete: true
Attachment #574663 - Flags: feedback?(dbaron)
Attachment #566755 - Attachment is obsolete: true
Attachment #566757 - Attachment is obsolete: true
Attachment #574672 - Flags: feedback?(dbaron)
Comment on attachment 574663 [details] [diff] [review]
part 1, v0.5c, style system parsing of simple font-variant subproperties (wip)

The structures you're producing in the parser seem reasonable.

Not sure why the showfunc = 1 in your diff settings isn't working.  (Are you using an ancient Mercurial or something?)

>+    // check for conflicting mutually exclusive values
>+    PRInt32 m1 = NS_FONT_VARIANT_ALTERNATES_CONTEXTUAL_MASK;
>+    PRInt32 c = 0;
>+
>+    PRInt32 other = ~feature & featureFlags;
>+    if (m1 & feature) {
>+      c = other & m1;
>+    }
>+
>+    if (c) {
>+      return PR_FALSE;
>+    }

I feel like this could be done in a much simpler way, but I didn't work through it.
Attachment #574663 - Flags: feedback?(dbaron) → feedback+
Comment on attachment 574672 [details] [diff] [review]
part 3, v0.3b, add support for @font-feature-values rule (wip)

A few notes so far (may apply to the other patch too):

 * you have a bunch of PR_TRUE that should now be true, etc.

 * you have some unbraced if-bodies where local style here is to brace 1-line if bodies
Comment on attachment 574672 [details] [diff] [review]
part 3, v0.3b, add support for @font-feature-values rule (wip)

As far as the parsing code goes, it seems a little odd that CSSParserImpl::ParseFontFeatureValuesDeclaration allows lists of length 0, e.g., "@styleset;" without reporting it as an error.  Otherwise the way the parsing works seems entirely reasonable.  (See also previous comment.)
Attachment #574672 - Flags: feedback?(dbaron) → feedback+
Updated to trunk
Attachment #574663 - Attachment is obsolete: true
Attachment #628229 - Attachment description: part 1, v0.6 - add style system parsing of simple font-variant subproperties → part 1, v0.6 - add style system parsing of simple font-variant subproperties (wip)
Updated to trunk
Attachment #574670 - Attachment is obsolete: true
Updated to trunk.
Attachment #574672 - Attachment is obsolete: true
Attachment #628229 - Attachment is obsolete: true
Attachment #628231 - Attachment is obsolete: true
Attachment #628232 - Attachment is obsolete: true
Attachment #630863 - Flags: review?(dbaron)
Parsing of font-variant-xxx subproperties except for font-variant-alternates plus font-kerning and font-synthesis
Attachment #630864 - Flags: review?(dbaron)
gfx changes to support font-variant-xxx subproperties and font-kerning (code for implementing synthesis support will follow)
Attachment #630865 - Flags: review?(jfkthame)
Attachment #630866 - Flags: review?(jfkthame)
Attached patch part 1e - font-kerning reftests (obsolete) — Splinter Review
Attachment #630868 - Flags: review?(jfkthame)
Attachment #630869 - Attachment description: gfx support for font-variant-alternates → part 2a - gfx support for font-variant-alternates
Attachment #630870 - Flags: review?(dbaron)
This is fully functional but cssText serialization is still unfinished.  Plus I need to write a mochitest along the lines of the one for @font-face rules.
Still to do:

 - figure out how to disable kerning in Graphite
 - CSSOM methods
 - font-synthesis gfx implementation
 - fallback for small-caps
 - fallback for subscript/superscript

The first three are relatively minor, the last two will require some hard thought.
Attachment #630872 - Attachment is patch: true
Comment on attachment 630865 [details] [diff] [review]
part 1c - gfx support for font-variant properties

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

Seems fine, though I would really like it if we could maintain the definitions of all these constants in a single place shared by this and the earlier style-system stuff (in patch 1a) - any chance?

::: gfx/src/nsFont.cpp
@@ +212,5 @@
>  
> +// mapping from bitflag to font feature tag/value pair
> +//
> +// these need to be kept in sync with the constants listed
> +// in gfxFontConstants.h (e.g. NS_FONT_VARIANT_EAST_ASIAN_JIS78)

Is there some way we could keep this stuff defined in a single place, perhaps wrapped with macro magic, and then #include that from each place that needs to know about bits of it?

@@ +293,5 @@
> +  setting.mValue = 1;
> +  switch (variantCaps) {
> +    case NS_FONT_VARIANT_CAPS_ALLSMALL:
> +      setting.mTag = TRUETYPE_TAG('c','2','s','c');
> +      aStyle->featureSettings.AppendElement(setting);

Please include a comment when deliberately falling through case statements, otherwise it looks too much like an accidentally-missed 'break'.

@@ +301,5 @@
> +      break;
> +
> +    case NS_FONT_VARIANT_CAPS_ALLPETITE:
> +      setting.mTag = TRUETYPE_TAG('c','2','p','c');
> +      aStyle->featureSettings.AppendElement(setting);

Ditto.

@@ +374,5 @@
> +  }
> +
> +  if (setting.mTag) {
> +    aStyle->featureSettings.AppendElement(setting);
> +  }

I think it'd be simpler/clearer to do the AppendElement within the switch cases, as in the other switches above, rather than the extra test on setting.mTag.

::: gfx/thebes/gfxFont.h
@@ +113,5 @@
>      // The style of font (normal, italic, oblique)
>      PRUint8 style : 2;
>  
> +    // whether kerning is enabled or not (true by default, can be disabled)
> +    bool kerning : 1;

Please re-order the bitfields to keep the 'bool' ones together (and re-order initializers accordingly in the .cpp file). I don't think it will affect anything, but it seems tidier that way.
Attachment #630865 - Flags: review?(jfkthame) → review+
Comment on attachment 630863 [details] [diff] [review]
part 1a - add font-variant constants and member vars

In nsFont.h, could you group PRUint8 and PRUint16 values together
better so the structure packs better (and is more likely to stay
that way in the face of future modifications)?

Why do you need to add includes to nsFont.h?




>+#define NS_FONT_SYNTHESIS_NONE                      0x0

You ran straight into a pet peeve of mine.  I really don't like having
a constant for "no bits" for a bitfield value, since people tend to
try to | it with the other constants or test it with &.  Is it ok if
you drop this constant and just use explicit 0-initialization where you
need it?

>+#define NS_FONT_VARIANT_ALTERNATES_NORMAL           0

Ditto.  And a bunch more below.

>+#define NS_FONT_VARIANT_ALTERNATES_CONTEXTUAL_OFF   0x2

Maybe use NO_CONTEXTUAL to match the name in the spec?

>+#define NS_FONT_VARIANT_ALTERNATES_CONTEXTUAL_MASK  0x3
>+#define NS_FONT_VARIANT_ALTERNATES_ENUMERATED_MASK  0x7
>+#define NS_FONT_VARIANT_ALTERNATES_FUNCTIONAL_MASK  0x1f8

From a future maintenance perspective I'd slightly prefer to have these
written out as the | of their constituent values.  Though again, if you
have a strong preference to keep it as-is I'm ok with that.

And a bunch more of the same below.

>+#define NS_FONT_VARIANT_NUMERIC_DIAGONAL            0x10
>+#define NS_FONT_VARIANT_NUMERIC_STACKED             0x20

Maybe include "_FRACTIONS" in the name so the average reader can
follow along?



r=dbaron with that
Attachment #630863 - Flags: review?(dbaron) → review+
Comment on attachment 630864 [details] [diff] [review]
part 1b - parsing of font-variant subproperties

Need to add to the 'font' subprop table:  both in nsCSSProps.cpp
and in property_database.js.  Likewise you need to make the code
that parses the 'font' shorthand reset all these new properties.

You should also make 'font-variant' a shorthand (nsCSSPropList.h,
nsCSSProps.cpp subprop table, parsing code, etc., though in
property_database.js terms it should be SHORTHAND_AND_LONGHAND since it
needs to exhibit longhand behavior for backwards-compatibility).  But
you probably don't want to make it support parsing the new stuff yet,
since the new stuff is prefixed.  (But when you eventually do, you'll
need to be careful with the 'font' shorthand.)

Declaration.cpp:

  You need to check that all new subproperties of the font shorthand have their
  initial values, except for the cases that can be expressed as part
  of the font shorthand, 


nsCSSParser.cpp:

  In ParseFontSynthesis:

>+  if (eCSSUnit_Enumerated == aValue.GetUnit() &&
>+      ((intValue = aValue.GetIntValue()) == NS_FONT_SYNTHESIS_NONE))

  The first check should be !=, followed by ||, since you also
  want to return for inherit and initial.  In other words, you
  currently incorrectly allow 'inherit weight'.  You should add a test
  that this is forbidden.  (I'm actually surprised there isn't
  already a test that tests that 'inherit' before any other value
  in property_database.js is rejected, with a list of special
  exceptions.  It's actually possible I have such a test in my
  patch queue.)

I don't like the idea that ParseBitmaskValues allows two ways to
represent the 'normal' value:  eCSSUnit_Normal or eCSSUnit_Enumerated
with the enumerated value being aNormalValue.  There should be one.
(There might be some advantages to going with eCSSUnit_Normal and keeping
'normal' out of the keyword tables.)

The |aMasks| parameter to ParseBitmaskValues should be better documented.
I'm presuming it's for "sets of values of which only one is allowed".

In nsCSSValue.cpp, many of the callers of AppendBitmaskCSSValue need
to have an alternative for when the value is 0.  This should have
caused failures in test_value_storage.html.  If it didn't, I'd like
to know why not.


(At some point we should:
 * remove nsFont::decorations
 * make nsStyleStruct::CalcDifference use nsFont::operator==.
It doesn't have to be now, though.)



There were some pieces I didn't look at really closely since I know
I'm going to want to review this again, but I'm hoping I won't have
any new major comments the second time around.
Attachment #630864 - Flags: review?(dbaron) → review-
Comment on attachment 630870 [details] [diff] [review]
part 2b - parsing of font-variant-alternates

This also requires the shorthand-related changes in the previous patch.
(Those could go in a patch on top, but they should land at at the same
time.)


>+ CSS_PROP_FONT(
>+    -moz-font-variant-alternates,
>+    font_variant_alternates,
>+    CSS_PROP_DOMPROP_PREFIXED(FontVariantAlternates),
>+    CSS_PROPERTY_PARSE_VALUE |
>+        CSS_PROPERTY_VALUE_PARSER_FUNCTION |
>+        CSS_PROPERTY_APPLIES_TO_FIRST_LETTER_AND_FIRST_LINE,
>+    "",
>+    VARIANT_HK,
>>+    kFontVariantAlternatesKTable,
>+    CSS_PROP_NO_OFFSET,
>+    eStyleAnimType_None)

Properties with CSS_PROPERTY_VALUE_PARSER_FUNCTION should have 0 in the
variant field rather than VARIANT_HK.  (This applies to the previous patch
too.)

>+  if (intValue &  NS_FONT_VARIANT_ALTERNATES_FUNCTIONAL_MASK) {

no double-space


>+  switch (variantAlternatesValue->GetUnit()) {
>+  case eCSSUnit_Initial:
>+    aFont->mFont.CopyAlternates(*defaultVariableFont);
>+    break;

This seems sensible, though it doesn't match the spec, which mandates
'normal'.


I'm not crazy about the function naming in nsStyleUtil.  It would be
nice if it was clearer that one function mapped specified -> computed
and the other mapped computed -> string.

And you need to rev the IID for nsIDOMCSS2Properties.  (In each patch
that touches it in theory, although it's ok if it's only one of a set
that land at the same time.)


>+// font-variant-alternates allows for a multiple number of
>+// both simple enumerated values and functional values with

"a multiple number of both" -> "multiple".  And then probably
number the items (1) and (2) to make it clear where they start and end.

>+// value for a bitmap of both simple and functional property values

bitmap -> bitmask

>+// and another value containing a valuelist with lists of idents

valuelist -> ValuePairList, I think



I still need to look at the nsCSSParser changes.
Comment on attachment 630870 [details] [diff] [review]
part 2b - parsing of font-variant-alternates

nsCSSParser.cpp:


+  nsCSSKeyword keyword = nsCSSKeywords::LookupKeyword(mToken.mIdent);
+  if (eCSSKeyword_UNKNOWN < keyword &&
+      nsCSSProps::FindKeyword(keyword,
+                              (isIdent ?
+                               nsCSSProps::kFontVariantAlternatesKTable :
+                               nsCSSProps::kFontVariantAlternatesFuncsKTable),
+                              aWhichFeature))
+  {

Please invert this test and make the UngetToken(); return false;
be indented and the rest by non-indented.

+    if (isIdent) {
+      aValue.SetIntValue(aWhichFeature, eCSSUnit_Enumerated);
+      return true;
+    } else {
+      PRUint16 maxElems = 1;
+      if (keyword == eCSSKeyword_styleset ||
+          keyword == eCSSKeyword_character_variant) {
+        maxElems = MAX_ALLOWED_FEATURES;
+      }
+      return ParseFunction(mToken.mIdent, nsnull, VARIANT_IDENTIFIER,
+                           1, maxElems, aValue);
+    }
+  }





+bool
+CSSParserImpl::ParseFontVariantAlternates(nsCSSValue& aValue)
+{
+  if (ParseVariant(aValue, VARIANT_INHERIT | VARIANT_NORMAL, nsnull)) {
+    if (!ExpectEndProperty()) {
+      return false;
+    }
+    return true;
+  }

You don't need the ExpectEndProperty() check.  You can just return true
if this ParseVariant succeeds.

You don't need it because:
 (1) ParseProperty does it where it calls ParseSingleValueProperty

 (2) ParseProperty doesn't even need it, I think, because all of its
     callers (should, at least) either check for what's next or check
     that there's nothing left in the buffer before transferring what's
     in mTempData into mData (which is the entire point of mTempData).

And you also don't want it for when you implement the full
'font-variant' shorthand.

(ExpectEndProperty() is in general bad form; I'd really like to get rid
of it and move everything towards the model of checking for what is
accepted rather than looking for a sample of what is not accepted.  This
is much more forward-compatible with things like reusing
property-parsing code for @supports, using property parsers like the one
you're writing as part of a shorthand, etc.)


>+  while (ParseSingleAlternate(feature, cur->mValue)) {
>+
>+    // check to make sure value not normal and not already set
>+    if (feature == NS_FONT_VARIANT_ALTERNATES_NORMAL ||
>+        feature & featureFlags) {
>+      return false;
>+    }

Seems better if ParseSingleAlternate doesn't accept 'normal' (which
I think fits with using eCSSUnit_Normal as I described above).  You
still need the repeated-feature check, though.

>+    PRInt32 m1 = NS_FONT_VARIANT_ALTERNATES_CONTEXTUAL_MASK;
>+    PRInt32 c = 0;
>+
>+    PRInt32 other = ~feature & featureFlags;
>+    if (m1 & feature) {
>+      c = other & m1;
>+    }
>+
>+    if (c) {
>+      return false;
>+    }

This simplifies to:
+    PRInt32 m1 = NS_FONT_VARIANT_ALTERNATES_CONTEXTUAL_MASK;
+    if (m1 & feature && m1 & ~feature & featureFlags) {
+      return false;
+    }

However, I think you should further simplify to:
+    PRInt32 m1 = NS_FONT_VARIANT_ALTERNATES_CONTEXTUAL_MASK;
+    if (m1 & feature && m1 & featureFlags) {
+      return false;
+    }
even though that's duplicating the check for repeated features that
you already made above.

(Are there cases in the previous patch where you accept repeated
values but shouldn't?)

>+    if (cur->mValue.GetUnit() == eCSSUnit_Function) {
>+      cur->mNext = new nsCSSValueList;
>+      cur = cur->mNext;
>+    }

This means that if the last value is not a function, you'll have an
extra value on the end of your linked list.  That's really weird.  I
think it would be better to avoid that, probably by passing
ParseSingleAlternate an nsCSSValue that's not already part of an
nsCSSValueList, and then calling either SetListValue or new
nsCSSValueList if there's an item to be created.  Alternately, track
it and delete it.

This should also let you remove the eCSSUnit_Function check in
+nsStyleUtil::AppendAlternateValues.



Please make ParseFunction and ParseFunctionInternals assert at the start
that exactly one of aAllowedTypes / aAllowedTypesAll is null/0.  And
also make the comment a little clearer that it's either one or the other.
(And fix the spelling of the new argument name in the comment.)

>+    PRInt32 m = aVariantMaskAll ? aVariantMaskAll : aVariantMask[index];

It's safer to branch on the null-ness of aVariantMask than the 0-ness
of aVariantMaskAll (in case somebody is dynamically manipulating the
variant fields, potentially all the way to 0, as calc() might).




nsStyleUtil.cpp:

>+      funcParams.Assign(v.value);
...
>+      funcParams.Append(v.value);

This need to use AppendEscapedCSSIdent, I believe.  You should add some
tests with wacky identifiers that require escapes to
property_database.js.


>+    if (key == eCSSKeyword_UNKNOWN ||
>+        !nsCSSProps::FindKeyword(key,
>+                                 nsCSSProps::kFontVariantAlternatesFuncsKTable,
>+                                 v.alternate)) {
>+      continue;
>+    }

The FindKeyword shouldn't fail either, so maybe move the assertion into
the if and make it an NS_NOTREACHED?

r=dbaron with that plus comment 37 (plus the relevant changes
from the review on the previous patch).

(I'm not going to ask you to change from i++ to ++i... though I do
prefer it, since it's a conceptually simpler operation and a good
habit to get into for when you use your integer habits on iterator
types where it might be more expensive.  But I did notice.)
Attachment #630870 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] from comment #38)
 
> >+    if (cur->mValue.GetUnit() == eCSSUnit_Function) {
> >+      cur->mNext = new nsCSSValueList;
> >+      cur = cur->mNext;
> >+    }
> 
> This means that if the last value is not a function, you'll have an
> extra value on the end of your linked list.  That's really weird.  I
> think it would be better to avoid that, probably by passing
> ParseSingleAlternate an nsCSSValue that's not already part of an
> nsCSSValueList, and then calling either SetListValue or new
> nsCSSValueList if there's an item to be created.  Alternately, track
> it and delete it.
> 
> This should also let you remove the eCSSUnit_Function check in
> +nsStyleUtil::AppendAlternateValues.

The font-variant-alternates property has both simple enumerated values (currently only 'historical-forms') and several functional values (e.g. 'swash(swishy)').  So the nsCSSValue for this is a pair - the x side of the pair reflects the set of property values and the y side a list of individual functional values.  So when only 'historical-forms' are specified, there are no functional values to store.  For this case, I tried simply using a null value or a dummy value but assertions fire in both cases.  So for now what I have is a single element dummy list when no functional values exist, and an actual list when there are functional values.  In that case, the length of the list now matches the number of functional values.
Updated based on comments.  Carrying forward r=dbaron
Attachment #630863 - Attachment is obsolete: true
Attachment #656338 - Flags: review+
(In reply to Jonathan Kew (:jfkthame) from comment #34)
> Comment on attachment 630865 [details] [diff] [review]
> part 1c - gfx support for font-variant properties
>
> ::: gfx/src/nsFont.cpp
> @@ +212,5 @@
> >  
> > +// mapping from bitflag to font feature tag/value pair
> > +//
> > +// these need to be kept in sync with the constants listed
> > +// in gfxFontConstants.h (e.g. NS_FONT_VARIANT_EAST_ASIAN_JIS78)
> 
> Is there some way we could keep this stuff defined in a single place,
> perhaps wrapped with macro magic, and then #include that from each place
> that needs to know about bits of it?

This seems like overkill for such a small set of values that probably won't change very much
over time.  Instead, I defined the constants to be based on enum values with an explicit count
so that a simple check can be set up to verify that the lists in the two places are in sync.
Updated based on review comments.
Attachment #630864 - Attachment is obsolete: true
Attachment #656340 - Flags: review?(dbaron)
Updated based on review comments.  Carrying forward r=jkew
Attachment #630865 - Attachment is obsolete: true
Attachment #656341 - Flags: review+
Updated based on other changes.
Attachment #630866 - Attachment is obsolete: true
Attachment #630866 - Flags: review?(jfkthame)
Attachment #656342 - Flags: review?(jfkthame)
Attached patch part 1e - font-kerning reftests (obsolete) — Splinter Review
Updated to trunk
Attachment #630868 - Attachment is obsolete: true
Attachment #630868 - Flags: review?(jfkthame)
Attachment #656343 - Flags: review?(jfkthame)
Updated based on other changes and comments.
Attachment #630869 - Attachment is obsolete: true
Attachment #656345 - Flags: review?(jfkthame)
Updated based on review comments.  Carrying forward r=dbaron.  Changes requiring re-review are in patch 2f.
Attachment #630870 - Attachment is obsolete: true
Attachment #656346 - Flags: review+
Implemented serialization code and fixed parsing bugs found while testing.
Attachment #630871 - Attachment is obsolete: true
Attachment #656348 - Flags: review?(dbaron)
@font-feature-values rule parse tests in mochitest form
Attachment #630872 - Attachment is obsolete: true
Attachment #656349 - Flags: review?(dbaron)
Attachment #656350 - Flags: review?(jfkthame)
Attachment #656351 - Flags: review?(dbaron)
(In reply to John Daggett (:jtd) from comment #39)
> The font-variant-alternates property has both simple enumerated values
> (currently only 'historical-forms') and several functional values (e.g.
> 'swash(swishy)').  So the nsCSSValue for this is a pair - the x side of the
> pair reflects the set of property values and the y side a list of individual
> functional values.  So when only 'historical-forms' are specified, there are
> no functional values to store.  For this case, I tried simply using a null
> value or a dummy value but assertions fire in both cases.  So for now what I
> have is a single element dummy list when no functional values exist, and an
> actual list when there are functional values.  In that case, the length of
> the list now matches the number of functional values.

Then (if you don't have one already), could you add a clear comment explaining that?
Comment on attachment 656340 [details] [diff] [review]
part 1b v2 - parsing of font-variant subproperties

Missed this part of comment 36:
> You should also make 'font-variant' a shorthand (nsCSSPropList.h,
> nsCSSProps.cpp subprop table, parsing code, etc., though in
> property_database.js terms it should be SHORTHAND_AND_LONGHAND since it
> needs to exhibit longhand behavior for backwards-compatibility).  But
> you probably don't want to make it support parsing the new stuff yet,
> since the new stuff is prefixed.  (But when you eventually do, you'll
> need to be careful with the 'font' shorthand.)

I think I remember saying it was ok (in fact, probably preferable) if
this was in another patch, but it still needs to happen at the same
time.


In nsCSSValue.cpp, the font-synthesis case no longer needs to
handle intValue==0, since that can't happen as eCSSUnit_Enumerated.

In ParseFontSynthesis and ParseBitmaskValues, it's probably easier to
make the check != eCSSUnit_Enumerated rather than checking whether it ==
None, Inherit, or Initial.

Also, in ParseFontSynthesis, this check:
>+    if (nextIntValue == 0 || nextIntValue & intValue) {
can drop the part before the || now that None isn't in the keyword
table.

And don't put extra spaces here:
>+  nsCSSValue  nextValue;
or, in ParseBitmaskValues:
>+  nsCSSValue  nextValue;
>+  int32_t     mergedValue = aValue.GetIntValue();

Also, in ParseBitmaskValues, I don't see why you need the |other|
variable; can't you just check mergedValue & *m instead of
other & *m?

Make maskEastAsian, maskLigatures and maskNumeric static in
addition to const.


The comments above the changed (relative to the previous patch)
SetDiscrete calls in nsRuleNode.cpp need updating (for now taking none
or normal), and those comments should probably mention that the
none/normal value is 0.  And also all but one comment needs updating for
-moz-system-font as well.


Either drop the change to nsAnimationManager.cpp or cast the first
half of the < to a size_t.

r=dbaron with that, plus comment 52
Attachment #656340 - Flags: review?(dbaron) → review+
Comment on attachment 656348 [details] [diff] [review]
part 2c v2 - parsing of @font-feature-values rule

A genral comment on the whole bug:  as far as using prefixes or a preference.  I think at this point we'd normally want to use a preference rather than prefixes.  However, given that we're already shipping -moz-font-feature-settings, I think a prefix is probably the right thing to do here for now -- but you should be aware that this is an exception case, and  in general
at this point our policy would be to put new features like this
behind a pref instead of a prefix.


nsDOMClassInfo*:

>+  NS_DEFINE_CLASSINFO_DATA(CSSMozFontFeatureValuesRule, nsDOMGenericSH,
>+                           DOM_DEFAULT_SCRIPTABLE_FLAGS)

Use MozCSS rather than CSSMoz, to match other things.
(applies to a bunch of places, including the name of the
interface and its filename)

(Probably the easiest way to fix this is to search-replace the
whole patchfile.)


nsIDOMCSSRule.idl:

  s/MOZ_FONT_FEATURE_VALUES/MOZ_FONT_FEATURE_VALUES_RULE/


nsCSSParser.cpp:

>+  bool ParseFontFeatureValuesRule(RuleAppendFunc aAppendFunc,
>+                                    void* aProcessData);

fix indent (2 fewer spaces)


ParseFontFeatureValuesRule:


+    if (!ParseFontFeatureValuesDeclaration(valuesRule)) {
+      REPORT_UNEXPECTED(PEDeclSkipped);
+      OUTPUT_ERROR();
+      if (!SkipDeclaration(true)) {
+        break;
+      }
+    }

You want to use SkipAtRule instead of SkipDeclaration here, so
that you match the general grammar.

(I also think you might want to not use the word Declaration
in that function name -- and probably also in the terminology
in the spec.  Not sure I have a better idea, though.)

+  if (!ExpectSymbol('}', true)) {
+    REPORT_UNEXPECTED_TOKEN(PEFFVUnexpectedBlockEnd);
+    return false;
+  }

This needs a SkipUntil('}') before returning false.

ParseFontFeatureValuesDeclaration:

>+  if (eCSSToken_AtKeyword != mToken.mType) {
>+    REPORT_UNEXPECTED_TOKEN(PEFontFeatureValuesNoAt);
>+    OUTPUT_ERROR();
>+    return false;
>+  }

You need to UngetToken() here before returning false.  (It
could be an {, in which case you have to skip to the }.)

Same for the next "return false" for a bad keyword.

>+    if (eCSSToken_Ident != mToken.mType) {
>+      REPORT_UNEXPECTED_TOKEN(PEFFVExpectedIdent);
>+      return false;
>+    }

UngetToken() here too.

But you should also move this into the loop and remove the
identical chunk before the only "continue" inside the loop.

Then you should change the loop to just be a for(;;), since
the only times the loop condition fails are:
 * the first time through the loop (so once you remove it,
   you can remove the check below for values.Length())
 * when you have a line ending with ,;, which is a case you
   shouldn't accept but currently do (if I'm reading things
   correctly)

Then you can swap the order of the ; and , checks at the end,
invert the conditions for both, so you have:
  if ;
    break
  if not ,
    return false
and you won't need to use continue at all.

Then you can also remove this bit after the loop:
>+  if (!mToken.IsSymbol(';')) {
>+    REPORT_UNEXPECTED_TOKEN(PEFFVUnexpectedBlockEnd);
>+    return false;
>+  }

nsCSSRules.cpp:

>+//DOMCI_DATA(MozCSSKeyframeRule, nsCSSKeyframeRule)

remove this

>+//   for (nsCSSFontDesc id = nsCSSFontDesc(eCSSFontDesc_UNKNOWN + 1);
>+//        id < eCSSFontDesc_COUNT;
>+//        id = nsCSSFontDesc(id + 1))
>+//     if ((mDecl.*nsCSSFontFaceStyleDecl::Fields[id]).GetUnit()
>+//         != eCSSUnit_Null) {
>+//       if (NS_FAILED(mDecl.GetPropertyValue(id, descStr)))
>+//         descStr.AssignLiteral("#<serialization error>");
>+//       else if (descStr.Length() == 0)
>+//         descStr.AssignLiteral("#<serialization missing>");
>+//       fprintf(out, "%s%s: %s\n",
>+//               descInd.get(), nsCSSProps::GetStringValue(id).get(),
>+//               NS_ConvertUTF16toUTF8(descStr).get());
>+//     }

Why is this commented out?


>+#if 0
>+
>+  struct ValueList {
>+    ValueList(const nsAString& aName, const nsTArray<uint32_t>& aSelectors)
>+      : name(aName), featureSelectors(aSelectors)
>+    {
>+    }
>+
>+    nsString             name;
>+    nsTArray<uint32_t>   featureSelectors;
>+  };
>+
>+  struct FeatureValues {
>+    int32_t              alternate;
>+    nsTArray<ValueList>  valuelist;
>+  };
>+
>+#endif

remove this if you don't need it

Seems like you should at least implement:

>+NS_IMETHODIMP
>+nsCSSFontFeatureValuesRule::GetFontFamily(nsAString& aFontFamily)
>+{
>+  return NS_ERROR_NOT_IMPLEMENTED;
>+}

and:

>+NS_IMETHODIMP
>+nsCSSFontFeatureValuesRule::GetValueText(nsAString& aValueText)
>+{
>+  return NS_ERROR_NOT_IMPLEMENTED;
>+}

perhaps by moving much of your GetCssText implementation into
them and making GetCssText call them?


>+void
>+nsCSSFontFeatureValuesRule::SetFamilyList(const nsAString& aFamilyList,
>+                                          bool& aContainsGeneric)
>+{
>+  nsAutoString silly(aFamilyList);
>+  nsFont font(silly, 0, 0, 0, 0, 0, 0);

Don't use |silly|.  Instead, change the nsFont::nsFont constructor
that you're using from taking const nsString& to taking
const nsSubstring&.  (That signature change should just compile
without any other changes.)


nsCSSFontFeatureValuesRule::AddValueList:

>+  // create a new list for a given property value
>+  if (!foundAlternate) {
>+    gfxFontFeatureValueSet::FeatureValues f;
>+    f.alternate = aVariantAlternate;
>+    f.valuelist.AppendElements(aValueList);
>+    mFeatureValues.AppendElement(f);

Instead of doing this, which means copying the inner array on the last
line, use:

  gfxFontFeatureValueSet::FeatureValues &f = *mFeatureValues.AppendElement();
  f.alternate = aVariantAlternate;
  f.valuelist.AppendElements(aValueList);

nsCSSRules.h:

The copy-constructor needs to copy mFamilyList and mFeatureValues.

Annotate all the methods that override methods on the base class
as MOZ_OVERRIDE (so that there's an error if they end up not
overriding).  I think that's GetType() and Clone() and List().

I originally wrote (but you should ignore this):

  # nsStyleSet.cpp:
  # 
  #   Please invert the sense (true/false) of mInitFontFeatureValuesLookup
  #   and probably call it mFontFeatureValuesLookupInitialized (since
  #   that's how we typically do such things).
  # 
  # nsStyleSet.h:
  # 
  # >+  // whether font feature values lookup object needs initialization
  # 
  # this comment is on the wrong one of your two new members.
  # (but see nsStyleSet.cpp comment)

Instead, I think the whole manner in which you cache the
mFontFeatureValuesLookup is wrong.

In particular, you need to throw this out when
nsPresContext::RebuildAllStyleData or
nsIPresShell::ReconstructStyleDataInternal is called.  (Notice that both
of those functions call nsPresContext::RebuildUserFontSet and
AnimationManager()->KeyframesListIsDirty().

I think you should store the mFontFeatureValuesLookup on the
nsPresContext (like the user font set is), and invalidate it at
similar times.

And I think now that you're adding a third thing that needs to be
cleared, you should consolidate those three calls into a method called
nsPresContext::AtRulesChanged rather than listing all 3 in 2 places.
(I probably should have done that sooner.)

It's also not clear to me if anything else needs to be invalidated
when these change, although I'd think something would be -- though
does invalidating the user font set cover it?



With those things fixed, I think this looks good, but I want to have
another look at:
 * the gfxFontFeatureValuesLookup management
 * ParseFontFeatureValuesDeclaration:
so I'm marking as review-.
Attachment #656348 - Flags: review?(dbaron) → review-
Comment on attachment 656349 [details] [diff] [review]
part 2d - parsing tests of @font-feature-values rule

Next time, it would be better to write a testharness.js test (which
can be hooked up to mochitest).  But since you've written it, it's
not worth redoing.

But rather than commenting the standalone stuff, maybe just use
the pattern:

 if (gStandalone) {
   window.ok = function(...) {
     ...
   }
 }

Though it used to be easy enough to run mochitests standalone;
I guess it looks like they reach SimpleTest.js etc. with a /tests
now rather than with ../../../SimpleTest/... .


Maybe put the gPrefix up at the top near gStandalone and gVerbose,
since it's configuration?


>+  { rule: makeRule("\\62 ongo", "@styleset blah 1;"), serializationSame: true },

I'm a little confused by the serializationSame here.  I'd have
expected you to need to give "bongo" for the serialization.

gSurroundingTests could use a comment

>+      if ("serializationSame" in test && test.serializationSame) {

You don't need to test both; undefined is false in boolean context.

(The reason people do if ("foo" in bar) is generally because they're
going to do something to foo.bar.baz, not just test foo.bar for
truthiness.)

>+      } else if ("serializationEmpty" in test && test.serializationEmpty) {

same here

>+/* strip out just values */
>+function valuesText(ruletext)

Maybe say "extract the part inside the last innermost {}"?

I guess this explains the "\\62 ongo" above.  It might be nice to strip
out a bit less, if it's not too hard, at least for the first set of
users of valuesText?

Also, maybe move ruleValues into the if where it's used?

And maybe make testParse not catch exceptions?  Seems like it could
hide things.


There are also two tests that set serializationEmpty but have a
rule instead of a ruleSrc.  This should probably trigger a failure
of some sort since it seems like a bug in the test.

>\ No newline at end of file

Newline, please.

r=dbaron
Attachment #656349 - Flags: review?(dbaron) → review+
Comment on attachment 656351 [details] [diff] [review]
part 2f - changes based on review comments for part 2b

>additional property_database.js tests

I'm guessing this patch is intended to be rolled into another one.
If not, it needs a better commit message.

>+  if (!(eCSSKeyword_UNKNOWN < keyword &&

Use != rather than <.

(Or, even better, use ==, change the && to ||, and move the ! to
the second half only.)

Also, CSSParserImpl::ParseFontVariantAlternates currently returns
true even if it doesn't consume any text.  That seems wrong.  The
first ParseSingleAlternate call should be required to succeed.

>+  NS_ASSERTION((aVariantMask && !aVariantMaskAll) ||
>+               (!aVariantMask && aVariantMaskAll),
>+               "only one of the two variant mask parameters can be set");
>+

>+  NS_ASSERTION((aAllowedTypes && !aAllowedTypesAll) ||
>+               (!aAllowedTypes && aAllowedTypesAll),
>+               "only one of the two allowed type parameter can be set");

I think asserting
  !aVariantMask ^ !aVariantMaskAll
would be clearer, since XOR is the actual relationship you're trying
to assert.


>-    nsStyleUtil::AppendFunctionalAlternates(GetStyleFont()->mFont.alternateValues,
>+    nsStyleUtil::SerializeFunctionalAlternates(GetStyleFont()->mFont.alternateValues,
>                                             valueStr);

fix indent


>-      nsStyleUtil::AppendAlternateValues(
>+      NS_ASSERTION(variantAlternatesValue->GetPairValue().mYValue.GetUnit() ==
>+                   eCSSUnit_List, "function list not a list value");
>+      nsStyleUtil::ComputeFunctionalAlternates(
>         variantAlternatesValue->GetPairValue().mYValue.GetListValue(),
>         aFont->mFont.alternateValues);

Maybe have a variable
  const nsCSSValue& funcList = variantAlternatesValue->GetPairValue().mYValue;
to avoid this duplication (and keep the assertion more valuable by
ensuring it stays in sync)?


>-      funcParams.Assign(v.value);
>+      AppendEscapedCSSIdent(v.value, funcParams);

This needs to do:
  funcParams.Truncate()
before appending.


Also, SerializeFunctionalAlternates should assert that v.alternate != 0
since you're relying on that being true by using 0 as the dummy value.

r=dbaron with that
Attachment #656351 - Flags: review?(dbaron) → review+
Comment on attachment 656345 [details] [diff] [review]
part 2a v2 - gfx support for font-variant-alternates

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

Looks OK, modulo some tidying-up as per suggestions below.

::: gfx/src/nsFont.cpp
@@ +158,5 @@
> +nsFont::CopyAlternates(const nsFont& aOther)
> +{
> +  alternateValues.Clear();
> +  variantAlternates = aOther.variantAlternates;
> +  alternateValues = aOther.alternateValues;

Why do we need the Clear() above, when we're about to assign a new value here anyway?

@@ +318,5 @@
>  
> +  // -- alternates
> +  if (variantAlternates) {
> +
> +    if (variantAlternates & NS_FONT_VARIANT_ALTERNATES_HISTORICAL) {

The outer "if" here is redundant.

::: gfx/src/nsFont.h
@@ +69,5 @@
> +  // -- list of value tags for font-specific alternate features
> +  nsTArray<gfxAlternateValue> alternateValues;
> +
> +  // -- object used to look these up once the font is matched
> +  nsRefPtr<gfxFontFeatureValueSet> featureValueLookup;

Can we re-order the fields here for tidier struct packing, please? We shouldn't intersperse pointer-sized fields at arbitrary places among byte/word-sized ones.

::: gfx/thebes/gfxFont.cpp
@@ +1319,5 @@
> +static void
> +LookupAlternateValues(gfxFontFeatureValueSet *featureLookup,
> +                      const nsAString& aFamily,
> +                      const nsTArray<gfxAlternateValue>& altValue,
> +                      nsTArray<gfxFontFeature>& aFontFeatures)

A general comment: I don't think we need to be quite so generous with the blank lines throughout this function - it seems excessively spaced-out vertically.

@@ +1323,5 @@
> +                      nsTArray<gfxFontFeature>& aFontFeatures)
> +{
> +    uint32_t numAlternates;
> +
> +    numAlternates = altValue.Length();

for conciseness, combine the declaration and assignment

@@ +1346,5 @@
> +        // CSS parsing should have caught errors
> +        NS_ASSERTION(numValues == 1 ||
> +            av.alternate == NS_FONT_VARIANT_ALTERNATES_CHARACTER_VARIANT ||
> +            av.alternate == NS_FONT_VARIANT_ALTERNATES_STYLESET,
> +            "too many values for font-specific font-variant-alternates");

I think it'd be clearer to put the appropriate (simpler) assertion within each of the branches below, rather than this rather cumbersome one here.

@@ +1350,5 @@
> +            "too many values for font-specific font-variant-alternates");
> +
> +        gfxFontFeature feature;
> +
> +        if (av.alternate == NS_FONT_VARIANT_ALTERNATES_CHARACTER_VARIANT)

The if ... else if ... else sequence here looks to me like it'd be clearer as a switch with specific cases, and then a default case. (That'll be increasingly true if there are future extensions to font-variant alternates.)

Or end each of the cvXX and ssXX "if" blocks with "continue"; then you won't need "else", and can unindent the final block with its switch for the various other alternates.

@@ +1351,5 @@
> +
> +        gfxFontFeature feature;
> +
> +        if (av.alternate == NS_FONT_VARIANT_ALTERNATES_CHARACTER_VARIANT)
> +        {

nit - brace goes on the end of the line above

@@ +1367,5 @@
> +
> +            // ignore values greater than 99
> +            if (nn == 0 || nn > 99) {
> +                continue;
> +            }

Seems more logical to validate nn first, before reading feature.mValue.

@@ +1379,5 @@
> +
> +            feature.mValue = 1;
> +            for (uint32_t v = 0; v < numValues; v++) {
> +                uint32_t nn = values.ElementAt(v);
> +                if (nn == 0 || nn > 20) {

Although the Microsoft registry only documents ss01..ss20, I've heard of at least one font developer extending this pattern (I think they went up to about ss50-ish, but obviously ss99 is the logical limit). Should we permit that in CSS/Gecko?

::: gfx/thebes/gfxFont.h
@@ +124,5 @@
> +    // -- list of value tags for specific alternate features
> +    nsTArray<gfxAlternateValue> alternateValues;
> +
> +    // -- object used to look these up once the font is matched
> +    nsRefPtr<gfxFontFeatureValueSet> featureValueLookup;

Please put these up with the other pointer-/array-based members, earlier in the struct.

::: gfx/thebes/gfxFontFeatures.cpp
@@ +15,5 @@
> +}
> +
> +bool
> +gfxFontFeatureValueSet::GetFontFeatureValuesFor(const nsAString& aFamily,
> +                                               int32_t aVariantProperty,

line up the declarations, please

@@ +44,5 @@
> +{
> +    nsAutoString family(aFamily);
> +    ToLowerCase(family);
> +
> +    uint32_t i, numFeatureValues;

You can declare these in the lines that actually assign to them below; no need for this separate line.

@@ +52,5 @@
> +    for (i = 0; i < numFeatureValues; i++) {
> +        const FeatureValues& fv = aValues.ElementAt(i);
> +        int32_t alternate = fv.alternate;
> +
> +        uint32_t j, numValues;

Ditto. And then most of the blank lines can go, too.

@@ +63,5 @@
> +            FeatureValueHashKey key(family, alternate, name);
> +            FeatureValueHashEntry *entry = mFontFeatureValues.PutEntry(key);
> +            entry->mKey = key;
> +            entry->mValues.Clear();  // new def'n wipes out old one
> +            entry->mValues.AppendElements(v.featureSelectors);

Can't we just assign, rather than doing Clear() and then AppendElements()?

::: gfx/thebes/gfxFontFeatures.h
@@ +30,5 @@
>  operator==(const gfxFontFeature& a, const gfxFontFeature& b)
>  {
>      return (a.mTag == b.mTag) && (a.mValue == b.mValue);
>  }
>  

A bunch of the new code here uses 2-space indent, but normal style in gfx is 4-space; please make it consistent.

@@ +32,5 @@
>      return (a.mTag == b.mTag) && (a.mValue == b.mValue);
>  }
>  
> +struct gfxAlternateValue {
> +  int32_t            alternate;  // constants in gfxFontConstants.h

Would it be more logical for all the feature-type constants, values, etc to be uint32_t? There seems to be a mixture at the moment, with 'alternate' here being int32_t, but featureSelectors (below) being uint32_t, for example; then again, mPropVal in the hash key is signed.

No need for so many spaces here.
Attachment #656345 - Flags: review?(jfkthame) → review+
Comment on attachment 656343 [details] [diff] [review]
part 1e - font-kerning reftests

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

Please add an HTML doctype to each of the test files, to avoid quirks mode.

Also, I think a few of these would benefit from comments highlighting the particular aspect being tested.

::: layout/reftests/font-features/font-kerning-1.html
@@ +10,5 @@
> +  font-family: libertine, sans-serif;
> +  font-size: 600%;
> +  line-height: 1.2em;
> +  -moz-font-feature-settings: "kern" off;
> +  -moz-font-kerning: normal;

comment: -moz-font-feature-settings should take precedence over -moz-font-kerning, so kerning should be DISabled here.

::: layout/reftests/font-features/font-kerning-2.html
@@ +10,5 @@
> +  font-family: libertine, sans-serif;
> +  font-size: 600%;
> +  line-height: 1.2em;
> +  -moz-font-feature-settings: "kern" on;
> +  -moz-font-kerning: none;

comment: -moz-font-feature-settings should take precedence over -moz-font-kerning, so kerning should be ENabled here.

::: layout/reftests/font-features/font-kerning-table-none.html
@@ +2,5 @@
> +<head>
> +<style type="text/css">
> +@font-face {
> +  font-family: gentium;
> +  src: url(../fonts/sil/GenR102.ttf);

comment: font that has an old-style 'kern' table rather than GPOS 'kern' feature
Attachment #656343 - Flags: review?(jfkthame) → review+
Comment on attachment 656342 [details] [diff] [review]
part 1d v2 - font-variant feature reftests

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

This looks great, but I'd like to see a bit of cleanup/clearer comments; more significantly, one of the testcases looks to me like it ought to fail as written here - does it?

::: layout/reftests/font-features/font-features-order-3.html
@@ +10,5 @@
> +body {
> +  font-family: libertine, sans-serif;
> +  font-size: 400%;
> +  line-height: 2em;
> +  -moz-font-feature-settings: "liga" 0, "hlig" off;

Let's not mix "0" and "off" values here, as that's not the point of this testcase; either use "off/on" or "0/1", but keep them consistent.

Also, it'd be nice to have a comment in each of these test files pointing out the specific issue being tested. (E.g., "style properties should take precedence over descriptors in @font-face" or something like that.)

::: layout/reftests/font-features/font-features-order-4.html
@@ +4,5 @@
> +<style type="text/css">
> +@font-face {
> +  font-family: libertine;
> +  src: url(../fonts/LinLibertine_Re-4.7.5.woff) format("woff");
> +  -moz-font-feature-settings: "liga" 1, "hlig" on;

Unnecessary mixing of numbers and keywords again.

::: layout/reftests/font-features/font-features-order-5.html
@@ +4,5 @@
> +<style type="text/css">
> +@font-face {
> +  font-family: libertine;
> +  src: url(../fonts/LinLibertine_Re-4.7.5.woff) format("woff");
> +  -moz-font-feature-settings: "liga" 1, "hlig" on;

And again.

@@ +11,5 @@
> +  font-family: libertine, sans-serif;
> +  font-size: 400%;
> +  line-height: 2em;
> +  -moz-font-variant-ligatures: no-common-ligatures no-historical-ligatures;
> +  -moz-font-feature-settings: "liga", "hlig";

This time the "on" value is implicit. Let's be consistent.

::: layout/reftests/font-features/font-variant-features.js
@@ +4,5 @@
> +// prefix
> +gPrefix = "-moz-";
> +
> +// equivalent properties
> +// setting prop: value should match the

the....what? (presumably the specified feature tags)

@@ +35,5 @@
> +  { prop: "font-variant-east-asian", value: "full-width", features: {"fwid": 1, "jp04": 0} },
> +  { prop: "font-variant-east-asian", value: "proportional-width", features: {"pwid": 1, "jp04": 0} },
> +  { prop: "font-variant-east-asian", value: "ruby", features: {"ruby": 1, "jp04": 0} },
> +  { prop: "font-variant-east-asian", value: "jis78 full-width", features: {"jp78": 1, "fwid": 1, "jp83": 0} },
> +  { prop: "font-variant-east-asian", value: "jis78 full-width ruby", features: {"jp78": 1, "fwid": 1, "jp83": 0} },

This looks wrong. Does 'ruby' really have no effect here?

@@ +37,5 @@
> +  { prop: "font-variant-east-asian", value: "ruby", features: {"ruby": 1, "jp04": 0} },
> +  { prop: "font-variant-east-asian", value: "jis78 full-width", features: {"jp78": 1, "fwid": 1, "jp83": 0} },
> +  { prop: "font-variant-east-asian", value: "jis78 full-width ruby", features: {"jp78": 1, "fwid": 1, "jp83": 0} },
> +  { prop: "font-variant-east-asian", value: "simplified proportional-width", features: {"smpl": 1, "pwid": 1, "jp83": 0} },
> +  { prop: "font-variant-east-asian", value: "ruby simplified", features: {"ruby": 1, "smpl": 1, "trad": 0} },

It's not clear why most of the lines above only choose one of the four JIS features to disable, or why that particular one is chosen to be mentioned but others aren't. (Or indeed why any of them need to be explicitly disabled at all, as they're not on-by-default, are they?)

@@ +109,5 @@
> +  { prop: "font-variant-position", value: "super sub", features: {"subs": 0, "sups": 0}, invalid: true },
> +];
> +
> +// The font defines feature lookups for all OpenType features for a specific
> +// set of PUA codepoints, listed below.  Using these codepoints and feature

"listed below" - where? it's not obvious what codepoints this is talking about.

@@ +174,5 @@
> +    for (var f in data.features) {
> +      var feature = data.features[f];
> +
> +      var cp, unsupported = "F".charCodeAt(0);
> +      var basecp = gFeatures[f];

Where's gFeatures defined? Oh, I see, it's in ../fonts/gsubtest/gsubtest-features.js, which therefore has to be included before this file. Please add some comments to help tie this stuff together.
Comment on attachment 656350 [details] [diff] [review]
part 2e v2 - font-variant-alternates reftests

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

Looks good, but here as well I think it'd be really helpful to have some additional comments about what's going on. See below for some suggestions.

::: layout/reftests/font-features/alternates-order.html
@@ +25,5 @@
> +}
> +
> +@-moz-font-feature-values fontA {
> +  @styleset aLtW 5;
> +  @styleset slip-sliding-w 5;

The second name defined for ss05 here doesn't seem to be tested at all.

@@ +50,5 @@
> +
> +div { margin: 0 20px; }
> +span {
> +
> +}

Would be nice to clean up left-over boilerplate like this.

@@ +63,5 @@
> +}
> +
> +#test3 {
> +  font-family: fontB;
> +  -moz-font-variant-alternates: styleset(ALTW);

comment: testing case-insensitivity of styleset name

@@ +73,5 @@
> +}
> +
> +#test5 {
> +  font-family: fontA;
> +  -moz-font-variant-alternates: styleset(defined-for-fontB);

comment: the styleset name that is only defined for fontB should not affect fontA rendering

@@ +78,5 @@
> +}
> +
> +#test6 {
> +  -moz-font-variant-alternates: styleset(somethingElse);
> +  -moz-font-feature-settings: "ss05" on;

comment: font-feature-settings takes precedence over font-variant-alternates

@@ +84,5 @@
> +
> +#test7 {
> +  font-family: fontA;
> +  -moz-font-variant-alternates: styleset(scriptJ);
> +  -moz-font-feature-settings: "ss06";

comment: neither of these properties should disable the "ss05" that was set in the @font-face rule

@@ +89,5 @@
> +}
> +
> +#test8 {
> +  font-family: fontB;
> +  -moz-font-variant-alternates: styleset(scriptJ, somethingUndefined, defined-for-fontB);

comment: undefined styleset name does not prevent the following one being recognized

::: layout/reftests/font-features/annotations-ref.html
@@ +17,5 @@
> +@font-face {
> +  font-family: testMeiryo-circled;
> +  src: local(Meiryo);
> +  -moz-font-feature-settings: "nalt" 4;
> +}

Are the circled and/or black-circled glyphs in these fonts accessible directly as characters in the Enclosed Alphanumerics or Dingbats blocks, or are the 'nalt' glyphs different from those? If they're the same glyphs as those Unicode characters, it'd be nice to code them that way in the reference.

::: layout/reftests/font-features/font-variant-features.js
@@ +116,5 @@
> +  { prop: "font-variant-alternates", value: "styleset(mixed-case)", features: {"ss02": 0, "ss03": 1, "ss04": 1, "ss05": 0, "ss06": 0, "ss07": 1, "ss08": 0} },
> +  { prop: "font-variant-alternates", value: "character-variant(ok-1)", features: {"cv78": 2, "cv79": 0, "cv77": 0} },
> +  { prop: "font-variant-alternates", value: "character-variant(ok-1, ok-3)", features: {"cv78": 2, "cv79": 0, "cv77": 0, "cv23": 1, "cv22": 0, "cv24": 0} },
> +  { prop: "font-variant-alternates", value: "annotation(circled)", features: {"nalt": 1} },
> +  { prop: "font-variant-alternates", value: "character-variant(multi-def)", features: {"cv03": 0, "cv04": 1} },

Did you plan to test whether annotation(multi-def) also works?

@@ +124,5 @@
> +  { prop: "font-variant-alternates", value: "styleset(moxie2)", features: {"ss01": 0, "ss14": 1} },
> +  { prop: "font-variant-alternates", value: "styleset(moxie3)", features: {"ss01": 0, "ss14": 1} },
> +  { prop: "font-variant-alternates", value: "styleset(out-of-bounds1)", features: {"ss00": 0, "ss01": 0} },
> +  { prop: "font-variant-alternates", value: "styleset(out-of-bounds2)", features: {"ss21": 0, "ss01": 0, "ss00": 0} },
> +  { prop: "font-variant-alternates", value: "styleset(bogus)", features: {"ss02": 0, "ss03": 0, "ss04": 0, "ss05": 0, "ss06": 0, "ss07": 0, "ss08": 0} },

I don't see any reference to the "bogus-[also]-too-high" names in the stylesheet; are those redundant?

::: layout/style/nsCSSParser.cpp
@@ +8122,5 @@
>  // both simple enumerated values and functional values with
>  // parameter lists with one or more idents (these are resolved
>  // later based on values defined in @font-feature-value rules).
>  //
> +// font-variant-alternates: swash(flowing), historical-forms, styleset(alt-g, alt-m);

Move this chunk to the nsCSSParser.cpp patch, please.
Attachment #656350 - Flags: review?(jfkthame) → review+
(In reply to David Baron [:dbaron] from comment #54)
> Comment on attachment 656348 [details] [diff] [review]
> part 2c v2 - parsing of @font-feature-values rule
> 
> ParseFontFeatureValuesRule:
> 
> 
> +    if (!ParseFontFeatureValuesDeclaration(valuesRule)) {
> +      REPORT_UNEXPECTED(PEDeclSkipped);
> +      OUTPUT_ERROR();
> +      if (!SkipDeclaration(true)) {
> +        break;
> +      }
> +    }
> 
> You want to use SkipAtRule instead of SkipDeclaration here, so that
> you match the general grammar.
> 
> (I also think you might want to not use the word Declaration in that
> function name -- and probably also in the terminology in the spec. 
> Not sure I have a better idea, though.)

The wording in the spec uses "declaration" here, i.e.
<font-feature-values-declaration>, so, yes, this is a spec issue.  

  http://dev.w3.org/csswg/css3-fonts/#font-feature-values

I don't think SkipAtRule is right here, because a single feature
values decl is ignored when a syntax error occurs, it doesn't result
in other feature values decl's getting dropped, just as for a
declaration within a style rule.

I'm perfectly happy with calling this something else (defintion?) but
I think the syntax error handling is appropriate here.
(In reply to John Daggett (:jtd) from comment #61)
> I don't think SkipAtRule is right here, because a single feature
> values decl is ignored when a syntax error occurs, it doesn't result
> in other feature values decl's getting dropped, just as for a
> declaration within a style rule.

So the reason you want to use SkipAtRule is that syntactically it's an @-rule, so it should follow the rules for parsing @-rules in the CSS general grammar.  That grammar says that an @-rule ends at either {} or ;.  While your particular @-rule normally ends with ;, the general grammar says that you need to end it if you find a {}.  In other words, based on the general grammar, the following:

  @font-feature-values Mercury Serif {
    @styleset foo {}    /* ignored */
    @styleset alt-g 1;  /* honored */
  }

should be treated as the indentation shows, rather than as

  @font-feature-values Mercury Serif {
    @styleset foo {} @styleset alt-g 1; /* ignored */
  }

which is what your current code would do.

I'd rather the spec rename the concept to something with "rule" in the name rather than "declaration" in the name, for similar reasons.



Also, it would be great to see this land soon.
Comment on attachment 656342 [details] [diff] [review]
part 1d v2 - font-variant feature reftests

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

Basically looks good, but marking r- for now to encourage posting a new patch, as I'd like to see some cleanup before this lands; see comment 59.
Attachment #656342 - Flags: review?(jfkthame) → review-
Depends on: 833169
Blocks: 835191
Depends on: 842211
refreshed patch, carrying forward r=dbaron
Attachment #656338 - Attachment is obsolete: true
Attachment #742169 - Flags: review+
refreshed patch, carrying forward r=dbaron
Attachment #656340 - Attachment is obsolete: true
Attachment #742170 - Flags: review+
refreshed, carrying forward r=jfkthame
Attachment #656341 - Attachment is obsolete: true
Attachment #742172 - Flags: review+
Updated based on review comments
Attachment #656342 - Attachment is obsolete: true
refreshed, carrying forward r=jfkthame
Attachment #742174 - Flags: review+
Attachment #656343 - Attachment is obsolete: true
Attachment #742173 - Flags: review?(jfkthame)
Updated based on review comments, carrying forward r=jfkthame
Attachment #656345 - Attachment is obsolete: true
Attachment #742176 - Flags: review+
Updated based on review comments, carrying forward r=dbaron
Attachment #656346 - Attachment is obsolete: true
Attachment #742179 - Flags: review+
Revised to match latest syntax and revised based on review comments.

> And I think now that you're adding a third thing that needs to be
> cleared, you should consolidate those three calls into a method called
> nsPresContext::AtRulesChanged rather than listing all 3 in 2 places.
> (I probably should have done that sooner.)

Will do this in a separate, follow-on patch.
Attachment #656348 - Attachment is obsolete: true
Attachment #742183 - Flags: review?(dbaron)
Switch to testharness.js format and revised to match latest syntax.  Given this, I'm re-requesting review of this piece even though the previous patch was r+'d.
Attachment #656349 - Attachment is obsolete: true
Attachment #742185 - Flags: review?(dbaron)
Revised to use unprefixed property names.  Carrying forward r=jfkthame
Attachment #656350 - Attachment is obsolete: true
Attachment #742186 - Flags: review+
Refreshed.  Carrying forward r=dbaron
Attachment #656351 - Attachment is obsolete: true
Attachment #742187 - Flags: review+
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #36)
> Comment on attachment 630864 [details] [diff] [review]
> part 1b - parsing of font-variant subproperties
> 
> You should also make 'font-variant' a shorthand (nsCSSPropList.h,
> nsCSSProps.cpp subprop table, parsing code, etc., though in
> property_database.js terms it should be SHORTHAND_AND_LONGHAND since it
> needs to exhibit longhand behavior for backwards-compatibility).  But
> you probably don't want to make it support parsing the new stuff yet,
> since the new stuff is prefixed.  (But when you eventually do, you'll
> need to be careful with the 'font' shorthand.)

This is tricky to do given that the subproperties are enabled/disabled based on a pref, the macros used with nsCSSPropList.h mean that it's tricky to switch between a property being a shorthand or longhand based on a pref.  Also, if the font-variant is a shorthand property it needs to store it's values in a subproperty; right now, font-variant-caps isn't equivalent to font-variant, it only enables the use of the small-caps feature but doesn't handle fallback at this point.

I would suggest we hold off on this for a few release cycles.  Once the fallback behavior is implemented and we're confident of it's functionality then we can make the switch.
Two remaining pieces needed:

- followup patch to reorganize how @rules are updated, including @font-feature-values rules
- gfx implementation of font-synthesis values
- hook up kerning property to logic used to skip word cache
s/Two/Three/
Does the new layout.css.font-features.enabled pref also enable support for unprefixed font-feature-settings?

If we're going to land the font feature (variant) stuff unprefixed but behind a pref, then I think it'd be much less confusing if that pref enabled unprefixed use of -all- the font feature stuff, including the low-level font-feature-settings property.

(Prefixed -moz-font-feature-settings would still be accepted for a while, of course, so as not to break existing content that uses it.)
Comment on attachment 742173 [details] [diff] [review]
part 1d v3 - font-variant feature reftests

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

::: layout/reftests/font-features/font-variant-features.js
@@ +1,5 @@
> +
> +// data associated with gsubtest test font for testing font features
> +
> +// prefix
> +gPrefix = "-moz-";

I thought the prefix was being dropped in favor of a the pref setting? In that case, doesn't it need to be removed from here?
Comment on attachment 742167 [details] [diff] [review]
part 0 - create a pref for font feature support

>+// Is support for CSS3 Fonts features enabled?
>+// (includes font-variant, font-kerning, font-synthesis
>+// and the @font-feature-values rule)

This "includes" comment seems like it might need a little updating (e.g., is it font-variant-* but *not* font-variant), and perhaps also to jfkthame's comment about an unprefixed font-feature-settings (which should be doable pref-controlled with a single entry in nsCSSPropAliasList, as long as you're ok with the canonical form being the prefixed one, which is probably ok as long as it's preffed off but should be switch the moment we pref it on for release) also being controlled by this pref.

r=dbaron
Attachment #742167 - Flags: review?(dbaron) → review+
Comment on attachment 742171 [details] [diff] [review]
part 1b2 - use feature pref for font-variant properties

r=dbaron.

(Also, should font-language-override go along with font-feature-settings in having an unprefixed alias immediately?)
Attachment #742171 - Flags: review?(dbaron) → review+
(In reply to Jonathan Kew (:jfkthame) from comment #81)
> Comment on attachment 742173 [details] [diff] [review]
> part 1d v3 - font-variant feature reftests
> 
> Review of attachment 742173 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/reftests/font-features/font-variant-features.js
> @@ +1,5 @@
> > +
> > +// data associated with gsubtest test font for testing font features
> > +
> > +// prefix
> > +gPrefix = "-moz-";
> 
> I thought the prefix was being dropped in favor of a the pref setting? In
> that case, doesn't it need to be removed from here?

Oops, missed the patch for that.
Switch tests to use unprefixed property names
Attachment #742863 - Flags: review?(jfkthame)
Adds property alias for font-feature-settings and font-language-override, pointing to the prefixed version if the pref is enabled.  Also, allow font-feature-settings within @font-face rules.  That part is slightly hacky, open to suggestions of a better way.
Attachment #742950 - Flags: review?(dbaron)
Comment on attachment 742172 [details] [diff] [review]
part 1c v2 - gfx support for font-variant properties (refreshed)

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

Now that harfbuzz controls both GPOS and legacy <kern> table kerning via the standard 'kern' feature setting, I think you can simplify this - there shouldn't be any need for the extra 'kerning' flag in the gfxFontStyle, etc., as the generic feature support ought to cover it.
Comment on attachment 742863 [details] [diff] [review]
part 1f - switched to unprefixed property names

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

Please fold this into the main patch that adds the tests, so that we don't have patches-upon-patches here - it's getting hard to keep track of what's new, what's existing stuff, what's being modified, etc.

At least in the tests where you're using the new properties (and therefore require the layout.css.font-features.enabled pref), I think it'd be preferable to switch to unprefixed (-moz-)font-feature-settings as well, so that we don't have such a mixture of prefixed and unprefixed in the same files.
Comment on attachment 742183 [details] [diff] [review]
part 2c v3 - parsing of @font-feature-values rule

Previous review of this patch in comment 54.

>* * *
>changes to support new @font-feature-values rule syntax
>* * *
>changes

drop these qfold remnants from the commit message.

I'm curious about the dropped #include in nsDOMClassInfo.cpp (relative
to the previous patch).

>+  const unsigned short      MOZ_FONT_FEATURE_VALUES_RULE   = 14;

Probably just drop the MOZ_ here, actually.  This constant is in
http://wiki.csswg.org/spec/cssom-constants and I think it's unlikely
to change.



In ParseFontFeatureValuesRule, I was serious last time about needing
SkipAtRule, for two reasons.  First, in a case like:

  @font-feature-values Jupiter Sans {
    @swash
  }

you need that closing brace to terminate the @font-feature-values rule.
(So you should pass aInsideBlock=true.)

Second, in a case like:

  @font-feature-values Jupiter Sans {
    @foo bar; /* or your "@ornaments ;" example in the spec */
    @swash {
      delicate: 1;
      flowing: 2;
    }
  }

the @swash should be handled.

You can't just use "SkipUntil('}')" for both of these reasons; you should
replace the inner SkipUntil call (but not the outer one) with SkipAtRule,
and there's really no need to check its return value (since the next
iteration of the loop will call and check GetToken).

(I also think the spec could probably be a bit clearer here as well.)



ParseFontFeatureValueSet:

Your early returns prior to reading the open brace shouldn't have
SkipRuleSet(false) calls; that should be handled by the SkipAtRule
call in the caller.  (The UngetToken that you added since the last
patch is correct, though.)

Any early returns *after* reading the open brace should, however, have a
SkipUntil('}') call (like you have now in the caller... except that is
incorrect for the cases where this function returns prior to reading the
open brace.  It's probably worth splitting the function into two
functions (the second reads what's inside the {}) just so that you can
have this error handling in a single place, though.

I think both places that you have:
>+    if (!GetToken(true)) {
>+      REPORT_UNEXPECTED_EOF(PEFFVUnexpectedEOF);
>+      return false;
>+    }
you should break here (and thus return true) but still report the
unexpected EOF, because of the CSS 2.1 general rules on closing open
constructs at end of file.


>+    // ';' or '}' to end definition
>+    if (!mToken.IsSymbol(';') && !mToken.IsSymbol('}')) {
>+      REPORT_UNEXPECTED_EOF(PEFFVValueDefinitionEndEOF);

The error here isn't an EOF; it should be an UNEXPECTED_TOKEN error
report.  (The l10n string should be renamed too, but it's the only use,
and the string looks right for UNEXPECTED_TOKEN other than its name...
except that it needs a "but found '%1$S'" like all the
UNEXPECTED_TOKEN errors need.

And, actually, you need that on a bunch of your other error strings
as well (all the ones used with UNEXPECTED_TOKEN)!

And you have some unused errors in css.properties too:
>+PEFFVDeclSkipped=Skipped to next feature value declaration.

>+PEFFVUnexpectedDeclEnd=Expected end of value list or declaration but found '%1$S'.

>+PEFFVNoAt=Expected '@' but found '%1$S'.



And also this:
>+    if (!NonMozillaVendorIdentifier(mToken.mIdent)) {
>+      REPORT_UNEXPECTED_P(PEFFVUnknownFontVariantPropValue, mToken.mIdent);
>+      OUTPUT_ERROR();
>+    }

should just use REPORT_UNEXPECTED_TOKEN.




>+  if (!values.IsEmpty()) {
>+    aFeatureValuesRule->AddValueList(whichVariant, values);
>+  }

Checking IsEmpty() feels wrong here; it produces a different OM.  I
think you shouldn't.



In nsCSSRuleProcessor.h and .cpp, you should probably be consistent
about whether @font-feature-values rules are listed before or after
@page rules.  (I think you've mostly been putting @font-feature-values
before @page.)

>+NS_IMETHODIMP
>+nsCSSFontFeatureValuesRule::GetCssText(nsAString& aCssText)
>+{
>+  List(stdout, 3);
>+  FontFeatureValuesRuleToString(mFamilyList, mFeatureValues, aCssText);
>+  return NS_OK;
>+}

remove that List call.



r=dbaron with those things fixed
Attachment #742183 - Flags: review?(dbaron) → review+
Comment on attachment 742185 [details] [diff] [review]
part 2d v3 - parsing tests of @font-feature-values rule

I think most of the comments in comment 55 still stand; other than that I didn't look too closely the second time around.
Attachment #742185 - Flags: review?(dbaron) → review+
Comment on attachment 742950 [details] [diff] [review]
part 2g - alias font-feature-settings to prefixed prop

> nsCSSFontDesc
> nsCSSProps::LookupFontDesc(const nsAString& aFontDesc)
> {
>   NS_ABORT_IF_FALSE(gFontDescTable, "no lookup table, needs addref");
>-  return nsCSSFontDesc(gFontDescTable->Lookup(aFontDesc));
>+  nsCSSFontDesc which = nsCSSFontDesc(gFontDescTable->Lookup(aFontDesc));
>+
>+  // check for unprefixed font-feature-settings/font-language-override
>+  if (which == eCSSFontDesc_UNKNOWN &&
>+      mozilla::Preferences::GetBool("layout.css.font-features.enabled")) {
>+    nsAutoString prefixedProp;
>+    prefixedProp.AppendLiteral("-moz-");
>+    prefixedProp.Append(aFontDesc);
>+    which = nsCSSFontDesc(gFontDescTable->Lookup(prefixedProp));
>+  }
>+  return which;
> }

Please add a comment to nsCSSFontDescList.h describing this behavior.


And please add a comment above the pref in modules/libpref/src/init/all.js that says that when we enable the pref by default, we should immediately switch these aliases to be the other way around so that the canonical form is the unprefixed property.

r=dbaron with that
Attachment #742950 - Flags: review?(dbaron) → review+
As requested, removed prefix property usage for both the font-kerning property and font-feature-settings.
Attachment #742174 - Attachment is obsolete: true
Attachment #746201 - Flags: review?(jfkthame)
Folded the font-kerning changes into p1e.  These are the changes needed for other font-variant proeprties, tests that already have been reviewed.
Attachment #742863 - Attachment is obsolete: true
Attachment #742863 - Flags: review?(jfkthame)
Attachment #746202 - Flags: review?(jfkthame)
As per comment 87, trim the 'kerning' field from gfxFontStyle.  Use the mKerningSet and mKerningEnabled flags to detect the case when kerning needs to be explicitly disabled (i.e. font-kerning: none or font-feature-settings: "kern" off).
Attachment #746203 - Flags: review?(jfkthame)
Fold the part 1f into part 1d (sorry for the bugzilla churn!).
Attachment #742173 - Attachment is obsolete: true
Attachment #746202 - Attachment is obsolete: true
Attachment #742173 - Flags: review?(jfkthame)
Attachment #746202 - Flags: review?(jfkthame)
Attachment #746208 - Flags: review?(jfkthame)
Updated based on part 1d changes.
Attachment #746201 - Attachment is obsolete: true
Attachment #746201 - Flags: review?(jfkthame)
Attachment #746210 - Flags: review?(jfkthame)
Carrying forward r=dbaron (added a comment about serializationSame meaning).

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #90)
> Comment on attachment 742185 [details] [diff] [review]
> part 2d v3 - parsing tests of @font-feature-values rule
> 
> I think most of the comments in comment 55 still stand; other than that I
> didn't look too closely the second time around.

Actually, I think I covered most of comment 55; the test is now in testharness.js form so the gStandalone variable was removed and I went through and hopefully eliminated each of the problems mentioned in your review.  Let me know if you still see remaining issues with this.
Attachment #742185 - Attachment is obsolete: true
Attachment #746218 - Flags: review+
Comment on attachment 746203 [details] [diff] [review]
part 2h - trim out kerning from gfxFontStyle

Looks fine. (I assume you're intending to fold a bunch of these patches together for landing?)
Attachment #746203 - Flags: review?(jfkthame) → review+
Comment on attachment 746208 [details] [diff] [review]
part 1d v3a - font-variant feature reftests

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

I didn't try to check every testcase manually(!), but it looks ok and I assume they actually pass. A couple nits could be tidied up...

::: layout/reftests/font-features/font-features-order-5.html
@@ +4,5 @@
> +<style type="text/css">
> +@font-face {
> +  font-family: libertine;
> +  src: url(../fonts/LinLibertine_Re-4.7.5.woff) format("woff");
> +  -moz-font-feature-settings: "liga" on, "hlig" on;

We can lose the prefix here.

::: layout/reftests/font-features/font-variant-caps-ref.html
@@ +7,5 @@
> +<script type="text/javascript" src="font-variant-features.js"></script>
> +<link rel="stylesheet" href="font-variant-features.css" type="text/css"/>
> +
> +<style type="text/css">
> +</style>

I'm sure it's just part of your boilerplate, but it'd be nice to trim this empty <style> out of the (several) files that have it.

::: layout/reftests/font-features/font-variant-features.js
@@ +112,5 @@
> +  { prop: "font-variant-position", value: "super sub", features: {"subs": 0, "sups": 0}, invalid: true },
> +];
> +
> +// note: the code below requires an array "gFeatures" from :
> +//   layout/reftest/fonts/gsubtest/gsubtest-features.js

That's /layout/reftests/... (plural) in the path. Same comment in the alternates file, too.
Attachment #746208 - Flags: review?(jfkthame) → review+
Attachment #746210 - Flags: review?(jfkthame) → review+
Updates to @font-feature-values rule parsing based on review comments (see comment 89).

I didn't separate out ParseFontFeatureValueSet into two functions, it turned out I didn't need to deal with an early return that requires SkipUntil('}').

I reworked the testharness.js test to tweak the name of 'serializationEmpty' to 'serializationNoValueDefn', that better reflects what's being tested.  The 'valuesText' function now strips out these empty blocks containing no value definitions (e.g. @swash { }).

There's also one small tweak to nsCSSFontFeatureValuesRule::List to get this code to compile on Windows.

With this patch, I'd like to land the set of patches here.  There are two outstanding changes that remain:

  - rework the updating of @rules into a single update function
  - gfx implementation of font-synthesis
  - fallback handling for sub/superscripts and small-caps

I'd like to tackle these in follow-on bugs.
Attachment #748675 - Flags: review?(dbaron)
Comment on attachment 748675 [details] [diff] [review]
part 2i - updates to @font-feature-values rule parsing based on review comments

>     if (!ParseFontFeatureValueSet(valuesRule)) {
>-      if (!SkipUntil('}')) {
>+      if (!SkipAtRule(false)) {
>         break;
>       }
>     }
>   }
>   if (!ExpectSymbol('}', true)) {
>     REPORT_UNEXPECTED_TOKEN(PEFFVUnexpectedBlockEnd);
>-    SkipUntil('}');
>+    SkipAtRule(false);
>     return false;
>   }

The first change here is correct.  The second one is not.  You should
leave the second one as it was.

(That's what I meant by "the inner SkipUntil call (but not the outer 
one)" in comment 89.)


These messages still need a param for the token:
>+PEFFVBlockStart=Expected opening { of @font-feature-values rule.
>+PEFFVValueSetStart=Expected opening { of feature value set.
>+PEFFVNoFamily=Expected font family list for @font-feature-values rule.

>+PEFFVTooManyValues=Too many values for feature type.
>+PEFFVGenericInFamilyList=Family list cannot contain generic font family name.

(Though perhaps FFVTooManyValues should be REPORT_UNEXPECTED_P instead
of REPORT_UNEXPECTED_TOKEN, or maybe just REPORT_UNEXPECTED with no
param?  Maybe the last one as well?)


And please merge this into patch 2c.

r=dbaron with that
Attachment #748675 - Flags: review?(dbaron) → review+
Blocks: 871451
Blocks: 871453
Keywords: dev-doc-needed
Depends on: 873222
Changes to test_interfaces.html require review from a DOM peer, as is clearly noted in the file. Backed out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f2fa4839e98
https://hg.mozilla.org/integration/mozilla-inbound/rev/90b73c3533fd
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The change that needs DOM-peer review:

--- a/dom/tests/mochitest/general/test_interfaces.html
+++ b/dom/tests/mochitest/general/test_interfaces.html
@@ -487,16 +487,17 @@ var interfaceNamesInGlobalScope =
     "BeforeUnloadEvent",
     "NSRGBAColor",
     "MozBrowserFrame",
     "SVGPreserveAspectRatio",
     "HTMLMenuElement",
     "CloseEvent",
     "IDBCursorWithValue",
     "CSSFontFaceRule",
+    "CSSFontFeatureValuesRule",
     "XMLHttpRequestEventTarget",
     "CompositionEvent",
     "HTMLOutputElement",
     "HTMLFormElement",
     "SVGLength",
     "SVGFilterElement",
     "HTMLScriptElement",
     "SVGPathSegCurvetoCubicAbs",

Relevant spec:
http://dev.w3.org/csswg/css-fonts/#font-feature-values
Attachment #751528 - Flags: review?(khuey)
Comment on attachment 751528 [details] [diff] [review]
patch, reland font-variant subproperties with DOM-peer review

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

r=me on the test_interfaces.html addition
Attachment #751528 - Flags: review?(khuey) → review+
https://hg.mozilla.org/mozilla-central/rev/0ca2dcc27f22
https://hg.mozilla.org/mozilla-central/rev/25a1e66c0f1d
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Depends on: 886691
Depends on: 892929
Whiteboard: [evang-wanted] → [evang-wanted][DocArea=CSS]
John, any timeframe on when this will be enabled by default? Is there a bug for this somewhere?
Flags: needinfo?(jdaggett)
(In reply to Florian Bender from comment #108)
> John, any timeframe on when this will be enabled by default? Is there a bug
> for this somewhere?

It looks like: 
 * we should enable this and probably other parts of css3-fonts since the spec is now in CR
 * there isn't already a bug on file, but one or more should be filed, blocking bug css3-fonts
Flags: needinfo?(jdaggett)
Blocks: 975744
No longer blocks: 835191
Depends on: 1144023
Depends on: 1261445
You need to log in before you can comment on or make changes to this bug.