[Webkit-unassigned] [Bug 20746] Port WebKit to Qt on Windows CE

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=20746


eric at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #23338|review?                     |review-
               Flag|                            |




------- Comment #18 from eric at webkit.org  2008-09-15 14:55 PDT -------
(From update of attachment 23338)
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. :)


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.