Move font-family check to CSSPropertyParser

Move font-family check to CSSPropertyParser

Move font-family check to CSSPropertyParser since when parsing @font-face
any font-family with more than one family-name is invalid. So in that case
drop that font-family declaration but allow any valid declarations in the
font-face rule. Add a test for this new behavior.

Behavior matches Firefox.

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

[rwlbuis, 2015-04-29 22:32:40]

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

[rwlbuis, 2015-04-29 22:32:40]

PTAL, this goes together with:
https://codereview.chromium.org/1103273010/

PS: I think the old bison parser has some regressions compared to the new
parser, I don't think it is this patch...

[Timothy Loh, 2015-04-30 00:37:36]

I don't get this, why don't we just move the check to parseFontFaceProperty?

[rwlbuis, 2015-04-30 00:47:17]

On 2015/04/30 00:37:36, Timothy Loh wrote:
> I don't get this, why don't we just move the check to parseFontFaceProperty?

Because AFAICS it only handles one property at the time, whereas this check
needs to be done at the end of the declaration block.

[rwlbuis, 2015-04-30 00:50:36]

On 2015/04/30 00:47:17, rwlbuis wrote:
> On 2015/04/30 00:37:36, Timothy Loh wrote:
> > I don't get this, why don't we just move the check to parseFontFaceProperty?
> 
> Because AFAICS it only handles one property at the time, whereas this check
> needs to be done at the end of the declaration block.

Note that this patch is a bit optional but could help
https://codereview.chromium.org/1103273010/. That one is more important.

[Timothy Loh, 2015-04-30 00:53:42]

On 2015/04/30 00:47:17, rwlbuis wrote:
> On 2015/04/30 00:37:36, Timothy Loh wrote:
> > I don't get this, why don't we just move the check to parseFontFaceProperty?
> 
> Because AFAICS it only handles one property at the time, whereas this check
> needs to be done at the end of the declaration block.

http://dev.w3.org/csswg/css-fonts/#font-family-prop
http://dev.w3.org/csswg/css-fonts/#font-family-desc

This check is just trying to make it so we only allow values allows in the
font-family descriptor. We should just make the property (descriptor?) parser
disallow them.

[rwlbuis, 2015-04-30 01:09:30]

On 2015/04/30 00:53:42, Timothy Loh wrote:
> On 2015/04/30 00:47:17, rwlbuis wrote:
> > On 2015/04/30 00:37:36, Timothy Loh wrote:
> > > I don't get this, why don't we just move the check to
parseFontFaceProperty?
> > 
> > Because AFAICS it only handles one property at the time, whereas this check
> > needs to be done at the end of the declaration block.
> 
> http://dev.w3.org/csswg/css-fonts/#font-family-prop
> http://dev.w3.org/csswg/css-fonts/#font-family-desc
> 
> This check is just trying to make it so we only allow values allows in the
> font-family descriptor. We should just make the property (descriptor?) parser
> disallow them.

Ah! I probably got confused by:
// FIXME: Shouldn't we fail if font-family or src aren't specified?

But we don't enforce that, so the loop is not about that but just checking
for any font-family prop. So I agree, that could go into CSSPropertyParser,
however maybe after parseFontFaceProperty is introduced in the first place.
Thanks!

[rwlbuis, 2015-04-30 21:59:24]

On 2015/04/29 22:32:40, rwlbuis wrote:
> PTAL, this goes together with:
> https://codereview.chromium.org/1103273010/
> 
> PS: I think the old bison parser has some regressions compared to the new
> parser, I don't think it is this patch...

@timloh, disregard this, I am assuming quite some are known regressions now I
saw:
https://codereview.chromium.org/1015303002

Still, it is really hard to maintain the old parser and get notified of real
regressions there :(

[rwlbuis, 2015-04-30 22:06:17]

PTAL. Note that the behavior for the old parser is unchanged, so the test will
fail there.

[Timothy Loh, 2015-05-01 03:48:31]

On 2015/04/30 22:06:17, rwlbuis wrote:
> PTAL. Note that the behavior for the old parser is unchanged, so the test will
> fail there.

lgtm

[rwlbuis, 2015-05-01 14:16:16]

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

[commit-bot: I haz the power, 2015-05-01 14:16:28]

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

[commit-bot: I haz the power, 2015-05-01 16:28:26]

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