[webkit-reviews] review denied: [Bug 62114] "WebCore/css/makeprop.pl" and "WebCore/css/makevalues.pl" should take ENABLE_ flags into account : [Attachment 96402] patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jun 16 10:38:29 PDT 2011


Eric Seidel <eric at webkit.org> has denied Chiculita Alexandru
<achicu at adobe.com>'s request for review:
Bug 62114: "WebCore/css/makeprop.pl" and "WebCore/css/makevalues.pl" should
take ENABLE_ flags into account
https://bugs.webkit.org/show_bug.cgi?id=62114

Attachment 96402: patch
https://bugs.webkit.org/attachment.cgi?id=96402&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=96402&action=review

> Source/WebCore/WebCore.gyp/scripts/action_csspropertynames.py:86
> +    # The defines come in as one flat string. Split it up into distinct
arguments.
> +    if '--defines' in options:
> +	   definesIndex = options.index('--defines')
> +	   if definesIndex + 1 < len(options):
> +	       splitOptions = shlex.split(options[definesIndex + 1])
> +	       if splitOptions:
> +		   options[definesIndex + 1] = ' '.join(splitOptions)

I would have written this as a helper function.

> Source/WebCore/WebCore.gyp/scripts/action_csspropertynames.py:-129
> -	       line = line.rstrip()
> -	       if line.startswith('#'):
> -		   line = ''
> -	       if line == '':
> -		   continue
> -	       if line in lineDict:
> -		   raise KeyError, 'Duplicate value %s' % line
> -	       lineDict[line] = True

Why is it OK to remove this?

> Source/WebCore/WebCore.gyp/scripts/action_cssvaluekeywords.py:90
> +    # The defines come in as one flat string. Split it up into distinct
arguments.
> +    if '--defines' in options:
> +	   definesIndex = options.index('--defines')
> +	   if definesIndex + 1 < len(options):
> +	       splitOptions = shlex.split(options[definesIndex + 1])
> +	       if splitOptions:
> +		   options[definesIndex + 1] = ' '.join(splitOptions)

Now for sure thsi should be a helper.  Even if it remains copy/pasted at least
some day we could chose to share it if they both use the same named function.

> Source/WebCore/WebCore.gyp/scripts/action_cssvaluekeywords.py:-135
> -	       line = line.rstrip()
> -	       if line.startswith('#'):
> -		   line = ''
> -	       if line == '':
> -		   continue
>	       line = line.lower()
> -	       if line in lineDict:
> -		   raise KeyError, 'Duplicate value %s' % line
> -	       lineDict[line] = True

Same question as before.


More information about the webkit-reviews mailing list