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

[Paul Eggert]

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,
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


> +    WEBKIT_CFLAGS="-D_REENTRANT 
> -I/System/Library/Frameworks/WebKit.framework/Headers"

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 */
>   
> +#ifdef HAVE_XWIDGETS
> +      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"
> +#endif

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"
>   #endif
> +#endif

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 
now.