[webkit-reviews] review denied: [Bug 8515] Linux porting compile bug : [Attachment 8741] patch and new files for linux port (tar.gz)

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 8515: Linux porting compile bug
http://bugzilla.opendarwin.org/show_bug.cgi?id=8515

Attachment 8741: patch and new files for linux port (tar.gz)
http://bugzilla.opendarwin.org/attachment.cgi?id=8741&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Overall looks great!

A few things need fixing before we can land all of this. Ideally I'd like to
separate out the part with no comments and land it first, so we can then
discuss just the parts that have issues. I reviewed only the patch this time
around, not the new files.

The large list of VK_ macros in PlatformKeyboardEvent.h under the PLATFORM(GDK)
guard seems strange. Why do those need to be there? Are they GDK-specific?

I'd suggest putting those in a separate header because they are as big as the
rest of the file combined.

Also, I suggest using C++ constants instead of #define for such things.

The formatting and indentation of the GDK PlatformKeyboardEvent constructors
doesn't match the rest of the file. Lets not have that large inline constructor
at all, and please indent those to match the rest of the class definition.

I'd also prefer to not have the PlatformWheelEvent constructor inline in the
header.

A number of files have whitespace changes unrelated to the GDK changes. I'd
like to see the patch without those. For example, the blank lines removed from
TransferJob.h.

ScrollView::setFrameGeometry is not properly indented either.

For code like this:

+      Widget(GdkDrawable *drawable);
+      virtual void setDrawable(GdkDrawable *drawable);
+      GdkDrawable *drawable() const;

our style is:

+      Widget(GdkDrawable*);
+      virtual void setDrawable(GdkDrawable*);
+      GdkDrawable* drawable() const;

The * goes next to the type name, and we don't give names to parameters unless
they are needed for clarity.

This section from config.h seems wrong:

+#if PLATFORM(GDK)
+#include <math.h>
+#include <assert.h>
+#endif

If we're going to include those in the config.h file, then it should be
unconditionally for all platforms. But I think this is working around missing
<math.h> and <assert.h> in a few files that we could address by just adding it
to those files.

>From XPathValue.cpp:

+#if PLATFORM(GDK)
+#include <math.h>
+#endif

If this is needed, we should not put it inside an #f.

In maketokenizer:

+if( $flex_version = ~/2.5/ ) {

The regular expression above means 2 followed by anything followed by 5. I
think instead you want /version 2\.5/ because you don't want to match versions
that just happen to have "2.5" in them. But I know that's not correct, because
the flex version on OS X 10.4 is 2.5.4 -- so this is not the right check. We
should discuss this change on IRC or the mailing list.

+#if PLATFORM(GDK)
+#include <errno.h>
+#endif

Again, if this is needed it should not be in an ifdef.

Could you explain the change to make-generated-sources.sh? Why is it needed?

Please include a ChangeLog entry to answer questions like these.