Bug 63618

Summary: Parsing CSS3 font-feature-settings property
Product: WebKit Reporter: Kenichi Ishibashi <bashi>
Component: TextAssignee: Kenichi Ishibashi <bashi>
Status: RESOLVED FIXED    
Severity: Normal CC: clagnut+webkitbugzilla, darin, hamaji, hyatt, phiw2, simon.fraser, yuzo
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 Kenichi Ishibashi 2011-06-29 03:15:29 PDT
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 Kenichi Ishibashi 2011-06-29 03:23:38 PDT
Created attachment 99063 [details]
WIP Patch V0
Comment 2 Kenichi Ishibashi 2011-06-29 03:24:36 PDT
(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.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 Kenichi Ishibashi 2011-07-03 23:57:24 PDT
Created attachment 99603 [details]
Patch V1
Comment 4 Hajime Morrita 2011-07-06 01:07:25 PDT
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.
Comment 5 Kenichi Ishibashi 2011-07-06 23:34:04 PDT
Created attachment 99941 [details]
Patch V2
Comment 6 Kenichi Ishibashi 2011-07-06 23:36:57 PDT
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.
Comment 7 Kenichi Ishibashi 2011-07-13 22:30:19 PDT
(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 Kenichi Ishibashi 2011-07-20 19:45:53 PDT
Could someone take a look?

Thanks in advance.
Comment 9 Kenichi Ishibashi 2011-08-01 18:14:28 PDT
Created attachment 102606 [details]
Patch V3(Rebased)
Comment 10 Kenichi Ishibashi 2011-08-01 18:21:52 PDT
(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.
Comment 11 Kent Tamura 2011-08-01 21:47:24 PDT
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.
Comment 12 Kenichi Ishibashi 2011-08-01 22:53:46 PDT
Created attachment 102615 [details]
Patch V4
Comment 13 Kenichi Ishibashi 2011-08-01 22:55:16 PDT
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.
Comment 14 Shinichiro Hamaji 2011-08-02 04:57:19 PDT
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?
Comment 15 Kenichi Ishibashi 2011-08-02 07:43:37 PDT
Created attachment 102648 [details]
Patch V5
Comment 16 Kenichi Ishibashi 2011-08-02 07:45:31 PDT
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.
Comment 17 Yuzo Fujishima 2011-08-02 18:51:46 PDT
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?
Comment 18 Yuzo Fujishima 2011-08-02 19:17:17 PDT
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?
Comment 19 Kenichi Ishibashi 2011-08-03 02:09:18 PDT
Created attachment 102755 [details]
Patch V6
Comment 20 Kenichi Ishibashi 2011-08-03 02:18:29 PDT
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.
Comment 21 Kenichi Ishibashi 2011-08-03 02:22:07 PDT
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 Shinichiro Hamaji 2011-08-03 23:25:20 PDT
(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 Shinichiro Hamaji 2011-08-03 23:56:28 PDT
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.
Comment 24 Kenichi Ishibashi 2011-08-04 00:55:23 PDT
Created attachment 102879 [details]
Patch V7
Comment 25 Shinichiro Hamaji 2011-08-04 00:59:48 PDT
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.
Comment 26 Kenichi Ishibashi 2011-08-04 01:01:29 PDT
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.
Comment 27 Kenichi Ishibashi 2011-08-04 01:05:11 PDT
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!
Comment 28 Kenichi Ishibashi 2011-08-04 01:29:45 PDT
(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 Kenichi Ishibashi 2011-08-04 18:12:44 PDT
Created attachment 103013 [details]
Patch V8
Comment 30 Kenichi Ishibashi 2011-08-04 18:14:42 PDT
(In reply to comment #29)
> Created an attachment (id=103013) [details]
> Patch V8

Rebased to ToT and addressed Hamaji-san's comment.
Comment 31 Yuzo Fujishima 2011-08-04 18:19:55 PDT
Thank you for addressing my drive-by comments. I have no more comments.
Comment 32 Kenichi Ishibashi 2011-08-05 00:35:09 PDT
Comment on attachment 103013 [details]
Patch V8

Clearing flags on attachment: 103013

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