Bug 22807

Summary: Cygwin/X WebKit/Gtk port
Product: WebKit Reporter: Yaakov Selkowitz <yselkowi>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED INVALID    
Severity: Normal CC: mrobinson
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Other   
URL: http://cygwinports.org/
Attachments:
Description Flags
autogen NOCONFIGURE patch
darin: review-
PLATFORM(CYGWIN) patch
none
no-undefined patch
darin: review-
int/uint32_t patch
darin: review-
PluginDatabase patch
none
PLATFORM(CYGWIN) patch, take two darin: review-

Description Yaakov Selkowitz 2008-12-11 09:27:39 PST
I'm attaching a series of patches built against the latest nightly (r39090) which help port WebKit/Gtk to Cygwin/X.
Comment 1 Yaakov Selkowitz 2008-12-11 09:29:23 PST
Created attachment 25948 [details]
autogen NOCONFIGURE patch

* autogen.sh: Do not run configure if NOCONFIGURE is set.
Comment 2 Yaakov Selkowitz 2008-12-11 09:32:57 PST
Created attachment 25949 [details]
PLATFORM(CYGWIN) patch

* JavaScriptCore/wtf/Platform.h: Define PLATFORM(CYGWIN) and PLATFORM(UNIX) on Cygwin.
JavaScriptCore/API/JSStringRef.h: Fix JSChar typedef on Cygwin.
JavaScriptCore/runtime/DateMath.h: No tm_gmtoff on Cygwin.
WebKitTools/DumpRenderTree/DumpRenderTree.h: No std::wstring on Cygwin.
Comment 3 Yaakov Selkowitz 2008-12-11 09:35:15 PST
Created attachment 25950 [details]
no-undefined patch

* configure.ac: Define OS_CYGWIN for Cygwin.
GNUmakefile.am (no_undefined): Define when OS_CYGWIN.
WebKitTools/GNUmakefile.am (TestNetscapePlugin_libtestnetscapeplugin_la_LDFLAGS): Define.
Comment 4 Yaakov Selkowitz 2008-12-11 09:39:02 PST
Created attachment 25951 [details]
int/uint32_t patch

This patch fixes mismatch errors between unsigned int and uint32_t on Cygwin.  It will need to be tested on other platforms before committing.

* JavaScriptCore/interpreter/Interpreter.cpp:
WebCore/platform/graphics/GlyphPageTreeNode.cpp:
WebCore/plugins/npfunctions.h: Fix mismatching between unsigned int and uint32_t.
Comment 5 Yaakov Selkowitz 2008-12-11 12:15:28 PST
Created attachment 25957 [details]
PluginDatabase patch

* WebCore/plugins/PluginDatabase.cpp: Cygwin does not use .so for plugins.
Comment 6 Yaakov Selkowitz 2008-12-11 17:58:43 PST
Created attachment 25971 [details]
PLATFORM(CYGWIN) patch, take two

Last patch was incorrect, this works (supersedes patches in comment 2 and comment 5):

* JavaScriptCore/wtf/Platform.h: Define PLATFORM(CYGWIN) and PLATFORM(UNIX) on
Cygwin.
* JavaScriptCore/API/JSStringRef.h: Fix JSChar typedef on Cygwin.
* JavaScriptCore/runtime/DateMath.h: No tm_gmtoff on Cygwin.
* WebCore/plugins/PluginDatabase.cpp: Cygwin plugins have .dll extension.
* WebKitTools/DumpRenderTree/DumpRenderTree.h: No std::wstring on Cygwin.
Comment 7 Holger Freyther 2008-12-12 08:13:14 PST
Comment on attachment 25951 [details]
int/uint32_t patch

> --- origsrc/WebKit-r39090/JavaScriptCore/interpreter/Interpreter.cpp	2008-12-07 21:56:15.000000000 -0600
> +++ src/WebKit-r39090/JavaScriptCore/interpreter/Interpreter.cpp	2008-12-11 00:34:55.471750000 -0600
> @@ -260,7 +260,7 @@
>  
>      if (rightIsNumber & leftIsString) {
>          RefPtr<UString::Rep> value = JSImmediate::isImmediate(v2) ?
> -            concatenate(asString(v1)->value().rep(), JSImmediate::getTruncatedInt32(v2)) :
> +            concatenate(asString(v1)->value().rep(), static_cast<int>(JSImmediate::getTruncatedInt32(v2))) :

could you please attach a build error?
Comment 8 Darin Adler 2008-12-12 10:13:04 PST
Comment on attachment 25951 [details]
int/uint32_t patch

What are these changes about? Are you actually porting to a platform where uint32_t and unsigned are not the same type? What does the compiler error look like?

Also, please don't post patches for review without include a change log.
Comment 9 Darin Adler 2009-01-02 10:59:52 PST
Comment on attachment 25948 [details]
autogen NOCONFIGURE patch

Patches need to include ChangeLog entries too. Please put this back up for review with a ChangeLog entry included.
Comment 10 Darin Adler 2009-01-02 11:00:15 PST
Comment on attachment 25950 [details]
no-undefined patch

Needs ChangeLog. Please re-post the patch including a change log.
Comment 11 Darin Adler 2009-01-02 11:07:20 PST
Comment on attachment 25951 [details]
int/uint32_t patch

Patch needs a ChangeLog.

We'll need more rationale for this change.

Throughout WebCore and JavaScriptCore, we use int and unsigned to mean 32 bit integers. We are not moving to a design where we use the <stdint.h> types.

Further, both WebCore and JavaScriptCore have been compiled with gcc and with Microsoft's C compiler.

So I don't understand what's driving these changes? Are these fixing warnings with a particular compiler? What version?

> -            concatenate(asString(v1)->value().rep(), JSImmediate::getTruncatedInt32(v2)) :
> +            concatenate(asString(v1)->value().rep(), static_cast<int>(JSImmediate::getTruncatedInt32(v2))) :

This change seems OK, but I'd like to understand why you see this warning and no one else does.

> -        unsigned i;
> +        uint32_t i;
>  
>          bool isUInt32 = JSImmediate::getUInt32(subscript, i);

This change seems good (and the others like it), with no argument against it. But I think it points out the trouble we get into when we start using uint32_t in a few places. I think a better change to be consistent across the project would be using unsigned rather than uint32_t in the definition of the getUInt32 function.

But again, I'm puzzled about why you're seeing this an no one else is.

> -                    int from = max(0, range.from() - static_cast<int>(start));
> -                    int to = 1 + min(range.to() - static_cast<int>(start), static_cast<int>(GlyphPage::size) - 1);
> +                    int from = max(0, static_cast<int>(range.from() - start));
> +                    int to = 1 + min(static_cast<int>(range.to() - start), static_cast<int>(GlyphPage::size) - 1);

I don't understand this change. Is there really a compiler that warns when you subtract an unsigned from an int and then use that as an int? What compiler? Maybe the issue is simply that you need to synchronize the list of ignored warnings with the other Windows port makefiles?

On the other hand, the change seems harmless so we could do this one without pondering further.

>  typedef NPObject* (*NPN_CreateObjectProcPtr) (NPP, NPClass *aClass);
>  typedef NPObject* (*NPN_RetainObjectProcPtr) (NPObject *obj);
>  typedef void (*NPN_ReleaseObjectProcPtr) (NPObject *obj);
> -typedef bool (*NPN_InvokeProcPtr) (NPP npp, NPObject *obj, NPIdentifier methodName, const NPVariant *args, unsigned argCount, NPVariant *result);
> -typedef bool (*NPN_InvokeDefaultProcPtr) (NPP npp, NPObject *obj, const NPVariant *args, unsigned argCount, NPVariant *result);
> +typedef bool (*NPN_InvokeProcPtr) (NPP npp, NPObject *obj, NPIdentifier methodName, const NPVariant *args, uint32_t argCount, NPVariant *result);
> +typedef bool (*NPN_InvokeDefaultProcPtr) (NPP npp, NPObject *obj, const NPVariant *args, uint32_t argCount, NPVariant *result);
>  typedef bool (*NPN_EvaluateProcPtr) (NPP npp, NPObject *obj, NPString *script, NPVariant *result);
>  typedef bool (*NPN_GetPropertyProcPtr) (NPP npp, NPObject *obj, NPIdentifier  propertyName, NPVariant *result);
>  typedef bool (*NPN_SetPropertyProcPtr) (NPP npp, NPObject *obj, NPIdentifier  propertyName, const NPVariant *value);

This is a public header, shared with other projects such as Mozilla, so changing this is inappropriate unless it absolutely must be done. Please don't make this change.
Comment 12 Darin Adler 2009-01-02 11:08:30 PST
Comment on attachment 25971 [details]
PLATFORM(CYGWIN) patch, take two

These changes look OK. Please re-post with a ChangeLog.
Comment 13 Martin Robinson 2014-03-25 15:18:43 PDT
I don't think this is something we are interested in maintaining and the patches are long out of date.