718539 - (font-feature-settings) update syntax used for -moz-font-feature-settings

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect, P2)

Description

[John Daggett (:jtd)]

Both Webkit and IE10 now include some level of support for font-feature-settings using the syntax in the current CSS3 Fonts spec:

  -ms-font-feature-settings: "liga" 1, "dlig" 0;

The syntax in Gecko is based on an older version of the spec:

  -moz-font-feature-settings: "liga=1, dlig=0";

I think we should rev to use the new syntax.  I think we can support the old syntax along with the new syntax, but only support the new syntax when we drop the prefix.

Will work:
  -moz-font-feature-settings: "liga=1, dlig=0";
  -moz-font-feature-settings: "liga" 1, "dlig" 0;
  font-feature-settings: "liga" 1, "dlig" 0;

Won't work:
  font-feature-settings: "liga=1, dlig=0";

As part of this, we should move the parsing of font-feature-settings out of gfx into layout/style.

Comment 1

[John Daggett (:jtd)]

As part of the the changes required to implement the new syntax, we need to fix the other small problems with the current implementation.

Comment 2

[John Daggett (:jtd)]

Attachment: patch, part 1 - add array of gfxFontFeature's to nsFont

Comment 3

[John Daggett (:jtd)]

Attachment: patch, part 2 - parse new syntax of font-feature-settings property

Adds a new parsing function for font-feature-settings and some custom serialization routines for the pair-value lists used.  I wasn't quite sure where to stick shared serialization code, for now it's in nsStyleUtil.

Comment 4

[John Daggett (:jtd)]

Attachment: patch, part 3 - update layout/style tests

Comment 5

[John Daggett (:jtd)]

Attachment: patch, part 4 - initialize gfxFontStyle using nsFont values

Comment 6

[John Daggett (:jtd)]

Attachment: patch, part 5 - merge per-font and style features in shapers

Merges per-font and style-rule based features by inserting them into a hashtable using the order specified in the CSS3 Fonts spec (secton 7).

Comment 7

[John Daggett (:jtd)]

Attachment: patch, part 6 - clean out featureSettings string from nsFont

Trim out featureSettings along with old parsing code.

Comment 8

[John Daggett (:jtd)]

Attachment: patch, part 7 - update reftests to use new syntax

Comment 9

[Jonathan Kew [:jfkthame]]

Comment on attachment 612794 [details] [diff] [review]
patch, part 1 - add array of gfxFontFeature's to nsFont

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

::: gfx/thebes/gfxFontFeatures.h
@@ +1,3 @@
> +/* -*- Mode: C++; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 4 -*-
> + * ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1

License boilerplate in new files should be MPL2 these days. (http://www.mozilla.org/MPL/headers/)

@@ +38,5 @@
> +
> +#ifndef GFX_FONT_FEATURES_H
> +#define GFX_FONT_FEATURES_H
> +
> +#include "prtypes.h"

Maybe we should take this opportunity to start using mfbt's "StandardInteger.h" instead, and define the feature type using uint32_t. I wonder if that would break anything?

Comment 10

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #9)
> Comment on attachment 612794 [details] [diff] [review]
> @@ +38,5 @@
> > +
> > +#ifndef GFX_FONT_FEATURES_H
> > +#define GFX_FONT_FEATURES_H
> > +
> > +#include "prtypes.h"
> 
> Maybe we should take this opportunity to start using mfbt's
> "StandardInteger.h" instead, and define the feature type using uint32_t. I
> wonder if that would break anything?

I don't have any problem with using different type names if that's a
project-wide decision.  But I'd prefer not to mix that change in with
other changes since it confuses changes for the bug with the name
swizzling. Type name changes are a simple delta when done separately.

Comment 11

[Jonathan Kew [:jfkthame]]

Fine; I'm not sure what the plans are w.r.t. a general migration to those std types (though in general it's something I'd like to see happen).

Comment 12

[Jonathan Kew [:jfkthame]]

Comment on attachment 612800 [details] [diff] [review]
patch, part 5 - merge per-font and style features in shapers

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

Looks fine; minor formatting nits below that would apply to both files. However, seeing how similar these are, I think it should be feasible to have a sinlge MergeFontFeatures helper (maybe in gfxFontUtils?) that is used by both the HarfBuzz and Graphite shapers, with each shaper passing in a suitable AddFeature callback and userData pointer. (It'd be fine to disable both 'liga' and 'clig' for disableLigatures in the Graphite case as well as HarfBuzz, I think. It's up to Graphite font developers who want interoperability between Graphite and OpenType features to use tags in appropriate ways, even though in principle tags can be used to mean whatever the designer wishes.)

::: gfx/thebes/gfxGraphiteShaper.cpp
@@ +153,5 @@
>      }
>      return grLangTag;
>  }
>  
> +struct GrFontFeature {

I think this needs to be called GrFontFeatures, at least, or even something like GrFontFeatureSet or Collection.

@@ +159,5 @@
> +    gr_feature_val *mFeatures;
> +};
> +
> +static PLDHashOperator
> +AddFeature(const PRUint32& aTag, PRUint32& aValue, void *aUserArg)

aValue should be const too, no?

@@ +161,5 @@
> +
> +static PLDHashOperator
> +AddFeature(const PRUint32& aTag, PRUint32& aValue, void *aUserArg)
> +{
> +    GrFontFeature *f = (GrFontFeature*) (aUserArg);

I know we have code that does this in some places, but in general, I think we should prefer a C++ static_cast<> here.

@@ +184,5 @@
> +    // bail immediately if nothing to do
> +    if (aStyleRuleFeatures.IsEmpty() &&
> +        aFontFeatures.IsEmpty() &&
> +        !aDisableLigatures)
> +    {

Move the brace up to the previous line; I'd be inclined to put the two IsEmpty()s on the same line, too, so we don't use up vertical space quite so fast.

@@ +191,5 @@
> +
> +    // hash table used to resolve duplicates
> +    nsDataHashtable<nsUint32HashKey,PRUint32>  featureSettings;
> +
> +    featureSettings.Init(20);

Lose blank line. Don't give it an explicit size; nsBaseHashtable defaults to 16, which should be more than enough in almost every case.

@@ +202,5 @@
> +
> +    // add feature values from font
> +    PRUint32 i, count;
> +
> +    count = aFontFeatures.Length();

And here.

@@ +267,2 @@
>  
> +    numFeat = MergeFontFeatures(style->featureSettings, entry->mFeatureSettings,

At least lose the blank line here; personally, I'd also go for
    PRUint32 numFeat = MergeFontFeatures(...)
rather than having the declaration separate.

Comment 13

[Jonathan Kew [:jfkthame]]

Comment on attachment 612801 [details] [diff] [review]
patch, part 6 - clean out featureSettings string from nsFont

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

This looks straightforward; however, it throws away the old ParseFontFeatureSettings, and AFAICS the current patch series does not support both old and new syntaxes for -moz-font-feature-settings, as was proposed in comment #0. I'm not comfortable with simply dropping the existing syntax, with no transition period. Which probably means revisiting earlier patches in the series.

Comment 14

[Jonathan Kew [:jfkthame]]

Comment on attachment 612795 [details] [diff] [review]
patch, part 2 - parse new syntax of font-feature-settings property

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

Comment #0 said we should support both old and new syntax for the prefixed property; it looks to me like this will *only* support the new syntax.

Comment 15

[Jonathan Kew [:jfkthame]]

Comment on attachment 612796 [details] [diff] [review]
patch, part 3 - update layout/style tests

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

::: layout/style/test/property_database.js
@@ +2049,5 @@
> +            '"liga"        ,"smcp" 0         , "blah"'
> +        ],
> +		invalid_values: [
> +            'liga', 'liga 1', 'liga normal', '"liga" normal', 'normal liga', 
> +            'normal "liga"', 'normal, "liga"', '"liga=1"', "'foobar' on",

Old syntax such as '"liga=1"' should continue to be supported.

Comment 16

[John Daggett (:jtd)]

Sorry, I should have noted the change in old/new format support.  I talked with dbaron during the layout work week and he felt that it was better to drop support for the old format, rather than try to support both formats.  Authors can still support both the old and the new formats:

  -moz-font-feature-settings: "liga";  /* new format, won't parse in existing code */
  -moz-font-feature-settings: "liga=1"; /* old format, won't parse in new code */

I think the key here is that the data collected as part of the prefix'ing survey indicated that the font-feature-settings property isn't used that much in existing code so there really isn't a strong reason to carry forward support for both the old/new syntax.

Comment 17

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #16)
> Sorry, I should have noted the change in old/new format support.  I talked
> with dbaron during the layout work week and he felt that it was better to
> drop support for the old format, rather than try to support both formats. 
> Authors can still support both the old and the new formats:
> 
>   -moz-font-feature-settings: "liga";  /* new format, won't parse in
> existing code */
>   -moz-font-feature-settings: "liga=1"; /* old format, won't parse in new
> code */

Hmm... OK, I guess that may be workable. Though in that case I think our tests need to cover this usage pattern.

Note that it's not true that the new format "won't parse in existing code"; it *will* parse at the CSS level, because syntactically the value is an arbitrary string, but will not contain any valid feature settings, and therefore is equivalent to resetting the property to normal.

Hence, I think

  -moz-font-feature-settings: "liga"; /* for new releases */
  -moz-font-feature-settings: "liga=1"; /* for older releases, ignored by new */

should work (though I haven't tested it), but

  -moz-font-feature-settings: "liga=1"; /* for older releases */
  -moz-font-feature-settings: "liga"; /* for new releases - gotcha! NOT ignored by old */

won't actually work as intended in old versions. Seems a bit tricky...

Comment 18

[Jonathan Kew [:jfkthame]]

Comment on attachment 612795 [details] [diff] [review]
patch, part 2 - parse new syntax of font-feature-settings property

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

::: layout/style/nsCSSParser.cpp
@@ +8369,5 @@
> +        cur->mYValue.SetIntValue(0, eCSSUnit_Integer);
> +      } else {
> +        // something other than value/on/off, set default value
> +        cur->mYValue.SetIntValue(1, eCSSUnit_Integer);
> +        UngetToken();

Shouldn't this be a parse error?

Comment 19

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #17)
> Hence, I think
> 
>   -moz-font-feature-settings: "liga"; /* for new releases */
>   -moz-font-feature-settings: "liga=1"; /* for older releases, ignored by
> new */
> 
> should work (though I haven't tested it)

Yes, "liga" will parse, "liga=1" will generate a syntax error.  For older versions, both will parse but "liga=1" will override as per normal CSS rule handling.

> but
> 
>   -moz-font-feature-settings: "liga=1"; /* for older releases */
>   -moz-font-feature-settings: "liga"; /* for new releases - gotcha! NOT
> ignored by old */
> 
> won't actually work as intended in old versions. Seems a bit tricky...

Yes, that's right.  That's the problem with doing the parsing in gfx code, syntax errors aren't handled correctly.  They should result in the rule being tossed but instead they are just ignored.

Example (using the old syntax):

  -moz-font-feature-settings: "liga=1";
  -moz-font-feature-settings: "The rain in Spain falls mainly on the plains.";

This *should* enable ligatures but it won't because the second rule will override the first, even though it's incorrect syntax.  When parsed by gfx, the code simply tosses the settings.

Comment 20

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #18)

> ::: layout/style/nsCSSParser.cpp
> @@ +8369,5 @@
> > +        cur->mYValue.SetIntValue(0, eCSSUnit_Integer);
> > +      } else {
> > +        // something other than value/on/off, set default value
> > +        cur->mYValue.SetIntValue(1, eCSSUnit_Integer);
> > +        UngetToken();
> 
> Shouldn't this be a parse error?

Nope.  The syntax doesn't require values and that's how these are handled.  The code below there either needs to find an end of property or a comma, anything else will cause a parse error.

  /* valid */
  -moz-font-feature-settings: "liga";
  -moz-font-feature-settings: "liga", "dlig";
  -moz-font-feature-settings: "liga" 999;

  /* invalid */
  -moz-font-feature-settings: "liga" "dlig"; /* no comma */
  -moz-font-feature-settings: "liga", dlig;  /* tags need to be strings */

Note: Webkit accepts the second invalid property rule which it shouldn't.

Comment 21

[John Daggett (:jtd)]

review ping, parts 5-7 (already pinged dbaron about the others)

Comment 22

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #19)
> Example (using the old syntax):
> 
>   -moz-font-feature-settings: "liga=1";
>   -moz-font-feature-settings: "The rain in Spain falls mainly on the
> plains.";
> 
> This *should* enable ligatures but it won't because the second rule will
> override the first, even though it's incorrect syntax.  When parsed by gfx,
> the code simply tosses the settings.

FWIW (though it's becoming irrelevant with the syntax change), I don't agree that this was wrong. The older definition of font-feature-settings was meant to be completely technology-neutral, with no assumptions in CSS about how features are identified, so the value was simply a string; it was the responsibility of the font-rendering technology to take that string and interpret it. A string containing no recognizable (by the back-end) features was therefore *not* incorrect at the CSS level; it was valid CSS even though it didn't have any effect on rendering.

Comment 23

[Jonathan Kew [:jfkthame]]

AIUI, in the new syntax,

  -moz-font-feature-settings: "liga" off;
  -moz-font-feature-settings: "foobar" on;

will disable ligatures, because "foobar" is invalid (right?), whereas

  -moz-font-feature-settings: "liga" off;
  -moz-font-feature-settings: "foo" on;

will NOT, because the second rule overrides the first. I think this difference is rather obscure/counter-intuitive, and makes the CSS syntax too dependent on the form of OpenType tags - while not *quite* enforcing it explicitly. (How are strings with length < 4 handled?)

(I notice that the current ED text says, "Although specifically defined for OpenType feature tags, feature tags for other modern font formats that support font features may be added in the future." This will present a problem if such an "other modern font format" expects tags that are something other than 4 characters long.)

Comment 24

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #23)
> AIUI, in the new syntax,
> 
>   -moz-font-feature-settings: "liga" off;
>   -moz-font-feature-settings: "foobar" on;
> 
> will disable ligatures, because "foobar" is invalid (right?), whereas
> 
>   -moz-font-feature-settings: "liga" off;
>   -moz-font-feature-settings: "foo" on;
> 
> will NOT, because the second rule overrides the first. 

Right, tags longer than 4 characters produce a parse error, tags less than 4 don't.
But I actually think it would make sense to simply set it to 4, since that's what
OpenType defines.

> (I notice that the current ED text says, "Although specifically defined for
> OpenType feature tags, feature tags for other modern font formats that
> support font features may be added in the future." This will present a
> problem if such an "other modern font format" expects tags that are
> something other than 4 characters long.)

Making the initial spec more restrictive and then loosening it is not
really a problem.  It's the reverse case that's a problem, it's harder
to clamp down on something once it's in general usage.

If there's a font format in general use that requires longer/more complex
tag syntax then we can address that once the tag syntax exists.

Comment 25

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #24)

> Making the initial spec more restrictive and then loosening it is not
> really a problem.

Yes it is. If the initial spec says that a string of > 4 chars is a syntax error, and therefore the entire rule is dropped (and the pre-existing value is maintained), then loosening it to accept longer strings (so that such a rule overrides the pre-existing value) will be a backward-incompatible change.

Comment 26

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #24)
> If there's a font format in general use that requires longer/more complex
> tag syntax then we can address that once the tag syntax exists.

AAT has a potentially longer/more complex tag syntax. Although we're not currently supporting user-specified features for AAT fonts, I think it's wrong for the CSS syntax to exclude that possibility by restricting features to OT-style tags.

For example, in a fully AAT-enabled implementation, it'd be nice to be able to say

  font-family: "Hoefler Text";
  font-feature-settings: "Style Options" "Engraved Text";
(or font-feature-settings: "Style Options=Engraved Text";)

and

  font-family: "Charis SIL";
  font-feature-settings: "Uppercase Eng alternates" "Capital N with tail";

as that's how the features are exposed to the user, rather than relying on some sort of mapping to synthetic ss## tags or something.

(This kind of thing was implemented years ago in XeTeX, which has a fairly analogous mechanism allowing users to apply arbitrary features to both OT and AAT fonts.)

Comment 27

[Jonathan Kew [:jfkthame]]

Comment on attachment 612800 [details] [diff] [review]
patch, part 5 - merge per-font and style features in shapers

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

Marking r- as I'd like to see a version that refactors this to use a shared MergeFontFeatures helper (comment 12).

Comment 28

[Jonathan Kew [:jfkthame]]

Comment on attachment 612802 [details] [diff] [review]
patch, part 7 - update reftests to use new syntax

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

Looks fine as far as it goes, but please also include a testcase (or several) using both old and new syntaxes, to check that they interact as expected. (In principle, the CSS parsing tests should cover this, but I think it's worthwhile to address it in reftest form as well.)

Comment 29

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

Comment on attachment 612795 [details] [diff] [review]
patch, part 2 - parse new syntax of font-feature-settings property

(BTW, what's the difference between the Fonts.html and the
Overview.html files in the css3-fonts/ directory in
https://dvcs.w3.org/hg/csswg/ ?)

But either way, I find the spec a little unclear about what happens
to feature tags *less* than four characters.  It says that they are
four characters, but then only says that ones greater than four
characters are invalid.  Judging from the code, I think the spec
should change the text:
  # As specified in the OpenType specification, feature tags contain
  # four characters.
to say something about *up to* four characters.  (Is zero characters
allowed?)  Though I'm actually having some second thoughts about
whether we want to be poking into the insides of strings as part of
determining whether something is valid syntax.

The nsFont.cpp changes in this patch (2) belong in the previous one
(1).  They look fine, though so r=dbaron on them (so you don't need
to ask for re-review).


nsCSSParser.cpp:

>+// edge case - "tagx" on 1, "tagx" "tagy", "tagx" -1, "tagx" big

this comment should be clear that these are "error cases", not edge
cases that you need to handle.  (Or it doesn't have to be there at
all.)

CSSParserImpl::ParseFontFeatureSettings:

Rather than have the whole function indented, how about starting
with:
  if (ParseVariant(aValue, VARIANT_INHERIT | VARIANT_NORMAL, nsnull)) {
    return true;
  }

>+    if (mToken.mType != eCSSToken_String ||
>+        mToken.mIdent.Length() > MAX_FEATURE_TAG_LENGTH)
>+    {

local style puts the { up on the previous line rather than on its
own.  There are a few more of these in the function.

>+      // end of prop val?
>+      if (CheckEndProperty()) {
>+        break;
>+      }
>+
>+      // not at the end of the prop value, need a comma followed by the next
tag
>+      if (!ExpectSymbol(',', true) || !GetToken(true)) {
>+        return false;
>+      }

You don't need CheckEndProperty() here (and I'd rather avoid
CheckEndProperty in general... it's a sign of parsing code that's
not properly composable as a piece of something larger).  You can
replace the above with:

  if (!ExpectSymbol(',', true)) {
    break;
  }

  if (!GetToken(true)) {
    return false;
  }

(This is because if it's not a comma and we return true, and there's
garbage after the property, that's fine; we'll still reject it.)


Once you do that, you can fix the duplication of code before and in
the loop.  (I think it's better to optimize for code size and
complexity than to optimize for number of memory allocations when
hitting a parse error.  Most property parses will be successful.)

In particular, after the above change, you can move these lines:
>+      cur->mNext = new nsCSSValuePairList;
>+      cur = cur->mNext;
up to between the ExpectSymbol and the GetToken calls, and then do
the same by moving this line that's right before the loop:
>+    nsCSSValuePairList *cur = aValue.SetPairListValue();
up to above the GetToken call.  Then you have identical code before
the loop and at the bottom of the loop that you can condense to
being in a single place, at the top of the loop (at the cost of
allocating memory a little earlier and thus doing it in the case of
some parse errors).



nsCSSValue.cpp:

>-    GetPairListValue()->AppendToString(aProperty, aResult);
>+    if (aProperty == eCSSProperty_font_feature_settings) {
>+      nsStyleUtil::AppendFontFeatureSettings(*this, aResult);
>+    } else {
>+      GetPairListValue()->AppendToString(aProperty, aResult);
>+    }

for extensibility, write this as a switch (aProperty) with case
eCSSProperty_font_feature_settings: and default:.  (Don't forget the
breaks.)


nsComputedDOMStyle.cpp:

Leave the line:
  nsROCSSPrimitiveValue* val = GetROCSSPrimitiveValue();
just once at the very top of the function, rather than twice.

And for that matter, why not just keep the existing if-else pattern
rather than an early return?

It seems like the diff here should only need to touch what's inside
the else{}; no reason to touch anything else.


nsRuleNode.cpp:

>+    //canStoreInRuleTree = false;
>+    // hmmm, what to do here...

How about exactly the code you removed, which should still compile:
>-    aCanStoreInRuleTree = false;
>-    aFont->mFont.featureSettings = aParentFont->mFont.featureSettings;

(That should have caused test failures in
layout/style/test_inherit_computation.html.  If it didn't, something's
wrong.)

>+
aFont->mFont.fontFeatureSettings.AppendElements(systemFont.fontFeatureSettings);

This should assign (=) rather than AppendElements.  There could already
be elements in the array that were inherited, because of how we set up
the style struct using copy constructors.

>+  case eCSSUnit_PairList:
>+  case eCSSUnit_PairListDep:
>+    ComputeFontFeatures(featureSettingsValue->GetPairListValue(),
>+                        aFont->mFont.fontFeatureSettings);

And you either need to clear the array here, or inside
ComputeFontFeatures.  Perhaps the latter?

>+  NS_ABORT_IF_FALSE(aFeaturesList->mXValue.GetUnit() == eCSSUnit_String,
>+                    "unexpected value unit");

This belongs inside the loop, with s/aFeaturesList/p/.

>+    gfxFontFeature feat = { 0 };

Could you make it explicitly "0, 0".  I hate depending on the rules
C++ has for that.


>+    // tag is a 4-byte ASCII sequence
>+    nsAutoString tag;
>+    p->mXValue.GetStringValue(tag);
>+    if (tag.Length() != 4) {
>+      continue;
>+    }

So, er, wait?  Now the tags of length less than 4 aren't any good?
Maybe they should be parse errors?  Or maybe length should never be a
parse error, and you just have this code?

>+    feat.mTag = ((tag[0] & 0xff) << 24) | ((tag[1] & 0xff) << 16) |
>+                ((tag[2] & 0xff) << 8)  | (tag[3] & 0xff);

Mmm, big endian.

Also, I really think non-ASCII characters should get ignored rather
than having their low bits used.  (This makes me inclined to suggest
that the contents of the string shouldn't be a parse error.)

>+    if (p->mYValue.GetUnit() == eCSSUnit_Integer) {
>+      feat.mValue = p->mYValue.GetIntValue();
>+    }

Given your parsing code, I think you can assert about the unit
rather than testing it.

nsStyleUtil.cpp:

>+  PRUint32 i, numFeat = aFeatures.Length();
>+  bool needSep = false;
>+
>+  for (i = 0; i < numFeat; i++) {

Just declare the two PRUint32s inside the for loop header, and replace
needSep with an i != 0 check (which should be no more expensive).

>+    char tag[7] = { 0 };

Aargh.  Could you just assign '\0' to tag[6] and skip the
initialization?

>+      aResult.AppendFloat(float(feat.mValue));

Please use AppendInt instead (with no float cast).


r=dbaron with those things fixed, though I'd like to know what's up
with the tags of length less than 4.

Comment 30

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

Comment on attachment 612796 [details] [diff] [review]
patch, part 3 - update layout/style tests

property_database.js uses tabs; you're not using them, so the
indentation is off.  (Yes, it probably shouldn't have.)

I'm not sure what the new -moz-font-language-override test is
adding, though I'm ok with having it.

r=dbaron with that (though I didn't look at jfkthame's comment... probably
should)

Comment 31

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

Comment on attachment 612798 [details] [diff] [review]
patch, part 4 - initialize gfxFontStyle using nsFont values

r=dbaron

Comment 32

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #25)
> > Making the initial spec more restrictive and then loosening it is not
> > really a problem.
> 
> Yes it is. If the initial spec says that a string of > 4 chars is a syntax
> error, and therefore the entire rule is dropped (and the pre-existing value
> is maintained), then loosening it to accept longer strings (so that such a
> rule overrides the pre-existing value) will be a backward-incompatible
> change.

In CSS this is the standard way of managing new property values,
there's no backward incompatibility in this respect.  Old browsers
recognize the old syntax but explicitly toss what would be new syntax.
So authors can write two property rules during the transition:

  prop: existing-value;   /* value for older user agents */ 
  prop: new-syntax-value; /* new value, old browsers explicitly toss this rule, using the previous rule */

Comment 33

[John Daggett (:jtd)]

(In reply to David Baron [:dbaron] from comment #29)
> (BTW, what's the difference between the Fonts.html and the
> Overview.html files in the css3-fonts/ directory in
> https://dvcs.w3.org/hg/csswg/ ?)

Fonts.html is the actual editor's draft and Overview.html is what I use when staging drafts for publication, since the contents will be ever so slightly different to reflect the status, etc.  The dev.w3.org/csswg/css3-fonts/ URL points to Fonts.html (via Apache magic).

Comment 34

[John Daggett (:jtd)]

(In reply to David Baron [:dbaron] from comment #29)
> But either way, I find the spec a little unclear about what happens
> to feature tags *less* than four characters.  It says that they are
> four characters, but then only says that ones greater than four
> characters are invalid.  Judging from the code, I think the spec
> should change the text:
>   # As specified in the OpenType specification, feature tags contain
>   # four characters.
> to say something about *up to* four characters.  (Is zero characters
> allowed?)  Though I'm actually having some second thoughts about
> whether we want to be poking into the insides of strings as part of
> determining whether something is valid syntax.

I think it would be simpler to go the other way and simply require the tags to be four-character ASCII strings (i.e. throw a parse error on anything else).

I posted revised wording for discussion on www-style here:
http://lists.w3.org/Archives/Public/www-style/2012Apr/0499.html

If anyone disagrees, please respond on www-style.

Comment 35

[John Daggett (:jtd)]

Attachment: patch, part 1v1a - add array of gfxFontFeature's to nsFont

Revised based on review comments, carrying forward r=jfkthame

Comment 36

[John Daggett (:jtd)]

Attachment: patch, part 2v2 - parse new syntax of font-feature-settings property

Revised based on review comments, carrying forward r=dbaron

Comment 37

[John Daggett (:jtd)]

Attachment: patch, part 3v2 - update layout/style tests

Revised based on review comments, carrying forward r=dbaron

Comment 38

[John Daggett (:jtd)]

Attachment: patch, part 5v2 - merge per-font and style features in shapers

The feature merge function is now a static method of gfxFontShaper.  The harfbuzz and Graphite shaper code enumerate the merged hashtable and create the appropriate shaper-specific modifications.

> > +static PLDHashOperator
> > +AddFeature(const PRUint32& aTag, PRUint32& aValue, void *aUserArg)
> 
> aValue should be const too, no?

This doesn't match what the templated method is looking for.

Comment 39

[John Daggett (:jtd)]

Attachment: patch, part 7 - update reftests to use new syntax

Added several new tests to test use of old/new syntax for font-feature-settings.

Comment 40

[John Daggett (:jtd)]

Note that the behavior implemented in patch 2 enforces the "anything other than a four-character tag" is invalid behavior (see comment 34).  I'm going to try and resolve this during the CSS WG telcon this week.

Comment 41

[Jonathan Kew [:jfkthame]]

Comment on attachment 617417 [details] [diff] [review]
patch, part 5v2 - merge per-font and style features in shapers

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

r=me, with a minor optimization, a couple nits, and a bonus bug-fix for gfxGraphiteShaper. :)

::: gfx/thebes/gfxFont.cpp
@@ +1356,5 @@
> +    const nsTArray<gfxFontFeature>& aFontFeatures,
> +    bool aDisableLigatures,
> +    nsDataHashtable<nsUint32HashKey,PRUint32>& aMergedFeatures)
> +{
> +    aMergedFeatures.Init();

Given that the "no features" case will probably be very common, I'd suggest deferring this until after the bail-if-nothing-to-do check so that we don't pay the cost of initializing the hashtable in that case.

This method could return a boolean indicating whether or not it actually did anything, and the callers can then do

    if (MergeFontFeatures(....)) {
        ...enumerate them...
    }

to avoid using the hash if it hasn't been initialized.

::: gfx/thebes/gfxGraphiteShaper.cpp
@@ +213,5 @@
>          grLang = GetGraphiteTagForLang(langString);
>      }
>      gr_feature_val *grFeatures = gr_face_featureval_for_lang(mGrFace, grLang);
>  
> +    nsDataHashtable<nsUint32HashKey,PRUint32> aMergedFeatures;

Nit: no "a" prefix, it's not an argument.

@@ +220,5 @@
> +                      aShapedWord->DisableLigatures(), aMergedFeatures);
> +
> +    // enumerate result and insert into Graphite feature list
> +    GrFontFeatures f = {mGrFace, grFeatures};
> +    aMergedFeatures.Enumerate(AddFeature, (void*) (&f));

I don't think the cast is needed, is it? For a void* parameter, any pointer should be acceptable.

@@ +231,1 @@
>          gr_featureval_destroy(grFeatures);

Hmmm. According to the graphite API comments, we should be destroying this unconditionally, regardless of whether we had user features. This is an existing bug (leak) in gfxGraphiteShaper, but let's fix it while we're here.

::: gfx/thebes/gfxHarfBuzzShaper.cpp
@@ +844,5 @@
>  
> +static PLDHashOperator
> +AddFeature(const PRUint32& aTag, PRUint32& aValue, void *aUserArg)
> +{
> +    nsTArray<hb_feature_t>* features = (nsTArray<hb_feature_t>*) (aUserArg);

Prefer C++ static_cast<>() rather than old C-style cast.

@@ +847,5 @@
> +{
> +    nsTArray<hb_feature_t>* features = (nsTArray<hb_feature_t>*) (aUserArg);
> +
> +    hb_feature_t feat = { 0, 0, 0, UINT_MAX };
> +    feat.tag = (hb_tag_t) aTag;

Does this really need an explicit cast?

@@ +988,4 @@
>      gfxFontEntry *entry = mFont->GetFontEntry();
>      const gfxFontStyle *style = mFont->GetStyle();
> +
> +    nsDataHashtable<nsUint32HashKey,PRUint32> aMergedFeatures;

Nit again - no "a" prefix.

@@ +994,5 @@
> +                      mFont->GetFontEntry()->mFeatureSettings,
> +                      aShapedWord->DisableLigatures(), aMergedFeatures);
> +
> +    // enumerate result and insert into hb_feature array
> +    aMergedFeatures.Enumerate(AddFeature, (void*) (&features));

Again, I don't think you need a cast.

Comment 42

[Jonathan Kew [:jfkthame]]

Comment on attachment 617420 [details] [diff] [review]
patch, part 7 - update reftests to use new syntax

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

Let's drop the redundant -nohlig file.

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

This file is redundant, as hlig isn't enabled by default. The manifest can just compare font-features-oldsyntax-[12] to font-features-ref, which already gives the default rendering for the font.

Comment 43

[John Daggett (:jtd)]

Attachment: patch, part 5v3 - merge per-font and style features in shapers

Revised based on review comments, carrying forward r=jkew

Comment 44

[John Daggett (:jtd)]

Attachment: patch, part 7 - update reftests to use new syntax

Revised based on review comments, carrying forward r=jkew

Comment 45

[John Daggett (:jtd)]

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3378bceed278
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bd790985819
https://hg.mozilla.org/integration/mozilla-inbound/rev/98afafcdbf53
https://hg.mozilla.org/integration/mozilla-inbound/rev/75b2fd8e0f16
https://hg.mozilla.org/integration/mozilla-inbound/rev/da881377f29d
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e7ff1af0e28
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e3152b3c7b0

Comment 46

[John Daggett (:jtd)]

(In reply to John Daggett (:jtd) from comment #40)
> Note that the behavior implemented in patch 2 enforces the "anything other
> than a four-character tag" is invalid behavior (see comment 34).  I'm going
> to try and resolve this during the CSS WG telcon this week.

The CSS WG resolved on this behavior during the telcon.  I'll include a link to the minutes when they are posted.

Comment 47

[John Daggett (:jtd)]

(In reply to John Daggett (:jtd) from comment #46)
> The CSS WG resolved on this behavior during the telcon.  I'll include a link
> to the minutes when they are posted.

http://lists.w3.org/Archives/Public/www-style/2012Apr/0757.html

Comment 48

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3378bceed278
https://hg.mozilla.org/mozilla-central/rev/2bd790985819
https://hg.mozilla.org/mozilla-central/rev/98afafcdbf53
https://hg.mozilla.org/mozilla-central/rev/75b2fd8e0f16
https://hg.mozilla.org/mozilla-central/rev/da881377f29d
https://hg.mozilla.org/mozilla-central/rev/7e7ff1af0e28
https://hg.mozilla.org/mozilla-central/rev/4e3152b3c7b0

Comment 49

[Jesse Ruderman]

https://developer.mozilla.org/en/CSS/-moz-font-feature-settings might need an update.

Comment 50

[Jean-Yves Perrier [:teoli]]

Updated https://developer.mozilla.org/en/CSS/font-feature-settings to the new syntax and https://developer.mozilla.org/en/Firefox_15_for_developers

:-)

Comment 51

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08eea487dbd0

Comment 52

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/08eea487dbd0