Add an helper function in CSSParser to check for '/' character.
Created attachment 177500 [details] Patch
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.
Created attachment 177505 [details] Patch
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.
Committed r136525: <http://trac.webkit.org/changeset/136525>
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?
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.
(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?
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 :/
(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.