[webkit-reviews] review denied: [Bug 63618] Parsing CSS3 font-feature-settings property : [Attachment 102615] Patch V4

[bugzilla-daemon at webkit.org]

Shinichiro Hamaji <hamaji at chromium.org> has denied Kenichi Ishibashi
<bashi at chromium.org>'s request for review:
Bug 63618: Parsing CSS3 font-feature-settings property
https://bugs.webkit.org/show_bug.cgi?id=63618

Attachment 102615: Patch V4
https://bugs.webkit.org/attachment.cgi?id=102615&action=review

------- Additional Comments from Shinichiro Hamaji <hamaji at chromium.org>
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().makeNormalFeatureSettin
gs()))

CSSStyleSelector::setFontDescription automatically updates m_fontDirty?

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

Use CSSStyleSelector::setFontDescription?