Bug 63618

Created attachment 99063 [details] WIP Patch V0

(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.

Created attachment 99603 [details] Patch V1

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.

Created attachment 99941 [details] Patch V2

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.

(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,

Could someone take a look? Thanks in advance.

Created attachment 102606 [details] Patch V3(Rebased)

(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 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.

Created attachment 102615 [details] Patch V4

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 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?

Created attachment 102648 [details] Patch V5

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 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 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?

Created attachment 102755 [details] Patch V6

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.

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.

(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 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.

Created attachment 102879 [details] Patch V7

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 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 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!

(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.

Created attachment 103013 [details] Patch V8

(In reply to comment #29) > Created an attachment (id=103013) [details] > Patch V8 Rebased to ToT and addressed Hamaji-san's comment.

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

Comment on attachment 103013 [details] Patch V8 Clearing flags on attachment: 103013 Committed r92457: <http://trac.webkit.org/changeset/92457>

All reviewed patches have been landed. Closing bug.

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).

(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.