Simplify parseShadowValue

Simplify parseShadowValue

Simplify parseShadowValue by getting rid of ShadowParseContext.

BUG=404023
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201874

[rwlbuis, 2015-08-27 21:26:53]

Patchset #3 (id:40001) has been deleted

[rwlbuis, 2015-08-31 22:25:41]

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

[rwlbuis, 2015-08-31 22:25:42]

PTAL. I am not sure it can be simplified any more since there is a bit of
non-determinism in there with 'inset'...

[Timothy Loh, 2015-09-01 07:18:08]

BTW this doesn't really get us closer to removing CSSParserValue, especially if
we're going to rewrite all the code paths.

https://codereview.chromium.org/1318093002/diff/80001/Source/core/css/parser/...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1318093002/diff/80001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:1008: return
parseShadow(m_valueList, propId, important);
parsedValue = parseShadow(..) and remove the important argument? :-)

https://codereview.chromium.org/1318093002/diff/80001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:5146: if
(!isComma(valueList->current()))
!consumeComma and then you don't need the next() below

https://codereview.chromium.org/1318093002/diff/80001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:5160:
RefPtrWillBeRawPtr<CSSValueList> lengths = CSSValueList::createSpaceSeparated();
Probably just use a vector (or have four CSSValues?)

https://codereview.chromium.org/1318093002/diff/80001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:5176: if (!val || !validUnit(val,
FLength, HTMLStandardMode))
Doesn't this fail if we have: color inset length length

I was thinking this would be something like:
while(true) {
    if (!style && allowInset && val->id == CSSValueInset)
        // set style, continue
    if (!color && (color = parseColor())
        // continue
    if (lengths.isEmpty()) {
        // consume lengths..
    }
    break;
}

https://codereview.chromium.org/1318093002/diff/80001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:5213: if (val && val->m_unit ==
CSSParserValue::Operator) {
We don't really need a check like this here (or the allowBreak flag). It should
be fine if this just consumes as many values as it can and returns a
CSSShadowValue out of them (or nullptr). The huge switch statement checks at the
bottom for trailing tokens, and the list parsing routing can bail if it sees a
non-comma.

[rwlbuis, 2015-09-02 22:38:59]

PTAL. I agree this doesn't really get us closer to removing CSSParserValue, but
you did mention that parseShadow should not be 200 lines long :)

https://codereview.chromium.org/1318093002/diff/80001/Source/core/css/parser/...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1318093002/diff/80001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:1008: return
parseShadow(m_valueList, propId, important);
On 2015/09/01 07:18:08, Timothy Loh wrote:
> parsedValue = parseShadow(..) and remove the important argument? :-)

Done.

https://codereview.chromium.org/1318093002/diff/80001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:5146: if
(!isComma(valueList->current()))
On 2015/09/01 07:18:08, Timothy Loh wrote:
> !consumeComma and then you don't need the next() below

Done.

https://codereview.chromium.org/1318093002/diff/80001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:5160:
RefPtrWillBeRawPtr<CSSValueList> lengths = CSSValueList::createSpaceSeparated();
On 2015/09/01 07:18:08, Timothy Loh wrote:
> Probably just use a vector (or have four CSSValues?)

Done.

https://codereview.chromium.org/1318093002/diff/80001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:5176: if (!val || !validUnit(val,
FLength, HTMLStandardMode))
On 2015/09/01 07:18:08, Timothy Loh wrote:
> Doesn't this fail if we have: color inset length length

In fact that syntax is not allowed, see fast/box-shadow/inset.html, same in
Firefox.

https://codereview.chromium.org/1318093002/diff/80001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:5213: if (val && val->m_unit ==
CSSParserValue::Operator) {
On 2015/09/01 07:18:08, Timothy Loh wrote:
> We don't really need a check like this here (or the allowBreak flag). It
should
> be fine if this just consumes as many values as it can and returns a
> CSSShadowValue out of them (or nullptr). The huge switch statement checks at
the
> bottom for trailing tokens, and the list parsing routing can bail if it sees a
> non-comma.

Done.

[Timothy Loh, 2015-09-04 05:04:51]

OK for the color inset length length case, seems FF and IE both reject this
anyway. The spec clearly allows this though :\

Can you check
http://test.csswg.org/harness/test/css-backgrounds-3_dev/section/7.1/box-shad...
and see if it improves anything? (Some of the failures are due to the case
mentioned above, but there might be some bugs fixed)

https://codereview.chromium.org/1318093002/diff/120001/Source/core/css/parser...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1318093002/diff/120001/Source/core/css/parser...
Source/core/css/parser/CSSPropertyParser.cpp:5198: if
(RefPtrWillBeMember<CSSPrimitiveValue> colorValue = parseColor(val)) {
if (val && !color && (color = parseColor(val))) seems better IMO

https://codereview.chromium.org/1318093002/diff/120001/Source/core/css/parser...
File Source/core/css/parser/CSSPropertyParser.h (right):

https://codereview.chromium.org/1318093002/diff/120001/Source/core/css/parser...
Source/core/css/parser/CSSPropertyParser.h:214:
PassRefPtrWillBeRawPtr<CSSShadowValue> parseSingleShadow(CSSParserValueList*,
bool allowInset = false, unsigned maxLength = 4);
Maybe just allowSpread instead of maxLength? Also I'd prefer to not have default
arguments :-)

[rwlbuis, 2015-09-04 15:04:36]

Patchset #7 (id:140001) has been deleted

[rwlbuis, 2015-09-04 16:00:42]

On 2015/09/04 05:04:51, Timothy Loh wrote:
> OK for the color inset length length case, seems FF and IE both reject this
> anyway. The spec clearly allows this though :\
> 
> Can you check
>
http://test.csswg.org/harness/test/css-backgrounds-3_dev/section/7.1/box-shad...
> and see if it improves anything? (Some of the failures are due to the case
> mentioned above, but there might be some bugs fixed)

Yes! It fixed two cases. Now the result is the same as in FF.

>
https://codereview.chromium.org/1318093002/diff/120001/Source/core/css/parser...
> File Source/core/css/parser/CSSPropertyParser.cpp (right):
> 
>
https://codereview.chromium.org/1318093002/diff/120001/Source/core/css/parser...
> Source/core/css/parser/CSSPropertyParser.cpp:5198: if
> (RefPtrWillBeMember<CSSPrimitiveValue> colorValue = parseColor(val)) {
> if (val && !color && (color = parseColor(val))) seems better IMO

I don't think this works, we need to return null in case there is a val and
color is already
parsed, I introduced colorValue to keep them apart. Of course I could be missing
something.

>
https://codereview.chromium.org/1318093002/diff/120001/Source/core/css/parser...
> File Source/core/css/parser/CSSPropertyParser.h (right):
> 
>
https://codereview.chromium.org/1318093002/diff/120001/Source/core/css/parser...
> Source/core/css/parser/CSSPropertyParser.h:214:
> PassRefPtrWillBeRawPtr<CSSShadowValue> parseSingleShadow(CSSParserValueList*,
> bool allowInset = false, unsigned maxLength = 4);
> Maybe just allowSpread instead of maxLength? Also I'd prefer to not have
default
> arguments :-)

Fair enough :) Should be fixed now.

[Timothy Loh, 2015-09-07 01:44:53]

lgtm

[rwlbuis, 2015-09-07 11:55:55]

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

[commit-bot: I haz the power, 2015-09-07 11:56:05]

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

[commit-bot: I haz the power, 2015-09-07 13:13:54]

Committed patchset #7 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201874