Bug 104009 - Add an helper function in CSSParser to check for '/' character.
Summary: Add an helper function in CSSParser to check for '/' character.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexis Menard (darktears)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-04 09:37 PST by Alexis Menard (darktears)
Modified: 2012-12-04 11:34 PST (History)
7 users (show)

See Also:


Attachments
Patch (3.36 KB, patch)
2012-12-04 09:39 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (3.45 KB, patch)
2012-12-04 10:13 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexis Menard (darktears) 2012-12-04 09:37:06 PST
Add an helper function in CSSParser to check for '/' character.
Comment 1 Alexis Menard (darktears) 2012-12-04 09:39:34 PST
Created attachment 177500 [details]
Patch
Comment 2 Ryosuke Niwa 2012-12-04 10:10:18 PST
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.
Comment 3 Alexis Menard (darktears) 2012-12-04 10:13:57 PST
Created attachment 177505 [details]
Patch
Comment 4 Daniel Bates 2012-12-04 10:23:48 PST
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.
Comment 5 Alexis Menard (darktears) 2012-12-04 10:26:42 PST
Committed r136525: <http://trac.webkit.org/changeset/136525>
Comment 6 Darin Adler 2012-12-04 10:35:10 PST
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 7 Alexis Menard (darktears) 2012-12-04 10:47:59 PST
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.
Comment 8 Alexis Menard (darktears) 2012-12-04 10:55:19 PST
(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 9 Ryosuke Niwa 2012-12-04 11:16:58 PST
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 :/
Comment 10 Alexis Menard (darktears) 2012-12-04 11:34:35 PST
(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.