Bug 121083 – CSSProperty::isInheritedProperty is large

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

[Ryosuke Niwa]

We should make it smaller!

[Ryosuke Niwa]

Created attachment 211274 [details]
Cleanup

[Ryosuke Niwa]

After my patch,

rniwa:webkit rniwa$ symbols -noSource WebKitBuild/Release/WebCore.framework/WebCore | grep isInheritedProperty
            0x0000000000187070 (    0x14) WebCore::CSSProperty::isInheritedProperty(WebCore::CSSPropertyID) [FUNC, PEXT, LENGTH, NameNList, MangledNameNList, Merged, NList, DebugMap, FunctionStarts] 
            0x00000000011d6860 (   0x1a0) WebCore::isInheritedPropertyTable [PEXT, NameNList, MangledNameNList, NList] 

isInheritedProperty is small.

[Daniel Bates]

Comment on attachment 211274 [details]
Cleanup

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

> Source/WebCore/css/makeprop.pl:210
> +  print GPERF '    ' . ($nameIsInherited{$name} ? 'true ' : 'false') . ", // CSSProperty" . $id . "\n";

Did you intend to put a space character after "true"?

We tend to use double quoted strings instead of single quoted strings in Perl code unless the string contains double quotes or we explicitly don't want the contents to be interpolated. We also tend to use string interpolation over string concatenation unless it would otherwise make it difficult to understand the string. So, I would write the last string in the string concatenation sequence as ", // CSSProperty$id\n"

[Ryosuke Niwa]

Comment on attachment 211274 [details]
Cleanup

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

>> Source/WebCore/css/makeprop.pl:210
>> +  print GPERF '    ' . ($nameIsInherited{$name} ? 'true ' : 'false') . ", // CSSProperty" . $id . "\n";
> 
> Did you intend to put a space character after "true"?
> 
> We tend to use double quoted strings instead of single quoted strings in Perl code unless the string contains double quotes or we explicitly don't want the contents to be interpolated. We also tend to use string interpolation over string concatenation unless it would otherwise make it difficult to understand the string. So, I would write the last string in the string concatenation sequence as ", // CSSProperty$id\n"

Yes, to align property names.

[Ryosuke Niwa]

Created attachment 211278 [details]
Addressed Daniel's comment

[Daniel Bates]

Comment on attachment 211278 [details]
Addressed Daniel's comment

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

> Source/WebCore/css/makeprop.pl:62
> +        die 'Options are specified on an alias: ', join(', ', @options) . "\n";

Please use double quoted strings.

Also, I suggest that we print the alias to standard error and/or line information to help a person identify the bad entry.

> Source/WebCore/css/makeprop.pl:68
> +      if ($option eq 'Inherited') {

Ditto.

> Source/WebCore/css/makeprop.pl:209
> +  $id =~ s/(^[^-])|-(.)/uc($1||$2)/ge;

Eww, duplicate code. This regular expression is used four times in this file (including the use in this patch). I suggest we factor it out into a common function, maybe dashedStringToTitleCase?

> Source/WebCore/css/makeprop.pl:257
> +my $first = 2;
> +my $i = 2;

Is there a way we can keep the value of these variables in sync with the expression "numCSSProperties + 2" (in line 202)?

[Ryosuke Niwa]

(In reply to comment #6)
> (From update of attachment 211278 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=211278&action=review
> 
> > Source/WebCore/css/makeprop.pl:62
> > +        die 'Options are specified on an alias: ', join(', ', @options) . "\n";
> 
> Please use double quoted strings.
> 
> Also, I suggest that we print the alias to standard error and/or line information to help a person identify the bad entry.

Done.

> > Source/WebCore/css/makeprop.pl:68
> > +      if ($option eq 'Inherited') {
> 
> Ditto.

Done.

> > Source/WebCore/css/makeprop.pl:209
> > +  $id =~ s/(^[^-])|-(.)/uc($1||$2)/ge;
> 
> Eww, duplicate code. This regular expression is used four times in this file (including the use in this patch). I suggest we factor it out into a common function, maybe dashedStringToTitleCase?

I added %nameToId instead to avoid executing this regular expression thrice.

> > Source/WebCore/css/makeprop.pl:257
> > +my $first = 2;
> > +my $i = 2;
> 
> Is there a way we can keep the value of these variables in sync with the expression "numCSSProperties + 2" (in line 202)?

Added a variable to do this.

[Ryosuke Niwa]

Committed r155511: <http://trac.webkit.org/changeset/155511>

[Darin Adler]

Comment on attachment 211278 [details]
Addressed Daniel's comment

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

> Source/WebCore/css/makeprop.pl:202
> +bool isInheritedPropertyTable[numCSSProperties + 2] = {

I think you should add a “static” here.

[Anders Carlsson]

(In reply to comment #9)
> (From update of attachment 211278 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=211278&action=review
> 
> > Source/WebCore/css/makeprop.pl:202
> > +bool isInheritedPropertyTable[numCSSProperties + 2] = {
> 
> I think you should add a “static” here.

Maybe a const too!

[Ryosuke Niwa]

Addressed that in http://trac.webkit.org/changeset/155550.

Somehow the change log is missing the bug number.

[Alexey Proskuryakov]

Why not "const" only?

[Ryosuke Niwa]

Looks better!

rniwa:webkit rniwa$ symbols WebKitBuild/Release/WebCore.framework/WebCore | grep isInheritedProperty
            0x0000000000185f50 (    0x14) WebCore::CSSProperty::isInheritedProperty(WebCore::CSSPropertyID) [FUNC, PEXT, LENGTH, NameNList, MangledNameNList, Merged, NList, DebugMap, FunctionStarts] 
            0x0000000000ce90d0 (   0x1a0) WebCore::isInheritedPropertyTable [NameNList, MangledNameNList, NList]