Bug 138208 - Move -webkit-marquee-increment property to the new StyleBuilder
Summary: Move -webkit-marquee-increment property to the new StyleBuilder
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on: 137910
Blocks:
  Show dependency tree / graph
 
Reported: 2014-10-29 21:20 PDT by Chris Dumez
Modified: 2014-10-31 16:46 PDT (History)
6 users (show)

See Also:

Attachments
Patch (10.65 KB, patch)
2014-10-29 21:52 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (12.50 KB, patch)
2014-10-30 10:29 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (12.38 KB, patch)
2014-10-30 14:37 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (12.38 KB, patch)
2014-10-30 20:42 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2014-10-29 21:20:52 PDT 
Move -webkit-marquee-increment property from DeprecatedStyleBuilder to the new StyleBuilder so that it is generated from CSSPropertyNames.in.
Comment 1 Chris Dumez 2014-10-29 21:52:53 PDT 
Created attachment 240657 [details]
Patch
Comment 2 Sam Weinig 2014-10-30 06:08:46 PDT 
Comment on attachment 240657 [details]
Patch

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

> Source/WebCore/css/StyleBuilderCustom.h:47
> +    auto& primitiveValue = downcast<CSSPrimitiveValue>(value);

What guarantees this cast is safe? Could we have applyValueWebkitMarqueeIncrement() take a CSSPrimitiveValue instead of a CSSValue?
Comment 3 Chris Dumez 2014-10-30 10:29:22 PDT 
Created attachment 240680 [details]
Patch
Comment 4 Chris Dumez 2014-10-30 10:33:46 PDT 
Comment on attachment 240657 [details]
Patch

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

>> Source/WebCore/css/StyleBuilderCustom.h:47
>> +    auto& primitiveValue = downcast<CSSPrimitiveValue>(value);
> 
> What guarantees this cast is safe? Could we have applyValueWebkitMarqueeIncrement() take a CSSPrimitiveValue instead of a CSSValue?

The CSS Parser is already marking sure the value for -webkit-marquee-increment property is a CSSPrimitiveValue so this cast should be safe and the assertion in downcast<>() should protect us against a mismatch between the CSS Parser and the StyleBuilder. The reason the StyleBuilder functions take a CSSValue and not a CSSPrimitiveValue is because not all properties have a CSSPrimitiveValue as value. Some have a CSSValueList for e.g. and make prop.pl does not have any way to know as far as I can tell because this information is coded in the CSSParser, not CSSPropertyNames.in.

I have added a new layout test in the latest patch iteration to gain coverage for non CSSPrimitive values on  -webkit-marquee-increment, and make sure we don't crash.
Comment 5 Chris Dumez 2014-10-30 14:37:13 PDT 
Created attachment 240702 [details]
Patch
Comment 6 Chris Dumez 2014-10-30 20:42:42 PDT 
Created attachment 240723 [details]
Patch
Comment 7 Chris Dumez 2014-10-31 09:58:09 PDT 
ping review?
Comment 8 Andreas Kling 2014-10-31 16:19:26 PDT 
Comment on attachment 240723 [details]
Patch

r=me
Comment 9 WebKit Commit Bot 2014-10-31 16:46:15 PDT 
Comment on attachment 240723 [details]
Patch

Clearing flags on attachment: 240723

Committed r175439: <http://trac.webkit.org/changeset/175439>
Comment 10 WebKit Commit Bot 2014-10-31 16:46:20 PDT 
All reviewed patches have been landed.  Closing bug.