Bug 63618

Summary: Parsing CSS3 font-feature-settings property
Product: WebKit Reporter: Kenichi Ishibashi <bashi@chromium.org>
Component: TextAssignee: Kenichi Ishibashi <bashi@chromium.org>
Status: RESOLVED FIXED    
Severity: Normal CC: clagnut+webkitbugzilla@gmail.com, darin@apple.com, hamaji@chromium.org, hyatt@apple.com, phiw@l-c-n.com, simon.fraser@apple.com, yuzo@google.com
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 63796    
Attachments:
Description Flags
WIP Patch V0
none
Patch V1
none
Patch V2
none
Patch V3(Rebased)
none
Patch V4
none
Patch V5
none
Patch V6
none
Patch V7
none
Patch V8 none

Description From 2011-06-29 03:15:29 PST
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.
------- Comment #1 From 2011-06-29 03:23:38 PST -------
Created an attachment (id=99063) [details]
WIP Patch V0
------- Comment #2 From 2011-06-29 03:24:36 PST -------
(In reply to comment #1)
> Created an attachment (id=99063) [details] [details]
> WIP Patch V0

Notes for this WIP patch:
- Should I enclose the code in ENABLE(...) macro, like http://www.webkit.org/b/61726 ?
- 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.
------- Comment #3 From 2011-07-03 23:57:24 PST -------
Created an attachment (id=99603) [details]
Patch V1
------- Comment #4 From 2011-07-06 01:07:25 PST -------
(From update of attachment 99603 [details])
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.
------- Comment #5 From 2011-07-06 23:34:04 PST -------
Created an attachment (id=99941) [details]
Patch V2
------- Comment #6 From 2011-07-06 23:36:57 PST -------
(From update of attachment 99603 [details])
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.
------- Comment #7 From 2011-07-13 22:30:19 PST -------
(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,
------- Comment #8 From 2011-07-20 19:45:53 PST -------
Could someone take a look?

Thanks in advance.
------- Comment #9 From 2011-08-01 18:14:28 PST -------
Created an attachment (id=102606) [details]
Patch V3(Rebased)
------- Comment #10 From 2011-08-01 18:21:52 PST -------
(In reply to comment #9)
> Created an attachment (id=102606) [details] [details]
> Patch V3(Rebased)

Just rebased to ToT.  Again, could someone review this patch?  Any feedback would be highly appreciated.
------- Comment #11 From 2011-08-01 21:47:24 PST -------
(From update of attachment 102606 [details])
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.
------- Comment #12 From 2011-08-01 22:53:46 PST -------
Created an attachment (id=102615) [details]
Patch V4
------- Comment #13 From 2011-08-01 22:55:16 PST -------
(From update of attachment 102606 [details])
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.
------- Comment #14 From 2011-08-02 04:57:19 PST -------
(From update of attachment 102615 [details])
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?
------- Comment #15 From 2011-08-02 07:43:37 PST -------
Created an attachment (id=102648) [details]
Patch V5
------- Comment #16 From 2011-08-02 07:45:31 PST -------
(From update of attachment 102615 [details])
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.
------- Comment #17 From 2011-08-02 18:51:46 PST -------
(From update of attachment 102648 [details])
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?
------- Comment #18 From 2011-08-02 19:17:17 PST -------
(From update of attachment 102648 [details])
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?
------- Comment #19 From 2011-08-03 02:09:18 PST -------
Created an attachment (id=102755) [details]
Patch V6
------- Comment #20 From 2011-08-03 02:18:29 PST -------
(From update of attachment 102648 [details])
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.
------- Comment #21 From 2011-08-03 02:22:07 PST -------
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.
------- Comment #22 From 2011-08-03 23:25:20 PST -------
(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
------- Comment #23 From 2011-08-03 23:56:28 PST -------
(From update of attachment 102755 [details])
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.
------- Comment #24 From 2011-08-04 00:55:23 PST -------
Created an attachment (id=102879) [details]
Patch V7
------- Comment #25 From 2011-08-04 00:59:48 PST -------
(From update of attachment 102879 [details])
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.
------- Comment #26 From 2011-08-04 01:01:29 PST -------
(From update of attachment 102755 [details])
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.
------- Comment #27 From 2011-08-04 01:05:11 PST -------
(From update of attachment 102879 [details])
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!
------- Comment #28 From 2011-08-04 01:29:45 PST -------
(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.
------- Comment #29 From 2011-08-04 18:12:44 PST -------
Created an attachment (id=103013) [details]
Patch V8
------- Comment #30 From 2011-08-04 18:14:42 PST -------
(In reply to comment #29)
> Created an attachment (id=103013) [details] [details]
> Patch V8

Rebased to ToT and addressed Hamaji-san's comment.
------- Comment #31 From 2011-08-04 18:19:55 PST -------
Thank you for addressing my drive-by comments. I have no more comments.
------- Comment #32 From 2011-08-05 00:35:09 PST -------
(From update of attachment 103013 [details])
Clearing flags on attachment: 103013

Committed r92457: <http://trac.webkit.org/changeset/92457>
------- Comment #33 From 2011-08-05 00:35:18 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #34 From 2011-08-09 07:56:36 PST -------
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).
------- Comment #35 From 2011-08-09 08:11:09 PST -------
(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.