Add initial/inherit/unset tests to font-family-parse-keyword.html

Add initial/inherit/unset tests to font-family-parse-keyword.html

In order to make the 'unset' case work add CSSValueUnset to the
isCSSWideKeyword helper function.

BUG=389148
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192394

[rwlbuis, 2015-03-20 17:06:27]

rob.buis@samsung.com changed reviewers:
+ timloh@chromium.org

[rwlbuis, 2015-03-20 17:06:28]

PTAL. Not really for review yet as we probably should know how to treat 'unset'
first. I can't find anything about it in the specs.
Another thing we could do is just copy font-family-parse-keyword.html for
initial/inherit/unset and just replace default with initial/inherit/unset
respectively.

[Timothy Loh, 2015-03-22 23:43:54]

On 2015/03/20 17:06:28, rwlbuis wrote:
> PTAL. Not really for review yet as we probably should know how to treat
'unset'
> first. I can't find anything about it in the specs.
> Another thing we could do is just copy font-family-parse-keyword.html for
> initial/inherit/unset and just replace default with initial/inherit/unset
> respectively.

We probably need to update isCSSWideKeyword.

http://dev.w3.org/csswg/css-values/#custom-idents
http://dev.w3.org/csswg/css-values/#common-keywords

[rwlbuis, 2015-03-23 17:09:16]

Patchset #2 (id:20001) has been deleted

[rwlbuis, 2015-03-23 21:48:02]

On 2015/03/22 23:43:54, Timothy Loh wrote:
> On 2015/03/20 17:06:28, rwlbuis wrote:
> > PTAL. Not really for review yet as we probably should know how to treat
> 'unset'
> > first. I can't find anything about it in the specs.
> > Another thing we could do is just copy font-family-parse-keyword.html for
> > initial/inherit/unset and just replace default with initial/inherit/unset
> > respectively.
> 
> We probably need to update isCSSWideKeyword.
> 
> http://dev.w3.org/csswg/css-values/#custom-idents
> http://dev.w3.org/csswg/css-values/#common-keywords

I added 'unset' to isCSSWideKeyword and adapted the test.

Note that currently bar unset would do the right thing but unset bar not. This
is because of the early inherit/initial/unset checks in
CSSPropertyParser::parseValue. Same thing with initial/inherit but default of
course works as before. Therefore I did not add those to the test.

[Timothy Loh, 2015-03-23 22:24:18]

On 2015/03/23 21:48:02, rwlbuis wrote:
> Note that currently bar unset would do the right thing but unset bar not. This
> is because of the early inherit/initial/unset checks in
> CSSPropertyParser::parseValue. Same thing with initial/inherit but default of
> course works as before. Therefore I did not add those to the test.

OK, lgtm.

[rwlbuis, 2015-03-23 22:28:57]

On 2015/03/23 22:24:18, Timothy Loh wrote:
> On 2015/03/23 21:48:02, rwlbuis wrote:
> > Note that currently bar unset would do the right thing but unset bar not.
This
> > is because of the early inherit/initial/unset checks in
> > CSSPropertyParser::parseValue. Same thing with initial/inherit but default
of
> > course works as before. Therefore I did not add those to the test.
> 
> OK, lgtm.

Thanks!

I could work on that btw. I forgot to ask, does the new CSS Parser provide
different tokens here in any way to detect something like inherit bar is not a
CSSValueInherit plus another CSS_IDENT? Does the spec force us to do something
specific here? I noticed inherit bar would be discarded in FireFox which seems
like the right thing to do.

[rwlbuis, 2015-03-23 22:31:32]

On 2015/03/23 22:28:57, rwlbuis wrote:
> On 2015/03/23 22:24:18, Timothy Loh wrote:
> > On 2015/03/23 21:48:02, rwlbuis wrote:
> > > Note that currently bar unset would do the right thing but unset bar not.
> This
> > > is because of the early inherit/initial/unset checks in
> > > CSSPropertyParser::parseValue. Same thing with initial/inherit but default
> of
> > > course works as before. Therefore I did not add those to the test.
> > 
> > OK, lgtm.
> 
> Thanks!
> 
> I could work on that btw. I forgot to ask, does the new CSS Parser provide
> different tokens here in any way to detect something like inherit bar is not a
> CSSValueInherit plus another CSS_IDENT? Does the spec force us to do something
> specific here? I noticed inherit bar would be discarded in FireFox which seems
> like the right thing to do.

Sorry, scratch that, I meant it actually correctly matches the font-face rules,
i.e. like default foo and bar default match, so will inherit foo and bar inherit
work in FF.

[rwlbuis, 2015-03-23 22:33:16]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2015-03-23 22:33:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1020093004/40001

[Timothy Loh, 2015-03-23 22:44:14]

On 2015/03/23 22:28:57, rwlbuis wrote:
> On 2015/03/23 22:24:18, Timothy Loh wrote:
> > On 2015/03/23 21:48:02, rwlbuis wrote:
> > > Note that currently bar unset would do the right thing but unset bar not.
> This
> > > is because of the early inherit/initial/unset checks in
> > > CSSPropertyParser::parseValue. Same thing with initial/inherit but default
> of
> > > course works as before. Therefore I did not add those to the test.
> > 
> > OK, lgtm.
> 
> Thanks!
> 
> I could work on that btw. I forgot to ask, does the new CSS Parser provide
> different tokens here in any way to detect something like inherit bar is not a
> CSSValueInherit plus another CSS_IDENT? Does the spec force us to do something
> specific here? I noticed inherit bar would be discarded in FireFox which seems
> like the right thing to do.

I think problem is just the (num != 1) size check, there's probably nothing
fundamentally broken. Eventually the property parser will be using the new
CSSParserTokens instead of CSSParserValues, but this is a pretty big change and
won't happen for a while (we just convert to the old tokens for now).

[commit-bot: I haz the power, 2015-03-23 23:58:27]

Committed patchset #2 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192394