Bug 164691

Created attachment 294633 [details] Work-in-progress patch When I wrote this patch I structured the JSON as a dictionary of dictionaries where the keys of the outer-most dictionary are the CSS property names. We may want to consider structuring this file as an array of dictionaries so as to be able to detect duplicates property definitions. I am open to suggestions.

Attachment 294633 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 294634 [details] make-json-prop I used this script to generate CSSProperties.json from CSSPropertyNames.in. It's not pretty, but this is one-off script.

Comment on attachment 294633 [details] Work-in-progress patch View in context: https://bugs.webkit.org/attachment.cgi?id=294633&action=review > Source/WebCore/css/CSSPropertyNames.in:-52 > -// * Custom=[Initial|Value|Inherit|All]: > -// Custom=Initial option is used to indicate that the CSS property requires > -// special handling to set its initial value. > -// Custom=Inherit option is used to indicate that the CSS property requires > -// special handling to set its inherit value. > -// Custom=Value option is used to indicate that the CSS property requires special > -// handling to set its value, and a regular Converter helper cannot be > -// used. The Custom code for the property should be located in > -// css/StyleBuilderCustom.h and named applyValue[CSSPropertyName](). > -// If special handling is also needed to apply inherit or initial value, use > -// Custom=All. Alternatively, several '|'-separated options can be passed: > -// e.g. 'Custom=Inherit|Value". Please don't lose the documentation.

(In reply to comment #4) > Comment on attachment 294633 [details] > Work-in-progress patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=294633&action=review > > > Source/WebCore/css/CSSPropertyNames.in:-52 > > -// * Custom=[Initial|Value|Inherit|All]: > > -// Custom=Initial option is used to indicate that the CSS property requires > > -// special handling to set its initial value. > > -// Custom=Inherit option is used to indicate that the CSS property requires > > -// special handling to set its inherit value. > > -// Custom=Value option is used to indicate that the CSS property requires special > > -// handling to set its value, and a regular Converter helper cannot be > > -// used. The Custom code for the property should be located in > > -// css/StyleBuilderCustom.h and named applyValue[CSSPropertyName](). > > -// If special handling is also needed to apply inherit or initial value, use > > -// Custom=All. Alternatively, several '|'-separated options can be passed: > > -// e.g. 'Custom=Inherit|Value". > > Please don't lose the documentation. One idea is to move this to a wiki page. Another idea is to put the documentation or a hyperlink to a wiki page with the documentation in the JSON file either in a way that is JSON compliant or non-compliant. Obviously if we went the non-compliant route then a web page will need to do some preprocessing of the file before it can JSON decode it. I am open to suggestions.

Or we can put it in a json property for this purpose.

Created attachment 294866 [details] Work-in-progress patch For EWS.

Comment on attachment 294866 [details] Work-in-progress patch Comments on the JSON: Would be nice to sort by property name (even though it's a dictionary). Lowercase keys would be nice. Not sure if camelCase or hyphenated better. Maybe group the generation-specific data ("SkipBuilder", "Converter", "Custom", "Setter") under their own key? So as a general approach, build the JSON to reflect how CSS specs things. Tack on our code-generation metadata in a non-intrusive way. Not sure how to do EnableIf for values.

Created attachment 294976 [details] Patch

Created attachment 294992 [details] Patch

Created attachment 294993 [details] Patch

Comment on attachment 294993 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=294993&action=review > Source/WebCore/css/CSSPropertyNames.in:-134 > -// Keep this in between the highest priority props and the lower ones. > --webkit-ruby-position [Inherited] I don't see this having a "medium" priority in the JSON. > Source/WebCore/css/CSSPropertyNames.in:-416 > -// -webkit-border-radius differs from border-radius only in the interpretation of > -// a value consisting of two lengths: "-webkit-border-radius: l1 l2;" is equivalent > -// to "border-radius: l1 / l2;" We've lost this comment. I would like it preserved in the JSON. > Source/WebCore/css/CSSPropertyNames.in:-434 > -// -webkit-box-shadow differs from box-shadow in its treatement of blur radii > 8px. > -// Let -webkit-box-shadow blur radius be w_r and box-shadow blur radius be b_r. For > -// w_r > 8px, b_r = 8 + 4 * sqrt((w_r - 8) / 2). We've also lost this comment.

Comment on attachment 294993 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=294993&action=review >> Source/WebCore/css/CSSPropertyNames.in:-134 >> --webkit-ruby-position [Inherited] > > I don't see this having a "medium" priority in the JSON. We do not have a concept of a "medium" priority. (Should we create such a concept?) Moreover, -webkit-ruby-position is the highest priority property and "it is resolved before all other properties to ensure that its value can be checked when determining a smart default font size" (<https://trac.webkit.org/browser/trunk/Source/WebCore/ChangeLog?rev=172861>; bug #136137). Will add a comment to explain this. For completeness, we resolve -webkit-ruby-position twice (we should fix this unless it is intentional. If intentional, why?). We hardcode resolve it before all other properties and then resolve it again when resolving the low priority properties because it is considered the first low-priority priority given its position in this file. We should look to convey the specialness of this property in CSSProperties.json. I suggest that we do this in a separate bug. >> Source/WebCore/css/CSSPropertyNames.in:-416 >> -// to "border-radius: l1 / l2;" > > We've lost this comment. I would like it preserved in the JSON. Will preserve. Will also preserve comments for the following properties that appear above this comment: -webkit-background-size, -webkit-opacity, and -webkit-box-sizing. >> Source/WebCore/css/CSSPropertyNames.in:-434 >> -// w_r > 8px, b_r = 8 + 4 * sqrt((w_r - 8) / 2). > > We've also lost this comment. Will preserve. Will also preserve comment for property -webkit-transform-style that appears below this comment.

Committed r209001: <http://trac.webkit.org/changeset/209001>

Could we switch this to JSON5? Blink is using it as their .in replacement. JSON5 allows comments and other conveniences. https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/NYM4po44U1A http://json5.org

(In reply to comment #15) > Could we switch this to JSON5? Blink is using it as their .in replacement. > JSON5 allows comments and other conveniences. > > https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/NYM4po44U1A > http://json5.org If we don't use plain-old-json, I'm not certain that json5 offers much over something like yaml, which has builtin support in many scripting languages and also supports comments. However, I don't feel all to strongly one way or another.