Bug 104009 – Add an helper function in CSSParser to check for '/' character.

• Component: CSS
• Status: RESOLVED FIXED

[Alexis Menard (darktears)]

Add an helper function in CSSParser to check for '/' character.

[Alexis Menard (darktears)]

Created attachment 177500 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 177500 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177500&action=review

> Source/WebCore/css/CSSParser.cpp:1655
> +static inline bool isSlash(CSSParserValue* value)

We might want to call this isOperatorSlash or some other CSS-spec terminology.

[Alexis Menard (darktears)]

Created attachment 177505 [details]
Patch

[Daniel Bates]

We should look to rename BorderImageParseContext::{allow, commit}Slash to be consistent with the naming of "isForwardSlashOperator".

For completeness, the paragraph under Example 22 in <http://www.w3.org/TR/css3-background/> (24 July 2012) references the forward slash character as "slash" with respect to the shorthand border-radius property.

[Alexis Menard (darktears)]

Committed r136525: <http://trac.webkit.org/changeset/136525>

[Darin Adler]

Comment on attachment 177505 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177505&action=review

> Source/WebCore/css/CSSParser.cpp:1657
> +    return value && value->unit == CSSParserValue::Operator && value->iValue == '/';

This adds an extra null check. Seems like an unwanted change. Maybe make this take a CSSParserValue& and put * characters at the call sites? Or just leave out the null check? Or replace it with an assertion?

[Alexis Menard (darktears)]

Comment on attachment 177505 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177505&action=review

>> Source/WebCore/css/CSSParser.cpp:1657
>> +    return value && value->unit == CSSParserValue::Operator && value->iValue == '/';
> 
> This adds an extra null check. Seems like an unwanted change. Maybe make this take a CSSParserValue& and put * characters at the call sites? Or just leave out the null check? Or replace it with an assertion?

I can just remove it sure so we keep the same behavior as before.

[Alexis Menard (darktears)]

(In reply to comment #7)
> (From update of attachment 177505 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177505&action=review
> 
> >> Source/WebCore/css/CSSParser.cpp:1657
> >> +    return value && value->unit == CSSParserValue::Operator && value->iValue == '/';
> > 
> > This adds an extra null check. Seems like an unwanted change. Maybe make this take a CSSParserValue& and put * characters at the call sites? Or just leave out the null check? Or replace it with an assertion?
> 
> I can just remove it sure so we keep the same behavior as before.

Should I rollout / reupload the patch here? Or I just commit leaving out the null check?

[Ryosuke Niwa]

Comment on attachment 177505 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177505&action=review

>>>> Source/WebCore/css/CSSParser.cpp:1657
>>>> +    return value && value->unit == CSSParserValue::Operator && value->iValue == '/';
>>> 
>>> This adds an extra null check. Seems like an unwanted change. Maybe make this take a CSSParserValue& and put * characters at the call sites? Or just leave out the null check? Or replace it with an assertion?
>> 
>> I can just remove it sure so we keep the same behavior as before.
> 
> Should I rollout / reupload the patch here? Or I just commit leaving out the null check?

Oh oops, I didn’t realize that. Please remove value && and replace it with ASSERT(value) although looking at validWidth, we don’t have assertions like that elsewhere :/

[Alexis Menard (darktears)]

(In reply to comment #9)
> (From update of attachment 177505 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177505&action=review
> 
> >>>> Source/WebCore/css/CSSParser.cpp:1657
> >>>> +    return value && value->unit == CSSParserValue::Operator && value->iValue == '/';
> >>> 
> >>> This adds an extra null check. Seems like an unwanted change. Maybe make this take a CSSParserValue& and put * characters at the call sites? Or just leave out the null check? Or replace it with an assertion?
> >> 
> >> I can just remove it sure so we keep the same behavior as before.
> > 
> > Should I rollout / reupload the patch here? Or I just commit leaving out the null check?
> 
> Oh oops, I didn’t realize that. Please remove value && and replace it with ASSERT(value) although looking at validWidth, we don’t have assertions like that elsewhere :/

Landed with http://trac.webkit.org/changeset/136540 . Unfortunately webkit-patch did not do the right thing as https://bugs.webkit.org/show_bug.cgi?id=40652. Sorry about that.