Bug 82624 – Rename CSSPropertyLonghand class to StylePropertyShorthand.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Alexis Menard (darktears)]

Remove CSSPropertyLonghand object to simplify CSS shorthand handling.

[Alexis Menard (darktears)]

Created attachment 134602 [details]
Patch

[Alexis Menard (darktears)]

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 :).

[Alexis Menard (darktears)]

Created attachment 134604 [details]
Patch

[Alexis Menard (darktears)]

Created attachment 134609 [details]
Patch

[Alexis Menard (darktears)]

Created attachment 134641 [details]
Patch

This should fix the regressions

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

I think this change, as written would be asking for crashes.

[Alexis Menard (darktears)]

Created attachment 134660 [details]
Patch

Alternative patch

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Also we should probably keep CSS prefix since shorthand is a quite generic name.

[Alexis Menard (darktears)]

(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()

[Alexis Menard (darktears)]

(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.

[Ryosuke Niwa]

(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.

[Alexis Menard (darktears)]

Created attachment 135072 [details]
Patch

[Alexis Menard (darktears)]

Created attachment 135082 [details]
Patch

[Antti Koivisto]

Comment on attachment 135082 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 135082 [details]
Patch

Clearing flags on attachment: 135082

Committed r112880: <http://trac.webkit.org/changeset/112880>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.