Added CSSPropertyCounterUtils which holds shared counter parsing logic.

Added CSSPropertyCounterUtils which holds shared counter parsing logic.

This patch:
- Added CSSPropertyCounterUtils
- Moved consumeCounter method from CSSPropertyParser to
  CSSPropertyCounterUtils
- Replaced the use of raw int in argument to consumeCounter with more
  readable named enum values

This is pre work to move parseSingleValue logic from CSSPropertyParser
to the property APIs for CounterIncrement and CounterReset properties.

BUG=668012
Review-Url: https://codereview.chromium.org/2772263004
Cr-Commit-Position: refs/heads/master@{#460019}
Committed: https://chromium.googlesource.com/chromium/src/+/45a8388f50dc32a97316656fe3170329fe5e1c6a

[Bugs Nash, 2017-03-28 01:14:10]

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

[commit-bot: I haz the power, 2017-03-28 01:14:33]

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-03-28 01:15:35]

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

[shend, 2017-03-28 01:58:48]

lgtm with nits.

https://codereview.chromium.org/2772263004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/properties/CSSPropertyCounterUtils.cpp
(right):

https://codereview.chromium.org/2772263004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/properties/CSSPropertyCounterUtils.cpp:26:
int i = static_cast<int>(defaultValue);
Could we change 'i' to a more informative name e.g. value?

[Bugs Nash, 2017-03-28 02:01:20]

On 2017/03/28 at 01:58:48, shend wrote:
> lgtm with nits.
> 
>
https://codereview.chromium.org/2772263004/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/css/properties/CSSPropertyCounterUtils.cpp
(right):
> 
>
https://codereview.chromium.org/2772263004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/css/properties/CSSPropertyCounterUtils.cpp:26:
int i = static_cast<int>(defaultValue);
> Could we change 'i' to a more informative name e.g. value?

Yes. Yes we can.

[Bugs Nash, 2017-03-28 02:05:37]

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

[commit-bot: I haz the power, 2017-03-28 02:05:55]

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-03-28 02:07:28]

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

[Bugs Nash, 2017-03-28 02:07:52]

+suzyh for owners

[suzyh_UTC10 (ex-contributor), 2017-03-28 02:26:08]

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

https://codereview.chromium.org/2772263004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/properties/CSSPropertyCounterUtils.cpp:26:
int value = static_cast<int>(defaultValue);
This int-iness enum-iness arrangement is a bit weird. It's not clear to me that
the concept of an enum is what you're looking for here, even though the values
are consecutive integers. I think two named integer constants would be a better
approach, and would avoid the static_cast.

https://codereview.chromium.org/2772263004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/properties/CSSPropertyCounterUtils.cpp:29:
value = clampTo<int>(counterValue->getDoubleValue());
Would it make sense to use getIntValue() instead then?

[Bugs Nash, 2017-03-28 02:32:50]

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

https://codereview.chromium.org/2772263004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/properties/CSSPropertyCounterUtils.cpp:29:
value = clampTo<int>(counterValue->getDoubleValue());
On 2017/03/28 at 02:26:08, suzyh wrote:
> Would it make sense to use getIntValue() instead then?

I think we're experiencing creep here? This is a refactor that moves code from
one place to another, and I don't believe the intention of Ribbon is to fix all
the problems in all the methods at the same time. What do you think?

[Bugs Nash, 2017-03-28 02:40:44]

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

[commit-bot: I haz the power, 2017-03-28 02:41:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[suzyh_UTC10 (ex-contributor), 2017-03-28 02:43:18]

On 2017/03/28 at 02:32:50, bugsnash wrote:
>
https://codereview.chromium.org/2772263004/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/css/properties/CSSPropertyCounterUtils.cpp
(right):
> 
>
https://codereview.chromium.org/2772263004/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/css/properties/CSSPropertyCounterUtils.cpp:29:
value = clampTo<int>(counterValue->getDoubleValue());
> On 2017/03/28 at 02:26:08, suzyh wrote:
> > Would it make sense to use getIntValue() instead then?
> 
> I think we're experiencing creep here? This is a refactor that moves code from
one place to another, and I don't believe the intention of Ribbon is to fix all
the problems in all the methods at the same time. What do you think?

Fair. I was commenting on it as connected to the int/enum/.. thing. I'm happy
for it to be done now or later, and leave it to you to weigh up the comparative
cost of CL review creep vs one line style fix in its own patch.

lgtm

[Bugs Nash, 2017-03-28 02:55:19]

On 2017/03/28 at 02:43:18, suzyh wrote:
> On 2017/03/28 at 02:32:50, bugsnash wrote:
> >
https://codereview.chromium.org/2772263004/diff/20001/third_party/WebKit/Sour...
> > File
third_party/WebKit/Source/core/css/properties/CSSPropertyCounterUtils.cpp
(right):
> > 
> >
https://codereview.chromium.org/2772263004/diff/20001/third_party/WebKit/Sour...
> >
third_party/WebKit/Source/core/css/properties/CSSPropertyCounterUtils.cpp:29:
value = clampTo<int>(counterValue->getDoubleValue());
> > On 2017/03/28 at 02:26:08, suzyh wrote:
> > > Would it make sense to use getIntValue() instead then?
> > 
> > I think we're experiencing creep here? This is a refactor that moves code
from one place to another, and I don't believe the intention of Ribbon is to fix
all the problems in all the methods at the same time. What do you think?
> 
> Fair. I was commenting on it as connected to the int/enum/.. thing. I'm happy
for it to be done now or later, and leave it to you to weigh up the comparative
cost of CL review creep vs one line style fix in its own patch.
> 
> lgtm

yeah i think the int/enum thing was creep too (as in changing from magic numbers
in the first place), but I let it in cause it was small and simple. but then it
set a precendent I think :P

[Bugs Nash, 2017-03-28 02:57:32]

On 2017/03/28 at 02:55:19, Bugs Nash wrote:
> On 2017/03/28 at 02:43:18, suzyh wrote:
> > On 2017/03/28 at 02:32:50, bugsnash wrote:
> > >
https://codereview.chromium.org/2772263004/diff/20001/third_party/WebKit/Sour...
> > > File
third_party/WebKit/Source/core/css/properties/CSSPropertyCounterUtils.cpp
(right):
> > > 
> > >
https://codereview.chromium.org/2772263004/diff/20001/third_party/WebKit/Sour...
> > >
third_party/WebKit/Source/core/css/properties/CSSPropertyCounterUtils.cpp:29:
value = clampTo<int>(counterValue->getDoubleValue());
> > > On 2017/03/28 at 02:26:08, suzyh wrote:
> > > > Would it make sense to use getIntValue() instead then?
> > > 
> > > I think we're experiencing creep here? This is a refactor that moves code
from one place to another, and I don't believe the intention of Ribbon is to fix
all the problems in all the methods at the same time. What do you think?
> > 
> > Fair. I was commenting on it as connected to the int/enum/.. thing. I'm
happy for it to be done now or later, and leave it to you to weigh up the
comparative cost of CL review creep vs one line style fix in its own patch.
> > 
> > lgtm
> 
> yeah i think the int/enum thing was creep too (as in changing from magic
numbers in the first place), but I let it in cause it was small and simple. but
then it set a precendent I think :P

(also without understanding how getIntValue vs getDoubleValue I'm less inclined
to jump into that one)

[commit-bot: I haz the power, 2017-03-28 04:49:04]

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

[commit-bot: I haz the power, 2017-03-28 04:49:05]

Dry run: This issue passed the CQ dry run.

[Bugs Nash, 2017-03-28 05:39:30]

The CQ bit was checked by bugsnash@chromium.org

[Bugs Nash, 2017-03-28 05:39:31]

The patchset sent to the CQ was uploaded after l-g-t-m from shend@chromium.org
Link to the patchset: https://codereview.chromium.org/2772263004/#ps40001
(title: "addressed reviewer comment")

[commit-bot: I haz the power, 2017-03-28 05:39:52]

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-03-28 05:45:09]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490679570814160,
"parent_rev": "8000869bbcac8cfd3e24de549d8cf07547d152a5", "commit_rev":
"45a8388f50dc32a97316656fe3170329fe5e1c6a"}

[commit-bot: I haz the power, 2017-03-28 05:46:40]

Description was changed from

==========
Added CSSPropertyCounterUtils which holds shared counter parsing logic.

This patch:
- Added CSSPropertyCounterUtils
- Moved consumeCounter method from CSSPropertyParser to
  CSSPropertyCounterUtils
- Replaced the use of raw int in argument to consumeCounter with more
  readable named enum values

This is pre work to move parseSingleValue logic from CSSPropertyParser
to the property APIs for CounterIncrement and CounterReset properties.

BUG=668012
==========

to

==========
Added CSSPropertyCounterUtils which holds shared counter parsing logic.

This patch:
- Added CSSPropertyCounterUtils
- Moved consumeCounter method from CSSPropertyParser to
  CSSPropertyCounterUtils
- Replaced the use of raw int in argument to consumeCounter with more
  readable named enum values

This is pre work to move parseSingleValue logic from CSSPropertyParser
to the property APIs for CounterIncrement and CounterReset properties.

BUG=668012

Review-Url: https://codereview.chromium.org/2772263004
Cr-Commit-Position: refs/heads/master@{#460019}
Committed:
https://chromium.googlesource.com/chromium/src/+/45a8388f50dc32a97316656fe317...
==========

[commit-bot: I haz the power, 2017-03-28 05:46:41]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/45a8388f50dc32a97316656fe317...