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

[bugzilla-daemon at webkit.org]

Chiculita Alexandru <achicu at adobe.com> has asked  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 97561: Updated patch
https://bugs.webkit.org/attachment.cgi?id=97561&action=review

------- Additional Comments from Chiculita Alexandru <achicu at adobe.com>
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.