Move font shorthand handling into CSSPropertyParser

Move font shorthand handling into CSSPropertyParser

Now that all the needed longhand properties are converted, add logic
for the font shorthand from LegacyCSSPropertyParserinto CSSPropertyParser.

BUG=499780
Committed: https://crrev.com/6c4b3dfb01fa5ba86c39143076aa4712ea7b4029
Cr-Commit-Position: refs/heads/master@{#352762}

[rwlbuis, 2015-10-02 21:25:05]

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

[rwlbuis, 2015-10-02 21:25:05]

PTAL.

Some comments:
- font-shorthand.html fails because of a difference in logic parsing the
optional font longhands (font-weight etc.). Specifically the order of the 4
longhands is now fixed. I don't think it is a big deal but could be fixed.
- font_property_normal.html "fail" should actually be an improvement and matches
Firefox. Basically italic 'Arial' is no longer accepted as a family name. If
needed we can turn back to the old behavior though the code could become uglier.
- the first loop to check for initial and inherit is not pretty but probably
hard to avoid.
- we should have some rule about the code in parseShorthand. I think like in
parseSingleValue it makes sense to basically have a big switch/case with calls
to static methods. Putting the consumeFont inline in parseShorthand would make
it unreadable, so I think we better make parseShorthand more like
parseSingleValue.

[rwlbuis, 2015-10-04 22:03:03]

I forgot to mention, this has the ShorthandScope removed. AFAIK this is still
important for inspector. Should we have an extra addProperty with a param that
does what m_currentShorthand does now?

[Timothy Loh, 2015-10-06 02:24:39]

Parsing code OK since it matches existing behaviour.

I think we updated the StylePropertySerializer a while back to not look at the
implicit flag for font shorthands, so we can maybe just leave out that argument.

On 2015/10/04 22:03:03, rwlbuis wrote:
> I forgot to mention, this has the ShorthandScope removed. AFAIK this is still
> important for inspector. Should we have an extra addProperty with a param that
> does what m_currentShorthand does now?

How about always setting m_currentShorthand (m_currentProperty?) in
construction, or at the top of parseShorthand? addProperty (addLonghand?) won't
get called when we're parsing a non-longhand (aside from viewport/font-face
descriptors maybe, but those can get a separate function).

[rwlbuis, 2015-10-06 22:58:08]

On 2015/10/06 02:24:39, Timothy Loh wrote:
> Parsing code OK since it matches existing behaviour.
> 
> I think we updated the StylePropertySerializer a while back to not look at the
> implicit flag for font shorthands, so we can maybe just leave out that
argument.

I removed it and indeed everything still shows up green.

> On 2015/10/04 22:03:03, rwlbuis wrote:
> > I forgot to mention, this has the ShorthandScope removed. AFAIK this is
still
> > important for inspector. Should we have an extra addProperty with a param
that
> > does what m_currentShorthand does now?
> 
> How about always setting m_currentShorthand (m_currentProperty?) in
> construction, or at the top of parseShorthand? addProperty (addLonghand?)
won't
> get called when we're parsing a non-longhand (aside from viewport/font-face
> descriptors maybe, but those can get a separate function).

I went for setting it in parseShorthand. Note that it really has to be restored
at the end of that function or tests will fail :)

PTAL.

[Timothy Loh, 2015-10-06 23:45:05]

lgtm

https://codereview.chromium.org/1375753002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1375753002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:874:
addProperty(CSSPropertyLineHeight,
cssValuePool().createIdentifierValue(CSSValueNormal), important, true);
implicit value here can be removed too

https://codereview.chromium.org/1375753002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:893:
CSSPropertyID oldShorthand = m_currentShorthand;
// TODO(rob.buis): Remove this when the legacy property parser is gone

[rwlbuis, 2015-10-07 01:37:40]

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

[rwlbuis, 2015-10-07 01:37:40]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1375753002/#ps140001
(title: "Patch for landing")

[commit-bot: I haz the power, 2015-10-07 01:40:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1375753002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1375753002/140001

[commit-bot: I haz the power, 2015-10-07 02:06:17]

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

[commit-bot: I haz the power, 2015-10-07 02:06:18]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rwlbuis, 2015-10-07 02:43:29]

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

[commit-bot: I haz the power, 2015-10-07 02:45:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1375753002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1375753002/140001

[commit-bot: I haz the power, 2015-10-07 03:49:57]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-10-07 03:50:53]

Patchset 8 (id:??) landed as
https://crrev.com/6c4b3dfb01fa5ba86c39143076aa4712ea7b4029
Cr-Commit-Position: refs/heads/master@{#352762}