"WebCore/css/makeprop.pl" and "WebCore/css/makevalues.pl" should take ENABLE_ flags into account

• Component: CSS
• Status: RESOLVED FIXED

[Alexandru Chiculita]

WebCore/css/makeprop.pl and WebCore/css/makevalues.pl should use the ENABLE_ flags to toggle properties on and off.

[Alexandru Chiculita]

Started discussion on webkit-dev list:
https://lists.webkit.org/pipermail/webkit-dev/2011-June/016955.html

[Alexandru Chiculita]

Created attachment 96088 [details]
Patch

[WebKit Review Bot]

Comment on attachment 96088 [details]
Patch

Attachment 96088 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8765912

[Alexandru Chiculita]

Created attachment 96092 [details]
Patch

fixed gyp script

[WebKit Review Bot]

Comment on attachment 96092 [details]
Patch

Attachment 96092 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8770413

[Alexandru Chiculita]

Created attachment 96218 [details]
Patch

Value keywords need to be lowercased and I accidentally removed that in the previous patch for the gyp python script.

[Alexandru Chiculita]

Created attachment 96402 [details]
patch

Removed "require ../bindings/script" from perl files, and added perl -Ibindings/script flag instead.

Used C++ preprocessor. The same approach is used by .idl files and other .in files. That required changing the commenting style from "#" to "//".

[Ojan Vafai]

I haven't looked closely at the code, but the ChangeLog description is very sparse. There's a lot of changes here and it would be good to have more of the high-level  or tricky changes described in the ChangeLog description. For example, why change all the "#" comments to "//" comments?

[Eric Seidel (no email)]

Comment on attachment 96402 [details]
patch

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.

[Tony Chang]

Comment on attachment 96402 [details]
patch

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

> Source/WebCore/DerivedSources.make:656
> +	perl -I$(WebCore)/bindings/scripts "$(WebCore)/css/makeprop.pl" --defines "$(FEATURE_DEFINES)"

Why isn't the preprocessor flag passed here?

> Source/WebCore/WebCore.gyp/scripts/action_csspropertynames.py:78
> +    (outputs, options, inputs) = SplitArgsIntoSections(args[1:])

Nit: No parentheses on the left side of the =.

> Source/WebCore/WebCore.gyp/scripts/action_csspropertynames.py:128
>      for inFilePath in inFiles:
>          inFile = open(inFilePath)
>          for line in inFile:

This for loop looks like a file copy.  Can we just copy the file directly (e.g., shutil.copyfile)?

> Source/WebCore/WebCore.gyp/scripts/action_cssvaluekeywords.py:134
>              line = line.lower()

If we moved the lower casing into the perl scripts, we could make this a file copy as well.

[Tony Chang]

(In reply to comment #8)
> For example, why change all the "#" comments to "//" comments?

The preprocess ignores // comments, but previously the makevalues.pl and makeprop.pl would strip # comments.


(In reply to comment #9)
> (From update of attachment 96402 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96402&action=review
> 
> > 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?

The comment stripping is now handled by the preprocessor.  The duplicate checking is now in makeprop.pl and makevalues.pl.

[Alexandru Chiculita]

Created attachment 97561 [details]
Updated patch

Thanks for review. I've updated the patch.

@Ojan:
> I haven't looked closely at the code, but the ChangeLog description is very sparse. There's a lot of changes here and it would be good to have more of the high-level  or tricky changes described in the ChangeLog description. For example, why change all the "#" comments to "//" comments?
Updated the ChangeLog.

@Eric:
> I would have written this as a helper function.
Added method called SplitDefines in both files. There are other methods that seem to be copy-pasted in those scripts. Should I add another bug to fix all of them?

> Why is it OK to remove this?
The comments are now stripped by the C++ preprocessor. Duplicate values are now handled in the perl scripts, so that we don't have to handle them in the makefiles anymore.

@Tony:
>> Source/WebCore/DerivedSources.make:656
>> +	perl -I$(WebCore)/bindings/scripts "$(WebCore)/css/makeprop.pl" --defines "$(FEATURE_DEFINES)"
>
>Why isn't the preprocessor flag passed here?
I just followed the convention that makefile. The "--preprocessor" argument is not used for any other pre-processed file in DerivedSources.make. For example the  ".idl" file generator doesn't use the '--preprocessor' flag. However, it works because the preprocessor script has harcoded default paths to gcc in case the '--preprocessor' is null.

> Nit: No parentheses on the left side of the =.
Fixed that.

> This for loop looks like a file copy.  Can we just copy the file directly (e.g., shutil.copyfile)?
Updated to use shutil.copyfileobj as we need to concatenate the source files in a single merged one.

> If we moved the lower casing into the perl scripts, we could make this a file copy as well.
Ok. Also removed it from other project files.

[WebKit Review Bot]

Attachment 97561 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1

Source/WebCore/WebCore.gyp/scripts/action_cssvaluekeywords.py:80:  expected 2 blank lines, found 1  [pep8/E302] [5]
Source/WebCore/WebCore.gyp/scripts/action_csspropertynames.py:76:  expected 2 blank lines, found 1  [pep8/E302] [5]
Source/WebCore/WebCore.gyp/scripts/action_csspropertynames.py:87:  trailing whitespace  [pep8/W291] [5]
Total errors found: 3 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexandru Chiculita]

Created attachment 97563 [details]
Patch

Fixed check-webkit-style warnings.

[Tony Chang]

Comment on attachment 97563 [details]
Patch

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

> Source/WebCore/CMakeLists.txt:2193
> +    COMMAND cat ${WebCore_CSS_PROPERTY_NAMES} > ${DERIVED_SOURCES_WEBCORE_DIR}/CSSPropertyNames.in

I would probably use cp, but cat seems OK too.

> Source/WebCore/WebCore.gyp/scripts/action_csspropertynames.py:129
> +    # Concatenate all the input files

Nit: Add a period to the end of this comment.

> Source/WebCore/WebCore.gyp/scripts/action_cssvaluekeywords.py:133
> +    # Concatenate all the input files

Nit: Add a period.

> Source/WebCore/css/makeprop.pl:27
>  use strict;
>  use warnings;
> +use Getopt::Long;
> +use preprocessor;

Nit: Sort these alphabetically.

> Source/WebCore/css/makevalues.pl:27
>  use strict;
>  use warnings;
> +use Getopt::Long;
> +use preprocessor;

Nit: alphabetize

> Source/WebCore/css/makevalues.pl:44
> +  # CSS values need to be lower case

Nit: Add a period.

[Alexandru Chiculita]

Created attachment 97759 [details]
Patch

Fixed comments.

[WebKit Review Bot]

Comment on attachment 97759 [details]
Patch

Clearing flags on attachment: 97759

Committed r89362: <http://trac.webkit.org/changeset/89362>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexis Menard (darktears)]

(In reply to comment #18)
> All reviewed patches have been landed.  Closing bug.

-cssprops.commands = perl -ne \"print lc\" ${QMAKE_FILE_NAME} $${DASHBOARDSUPPORTCSSPROPERTIES} $${EXTRACSSPROPERTIES} > $${WC_GENERATED_SOURCES_DIR}/${QMAKE_FILE_BASE}.in && cd $$WC_GENERATED_SOURCES_DIR && perl $$cssprops.wkScript && $(DEL_FILE) ${QMAKE_FILE_BASE}.in ${QMAKE_FILE_BASE}.gperf
+cssprops.commands = cat ${QMAKE_FILE_NAME} $${DASHBOARDSUPPORTCSSPROPERTIES} $${EXTRACSSPROPERTIES} > $${WC_GENERATED_SOURCES_DIR}/${QMAKE_FILE_BASE}.in && cd $$WC_GENERATED_SOURCES_DIR && perl -I$$PWD/bindings/scripts $$cssprops.wkScript --defines \"$${FEATURE_DEFINES_JAVASCRIPT}\" --preprocessor \"$${QMAKE_MOC} -E\" ${QMAKE_FILE_NAME} && $(DEL_FILE) ${QMAKE_FILE_BASE}.in ${QMAKE_FILE_BASE}.gperf

broke the Qt port on Windows with Visual Studio. "cat" doesn't exist on Windows.

Unfortunately the Qt Windows bots are built with Cygwin so cat exists.

[Alexandru Chiculita]

(In reply to comment #19)

> broke the Qt port on Windows with Visual Studio. "cat" doesn't exist on Windows.
> 
> Unfortunately the Qt Windows bots are built with Cygwin so cat exists.

Sorry about that. I've added https://bugs.webkit.org/show_bug.cgi?id=63924 and posted a patch. Can you please review and submit it please?