Move counter-increment/counter-reset handling into CSSPropertyParser

Move counter-increment/counter-reset handling into CSSPropertyParser

Move counter-increment/counter-reset handling from
LegacyCSSPropertyParser into CSSPropertyParser.

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

[rwlbuis, 2015-10-08 22:15:41]

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

[rwlbuis, 2015-10-08 22:15:42]

PTAL.

I want to share my thoughts on the next steps.
Right now I am favoring single, low priority properties. Obviously shorthands
can only be converted after its longhands :)

It would be great to introduce new consume helpers and at the same time remove
the old helpers. For instance parseColor is a big chunk but we'll probably have
to support the old parseColor together with the consumeColor for a bit.

Since there is still much to convert and part of it is boring/routine, maybe it
is a good idea if more people work on this.  As an example Sydney could take on
the transform/animation/transition related properties as it is a decent chunk of
code and you guys know this code anyway.

Originally I had somehow hoped to be mostly done before Blinkon but that seems
unlikely, especially with 100 line patches (which make sense though) :)

Let me know what you think. I'll be happy to keep plugging away the next weeks
on this anyway.

[Timothy Loh, 2015-10-08 23:03:02]

On 2015/10/08 22:15:42, rwlbuis wrote:
> PTAL.
> 
> I want to share my thoughts on the next steps.
> Right now I am favoring single, low priority properties. Obviously shorthands
> can only be converted after its longhands :)
> 
> It would be great to introduce new consume helpers and at the same time remove
> the old helpers. For instance parseColor is a big chunk but we'll probably
have
> to support the old parseColor together with the consumeColor for a bit.
>
> Since there is still much to convert and part of it is boring/routine, maybe
it
> is a good idea if more people work on this.  As an example Sydney could take
on
> the transform/animation/transition related properties as it is a decent chunk
of
> code and you guys know this code anyway.
>
> Originally I had somehow hoped to be mostly done before Blinkon but that seems
> unlikely, especially with 100 line patches (which make sense though) :)
> 
> Let me know what you think. I'll be happy to keep plugging away the next weeks
> on this anyway.

I'm happy to help out writing code here, I've been meaning to fix up something
(bug 386459) with animation/transition parsing so I'm happy to do those first.
BTW, we actually had (on the syd blink team) last quarter finishing this as one
of our goals, so I think we underestimated it even more than yourself. That
said, I don't think keeping the patches small makes things take any longer,
since it makes everything way easier to review.

I'll put together a spreadsheet with the properties that need to be moved across
and the dependencies so we can coordinate this.

[Timothy Loh, 2015-10-08 23:13:56]

lgtm

https://codereview.chromium.org/1398553002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1398553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:637:
RefPtrWillBeRawPtr<CSSValueList> list = CSSValueList::createCommaSeparated();
TODO to create a space-separated value list here :-)

https://codereview.chromium.org/1398553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:639: if
(range.peek().type() != IdentToken)
btw I'd prefer this as follows

RefPtr..<CSSCustomIdentValue> counterName = consumeCustomIdent(range);
if (!counterName)
    return nullptr;

So this way type checking is the responsibility of the consume helper, and
there's no change we accidentally check the type incorrectly and then make a
counter with a null counter name.

https://codereview.chromium.org/1398553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:643: if
(RefPtrWillBeRawPtr<CSSPrimitiveValue> counterValue = consumeInteger(range,
cssParserMode, std::numeric_limits<double>::lowest()))
How about (don't need to pass in the limit if it's a default arg):

RefPtrWillBeRawPtr<CSSPrimitiveValue> counterValue = consumeInteger(range,
cssParserMode);
if (!counterValue)
    counterValue = ..create(defaultValue)

[rwlbuis, 2015-10-09 02:15:26]

On 2015/10/08 23:03:02, Timothy Loh wrote:
> On 2015/10/08 22:15:42, rwlbuis wrote:
> > PTAL.
> > 
> > I want to share my thoughts on the next steps.
> > Right now I am favoring single, low priority properties. Obviously
shorthands
> > can only be converted after its longhands :)
> > 
> > It would be great to introduce new consume helpers and at the same time
remove
> > the old helpers. For instance parseColor is a big chunk but we'll probably
> have
> > to support the old parseColor together with the consumeColor for a bit.
> >
> > Since there is still much to convert and part of it is boring/routine, maybe
> it
> > is a good idea if more people work on this.  As an example Sydney could take
> on
> > the transform/animation/transition related properties as it is a decent
chunk
> of
> > code and you guys know this code anyway.
> >
> > Originally I had somehow hoped to be mostly done before Blinkon but that
seems
> > unlikely, especially with 100 line patches (which make sense though) :)
> > 
> > Let me know what you think. I'll be happy to keep plugging away the next
weeks
> > on this anyway.
> 
> I'm happy to help out writing code here, I've been meaning to fix up something
> (bug 386459) with animation/transition parsing so I'm happy to do those first.
> BTW, we actually had (on the syd blink team) last quarter finishing this as
one
> of our goals, so I think we underestimated it even more than yourself. That
> said, I don't think keeping the patches small makes things take any longer,
> since it makes everything way easier to review.
> 
> I'll put together a spreadsheet with the properties that need to be moved
across
> and the dependencies so we can coordinate this.

A spreadsheet sounds great!

[rwlbuis, 2015-10-09 02:31:12]

https://codereview.chromium.org/1398553002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1398553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:643: if
(RefPtrWillBeRawPtr<CSSPrimitiveValue> counterValue = consumeInteger(range,
cssParserMode, std::numeric_limits<double>::lowest()))
On 2015/10/08 23:13:56, Timothy Loh wrote:
> How about (don't need to pass in the limit if it's a default arg):
> 
> RefPtrWillBeRawPtr<CSSPrimitiveValue> counterValue = consumeInteger(range,
> cssParserMode);
> if (!counterValue)
>     counterValue = ..create(defaultValue)

Sorry, should have mentioned the s/min/lowest change. In the past I assumed min
did what lowest actually does...
IIRC going with <int>::lowest rather than <double>::lowest causes some test
failure, will double check tomorrow.

[rwlbuis, 2015-10-09 16:58:35]

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

[rwlbuis, 2015-10-09 16:58:35]

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/1398553002/#ps20001
(title: "Patch for landing")

[commit-bot: I haz the power, 2015-10-09 17:00:52]

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

[commit-bot: I haz the power, 2015-10-09 17:08:58]

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

[commit-bot: I haz the power, 2015-10-09 17:08:59]

Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[rwlbuis, 2015-10-09 17:54:48]

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

[rwlbuis, 2015-10-09 17:54:48]

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/1398553002/#ps40001
(title: "Work around ::lowest problem")

[commit-bot: I haz the power, 2015-10-09 17:57:28]

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

[commit-bot: I haz the power, 2015-10-09 20:52:56]

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

[commit-bot: I haz the power, 2015-10-09 20:52:57]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[rwlbuis, 2015-10-09 20:55:05]

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

[commit-bot: I haz the power, 2015-10-09 20:55:30]

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

[rwlbuis, 2015-10-09 20:56:04]

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

[rwlbuis, 2015-10-09 20:58:55]

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

[commit-bot: I haz the power, 2015-10-09 20:59:37]

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

[commit-bot: I haz the power, 2015-10-09 22:14:03]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-09 22:16:47]

Patchset 3 (id:??) landed as
https://crrev.com/f4b4583bcc7ccb6b0191f5b4c72b94154a78bdd1
Cr-Commit-Position: refs/heads/master@{#353400}