Implement parseShorthand API for shorthand properties, "overflow", "font" and "font-variant"

Implement parseShorthand API for shorthand properties, "overflow", "font" and "font-variant"

This cl contains the following changes
* API impl of the three shorthand properties (CSSShorthandPropertyAPIOverflow, 
 CSSShorthandPropertyAPIFont, CSSShorthandPropertyAPIFontVariant).

* Moved ConsumeFontVariantCSS21 from CSSPropertyParser.cpp to CSSPropertyFontUtils. 
- this function is being used by CSSShorthandPropertyAPIFont and also another function
from CSSPropertyParser.cpp.

* Changed CSSPropertyParser.cpp to use the new API parseShorthand for the three properties 
above. Additional changes are:
- moved two helper functions (ConsumeSystemFont and ConsumeFont) to be private functions of CSSShorthandPropertyAPIFont.
- removed ConsumeFontVariantShorthand that's no longer needed.

* Added the properties API to BUILD file and CSSProperties.json5.

* Changed AddProperty signature so that "implicit" is now an enum.
- We previously passed on bool literals to this function. This change makes the function follow the style guide.

Diff: https://gist.github.com/810c7e46324ff8db8efb6f34ddfdad42/revisions
Review-Url: https://codereview.chromium.org/2938983002
Cr-Commit-Position: refs/heads/master@{#481037}
Committed: https://chromium.googlesource.com/chromium/src/+/d061cbb5a9efad51a47fed2becf04e688ac51dea

[Jia, 2017-06-15 06:53:50]

Description was changed from

==========
Implement parseShorthand API for shorthand properties, "overflow", "font" and
"font-variant"

BUG=
==========

to

==========
Implement parseShorthand API for shorthand properties, "overflow", "font" and
"font-variant"

This cl contains the following changes
* API impl of the three shorthand properties (CSSShorthandPropertyAPIOverflow, 
 CSSShorthandPropertyAPIFont, CSSShorthandPropertyAPIFontVariant).

* Moved ConsumeFontVariantCSS21 from CSSPropertyParser.cpp to
CSSPropertyFontUtils. 
- this function is being used by CSSShorthandPropertyAPIFont and also another
function
from CSSPropertyParser.cpp.

* Changed CSSPropertyParser.cpp to use the new API parseShorthand for the three
properties 
above. Additional changes are:
- moved two helper functions (ConsumeSystemFont and ConsumeFont) to be private
functions of CSSShorthandPropertyAPIFont.
- removed ConsumeFontVariantShorthand that's no longer needed.

* Added the properties API to BUILD file and CSSProperties.json5.

Diff: https://gist.github.com/810c7e46324ff8db8efb6f34ddfdad42/revisions
BUG=668012
==========

[Jia, 2017-06-15 06:54:33]

jiameng@chromium.org changed reviewers:
+ bugsnash@chromium.org

[Jia, 2017-06-15 06:54:40]

The CQ bit was checked by jiameng@chromium.org

[commit-bot: I haz the power, 2017-06-15 06:54:54]

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

[commit-bot: I haz the power, 2017-06-15 06:54:55]

This CL has an open dependency (Issue 2930703005 Patch 40001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[Jia, 2017-06-15 22:22:10]

The CQ bit was checked by jiameng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-15 22:22:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-15 23:08:03]

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

[commit-bot: I haz the power, 2017-06-15 23:08:04]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Bugs Nash, 2017-06-16 03:19:01]

https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/css/properties/CSSShorthandPropertyAPIFont.cpp
(right):

https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/properties/CSSShorthandPropertyAPIFont.cpp:42:
important, false /* implicit */, properties);
avoid passing literal bools. I would either change this arg to an enum class or
make the implicit arg default to false like in AddParsedProperty (it would then
need to be the last arg, but I think this is a good enough reason to ignore the
outputs last guideline). I have a slight preference for the enum class solution

[Bugs Nash, 2017-06-16 03:51:51]

https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSProperties.json5 (right):

https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSProperties.json5:3291: api_class: true,
'api_class' and 'api_methods' should go after 'name' and before 'is_descriptor'
for consistency with longhands

https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:14: #include
"core/css/CSSFontFamilyValue.h"
this can be deleted now

https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:39: #include
"core/css/parser/FontVariantLigaturesParser.h"
this can be deleted now

https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:40: #include
"core/css/parser/FontVariantNumericParser.h"
this can be deleted now

https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/css/properties/CSSShorthandPropertyAPIFont.cpp
(right):

https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/properties/CSSShorthandPropertyAPIFont.cpp:18:
class CSSParserLocalContext;
not necessary as guaranteed to be in header file

https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/css/properties/CSSShorthandPropertyAPIFontVariant.cpp
(right):

https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/properties/CSSShorthandPropertyAPIFontVariant.cpp:14:
class CSSParserContext;
as above, these class declarations are guaranteed to be in the header file so
aren't needed here

https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/css/properties/CSSShorthandPropertyAPIOverflow.cpp
(right):

https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/properties/CSSShorthandPropertyAPIOverflow.cpp:14:
class CSSParserLocalContext;
as above

[Jia, 2017-06-19 03:54:41]

Thanks Bugs. PTAL.

https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSProperties.json5 (right):

https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSProperties.json5:3291: api_class: true,
On 2017/06/16 03:51:50, Bugs Nash wrote:
> 'api_class' and 'api_methods' should go after 'name' and before
'is_descriptor'
> for consistency with longhands

Done.

https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:14: #include
"core/css/CSSFontFamilyValue.h"
On 2017/06/16 03:51:50, Bugs Nash wrote:
> this can be deleted now

Done.

https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:39: #include
"core/css/parser/FontVariantLigaturesParser.h"
On 2017/06/16 03:51:50, Bugs Nash wrote:
> this can be deleted now

Done.

https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:40: #include
"core/css/parser/FontVariantNumericParser.h"
On 2017/06/16 03:51:50, Bugs Nash wrote:
> this can be deleted now

Done.

https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/css/properties/CSSShorthandPropertyAPIFont.cpp
(right):

https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/properties/CSSShorthandPropertyAPIFont.cpp:18:
class CSSParserLocalContext;
On 2017/06/16 03:51:51, Bugs Nash wrote:
> not necessary as guaranteed to be in header file

Done.

https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/properties/CSSShorthandPropertyAPIFont.cpp:42:
important, false /* implicit */, properties);
On 2017/06/16 03:19:01, Bugs Nash wrote:
> avoid passing literal bools. I would either change this arg to an enum class
or
> make the implicit arg default to false like in AddParsedProperty (it would
then
> need to be the last arg, but I think this is a good enough reason to ignore
the
> outputs last guideline). I have a slight preference for the enum class
solution

I can change the signature of AddProperty and replace two bool params (important
and implicit) by enums. I'll make them in another cl to avoid bundling up
unrelated changes.

https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/css/properties/CSSShorthandPropertyAPIFontVariant.cpp
(right):

https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/properties/CSSShorthandPropertyAPIFontVariant.cpp:14:
class CSSParserContext;
On 2017/06/16 03:51:51, Bugs Nash wrote:
> as above, these class declarations are guaranteed to be in the header file so
> aren't needed here

Done.

[Bugs Nash, 2017-06-19 04:29:18]

https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/css/properties/CSSShorthandPropertyAPIFont.cpp
(right):

https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/properties/CSSShorthandPropertyAPIFont.cpp:42:
important, false /* implicit */, properties);
On 2017/06/19 at 03:54:41, Jia wrote:
> On 2017/06/16 03:19:01, Bugs Nash wrote:
> > avoid passing literal bools. I would either change this arg to an enum class
or
> > make the implicit arg default to false like in AddParsedProperty (it would
then
> > need to be the last arg, but I think this is a good enough reason to ignore
the
> > outputs last guideline). I have a slight preference for the enum class
solution
> 
> I can change the signature of AddProperty and replace two bool params
(important and implicit) by enums. I'll make them in another cl to avoid
bundling up unrelated changes.

I wouldn't worry about changing the important bool. You're not passing in
literal bools for it here so you're not violating the style guide.

I disagree that changing the implicit argument to an enum is an unrelated
change, as this patch introduces passing literal bools that didn't exist before
(a violation of the style guide) so I believe this patch is the right place to
change it to an enum (this patch is the reason it's required). But it's up to
you, however if you do want to do it in a separate patch I think that patch
should land first to avoid the style guide violation.

[Jia, 2017-06-19 04:41:09]

On 2017/06/19 04:29:18, Bugs Nash wrote:
>
https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
> File
> third_party/WebKit/Source/core/css/properties/CSSShorthandPropertyAPIFont.cpp
> (right):
> 
>
https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/core/css/properties/CSSShorthandPropertyAPIFont.cpp:42:
> important, false /* implicit */, properties);
> On 2017/06/19 at 03:54:41, Jia wrote:
> > On 2017/06/16 03:19:01, Bugs Nash wrote:
> > > avoid passing literal bools. I would either change this arg to an enum
class
> or
> > > make the implicit arg default to false like in AddParsedProperty (it would
> then
> > > need to be the last arg, but I think this is a good enough reason to
ignore
> the
> > > outputs last guideline). I have a slight preference for the enum class
> solution
> > 
> > I can change the signature of AddProperty and replace two bool params
> (important and implicit) by enums. I'll make them in another cl to avoid
> bundling up unrelated changes.
> 
> I wouldn't worry about changing the important bool. You're not passing in
> literal bools for it here so you're not violating the style guide.
> 
> I disagree that changing the implicit argument to an enum is an unrelated
> change, as this patch introduces passing literal bools that didn't exist
before
> (a violation of the style guide) so I believe this patch is the right place to
> change it to an enum (this patch is the reason it's required). But it's up to
> you, however if you do want to do it in a separate patch I think that patch
> should land first to avoid the style guide violation.

First, I assume we're talking about the change in AddProperty signature, i.e.
replace "bool implicit" by something like "ImplicitProperty implicit".
I can leave "important" unchanged for now, but I suspect a request will come in
later to change it to enum too because 
(i). signatures look strange with one bool and one enum that is essentially a
bool. 
ii). we have other calling functions that will pass true/false as value of
"important".

Overall, I'm fine with keeping "important" a bool, as long as there's no
violation with style guide.

[Bugs Nash, 2017-06-19 05:05:23]

On 2017/06/19 at 04:41:09, jiameng wrote:
> On 2017/06/19 04:29:18, Bugs Nash wrote:
> >
https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
> > File
> >
third_party/WebKit/Source/core/css/properties/CSSShorthandPropertyAPIFont.cpp
> > (right):
> > 
> >
https://codereview.chromium.org/2938983002/diff/20001/third_party/WebKit/Sour...
> >
third_party/WebKit/Source/core/css/properties/CSSShorthandPropertyAPIFont.cpp:42:
> > important, false /* implicit */, properties);
> > On 2017/06/19 at 03:54:41, Jia wrote:
> > > On 2017/06/16 03:19:01, Bugs Nash wrote:
> > > > avoid passing literal bools. I would either change this arg to an enum
class
> > or
> > > > make the implicit arg default to false like in AddParsedProperty (it
would
> > then
> > > > need to be the last arg, but I think this is a good enough reason to
ignore
> > the
> > > > outputs last guideline). I have a slight preference for the enum class
> > solution
> > > 
> > > I can change the signature of AddProperty and replace two bool params
> > (important and implicit) by enums. I'll make them in another cl to avoid
> > bundling up unrelated changes.
> > 
> > I wouldn't worry about changing the important bool. You're not passing in
> > literal bools for it here so you're not violating the style guide.
> > 
> > I disagree that changing the implicit argument to an enum is an unrelated
> > change, as this patch introduces passing literal bools that didn't exist
before
> > (a violation of the style guide) so I believe this patch is the right place
to
> > change it to an enum (this patch is the reason it's required). But it's up
to
> > you, however if you do want to do it in a separate patch I think that patch
> > should land first to avoid the style guide violation.
> 
> First, I assume we're talking about the change in AddProperty signature, i.e.
replace "bool implicit" by something like "ImplicitProperty implicit".
> I can leave "important" unchanged for now, but I suspect a request will come
in later to change it to enum too because 
> (i). signatures look strange with one bool and one enum that is essentially a
bool. 
> ii). we have other calling functions that will pass true/false as value of
"important".
> 
> Overall, I'm fine with keeping "important" a bool, as long as there's no
violation with style guide.

Yes I'm talking about AddProperty :) The other thing about the important bool is
that it's used all over the place in the code, so changing it would be a big
job, definitely a separate CL. Feel free to do that if you like, but it's not
necessary for this patch to land.

[Jia, 2017-06-19 07:00:00]

Description was changed from

==========
Implement parseShorthand API for shorthand properties, "overflow", "font" and
"font-variant"

This cl contains the following changes
* API impl of the three shorthand properties (CSSShorthandPropertyAPIOverflow, 
 CSSShorthandPropertyAPIFont, CSSShorthandPropertyAPIFontVariant).

* Moved ConsumeFontVariantCSS21 from CSSPropertyParser.cpp to
CSSPropertyFontUtils. 
- this function is being used by CSSShorthandPropertyAPIFont and also another
function
from CSSPropertyParser.cpp.

* Changed CSSPropertyParser.cpp to use the new API parseShorthand for the three
properties 
above. Additional changes are:
- moved two helper functions (ConsumeSystemFont and ConsumeFont) to be private
functions of CSSShorthandPropertyAPIFont.
- removed ConsumeFontVariantShorthand that's no longer needed.

* Added the properties API to BUILD file and CSSProperties.json5.

Diff: https://gist.github.com/810c7e46324ff8db8efb6f34ddfdad42/revisions
BUG=668012
==========

to

==========
Implement parseShorthand API for shorthand properties, "overflow", "font" and
"font-variant"

This cl contains the following changes
* API impl of the three shorthand properties (CSSShorthandPropertyAPIOverflow, 
 CSSShorthandPropertyAPIFont, CSSShorthandPropertyAPIFontVariant).

* Moved ConsumeFontVariantCSS21 from CSSPropertyParser.cpp to
CSSPropertyFontUtils. 
- this function is being used by CSSShorthandPropertyAPIFont and also another
function
from CSSPropertyParser.cpp.

* Changed CSSPropertyParser.cpp to use the new API parseShorthand for the three
properties 
above. Additional changes are:
- moved two helper functions (ConsumeSystemFont and ConsumeFont) to be private
functions of CSSShorthandPropertyAPIFont.
- removed ConsumeFontVariantShorthand that's no longer needed.

* Added the properties API to BUILD file and CSSProperties.json5.

* Changed AddProperty signature so that "implicit" is now an enum.
- We previously passed on bool literals to this function. This change makes the
function follow the style guide.

Diff: https://gist.github.com/810c7e46324ff8db8efb6f34ddfdad42/revisions
BUG=668012
==========

[Jia, 2017-06-19 07:00:58]

Thanks for the review. I've replaced bool "implicit" by an enum. I've also
updated the description of this cl. PTAL.

[Jia, 2017-06-19 09:37:37]

The CQ bit was checked by jiameng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-19 09:37:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-19 09:39:44]

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

[commit-bot: I haz the power, 2017-06-19 09:39:45]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Jia, 2017-06-19 23:10:26]

The CQ bit was checked by jiameng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-19 23:11:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-19 23:15:10]

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

[commit-bot: I haz the power, 2017-06-19 23:15:11]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Bugs Nash, 2017-06-19 23:30:12]

lgtm with nit

https://codereview.chromium.org/2938983002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/2938983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:71: bool
implicit) {
just for documentation I want to note that this bool here is not being changed
because this method will be removed soon

https://codereview.chromium.org/2938983002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h
(right):

https://codereview.chromium.org/2938983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:114: enum
ImplicitProperty { kNotImplicit, kImplicit };
nit: enum class is safer as it's type safe. especially if the important argument
is changed to an enum later, you don't want people being able to accidentally
pass the arguments in the wrong order. with an enum class you need to include
the prefix though, so I would shorten the enum name to Implicit.

[Jia, 2017-06-19 23:31:01]

The CQ bit was checked by jiameng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-19 23:31:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-19 23:34:45]

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

[commit-bot: I haz the power, 2017-06-19 23:34:46]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Jia, 2017-06-20 01:05:30]

The CQ bit was checked by jiameng@chromium.org to run a CQ dry run

[Jia, 2017-06-20 01:05:51]

Thanks for the review! PTAL.

https://codereview.chromium.org/2938983002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/2938983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:71: bool
implicit) {
On 2017/06/19 23:30:12, Bugs Nash wrote:
> just for documentation I want to note that this bool here is not being changed
> because this method will be removed soon

Done.

https://codereview.chromium.org/2938983002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h
(right):

https://codereview.chromium.org/2938983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:114: enum
ImplicitProperty { kNotImplicit, kImplicit };
On 2017/06/19 23:30:12, Bugs Nash wrote:
> nit: enum class is safer as it's type safe. especially if the important
argument
> is changed to an enum later, you don't want people being able to accidentally
> pass the arguments in the wrong order. with an enum class you need to include
> the prefix though, so I would shorten the enum name to Implicit.

I've changed it to enum class. However, I think ImplicitProperty is clearer in
meaning (we know what is implicit).

[commit-bot: I haz the power, 2017-06-20 01:05:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bugs Nash, 2017-06-20 01:18:41]

lgtm

[Jia, 2017-06-20 01:26:32]

jiameng@chromium.org changed reviewers:
+ alancutter@chromium.org

[alancutter (OOO until 2018), 2017-06-20 01:50:45]

https://codereview.chromium.org/2938983002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h
(right):

https://codereview.chromium.org/2938983002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:114: enum
class ImplicitProperty { kNotImplicit, kImplicit };
Minor bikeshed: Maybe rename as IsImplicitProperty for clarity that it's
representing a boolean state?

[Jia, 2017-06-20 03:29:45]

Thanks Alan. PTAL.

https://codereview.chromium.org/2938983002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h
(right):

https://codereview.chromium.org/2938983002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:114: enum
class ImplicitProperty { kNotImplicit, kImplicit };
On 2017/06/20 01:50:44, alancutter wrote:
> Minor bikeshed: Maybe rename as IsImplicitProperty for clarity that it's
> representing a boolean state?

Done.

[Jia, 2017-06-20 03:30:22]

The CQ bit was checked by jiameng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 03:30:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-20 05:30:05]

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

[commit-bot: I haz the power, 2017-06-20 05:30:07]

Dry run: This issue passed the CQ dry run.

[alancutter (OOO until 2018), 2017-06-20 23:58:38]

lgtm

[Jia, 2017-06-20 23:59:11]

The CQ bit was checked by jiameng@chromium.org

[Jia, 2017-06-20 23:59:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
bugsnash@chromium.org
Link to the patchset: https://codereview.chromium.org/2938983002/#ps140001
(title: "Replace ImplicitProperty by IsImplicitProperty")

[commit-bot: I haz the power, 2017-06-20 23:59:45]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 00:07:15]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1498003151552140,
"parent_rev": "7ea51c20d791547a295d548957bcdae117cdf7c8", "commit_rev":
"d061cbb5a9efad51a47fed2becf04e688ac51dea"}

[commit-bot: I haz the power, 2017-06-21 00:07:33]

Description was changed from

==========
Implement parseShorthand API for shorthand properties, "overflow", "font" and
"font-variant"

This cl contains the following changes
* API impl of the three shorthand properties (CSSShorthandPropertyAPIOverflow, 
 CSSShorthandPropertyAPIFont, CSSShorthandPropertyAPIFontVariant).

* Moved ConsumeFontVariantCSS21 from CSSPropertyParser.cpp to
CSSPropertyFontUtils. 
- this function is being used by CSSShorthandPropertyAPIFont and also another
function
from CSSPropertyParser.cpp.

* Changed CSSPropertyParser.cpp to use the new API parseShorthand for the three
properties 
above. Additional changes are:
- moved two helper functions (ConsumeSystemFont and ConsumeFont) to be private
functions of CSSShorthandPropertyAPIFont.
- removed ConsumeFontVariantShorthand that's no longer needed.

* Added the properties API to BUILD file and CSSProperties.json5.

* Changed AddProperty signature so that "implicit" is now an enum.
- We previously passed on bool literals to this function. This change makes the
function follow the style guide.

Diff: https://gist.github.com/810c7e46324ff8db8efb6f34ddfdad42/revisions
BUG=668012
==========

to

==========
Implement parseShorthand API for shorthand properties, "overflow", "font" and
"font-variant"

This cl contains the following changes
* API impl of the three shorthand properties (CSSShorthandPropertyAPIOverflow, 
 CSSShorthandPropertyAPIFont, CSSShorthandPropertyAPIFontVariant).

* Moved ConsumeFontVariantCSS21 from CSSPropertyParser.cpp to
CSSPropertyFontUtils. 
- this function is being used by CSSShorthandPropertyAPIFont and also another
function
from CSSPropertyParser.cpp.

* Changed CSSPropertyParser.cpp to use the new API parseShorthand for the three
properties 
above. Additional changes are:
- moved two helper functions (ConsumeSystemFont and ConsumeFont) to be private
functions of CSSShorthandPropertyAPIFont.
- removed ConsumeFontVariantShorthand that's no longer needed.

* Added the properties API to BUILD file and CSSProperties.json5.

* Changed AddProperty signature so that "implicit" is now an enum.
- We previously passed on bool literals to this function. This change makes the
function follow the style guide.

Diff: https://gist.github.com/810c7e46324ff8db8efb6f34ddfdad42/revisions
Review-Url: https://codereview.chromium.org/2938983002
Cr-Commit-Position: refs/heads/master@{#481037}
Committed:
https://chromium.googlesource.com/chromium/src/+/d061cbb5a9efad51a47fed2becf0...
==========

[commit-bot: I haz the power, 2017-06-21 00:07:35]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/d061cbb5a9efad51a47fed2becf0...