Summary: | Rename CSSPropertyLonghand class to StylePropertyShorthand. | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexis Menard (darktears) <menard> | ||||||||||||||||
Component: | CSS | Assignee: | Alexis Menard (darktears) <menard> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | benjamin, eric, inferno, kling, koivisto, macpherson, rniwa, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Attachments: |
|
Description
Alexis Menard (darktears)
2012-03-29 09:41:46 PDT
Created attachment 134602 [details]
Patch
Antti I implemented what we talked yesterday. What do you think? Do you think it is better or should we go back to the old approach and just rename the CSSPropertyLonghand class to ShorthandProperties? I find this approach more error prone. Let's see what EWS says also :). Created attachment 134604 [details]
Patch
Created attachment 134609 [details]
Patch
Created attachment 134641 [details]
Patch
This should fix the regressions
Comment on attachment 134641 [details]
Patch
I'm confused why a CSSPropertyID* which points to an array of unspecified length, is better/safer than a class. This seems backwards.
I think this change, as written would be asking for crashes. Created attachment 134660 [details]
Patch
Alternative patch
Comment on attachment 134660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134660&action=review > Source/WebCore/css/CSSPropertyLonghand.h:61 > +const ShorthandProperties& backgroundLonghand(); I think the class name should be singular here since this class represents a list of longhand properties for a signle shorthand property. Also we should probably keep CSS prefix since shorthand is a quite generic name. (In reply to comment #9) > (From update of attachment 134660 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=134660&action=review > > > Source/WebCore/css/CSSPropertyLonghand.h:61 > > +const ShorthandProperties& backgroundLonghand(); > > I think the class name should be singular here since this class represents a list of longhand properties for a signle shorthand property. so you would do const ShorthandProperty& backgroundShorthand() (In reply to comment #10) > Also we should probably keep CSS prefix since shorthand is a quite generic name. Well it seems we start using CSS prefix only for CSSOM classes. This one is implementation detail. (In reply to comment #12) > (In reply to comment #10) > > Also we should probably keep CSS prefix since shorthand is a quite generic name. > > Well it seems we start using CSS prefix only for CSSOM classes. This one is implementation detail. Okay. Created attachment 135072 [details]
Patch
Created attachment 135082 [details]
Patch
Comment on attachment 135082 [details]
Patch
r=me
Comment on attachment 135082 [details] Patch Clearing flags on attachment: 135082 Committed r112880: <http://trac.webkit.org/changeset/112880> All reviewed patches have been landed. Closing bug. |