Bug 22807 – Cygwin/X WebKit/Gtk port

• Component: WebKitGTK
• Status: RESOLVED INVALID

[Yaakov Selkowitz]

I'm attaching a series of patches built against the latest nightly (r39090) which help port WebKit/Gtk to Cygwin/X.

[Yaakov Selkowitz]

Created attachment 25948 [details]
autogen NOCONFIGURE patch

* autogen.sh: Do not run configure if NOCONFIGURE is set.

[Yaakov Selkowitz]

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.

[Yaakov Selkowitz]

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.

[Yaakov Selkowitz]

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.

[Yaakov Selkowitz]

Created attachment 25957 [details]
PluginDatabase patch

* WebCore/plugins/PluginDatabase.cpp: Cygwin does not use .so for plugins.

[Yaakov Selkowitz]

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.

[Holger Freyther]

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?

[Darin Adler]

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.

[Darin Adler]

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.

[Darin Adler]

Comment on attachment 25950 [details]
no-undefined patch

Needs ChangeLog. Please re-post the patch including a change log.

[Darin Adler]

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.

[Darin Adler]

Comment on attachment 25971 [details]
PLATFORM(CYGWIN) patch, take two

These changes look OK. Please re-post with a ChangeLog.

[Martin Robinson]

I don't think this is something we are interested in maintaining and the patches are long out of date.