[webkit-reviews] review denied: [Bug 18428] Patch to support building wxWebKit on mingw : [Attachment 21460] updated mingw port patch

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied Alexander Vassilev
<avasilev at voipgate.com>'s request for review:
Bug 18428: Patch to support building wxWebKit on mingw
https://bugs.webkit.org/show_bug.cgi?id=18428

Attachment 21460: updated mingw port patch
https://bugs.webkit.org/attachment.cgi?id=21460&action=edit

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
I can't comment on the Bakefile or WX-specific changes, but there are some
other issues that need to be addressed before this patch may land.

>--- JavaScriptCore/ChangeLog	(revision 34300)
>+++ JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,16 @@
>+2008-06-02  Alexander Vassilev  <avasilev at voipgate.com>
>+
>+	  Reviewed by NOBODY (OOPS!).
>+
>+	  * jscore.bkl:
>+	  * kjs/DateMath.cpp:
>+	  (KJS::getLocalTime):
>+	  * kjs/testkjs.cpp:
>+	  * wtf/Platform.h:
>+	  * wtf/Threading.h:
>+	  (WTF::atomicIncrement):
>+	  (WTF::atomicDecrement):

Please reference the bug number, but title and bug URL in every ChangeLog
entry.	See other entries for reference.

Also, please provide a summary statement about the changes, or comment on each
individual file (or method) change.  Again, see other ChangeLog entries for
examples.

>-    #if COMPILER(MSVC7)
>+    #if (COMPILER(MSVC7) || COMPILER(MINGW32))

We don't normally add outer parenthesis in this case since they're not
necessary.

>Index: JavaScriptCore/kjs/testkjs.cpp
>===================================================================
>--- JavaScriptCore/kjs/testkjs.cpp	(revision 34267)
>+++ JavaScriptCore/kjs/testkjs.cpp	(working copy)
>@@ -53,7 +53,7 @@
> #endif
> 
> #if PLATFORM(WIN_OS)
>-#include <crtdbg.h>
>+//#include <crtdbg.h>
> #include <windows.h>
> #endif
> 

We don't commit commented-out code.  This probably needs #if/#endif macro
protection.

Also note that testkjs.cpp has changed to Shell.cpp, so this patch has some
minor "bit rot".

>+/*in mingw windows header winbase.h InterlckedIncrement() and the like are
defined to accept plain LPVOID, not volatile,
>+    so we avoid compile error, but what should really be fixed is hte mingw
header! */

Please format comments like others found in the source, specifically use
capital letters to start sentences and add spaces betweeen "/*" and the text.

>+2008-06-02  Alexander Vassilev  <avasilev at voipgate.com>
>+
>+	  Reviewed by NOBODY (OOPS!).
>+
>+	  Port to the MINGW compiler and the cygwin environment. Fix for time
functions that
>+	  are not available under MINGW. Removed includes for gdiplus system
headers that are
>+	  not necessary but add gdiplus dependency which MINGW does not have.
Changed order of
>+	  includes in FormDataStreamCurl.cpp since stdint.h gets included
before the point
>+	  where we define the macro __STDC_LIMIT_MACROS, so we did not get
SIZE_MAX defined
>+
>+	  * loader/FTPDirectoryDocument.cpp:
>+	  (WebCore::processFileDateString):
>+	  * loader/FTPDirectoryParser.cpp:
>+	  * platform/network/curl/FormDataStreamCurl.cpp:
>+	  * platform/wx/wxcode/win/non-kerned-drawing.cpp:
>+	  * webcore-base.bkl:
>+	  * webcore-wx.bkl:

Please include bug number, bug title and bug URL in the ChangeLog.

The description of the changes is good, but it may be more useful to denote
individual changes in the list of files.

>Index: WebCore/loader/FTPDirectoryDocument.cpp
>===================================================================
>--- WebCore/loader/FTPDirectoryDocument.cpp	(revision 34267)
>+++ WebCore/loader/FTPDirectoryDocument.cpp	(working copy)
>@@ -264,7 +264,12 @@ static String processFileDateString(cons
>     // If it was today or yesterday, lets just do that - but we have to
compare to the current time
>     struct tm now;
>     time_t now_t = time(NULL);
>-    localtime_r(&now_t, &now);
>+	#ifndef __MINGW32__
>+		localtime_r(&now_t, &now);
>+	#else
>+	/*MINGW doesnt have localtime_r() or localtime_s() yet*/
>+		now = *localtime(&now_t);
>+	#endif
>     
>     // localtime does "year = current year - 1900", compensate for that for
readability and comparison purposes
>     now.tm_year += 1900;

Don't re-indent code when adding #ifndef/#else/#endif macros.

Please add spaces between "/*' and "*/" and comment text.  (This comment is
kind of stating the obvious as well; it may not be needed.)

>Index: WebCore/platform/network/curl/FormDataStreamCurl.cpp
>===================================================================
>--- WebCore/platform/network/curl/FormDataStreamCurl.cpp	(revision
34267)
>+++ WebCore/platform/network/curl/FormDataStreamCurl.cpp	(working copy)
>@@ -27,8 +27,6 @@
>  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  */
> 
>-#include "config.h"
>-
> // We need to define __STDC_LIMIT_MACROS to define SIZE_MAX.
> #ifndef __STDC_LIMIT_MACROS
> #define __STDC_LIMIT_MACROS
>@@ -38,6 +36,7 @@
> #include <stdint.h>
> #endif
> 
>+#include "config.h"
> #include "FormDataStreamCurl.h"
> 
> #include "CString.h"

The #include "config.h" line is a special case; please do not move it.

>-#include "gdiplus.h"
>- 
>+//#include "gdiplus.h"
>+

Again, we don't commit commented-out code.  Please use a #if/#endif guard.

>Index: WebKit/wx/ChangeLog
>===================================================================
>--- WebKit/wx/ChangeLog	(revision 34300)
>+++ WebKit/wx/ChangeLog	(working copy)
>@@ -1,3 +1,16 @@
>+2008-06-02  Alexander Vassilev  <avasilev at voipgate.com>
>+
>+	  Reviewed by NOBODY (OOPS!).
>+	  This patch ports the wxWebkit build system to support the MINGW
compiler
>+	  in combiantion with the cygwin environment
>+	  
>+	  * Bakefiles.bkgen:
>+	  * WebView.cpp:
>+	  * dependencies.bkl:
>+	  * presets/wxwebkit.bkl:
>+	  * wxwebkit.bkl:
>+	  * wxwk-settings.bkl:

Please reference bug number, bug title and bug URL; leave a blank line after
the "Reviewed by" line.


>+#ifdef __WXMSW__
>+#include <wx/msw/private.h> //for wxGetInstance()
>+#endif

Please leave a space between "//" and the comment text.

>Index: WebKitTools/ChangeLog
>===================================================================
>--- WebKitTools/ChangeLog	(revision 34300)
>+++ WebKitTools/ChangeLog	(working copy)
>@@ -1,3 +1,12 @@
>+2008-06-02  Alexander Vassilev  <avasilev at voipgate.com>
>+
>+	  PAtch to the wxWebkit build system to support building with the MINGW
compiler
>+	  and cygwin environment
>+	  Reviewed by NOBODY (OOPS!).
>+
>+	  * wx/browser/browser.bkl:
>+	  * wx/build-wxwebkit:

Same ChangeLog comments apply.

r- for comment formatting and commented out code in patch.

Please address these issues, and resubmit the patch for review.  Thanks for
contributing, Alexander!