[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] Add xwidget webkit support for macOS Cocoa

From: Paul Eggert
Subject: Re: [PATCH] Add xwidget webkit support for macOS Cocoa
Date: Sat, 25 May 2019 10:47:20 -0700
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1

Thanks for doing all that work! Although I have no expertise in macOS I have a few minor comments about the patch from the perspective of someone who builds and ports Emacs, and who wants to help navigate this patch through the Emacs build bureaucracy. First, when I tried to use "git am" to apply the patch, I got these diagnostics about white-space problems that should be looked at:

Applying: Add xwidget webkit support for macOS Cocoa
.git/rebase-apply/patch:594: space before tab in indent.
             for detail information about `NSApplicationDefinedMask'. -->
warning: 1 line adds whitespace errors.
nextstep/templates/Info.plist.in:682: space before tab in indent.
+            for detail information about `NSApplicationDefinedMask'. -->

Next, when I tried to build by doing "./configure --enable-gcc-warnings --with-xwidgets" on Fedora 30, I got the following diagnostics that should get fixed:

xwidget.c:258:1: warning: no previous prototype for ‘store_xwidget_event_string’ [-Wmissing-prototypes]
  258 | store_xwidget_event_string (struct xwidget *xw, const char *eventname,
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~
xwidget.c:272:1: warning: no previous prototype for ‘store_xwidget_response_callback_event’ [-Wmissing-prototypes]
  272 | store_xwidget_response_callback_event (struct xwidget *xw,
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
xwidget.c:292:1: warning: no previous prototype for ‘store_xwidget_js_callback_event’ [-Wmissing-prototypes]
  292 | store_xwidget_js_callback_event (struct xwidget *xw,
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


Why is that -D_REENTRANT needed? I thought _REENTRANT was obsolete on macOS.

-  Does Emacs support Xwidgets (requires gtk3)?            ${HAVE_XWIDGETS}
+  Does Emacs support Xwidgets?                            ${HAVE_XWIDGETS}
+    (requires gtk3 or macOS Cocoa)

Probably better to omit that second line; that part of the output is getting too long anyway.

-      syms_of_xwidget ();
        syms_of_xsettings ();
  #ifdef HAVE_X_SM
        syms_of_xsmfns ();
@@ -1855,6 +1854,10 @@ Using an Emacs configured with --with-x-toolkit=lucid 
does not have this problem
  #endif /* HAVE_W32NOTIFY */
  #endif /* WINDOWSNT */
+      syms_of_xwidget ();
+#endif /* HAVE_XWIDGETS */

Why move the call to syms_of_xwidget? And why surround it with "#ifdef", since syms_of_xwidget is a no-op if HAVE_XWIDGETS is not defined? It's better to avoid #if when that's convenient.

+#include <stdio.h> /* FIXME: Emacs error? message? instead of printf.  */

Yes, we don't want printfs there.

+#if defined (USE_GTK)
  #include <webkit2/webkit2.h>
  #include <JavaScriptCore/JavaScript.h>
+#elif defined (NS_IMPL_COCOA)
+#include "nsxwidget.h"

Indent preprocessor directives consistently. No parens needed in "defined X". "#ifdef X" is easier to read than "#if defined X".

+#if defined (USE_GTK)
  #if WEBKIT_CHECK_VERSION (2, 21, 1) && GNUC_PREREQ (4, 2, 0)
  # pragma GCC diagnostic ignored "-Wdeprecated-declarations"

Use just one "#if" rather than nested ones.

I didn't look in detail at the .m or .el changes, but this is good enough for 

reply via email to

[Prev in Thread] Current Thread [Next in Thread]