[webkit-reviews] review denied: [Bug 8515] Linux porting compile bug : [Attachment 8762] Gdk build patch

[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 8762: Gdk build patch
http://bugzilla.opendarwin.org/attachment.cgi?id=8762&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Still a few things that need fixing. Some minor, but some that would break
Macintosh and WIndows build.

+    typedef GdkCursor * PlatformCursor;
+    void setFont(cairo_t *cr) const;
+    TransferJobInternal * getInternal() { return d;}
+      Widget(GdkDrawable *drawable);
+      virtual void setDrawable(GdkDrawable *drawable);
+      GdkDrawable *drawable() const;

Should not have a space before the *.

+    Glyph getGlyphIndex( UChar c ) const { return m_font.index (c); }

Should not have spaces around the UChar c.

Why the patch to make-charset-table.pl?

+#include "config.h"

Header files must *not* include config.h, so it's wrong to add this to headers.
Instead, cpp files must include config.h.

-    
     TransferJobClient* client() const;
-
     void receivedResponse(PlatformResponse);
 
 private:
     void assembleResponseHeaders() const;
     void retrieveCharset() const;
-
     TransferJobInternal* d;

Why remove these newlines?

+#if WIN32 || PLATFORM(GDK)
	 ScrollView();
	 ~ScrollView();
+	 virtual void setFrameGeometry(const IntRect&);

Why does WIN32 now have a setFrameGeometry virtual function in ScrollView? I
think this will break the WIN32 compile.

The implementation of GraphicsContext::roundToDevicePixels is incorrect. The
purpose of that function is to round a rectangle to pixel boundaries in device
space and then convert it back to user space. But the function in this patch
converts a rectangle to device space, which is something different.

 #include "config.h"
+#include <math.h>
 #include "XPathValue.h"

The include should go below inside the XPATH_SUPPORT ifdef. In general, the
first include in every .cpp file should be the corresponding .h file, which
serves as a test that the .h file includes enough to stand alone.

+$flex_version=`flex --version`;
+if( $flex_version = ~/2\.5/ ) {

As we discussed, we can't land this change as-is since it will break the
Macintosh build.

+#note ENCODINGS_PREFIX needs to be a ws string so it does not turn into
+#a null value

We normally put spaces after the "#" and before the comment.