Expand system font values during font property parsing

Expand system font values during font property parsing

This patch removes special case handling of the font shorthand property.
Previously this property would be carried through to the style resolving
stage if system font values were used. This patch refactors this process
to happen in the CSS parser so that font is always expanded into longhand
values.

BUG=353932

[dglazkov, 2014-03-20 05:24:43]

lgtm

https://codereview.chromium.org/205743004/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderTheme.h (right):

https://codereview.chromium.org/205743004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderTheme.h:149: virtual void systemFont(CSSValueID
systemFontID, FontStyle&, FontWeight&, float& fontSize, AtomicString&
fontFamily) const = 0;
I am curious why you cracked it open like that. Why not use FontDescription?

[alancutter (OOO until 2018), 2014-03-20 05:34:12]

https://codereview.chromium.org/205743004/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderTheme.h (right):

https://codereview.chromium.org/205743004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderTheme.h:149: virtual void systemFont(CSSValueID
systemFontID, FontStyle&, FontWeight&, float& fontSize, AtomicString&
fontFamily) const = 0;
On 2014/03/20 05:24:44, dglazkov wrote:
> I am curious why you cracked it open like that. Why not use FontDescription?

Splitting the components out ensures the font style that the RenderThemes decide
upon maps cleanly to the font longhand properties, otherwise it's not clear
whether we're capturing the appropriate values off the FontDescription in the
CSSPropertyParser.

[alancutter (OOO until 2018), 2014-03-20 06:03:20]

The CQ bit was checked by alancutter@chromium.org

[commit-bot: I haz the power, 2014-03-20 06:03:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alancutter@chromium.org/205743004/20001

[commit-bot: I haz the power, 2014-03-20 06:18:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 06:18:26]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[Timothy Loh, 2014-03-20 06:19:09]

The CQ bit was checked by timloh@chromium.org

[commit-bot: I haz the power, 2014-03-20 06:19:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alancutter@chromium.org/205743004/20001

[commit-bot: I haz the power, 2014-03-20 06:44:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 06:44:10]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[Timothy Loh, 2014-03-20 06:44:47]

The CQ bit was checked by timloh@chromium.org

[commit-bot: I haz the power, 2014-03-20 06:44:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alancutter@chromium.org/205743004/20001

[commit-bot: I haz the power, 2014-03-20 07:24:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 07:24:15]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[alancutter (OOO until 2018), 2014-03-20 12:31:00]

The CQ bit was checked by alancutter@chromium.org

[commit-bot: I haz the power, 2014-03-20 12:31:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alancutter@chromium.org/205743004/20001

[commit-bot: I haz the power, 2014-03-20 13:10:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 13:10:55]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[apavlov, 2014-03-20 14:48:23]

The CQ bit was checked by apavlov@chromium.org

[commit-bot: I haz the power, 2014-03-20 14:48:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alancutter@chromium.org/205743004/20001

[apavlov, 2014-03-20 15:23:18]

The CQ bit was unchecked by apavlov@chromium.org

[apavlov, 2014-03-20 15:27:48]

Incidentally, I tried to land a similar patch (but without the theme
refactoring) an hour ago, that's why I'm here :)

https://codereview.chromium.org/205743004/diff/20001/LayoutTests/fast/css/fon...
File LayoutTests/fast/css/font-shorthand.html (right):

https://codereview.chromium.org/205743004/diff/20001/LayoutTests/fast/css/fon...
LayoutTests/fast/css/font-shorthand.html:50: showFontLonghands('12px/italic
serif');
This test is missing invalid "font" values involving system font names (should
you also test -webkit-mini-control, -webkit-small-control and -webkit-control?)
Feel free to borrow test cases from my patch at
https://codereview.chromium.org/150333003/diff/100001/LayoutTests/fast/css/fo...

https://codereview.chromium.org/205743004/diff/20001/Source/core/css/parser/C...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/205743004/diff/20001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:1417: if (id >= CSSValueCaption &&
id <= CSSValueStatusBar) {
This condition is missing "num == 1" - otherwise we will successfully parse
props like "font: menu 12px Tahoma" and similar.

[alancutter (OOO until 2018), 2014-03-21 02:28:31]

On 2014/03/20 15:27:48, apavlov wrote:
> Incidentally, I tried to land a similar patch (but without the theme
> refactoring) an hour ago, that's why I'm here :)

Crazy coincidence. :)

>
https://codereview.chromium.org/205743004/diff/20001/LayoutTests/fast/css/fon...
> File LayoutTests/fast/css/font-shorthand.html (right):
> 
>
https://codereview.chromium.org/205743004/diff/20001/LayoutTests/fast/css/fon...
> LayoutTests/fast/css/font-shorthand.html:50: showFontLonghands('12px/italic
> serif');
> This test is missing invalid "font" values involving system font names (should
> you also test -webkit-mini-control, -webkit-small-control and
-webkit-control?)
> Feel free to borrow test cases from my patch at
>
https://codereview.chromium.org/150333003/diff/100001/LayoutTests/fast/css/fo...

Thanks! Done.

>
https://codereview.chromium.org/205743004/diff/20001/Source/core/css/parser/C...
> File Source/core/css/parser/CSSPropertyParser.cpp (right):
> 
>
https://codereview.chromium.org/205743004/diff/20001/Source/core/css/parser/C...
> Source/core/css/parser/CSSPropertyParser.cpp:1417: if (id >= CSSValueCaption
&&
> id <= CSSValueStatusBar) {
> This condition is missing "num == 1" - otherwise we will successfully parse
> props like "font: menu 12px Tahoma" and similar.

This is what I get for not having enough tests, done. :)

[apavlov, 2014-03-21 08:06:55]

On 2014/03/21 02:28:31, alancutter wrote:
> On 2014/03/20 15:27:48, apavlov wrote:
> > Incidentally, I tried to land a similar patch (but without the theme
> > refactoring) an hour ago, that's why I'm here :)
> 
> Crazy coincidence. :)

Indeed! But I had had this patch in the works for a month and a half (darktears
was on vacation)!

> > Feel free to borrow test cases from my patch at
> >
>
https://codereview.chromium.org/150333003/diff/100001/LayoutTests/fast/css/fo...
> 
> Thanks! Done.
> 
>
https://codereview.chromium.org/205743004/diff/20001/Source/core/css/parser/C...
> > Source/core/css/parser/CSSPropertyParser.cpp:1417: if (id >= CSSValueCaption
> &&
> > id <= CSSValueStatusBar) {
> > This condition is missing "num == 1" - otherwise we will successfully parse
> > props like "font: menu 12px Tahoma" and similar.
> 
> This is what I get for not having enough tests, done. :)

Awesome, lgtm!

[apavlov, 2014-03-21 08:07:14]

The CQ bit was checked by apavlov@chromium.org

[commit-bot: I haz the power, 2014-03-21 08:07:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alancutter@chromium.org/205743004/60001

[alancutter (OOO until 2018), 2014-03-21 08:21:29]

The changes to Mac have been causing problems for Web Workers, they don't seem
to be able to read system font information. Strangely though I haven't been able
to repro it locally on my Mac 10.9. It appears this problem may have previously
been covered up by having the FontDescriptions cached so I've reintroduced
caching in a recent patchset.

[apavlov, 2014-03-21 08:24:23]

On 2014/03/21 08:21:29, alancutter wrote:
> The changes to Mac have been causing problems for Web Workers, they don't seem
> to be able to read system font information. Strangely though I haven't been
able
> to repro it locally on my Mac 10.9. It appears this problem may have
previously
> been covered up by having the FontDescriptions cached so I've reintroduced
> caching in a recent patchset.

Uh-oh, I see. Thanks for dealing with this.

[apavlov, 2014-03-21 09:32:03]

On 2014/03/21 08:24:23, apavlov wrote:
> On 2014/03/21 08:21:29, alancutter wrote:
> > The changes to Mac have been causing problems for Web Workers, they don't
seem
> > to be able to read system font information. Strangely though I haven't been
> able
> > to repro it locally on my Mac 10.9. It appears this problem may have
> previously
> > been covered up by having the FontDescriptions cached so I've reintroduced
> > caching in a recent patchset.

Still Mac-related timeouts?

[alancutter (OOO until 2018), 2014-03-21 13:20:52]

On 2014/03/21 09:32:03, apavlov wrote:
> On 2014/03/21 08:24:23, apavlov wrote:
> > On 2014/03/21 08:21:29, alancutter wrote:
> > > The changes to Mac have been causing problems for Web Workers, they don't
> seem
> > > to be able to read system font information. Strangely though I haven't
been
> > able
> > > to repro it locally on my Mac 10.9. It appears this problem may have
> > previously
> > > been covered up by having the FontDescriptions cached so I've reintroduced
> > > caching in a recent patchset.
> 
> Still Mac-related timeouts?

Indeed, I'm very tempted to say let's just go with your patch however I see it
is also failing on the same set of worker tests for mac_blink_rel and yet it
doesn't touch that side of the code at all. Quite perplexing!

[alancutter (OOO until 2018), 2014-03-22 03:06:17]

On 2014/03/21 13:20:52, alancutter wrote:
> On 2014/03/21 09:32:03, apavlov wrote:
> > On 2014/03/21 08:24:23, apavlov wrote:
> > > On 2014/03/21 08:21:29, alancutter wrote:
> > > > The changes to Mac have been causing problems for Web Workers, they
don't
> > seem
> > > > to be able to read system font information. Strangely though I haven't
> been
> > > able
> > > > to repro it locally on my Mac 10.9. It appears this problem may have
> > > previously
> > > > been covered up by having the FontDescriptions cached so I've
reintroduced
> > > > caching in a recent patchset.
> > 
> > Still Mac-related timeouts?
> 
> Indeed, I'm very tempted to say let's just go with your patch however I see it
> is also failing on the same set of worker tests for mac_blink_rel and yet it
> doesn't touch that side of the code at all. Quite perplexing!

We use font: -webkit-small-control in our html.css for input type elements, this
seems to cause Web Workers to hit the call to systemFont() and die.
There are a few options from here:
 - Make system font calls work in worker threads on the Mac trybots
 - Avoid parsing the user agent stylesheet in worker threads
 - Allow the same set of system font keywords in the longhand properties,
Firefox basically does this (http://jsbin.com/cefuyodo/1/)

[apavlov, 2014-04-21 07:02:40]

On 2014/03/22 03:06:17, alancutter wrote:
> On 2014/03/21 13:20:52, alancutter wrote:
> > On 2014/03/21 09:32:03, apavlov wrote:
> > > On 2014/03/21 08:24:23, apavlov wrote:
> > > > On 2014/03/21 08:21:29, alancutter wrote:
> > > > > The changes to Mac have been causing problems for Web Workers, they
> don't
> > > seem
> > > > > to be able to read system font information. Strangely though I haven't
> > been
> > > > able
> > > > > to repro it locally on my Mac 10.9. It appears this problem may have
> > > > previously
> > > > > been covered up by having the FontDescriptions cached so I've
> reintroduced
> > > > > caching in a recent patchset.
> > > 
> > > Still Mac-related timeouts?
> > 
> > Indeed, I'm very tempted to say let's just go with your patch however I see
it
> > is also failing on the same set of worker tests for mac_blink_rel and yet it
> > doesn't touch that side of the code at all. Quite perplexing!
> 
> We use font: -webkit-small-control in our html.css for input type elements,
this
> seems to cause Web Workers to hit the call to systemFont() and die.
> There are a few options from here:
>  - Make system font calls work in worker threads on the Mac trybots
>  - Avoid parsing the user agent stylesheet in worker threads
>  - Allow the same set of system font keywords in the longhand properties,
> Firefox basically does this (http://jsbin.com/cefuyodo/1/)

Hey Alan,

Any decision or progress on this?

[alancutter (OOO until 2018), 2014-04-24 08:10:02]

On 2014/04/21 07:02:40, apavlov wrote:
> 
> Hey Alan,
> 
> Any decision or progress on this?

I think the route of avoiding parsing the user agent stylesheet for shared
worker threads is the best approach here.
I had a quick attempt of doing so but ended up putting this on the back burner.
This is all one big yak shave that spawned from "I don't like the name
isExpandedShorthand()".

[apavlov, 2014-04-24 08:19:01]

On 2014/04/24 08:10:02, alancutter wrote:
> On 2014/04/21 07:02:40, apavlov wrote:
> > 
> > Hey Alan,
> > 
> > Any decision or progress on this?
> 
> I think the route of avoiding parsing the user agent stylesheet for shared
> worker threads is the best approach here.
> I had a quick attempt of doing so but ended up putting this on the back
burner.
> This is all one big yak shave that spawned from "I don't like the name
> isExpandedShorthand()".

When I hear the word "shorthand," I feel like looping in Alexis Menard (he's on
the cc list for this issue) :)

[apavlov, 2014-10-31 12:34:23]

What should we do about this patch? Is it already obsolete due to the recent
work on shorthands?

[Timothy Loh, 2014-10-31 22:52:59]

On 2014/10/31 12:34:23, apavlov wrote:
> What should we do about this patch? Is it already obsolete due to the recent
> work on shorthands?

andersr recently landed this https://codereview.chromium.org/677103002/,
closing.