Add a consumeIdent<CSSValueID...> helper

Add a consumeIdent<CSSValueID...> helper

This patch adds an additional consumeIdent helper function to the new
CSSPropertyParser, which takes a list of CSSValueIDs as a template
argument to accept.

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

[Timothy Loh, 2015-10-29 04:59:40]

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

[Timothy Loh, 2015-10-29 04:59:41]

So there are more similar helpers that we could potentially add, but they might
not be too useful without std::bind (which I'm not sure is allowed yet since
it's a c++11 library feature). I think this patch makes things better, but I'm
not sure about the other ideas below like consumeEither and consumeList (they
might also make things a little bit slower):


template<typename T, typename U> inline consumeEither(CSSParserTokenRange&
range, T firstConsumeOption, U secondConsumeOption)
{
    if (RefPtrWillBeRawPtr<CSSValue> value = firstConsumeOption(range))
        return value;
    return secondConsumeOption(range);
}

Which would let us write things like:

// consumeColumnCount
consumeEither(consumeIdent<CSSValueAuto>, consumePositiveInteger);

// consumeSpacing
consumeEither(
    consumeIdent<CSSValueNormal>,
    std::bind(consumeLength, _1, cssParserMode, ValueRangeAll,
UnitlessQuirk::Allow)
)

// consumeTabSize
consumeEither(
    std::bind(consumeInteger, _1, 0),
    std::bind(consumeLength, _1, cssParserMode, ValueRangeNonNegative);
)


And there's also possibly consumeList (could be consumeSpaceSeparatedList and
consumeCommaSeparatedList):
enum ListDelimiter { NoDelimiter, CommaDelimiter };
template<ListDelimiter delimiter, typename T> consumeList(CSSParserTokenRange&
range, T consume)
{
    RefPtrWillBeRawPtr<CSSValueList> list = delimiter == NoDelimiter ?
CSSValueList::createCommaSeparated() : CSSValueList::createSpaceSeparated();
    do {
        RefPtrWillBeRawPtr<CSSValue> value = consume(range);
        if (!value)
            return nullptr;
        list->append(value.release());
    } while (delimiter == NoDelimiter ? range.atEnd() :
consumeCommaIncludingWhitespace(range));
    return list.release();
}

Some possible example usage:
// consumeFontFeatureSettings
consumeEither(consumeIdent<CSSValueNormal>,
consumeList<CommaDelimiter>(consumeFontFeatureTag))

// consumeFontFamily
consumeList<CommaDelimiter>(consumeEither(consumeGenericFamily,
consumeFamilyName))

// first part of consumeAnimationPropertyList
consumeList<CommaDelimiter>(std::bind(consumeAnimationValue, property, _1,
context, useLegacyParsing))

// consumeFontFaceUnicodeRange
consumeList<CommaDelimiter>([] (CSSParserTokenRange& range) {
    const CSSParserToken& token = range.consumeIncludingWhitespace();
    if (token.type() != UnicodeRangeToken)
        return nullptr;
    
    UChar32 start = token.unicodeRangeStart();
    UChar32 end = token.unicodeRangeEnd();
    if (start > end)
        return nullptr;
    return CSSUnicodeRangeValue::create(start, end);
})


Also, we can perhaps just make a simple version of std::bind:
template<typename Consumer, typename... Ts> void bindConsume(Consumer consume,
Ts... parameters)
{
    return [consumer, parameters...] (CSSParserTokenRange& range) {
        return consumer(range, parameters...);
    }
}

[Timothy Loh, 2015-10-29 05:35:24]

On 2015/10/29 04:59:41, Timothy Loh wrote:
> So there are more similar helpers that we could potentially add, but they
might
> not be too useful without std::bind (which I'm not sure is allowed yet since
> it's a c++11 library feature). I think this patch makes things better, but I'm
> not sure about the other ideas below like consumeEither and consumeList (they
> might also make things a little bit slower):
> 
> 
> template<typename T, typename U> inline consumeEither(CSSParserTokenRange&
> range, T firstConsumeOption, U secondConsumeOption)
> {
>     if (RefPtrWillBeRawPtr<CSSValue> value = firstConsumeOption(range))
>         return value;
>     return secondConsumeOption(range);
> }
> 
> Which would let us write things like:
> 
> // consumeColumnCount
> consumeEither(consumeIdent<CSSValueAuto>, consumePositiveInteger);
> 
> // consumeSpacing
> consumeEither(
>     consumeIdent<CSSValueNormal>,
>     std::bind(consumeLength, _1, cssParserMode, ValueRangeAll,
> UnitlessQuirk::Allow)
> )
> 
> // consumeTabSize
> consumeEither(
>     std::bind(consumeInteger, _1, 0),
>     std::bind(consumeLength, _1, cssParserMode, ValueRangeNonNegative);
> )
> 
> 
> And there's also possibly consumeList (could be consumeSpaceSeparatedList and
> consumeCommaSeparatedList):
> enum ListDelimiter { NoDelimiter, CommaDelimiter };
> template<ListDelimiter delimiter, typename T> consumeList(CSSParserTokenRange&
> range, T consume)
> {
>     RefPtrWillBeRawPtr<CSSValueList> list = delimiter == NoDelimiter ?
> CSSValueList::createCommaSeparated() : CSSValueList::createSpaceSeparated();
>     do {
>         RefPtrWillBeRawPtr<CSSValue> value = consume(range);
>         if (!value)
>             return nullptr;
>         list->append(value.release());
>     } while (delimiter == NoDelimiter ? range.atEnd() :
> consumeCommaIncludingWhitespace(range));
>     return list.release();
> }
> 
> Some possible example usage:
> // consumeFontFeatureSettings
> consumeEither(consumeIdent<CSSValueNormal>,
> consumeList<CommaDelimiter>(consumeFontFeatureTag))
> 
> // consumeFontFamily
> consumeList<CommaDelimiter>(consumeEither(consumeGenericFamily,
> consumeFamilyName))
> 
> // first part of consumeAnimationPropertyList
> consumeList<CommaDelimiter>(std::bind(consumeAnimationValue, property, _1,
> context, useLegacyParsing))
> 
> // consumeFontFaceUnicodeRange
> consumeList<CommaDelimiter>([] (CSSParserTokenRange& range) {
>     const CSSParserToken& token = range.consumeIncludingWhitespace();
>     if (token.type() != UnicodeRangeToken)
>         return nullptr;
>     
>     UChar32 start = token.unicodeRangeStart();
>     UChar32 end = token.unicodeRangeEnd();
>     if (start > end)
>         return nullptr;
>     return CSSUnicodeRangeValue::create(start, end);
> })
> 
> 
> Also, we can perhaps just make a simple version of std::bind:
> template<typename Consumer, typename... Ts> void bindConsume(Consumer consume,
> Ts... parameters)
> {
>     return [consumer, parameters...] (CSSParserTokenRange& range) {
>         return consumer(range, parameters...);
>     }
> }

[Timothy Loh, 2015-10-29 05:35:30]

On 2015/10/29 05:35:24, Timothy Loh wrote:
> On 2015/10/29 04:59:41, Timothy Loh wrote:
> > So there are more similar helpers that we could potentially add, but they
> might
> > not be too useful without std::bind (which I'm not sure is allowed yet since
> > it's a c++11 library feature). I think this patch makes things better, but
I'm
> > not sure about the other ideas below like consumeEither and consumeList
(they
> > might also make things a little bit slower):
> > 
> > 
> > template<typename T, typename U> inline consumeEither(CSSParserTokenRange&
> > range, T firstConsumeOption, U secondConsumeOption)
> > {
> >     if (RefPtrWillBeRawPtr<CSSValue> value = firstConsumeOption(range))
> >         return value;
> >     return secondConsumeOption(range);
> > }
> > 
> > Which would let us write things like:
> > 
> > // consumeColumnCount
> > consumeEither(consumeIdent<CSSValueAuto>, consumePositiveInteger);
> > 
> > // consumeSpacing
> > consumeEither(
> >     consumeIdent<CSSValueNormal>,
> >     std::bind(consumeLength, _1, cssParserMode, ValueRangeAll,
> > UnitlessQuirk::Allow)
> > )
> > 
> > // consumeTabSize
> > consumeEither(
> >     std::bind(consumeInteger, _1, 0),
> >     std::bind(consumeLength, _1, cssParserMode, ValueRangeNonNegative);
> > )
> > 
> > 
> > And there's also possibly consumeList (could be consumeSpaceSeparatedList
and
> > consumeCommaSeparatedList):
> > enum ListDelimiter { NoDelimiter, CommaDelimiter };
> > template<ListDelimiter delimiter, typename T>
consumeList(CSSParserTokenRange&
> > range, T consume)
> > {
> >     RefPtrWillBeRawPtr<CSSValueList> list = delimiter == NoDelimiter ?
> > CSSValueList::createCommaSeparated() : CSSValueList::createSpaceSeparated();
> >     do {
> >         RefPtrWillBeRawPtr<CSSValue> value = consume(range);
> >         if (!value)
> >             return nullptr;
> >         list->append(value.release());
> >     } while (delimiter == NoDelimiter ? range.atEnd() :
> > consumeCommaIncludingWhitespace(range));
> >     return list.release();
> > }
> > 
> > Some possible example usage:
> > // consumeFontFeatureSettings
> > consumeEither(consumeIdent<CSSValueNormal>,
> > consumeList<CommaDelimiter>(consumeFontFeatureTag))
> > 
> > // consumeFontFamily
> > consumeList<CommaDelimiter>(consumeEither(consumeGenericFamily,
> > consumeFamilyName))
> > 
> > // first part of consumeAnimationPropertyList
> > consumeList<CommaDelimiter>(std::bind(consumeAnimationValue, property, _1,
> > context, useLegacyParsing))
> > 
> > // consumeFontFaceUnicodeRange
> > consumeList<CommaDelimiter>([] (CSSParserTokenRange& range) {
> >     const CSSParserToken& token = range.consumeIncludingWhitespace();
> >     if (token.type() != UnicodeRangeToken)
> >         return nullptr;
> >     
> >     UChar32 start = token.unicodeRangeStart();
> >     UChar32 end = token.unicodeRangeEnd();
> >     if (start > end)
> >         return nullptr;
> >     return CSSUnicodeRangeValue::create(start, end);
> > })
> > 
> > 
> > Also, we can perhaps just make a simple version of std::bind:
> > template<typename Consumer, typename... Ts> void bindConsume(Consumer
consume,
> > Ts... parameters)
> > {
> >     return [consumer, parameters...] (CSSParserTokenRange& range) {
> >         return consumer(range, parameters...);
> >     }
> > }

[Timothy Loh, 2015-10-29 05:35:41]

On 2015/10/29 05:35:30, Timothy Loh wrote:
> On 2015/10/29 05:35:24, Timothy Loh wrote:
> > On 2015/10/29 04:59:41, Timothy Loh wrote:
> > > So there are more similar helpers that we could potentially add, but they
> > might
> > > not be too useful without std::bind (which I'm not sure is allowed yet
since
> > > it's a c++11 library feature). I think this patch makes things better, but
> I'm
> > > not sure about the other ideas below like consumeEither and consumeList
> (they
> > > might also make things a little bit slower):
> > > 
> > > 
> > > template<typename T, typename U> inline consumeEither(CSSParserTokenRange&
> > > range, T firstConsumeOption, U secondConsumeOption)
> > > {
> > >     if (RefPtrWillBeRawPtr<CSSValue> value = firstConsumeOption(range))
> > >         return value;
> > >     return secondConsumeOption(range);
> > > }
> > > 
> > > Which would let us write things like:
> > > 
> > > // consumeColumnCount
> > > consumeEither(consumeIdent<CSSValueAuto>, consumePositiveInteger);
> > > 
> > > // consumeSpacing
> > > consumeEither(
> > >     consumeIdent<CSSValueNormal>,
> > >     std::bind(consumeLength, _1, cssParserMode, ValueRangeAll,
> > > UnitlessQuirk::Allow)
> > > )
> > > 
> > > // consumeTabSize
> > > consumeEither(
> > >     std::bind(consumeInteger, _1, 0),
> > >     std::bind(consumeLength, _1, cssParserMode, ValueRangeNonNegative);
> > > )
> > > 
> > > 
> > > And there's also possibly consumeList (could be consumeSpaceSeparatedList
> and
> > > consumeCommaSeparatedList):
> > > enum ListDelimiter { NoDelimiter, CommaDelimiter };
> > > template<ListDelimiter delimiter, typename T>
> consumeList(CSSParserTokenRange&
> > > range, T consume)
> > > {
> > >     RefPtrWillBeRawPtr<CSSValueList> list = delimiter == NoDelimiter ?
> > > CSSValueList::createCommaSeparated() :
CSSValueList::createSpaceSeparated();
> > >     do {
> > >         RefPtrWillBeRawPtr<CSSValue> value = consume(range);
> > >         if (!value)
> > >             return nullptr;
> > >         list->append(value.release());
> > >     } while (delimiter == NoDelimiter ? range.atEnd() :
> > > consumeCommaIncludingWhitespace(range));
> > >     return list.release();
> > > }
> > > 
> > > Some possible example usage:
> > > // consumeFontFeatureSettings
> > > consumeEither(consumeIdent<CSSValueNormal>,
> > > consumeList<CommaDelimiter>(consumeFontFeatureTag))
> > > 
> > > // consumeFontFamily
> > > consumeList<CommaDelimiter>(consumeEither(consumeGenericFamily,
> > > consumeFamilyName))
> > > 
> > > // first part of consumeAnimationPropertyList
> > > consumeList<CommaDelimiter>(std::bind(consumeAnimationValue, property, _1,
> > > context, useLegacyParsing))
> > > 
> > > // consumeFontFaceUnicodeRange
> > > consumeList<CommaDelimiter>([] (CSSParserTokenRange& range) {
> > >     const CSSParserToken& token = range.consumeIncludingWhitespace();
> > >     if (token.type() != UnicodeRangeToken)
> > >         return nullptr;
> > >     
> > >     UChar32 start = token.unicodeRangeStart();
> > >     UChar32 end = token.unicodeRangeEnd();
> > >     if (start > end)
> > >         return nullptr;
> > >     return CSSUnicodeRangeValue::create(start, end);
> > > })
> > > 
> > > 
> > > Also, we can perhaps just make a simple version of std::bind:
> > > template<typename Consumer, typename... Ts> void bindConsume(Consumer
> consume,
> > > Ts... parameters)
> > > {
> > >     return [consumer, parameters...] (CSSParserTokenRange& range) {
> > >         return consumer(range, parameters...);
> > >     }
> > > }

[Timothy Loh, 2015-10-29 05:36:48]

oops, ignore the replies, rietveld was being silly...

Anyway, I just realised that consumeEither and consumeList won't work because
they need to take a range argument and it'd probably end up being horrible. Oh
well.

[Timothy Loh, 2015-10-29 05:40:59]

To make consumeEither and consumeList work without being too horrible, they'd
have to use currying, i.e. take their arguments and return a function that takes
a range. I expect it'd probably end up a bit slower and probably not an
improvement. That said, I still think the templated consumeIdent is worthwhile.

[rwlbuis, 2015-10-29 20:23:37]

lgtm

This looks great!

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

https://codereview.chromium.org/1430603003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1723:
ASSERT_NOT_REACHED();
You could even remove this since we do this a bit below.

[Timothy Loh, 2015-10-30 00:00:31]

The CQ bit was checked by timloh@chromium.org

[Timothy Loh, 2015-10-30 00:00:36]

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

https://codereview.chromium.org/1430603003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1723:
ASSERT_NOT_REACHED();
On 2015/10/29 20:23:37, rwlbuis wrote:
> You could even remove this since we do this a bit below.

I think the compiler complains without a default case.

[commit-bot: I haz the power, 2015-10-30 00:02:37]

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

[commit-bot: I haz the power, 2015-10-30 00:08:51]

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

[commit-bot: I haz the power, 2015-10-30 00:08:52]

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

[Timothy Loh, 2015-10-30 02:16:03]

The CQ bit was checked by timloh@chromium.org

[commit-bot: I haz the power, 2015-10-30 02:16:51]

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

[commit-bot: I haz the power, 2015-10-30 02:21:36]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-10-30 02:22:32]

Patchset 1 (id:??) landed as
https://crrev.com/770eaea66cc4cfbb14ced3dc8dc5434716ed540a
Cr-Commit-Position: refs/heads/master@{#357026}

[esprehn, 2015-10-30 03:23:19]

Does the compiler turn this into a jump table? The switch statements are
generally better than this big set of || cases with recursion unless the
compiler is smart enough to transform those templates.

[esprehn, 2015-10-30 03:25:34]

On 2015/10/30 at 03:23:19, esprehn wrote:
> Does the compiler turn this into a jump table? The switch statements are
generally better than this big set of || cases with recursion unless the
compiler is smart enough to transform those templates.

You also made GenericFontFamily (which is very common) go from two branches:

if (range.peek().id() >= CSSValueSerif && range.peek().id() <=
CSSValueWebkitBody

to 6 branches.

consumeIdent<CSSValueSerif, CSSValueSansSerif, CSSValueCursive, CSSValueFantasy,
CSSValueMonospace, CSSValueWebkitBody>

maybe that doesn't impact real content enough though.

[Timothy Loh, 2015-10-30 03:38:54]

On 2015/10/30 03:25:34, esprehn wrote:
> On 2015/10/30 at 03:23:19, esprehn wrote:
> > Does the compiler turn this into a jump table? The switch statements are
> generally better than this big set of || cases with recursion unless the
> compiler is smart enough to transform those templates.
> 
> You also made GenericFontFamily (which is very common) go from two branches:
> 
> if (range.peek().id() >= CSSValueSerif && range.peek().id() <=
> CSSValueWebkitBody
> 
> to 6 branches.
> 
> consumeIdent<CSSValueSerif, CSSValueSansSerif, CSSValueCursive,
CSSValueFantasy,
> CSSValueMonospace, CSSValueWebkitBody>
> 
> maybe that doesn't impact real content enough though.

I made a mini test case (of course, I can't say from this whether the actual
Chrome build will be optimised similarly). https://gcc.godbolt.org/ says:
-O2 makes this range checks on clang and gcc
-O1 on gcc has everything inlined but doesn't make range checks
-O1 on clang doesn't inline

enum CSSValueID {
    A, B, C, D, E, F, G, H, I, J, K, L, M, N, O, P,
};

template<typename... emptyBaseCase> inline bool identMatches(CSSValueID id) {
return false ; } 
template<CSSValueID head, CSSValueID... tail> inline bool
identMatches(CSSValueID id) 
{
    return id == head || identMatches<tail...>(id);
}

bool test(CSSValueID id) {
    return identMatches<D, E, F, G, H, I>(id);
}