[webkit-reviews] review denied: [Bug 20746] Port WebKit to Qt on Windows CE : [Attachment 23338] Make the Qt port of WebKit compile on Windows CE (updated for WINCE -> WIN_CE)

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Simon Hausmann <hausmann at webkit.org>'s
request for review:
Bug 20746: Port WebKit to Qt on Windows CE
https://bugs.webkit.org/show_bug.cgi?id=20746

Attachment 23338: Make the Qt port of WebKit compile on Windows CE (updated for
WINCE -> WIN_CE)
https://bugs.webkit.org/attachment.cgi?id=23338&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
In general this looks OK. I can't say I've given this the most thorough review.
 The patch is a bit too big. :(

Tab:
+    time_t timet = mktime(&tmtime);

This feels strange, that generally the check is a compile check except for
WIN_CE:
+#if COMPILER(MSVC7) || COMPILER(MINGW) || PLATFORM(WIN_CE)

Makes me wonder which set of checks is wrong...

Again, should the MSVC one be WIN_OS instead?
+#if COMPILER(MSVC) && !PLATFORM(WIN_CE)


It seems you do this in a couple places:

+#if PLATFORM(WIN_CE)
+	 /* On Windows CE there's no abort() and on regular Windows
+	    abort() exits with exit code 3. */
+	 exit(3);
+#else
	 abort();
+#endif

Maybe abort() should just be defined as an inline in some high-level header?

Again, this seems strange, but maybe this is the best check to go with?
+#if !COMPILER(MSVC7) && !PLATFORM(WIN_CE)

Seems this should be an #else clause on the above HAVE(ERRNO_H):
+#if PLATFORM(WIN_CE)
+#define NO_ERRNO
+#endif
or?  Are there platforms which have errno but not in ERRNO_H?

I'm not sure whether we'd generally try to convert such a file to WebKit style
or not:
+++ b/JavaScriptCore/os-wince/ce_time.cpp
probably not.  The license on that file looks fine.

Redundant, since you included it in Assertions.h as well:
+
+#if PLATFORM(WIN_CE)
+#include <windows.h>
+#endif
+

Again, seems like the MSVC checks shoudl really be WIN_OS checks:
+#if COMPILER(MSVC) && !PLATFORM(WIN_CE)
 #ifndef WINVER

Is there any cleaner way to do this?  In Chromium, we avoid including windows.h
in any header.	I assume you'd probably want to do the same.  I think we avoid
including it to avoid compile slowness or object file bloat.  But maybe I'm
misinformed.

If you defined this in FastMalloc.h instead of FastMalloc.cpp, all .cpp files
in WebCore would get access to your abort() function:
+#if PLATFORM(WIN_CE)
+/* On Windows CE there's no abort() and on regular Windows
+   abort() exits with exit code 3. */
+static void abort()
+{
+    exit(3);
+}
+#endif

I'm somewhat confused by this statement:
+/* PLATFORM(WIN_CE) && PLATFORM(QT)
+   We can not determine the endianess at compile time. For
+   Qt for Windows CE the endianess is specified in the
+   device specific makespec
+*/

Why did you add:
+#define HAVE_SYS_TYPES_H 1
in 3 places in Platform.h?

Another place that I wonder why 2 of these are compiler checks and one is an OS
check:
+#if COMPILER(MINGW) || COMPILER(MSVC7) || PLATFORM(WIN_CE)

I guess I'll r- this, requesting a second revision (possibly broken into
smaller pieces).  I guess an alternative would be to find someone more
windows-savvy who'd like to review the whole thing for you. :)