Bug 63618 – Parsing CSS3 font-feature-settings property

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Kenichi Ishibashi]

I'd like to add CSS3 font feature properties support.

http://www.w3.org/TR/css3-fonts/#font-rend-props

For ports which can handle OpenType features, starting with the font-feature-settings property (http://www.w3.org/TR/css3-fonts/#propdef-font-feature-settings) would be good because other properties can be treated as sort of shorthands.  This bug entry is for adding parsing feature of the font-feature-settings as a first step.

I'll create a master-bug for adding CSS3 font feature properties support once we decide to move forward on this.

[Kenichi Ishibashi]

Created attachment 99063 [details]
WIP Patch V0

[Kenichi Ishibashi]

(In reply to comment #1)
> Created an attachment (id=99063) [details]
> WIP Patch V0

Notes for this WIP patch:
- Should I enclose the code in ENABLE(...) macro, like http://www.﷒0﷓ ?
- Build files are not updated yet except for GYP. I'll update them once I can move foward with this.
- ChangeLog is also not prepared yet.

[Kenichi Ishibashi]

Created attachment 99603 [details]
Patch V1

[Hajime Morrita]

Comment on attachment 99603 [details]
Patch V1

View in context: https://bugs.webkit.org/attachment.cgi?id=99603&action=review

> LayoutTests/ChangeLog:12
> +        * css3/script-tests/TEMPLATE.html: Added.

Script test in separete js file is obsolete. Please put it inside HTML.

> Source/WebCore/css/CSSParser.cpp:6021
> +    while (value) {

Is it possible to rewrite this with for statement? If it's possible, it's desirable
because this loops is for list traversal.

This loop looks too complicated anyway...
I guess we can make this clear by extracting a per-item function.
What do you think?

> Source/WebCore/css/CSSParser.cpp:6026
> +        if (value->string.length != 4)

having a named constant would be preferable.

> Source/WebCore/css/CSSParser.cpp:6051
> +        settings->append(parsedValue.release());

You don't need release(). PassRefPtr does its job for you.

> Source/WebCore/css/CSSParser.cpp:6054
> +    if (settings && settings->length()) {

Can setting be null?

> Source/WebCore/css/CSSStyleSelector.cpp:5317
> +            fontDescription.setFeatureSettings(0);

How about to define FontDescription::makeNormal() ? 
Then we can write something like
m_style->setFontDescription(m_style->fontDescription().makeNormal())

> Source/WebCore/css/FontFeatureValue.cpp:55
> +    String result = "'" + m_tag + "'";

How about to try StringBuilder? It would be more effective in this case.

> Source/WebCore/platform/graphics/FontDescription.h:78
> +        , m_featureSettings(0)

We don't need this. RefPtr automatically gets zero-cleared.

> Source/WebCore/platform/graphics/FontDescription.h:160
> +    RefPtr<FontFeatureSettings> m_featureSettings; // The list of OpenType font feature settings.

I think we don't need this comment. The class name is clear enough.

> Source/WebCore/platform/graphics/FontDescription.h:200
> +        && m_featureSettings.get() == other.m_featureSettings.get();

It looks you are comparing pointers. Is this intentional?

> Source/WebCore/platform/graphics/FontFeatureSettings.h:30
> +#include <wtf/RefPtr.h>

PassRefPtr.h?

> Source/WebCore/platform/graphics/FontFeatureSettings.h:39
> +    FontFeature(const FontFeature& other);

Do we need the explicit copy constructor?

> Source/WebCore/platform/graphics/FontFeatureSettings.h:40
> +    FontFeature& operator=(const FontFeature&);

...and an assignment operator?

> Source/WebCore/platform/graphics/FontFeatureSettings.h:56
> +    void appendFeature(const FontFeature& feature) { m_list.append(feature); }

append() looks enough. (we have at() anyway.)

> Source/WebCore/platform/graphics/FontFeatureSettings.h:59
> +    const FontFeature& at(size_t index) const { return m_list[index]; }

let's use m_list.at() for similarity.

[Kenichi Ishibashi]

Created attachment 99941 [details]
Patch V2

[Kenichi Ishibashi]

Comment on attachment 99603 [details]
Patch V1

View in context: https://bugs.webkit.org/attachment.cgi?id=99603&action=review

Hi Morita-san,

Thank you very much for detailed review! I've addressed most of your comments but not all (please see the comment below).

>> LayoutTests/ChangeLog:12
>> +        * css3/script-tests/TEMPLATE.html: Added.
> 
> Script test in separete js file is obsolete. Please put it inside HTML.

Done.

>> Source/WebCore/css/CSSParser.cpp:6021
>> +    while (value) {
> 
> Is it possible to rewrite this with for statement? If it's possible, it's desirable
> because this loops is for list traversal.
> 
> This loop looks too complicated anyway...
> I guess we can make this clear by extracting a per-item function.
> What do you think?

Thank you for suggestion, but I didn't rewrite with for statement.  At the end of each iteration, we need to check whether the current cssValue is NULL (end of the list) or comma so we cannot simply advance value by value = m_valueList->next().

Extracted the code in the loop as parseFontFeatureTag().

>> Source/WebCore/css/CSSParser.cpp:6026
>> +        if (value->string.length != 4)
> 
> having a named constant would be preferable.

Done.

>> Source/WebCore/css/CSSParser.cpp:6051
>> +        settings->append(parsedValue.release());
> 
> You don't need release(). PassRefPtr does its job for you.

I followed the way that CSSParser::parseFontWeight() does.  Do we really need not call release()?

>> Source/WebCore/css/CSSParser.cpp:6054
>> +    if (settings && settings->length()) {
> 
> Can setting be null?

Removed checking null.

>> Source/WebCore/css/CSSStyleSelector.cpp:5317
>> +            fontDescription.setFeatureSettings(0);
> 
> How about to define FontDescription::makeNormal() ? 
> Then we can write something like
> m_style->setFontDescription(m_style->fontDescription().makeNormal())

Added as makeNoFeatureSettings() because makeNormal() looks too generic when I glance at other methods in FontDescription.

BTW, IMHO, the method looks not to be in harmony with other methods in FontDescription.  Should we still do this?

>> Source/WebCore/css/FontFeatureValue.cpp:55
>> +    String result = "'" + m_tag + "'";
> 
> How about to try StringBuilder? It would be more effective in this case.

Done.

>> Source/WebCore/platform/graphics/FontDescription.h:78
>> +        , m_featureSettings(0)
> 
> We don't need this. RefPtr automatically gets zero-cleared.

Removed.

>> Source/WebCore/platform/graphics/FontDescription.h:160
>> +    RefPtr<FontFeatureSettings> m_featureSettings; // The list of OpenType font feature settings.
> 
> I think we don't need this comment. The class name is clear enough.

Removed the comment.

>> Source/WebCore/platform/graphics/FontDescription.h:200
>> +        && m_featureSettings.get() == other.m_featureSettings.get();
> 
> It looks you are comparing pointers. Is this intentional?

Changed to compare RefPtr itself.

>> Source/WebCore/platform/graphics/FontFeatureSettings.h:30
>> +#include <wtf/RefPtr.h>
> 
> PassRefPtr.h?

Added.

>> Source/WebCore/platform/graphics/FontFeatureSettings.h:39
>> +    FontFeature(const FontFeature& other);
> 
> Do we need the explicit copy constructor?

Removed.

>> Source/WebCore/platform/graphics/FontFeatureSettings.h:40
>> +    FontFeature& operator=(const FontFeature&);
> 
> ...and an assignment operator?

Removed.

>> Source/WebCore/platform/graphics/FontFeatureSettings.h:56
>> +    void appendFeature(const FontFeature& feature) { m_list.append(feature); }
> 
> append() looks enough. (we have at() anyway.)

Done.

>> Source/WebCore/platform/graphics/FontFeatureSettings.h:59
>> +    const FontFeature& at(size_t index) const { return m_list[index]; }
> 
> let's use m_list.at() for similarity.

Done.

[Kenichi Ishibashi]

(Cc'ed people who I can see their name as reviewers on similar patches of this.)

I'd appreciate if someone who has a vast experience on CSS area could take a look at the latest patch.

I'd also like to ask I should enclose these change by ENABLE macros.

Regards,

[Kenichi Ishibashi]

Could someone take a look?

Thanks in advance.

[Kenichi Ishibashi]

Created attachment 102606 [details]
Patch V3(Rebased)

[Kenichi Ishibashi]

(In reply to comment #9)
> Created an attachment (id=102606) [details]
> Patch V3(Rebased)

Just rebased to ToT.  Again, could someone review this patch?  Any feedback would be highly appreciated.

[Kent Tamura]

Comment on attachment 102606 [details]
Patch V3(Rebased)

View in context: https://bugs.webkit.org/attachment.cgi?id=102606&action=review

I won't set r+ because I'm not familiar with CSS code.  Some minor comments follow.

> Source/WebCore/css/FontFeatureValue.cpp:17
> + * You should have received a copy of the GNU Library General Public License
> + * along with this library; see the file COPYING.LIB.  If not, write to
> + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> + * Boston, MA 02110-1301, USA.

Why FontFeatueValue.{cpp,h} have LGPL copyright headers unlike FontFeatureSettings.h?

> Source/WebCore/platform/graphics/FontFeatureSettings.h:44
> +    int value() const { return m_value; }
> +private:

Insert a blank line before "private:".

> Source/WebCore/platform/graphics/FontFeatureSettings.h:59
> +    const FontFeature& at(size_t index) const { return m_list.at(index); }
> +private:

ditto.

[Kenichi Ishibashi]

Created attachment 102615 [details]
Patch V4

[Kenichi Ishibashi]

Comment on attachment 102606 [details]
Patch V3(Rebased)

View in context: https://bugs.webkit.org/attachment.cgi?id=102606&action=review

Hi Kent-san,

Thank you so much taking a look! Revised the patch.

>> Source/WebCore/css/FontFeatureValue.cpp:17
>> + * Boston, MA 02110-1301, USA.
> 
> Why FontFeatueValue.{cpp,h} have LGPL copyright headers unlike FontFeatureSettings.h?

Oops, I referred to FontValue.{cpp,h} for copyright headers here..  Revised to have the same copyright headers as FontFeatureSettings.h has.

>> Source/WebCore/platform/graphics/FontFeatureSettings.h:44
>> +private:
> 
> Insert a blank line before "private:".

Done.

>> Source/WebCore/platform/graphics/FontFeatureSettings.h:59
>> +private:
> 
> ditto.

Done.

[Shinichiro Hamaji]

Comment on attachment 102615 [details]
Patch V4

View in context: https://bugs.webkit.org/attachment.cgi?id=102615&action=review

Putting r- as you may think some of my comments are useful.

> LayoutTests/css3/font-feature-settings-parsing.html:13
> +function parseResultOf(value) {

I'd prefer to use <style> or style attribute for testcases of newly added CSS properties.

> LayoutTests/css3/font-feature-settings-parsing.html:19
> +var validInputsAndExpectedResults = [

How about using examples in http://www.w3.org/TR/2011/WD-css3-fonts-20110324/#propdef-font-feature-settings as testcases?

> LayoutTests/css3/font-feature-settings-parsing.html:59
> +</html>

I want to see a test to check if font-feature-settings is inherited.

Also, I think font-feature-settings can be written in @font-face, right? If so, please add a testcase for it.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1704
> +        case CSSPropertyWebkitFontFeatureSettings:

Hmm... I think we should implement this and a testcase. Note that TextLineThrough, TextOverline, and TextUnderline are actually not implemented. (I guess computed value for WebkitTextEmphasis must be implemented, too).

If you want to do this in another patch, you may need to do

1. put this case in another place and add a FIXME comment
2. write a failing testcase

> Source/WebCore/css/CSSStyleSelector.cpp:5156
> +            if (m_style->setFontDescription(m_style->fontDescription().makeNormalFeatureSettings()))

CSSStyleSelector::setFontDescription automatically updates m_fontDirty?

> Source/WebCore/css/CSSStyleSelector.cpp:5176
> +        if (m_style->setFontDescription(fontDescription))

Use CSSStyleSelector::setFontDescription?

[Kenichi Ishibashi]

Created attachment 102648 [details]
Patch V5

[Kenichi Ishibashi]

Comment on attachment 102615 [details]
Patch V4

View in context: https://bugs.webkit.org/attachment.cgi?id=102615&action=review

Hamaji-san,

Thank you so much for review.  I've revised the patch.

>> LayoutTests/css3/font-feature-settings-parsing.html:13
>> +function parseResultOf(value) {
> 
> I'd prefer to use <style> or style attribute for testcases of newly added CSS properties.

Done.

>> LayoutTests/css3/font-feature-settings-parsing.html:19
>> +var validInputsAndExpectedResults = [
> 
> How about using examples in http://www.w3.org/TR/2011/WD-css3-fonts-20110324/#propdef-font-feature-settings as testcases?

Used the examples for valid inputs.

>> LayoutTests/css3/font-feature-settings-parsing.html:59
>> +</html>
> 
> I want to see a test to check if font-feature-settings is inherited.
> 
> Also, I think font-feature-settings can be written in @font-face, right? If so, please add a testcase for it.

Added a test case for inherited.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1704
>> +        case CSSPropertyWebkitFontFeatureSettings:
> 
> Hmm... I think we should implement this and a testcase. Note that TextLineThrough, TextOverline, and TextUnderline are actually not implemented. (I guess computed value for WebkitTextEmphasis must be implemented, too).
> 
> If you want to do this in another patch, you may need to do
> 
> 1. put this case in another place and add a FIXME comment
> 2. write a failing testcase

Thank you for pointing this out.  I didn't recognize that I need to implement this.  Implemented.

>> Source/WebCore/css/CSSStyleSelector.cpp:5156
>> +            if (m_style->setFontDescription(m_style->fontDescription().makeNormalFeatureSettings()))
> 
> CSSStyleSelector::setFontDescription automatically updates m_fontDirty?

Done.

>> Source/WebCore/css/CSSStyleSelector.cpp:5176
>> +        if (m_style->setFontDescription(fontDescription))
> 
> Use CSSStyleSelector::setFontDescription?

Done.

[Yuzo Fujishima]

Comment on attachment 102648 [details]
Patch V5

View in context: https://bugs.webkit.org/attachment.cgi?id=102648&action=review

Drive-by comments.

> LayoutTests/css3/font-feature-settings-parsing.html:5
> +<style>

I'd recommend adding following tests:

two normal's: normal, normal
normal and feature tag: normal, "dlig" 1
value greater than 1:  "swsh" 2
value with fraction: "swsh" 1.5
valid tag with negative value: "swsh" -1
tag starts with valid name but with extra characters at the end: dliga
same tag name with different values: dlig 1, dlig 0
mixture of valid tags and invalid tags: aaaaa on, dlig on, PKRN -1
valid tag name contains unusual letters: "@ #}"
combination of cases: dlig, DLIG, DLig

> LayoutTests/css3/font-feature-settings-parsing.html:6
> +#valid1 {

I'd recommend using more descriptive test names, for example:

valid1_normal
valid2_value_1
valid3_value_on
valid4_value_missing
...

> Source/WebCore/ChangeLog:6
> +        Introduces CSS3 font-feature-settings property as -webkit-font-feature-settings.  This change only contains parsing and part.  Parsed information are stored in FontDescription class.

s/parsing and part/parsing part/ ?

> Source/WebCore/css/CSSParser.cpp:6329
> +        if (value->unit == CSSPrimitiveValue::CSS_NUMBER && value->fValue >= 0.)

What should we do if value->fValue < 0?
- Should we use the default value, i.e., 1?
- Should we drop the tag-value pair?
- Or should we drop the whole property?

> Source/WebCore/css/CSSParser.cpp:6330
> +            featureValue = primitiveValueCache()->createValue(value->fValue, static_cast<CSSPrimitiveValue::UnitTypes>(value->unit));

The spec says the value must be an integer (if not on or off).
- Is it OK to store it as float?
- Should we check if it is actually an integer?

[Yuzo Fujishima]

Comment on attachment 102648 [details]
Patch V5

View in context: https://bugs.webkit.org/attachment.cgi?id=102648&action=review

Another drive-by.

> Source/WebCore/css/CSSParser.cpp:6332
> +            featureValue = primitiveValueCache()->createIdentifierValue(value->id);

Is it important to keep the information that the value is given as on/off, rather than as 1/0?
If not, always storing the value as integer would save later processing.

> Source/WebCore/css/CSSParser.cpp:6353
> +    CSSParserValue* value = m_valueList->current();

nit: Rewriting as follows will save a few lines and one if-depth.

for (CSSParserValue* value = m_valueList->current(); value; value = m_valueList->next()) {
  ...
  if (!value)
    break;
  if (...)
    return false;
}

> Source/WebCore/css/FontFeatureValue.cpp:45
> +    if (m_value->primitiveType() == CSSPrimitiveValue::CSS_IDENT) {

You can save processing here if you store 1/0 for on/off as I mentioned above.

> Source/WebCore/platform/graphics/FontFeatureSettings.h:47
> +    int m_value;

const int?

[Kenichi Ishibashi]

Created attachment 102755 [details]
Patch V6

[Kenichi Ishibashi]

Comment on attachment 102648 [details]
Patch V5

View in context: https://bugs.webkit.org/attachment.cgi?id=102648&action=review

Yuzo-san,

Thank you for your detailed review! Revised the patch.

>> LayoutTests/css3/font-feature-settings-parsing.html:5
>> +<style>
> 
> I'd recommend adding following tests:
> 
> two normal's: normal, normal
> normal and feature tag: normal, "dlig" 1
> value greater than 1:  "swsh" 2
> value with fraction: "swsh" 1.5
> valid tag with negative value: "swsh" -1
> tag starts with valid name but with extra characters at the end: dliga
> same tag name with different values: dlig 1, dlig 0
> mixture of valid tags and invalid tags: aaaaa on, dlig on, PKRN -1
> valid tag name contains unusual letters: "@ #}"
> combination of cases: dlig, DLIG, DLig

Modified the tests following the suggestion.

>> LayoutTests/css3/font-feature-settings-parsing.html:6
>> +#valid1 {
> 
> I'd recommend using more descriptive test names, for example:
> 
> valid1_normal
> valid2_value_1
> valid3_value_on
> valid4_value_missing
> ...

Ditto.

>> Source/WebCore/ChangeLog:6
>> +        Introduces CSS3 font-feature-settings property as -webkit-font-feature-settings.  This change only contains parsing and part.  Parsed information are stored in FontDescription class.
> 
> s/parsing and part/parsing part/ ?

Oops, thank you for pointing out. Done.

>> Source/WebCore/css/CSSParser.cpp:6329
>> +        if (value->unit == CSSPrimitiveValue::CSS_NUMBER && value->fValue >= 0.)
> 
> What should we do if value->fValue < 0?
> - Should we use the default value, i.e., 1?
> - Should we drop the tag-value pair?
> - Or should we drop the whole property?

I took the last option because it seems that other properties which accepts comma separated values drop whole value if an error occurs during parsing phase.

>> Source/WebCore/css/CSSParser.cpp:6330
>> +            featureValue = primitiveValueCache()->createValue(value->fValue, static_cast<CSSPrimitiveValue::UnitTypes>(value->unit));
> 
> The spec says the value must be an integer (if not on or off).
> - Is it OK to store it as float?
> - Should we check if it is actually an integer?

Added integer check.

>> Source/WebCore/css/CSSParser.cpp:6332
>> +            featureValue = primitiveValueCache()->createIdentifierValue(value->id);
> 
> Is it important to keep the information that the value is given as on/off, rather than as 1/0?
> If not, always storing the value as integer would save later processing.

Modified to hold the value as integer instead of string.

>> Source/WebCore/css/CSSParser.cpp:6353
>> +    CSSParserValue* value = m_valueList->current();
> 
> nit: Rewriting as follows will save a few lines and one if-depth.
> 
> for (CSSParserValue* value = m_valueList->current(); value; value = m_valueList->next()) {
>   ...
>   if (!value)
>     break;
>   if (...)
>     return false;
> }

Done.

>> Source/WebCore/css/FontFeatureValue.cpp:45
>> +    if (m_value->primitiveType() == CSSPrimitiveValue::CSS_IDENT) {
> 
> You can save processing here if you store 1/0 for on/off as I mentioned above.

Done.

>> Source/WebCore/platform/graphics/FontFeatureSettings.h:47
>> +    int m_value;
> 
> const int?

Done.

[Kenichi Ishibashi]

Hamaji-san,

(In reply to comment #14)
> Also, I think font-feature-settings can be written in @font-face, right? If so, please add a testcase for it.

(I forgot to reply this comment.)
Since I don't implement rendering part, I cannot add a testcase for @font-face.  I left a note on ChangeLog and I'll add a test for it after implementing rendering part.

[Shinichiro Hamaji]

(In reply to comment #21)
> Hamaji-san,
> 
> (In reply to comment #14)
> > Also, I think font-feature-settings can be written in @font-face, right? If so, please add a testcase for it.
> 
> (I forgot to reply this comment.)
> Since I don't implement rendering part, I cannot add a testcase for @font-face.  I left a note on ChangeLog and I'll add a test for it after implementing rendering part.

Cannot we test the parsing of font-feature-settings?

http://plexode.com/eval3/#ht=%3Cstyle%3E%0A%40font-face%20%7B%0A%20%20font-size%3A%2014px%3B%0A%7D%0A%3C%2Fstyle%3E%0A&ohh=1&ohj=1&jt=document.styleSheets%5B2%5D.cssRules%5B0%5D.style.fontSize&ojh=1&ojj=1&ms=100&oth=0&otj=0&cex=1

[Shinichiro Hamaji]

Comment on attachment 102755 [details]
Patch V6

View in context: https://bugs.webkit.org/attachment.cgi?id=102755&action=review

Otherwise, looks good to me.

> Source/WebCore/css/FontFeatureValue.h:36
> +    static PassRefPtr<FontFeatureValue> create(const String& tag)

Why don't you take both tag and value here? In this way we'll never forget initializing m_value.

[Kenichi Ishibashi]

Created attachment 102879 [details]
Patch V7

[Shinichiro Hamaji]

Comment on attachment 102879 [details]
Patch V7

View in context: https://bugs.webkit.org/attachment.cgi?id=102879&action=review

> Source/WebCore/css/CSSParser.cpp:6329
> +    RefPtr<FontFeatureValue> parsedValue = FontFeatureValue::create(value->string, 1);

Cannot we create this object after the following if-clause? I think we can eliminate setValue from FontFeatureValue.

[Kenichi Ishibashi]

Comment on attachment 102755 [details]
Patch V6

View in context: https://bugs.webkit.org/attachment.cgi?id=102755&action=review

Hamaji-san,

Thank you for took another look and suggestion for testing with @font-face.  I've added a few tests(for @font-face and duplicate "normal" value).

>> Source/WebCore/css/FontFeatureValue.h:36
>> +    static PassRefPtr<FontFeatureValue> create(const String& tag)
> 
> Why don't you take both tag and value here? In this way we'll never forget initializing m_value.

Done.

[Kenichi Ishibashi]

Comment on attachment 102879 [details]
Patch V7

View in context: https://bugs.webkit.org/attachment.cgi?id=102879&action=review

>> Source/WebCore/css/CSSParser.cpp:6329
>> +    RefPtr<FontFeatureValue> parsedValue = FontFeatureValue::create(value->string, 1);
> 
> Cannot we create this object after the following if-clause? I think we can eliminate setValue from FontFeatureValue.

I was tempted not to create a temporary variable.  Will fix.  Thanks!

[Kenichi Ishibashi]

(In reply to comment #27)
> I was tempted not to create a temporary variable.  Will fix.  Thanks!

I'll land the revised patch after https://bugs.webkit.org/show_bug.cgi?id=10874 is landed because it will conflict with this patch.

[Kenichi Ishibashi]

Created attachment 103013 [details]
Patch V8

[Kenichi Ishibashi]

(In reply to comment #29)
> Created an attachment (id=103013) [details]
> Patch V8

Rebased to ToT and addressed Hamaji-san's comment.

[Yuzo Fujishima]

Thank you for addressing my drive-by comments. I have no more comments.

[Kenichi Ishibashi]

Comment on attachment 103013 [details]
Patch V8

Clearing flags on attachment: 103013

Committed r92457: <http://trac.webkit.org/changeset/92457>

[Kenichi Ishibashi]

All reviewed patches have been landed.  Closing bug.

[Richard Rutter]

Excuse my ignorance, but should this fix be working in the current nightlies? It doesn't seem to be at the moment (nightly build at time of writing being r92670).

[Kenichi Ishibashi]

(In reply to comment #34)
> Excuse my ignorance, but should this fix be working in the current nightlies? It doesn't seem to be at the moment (nightly build at time of writing being r92670).

No. As I noted in ChangeLog, this patch contains only CSS parsing part so you wouldn't see any effect of the property for now.