Bug 121083 - CSSProperty::isInheritedProperty is large
Summary: CSSProperty::isInheritedProperty is large
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 121740
  Show dependency treegraph
 
Reported: 2013-09-10 00:27 PDT by Ryosuke Niwa
Modified: 2013-09-21 00:55 PDT (History)
12 users (show)

See Also:


Attachments
Cleanup (31.11 KB, patch)
2013-09-10 18:48 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Addressed Daniel's comment (31.12 KB, patch)
2013-09-10 21:26 PDT, Ryosuke Niwa
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2013-09-10 00:27:36 PDT
We should make it smaller!
Comment 1 Ryosuke Niwa 2013-09-10 18:48:54 PDT
Created attachment 211274 [details]
Cleanup
Comment 2 Ryosuke Niwa 2013-09-10 19:53:17 PDT
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 3 Daniel Bates 2013-09-10 20:04:21 PDT
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 4 Ryosuke Niwa 2013-09-10 20:13:15 PDT
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.
Comment 5 Ryosuke Niwa 2013-09-10 21:26:56 PDT
Created attachment 211278 [details]
Addressed Daniel's comment
Comment 6 Daniel Bates 2013-09-11 00:08:46 PDT
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)?
Comment 7 Ryosuke Niwa 2013-09-11 01:30:58 PDT
(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.
Comment 8 Ryosuke Niwa 2013-09-11 01:42:15 PDT
Committed r155511: <http://trac.webkit.org/changeset/155511>
Comment 9 Darin Adler 2013-09-11 09:19:33 PDT
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.
Comment 10 Anders Carlsson 2013-09-11 11:05:17 PDT
(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!
Comment 11 Ryosuke Niwa 2013-09-11 12:05:14 PDT
Addressed that in http://trac.webkit.org/changeset/155550.

Somehow the change log is missing the bug number.
Comment 12 Alexey Proskuryakov 2013-09-11 12:05:57 PDT
Why not "const" only?
Comment 13 Ryosuke Niwa 2013-09-11 12:06:27 PDT
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]