| Summary: | CSSProperty::isInheritedProperty is large | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||
| Component: | CSS | Assignee: | Ryosuke Niwa <rniwa> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | andersca, ap, benjamin, commit-queue, darin, dbates, esprehn+autocc, glenn, kling, koivisto, macpherson, menard | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 121740 | ||||||||
| Attachments: |
|
||||||||
|
Description
Ryosuke Niwa
2013-09-10 00:27:36 PDT
Created attachment 211274 [details]
Cleanup
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.
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" 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. Created attachment 211278 [details]
Addressed Daniel's comment
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)? (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. Committed r155511: <http://trac.webkit.org/changeset/155511> 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. (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! Addressed that in http://trac.webkit.org/changeset/155550. Somehow the change log is missing the bug number. Why not "const" only? 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]
|