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

[조성빈]

2019. 5. 26. 오전 2:47, Paul Eggert <address@hidden> 작성:

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

Definitely a mistake, will fix as soon as I get home :-)

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

Will add prototypes where appropriate...

>> +    WEBKIT_CFLAGS="-D_REENTRANT 
>> -I/System/Library/Frameworks/WebKit.framework/Headers"
> 
> Why is that -D_REENTRANT needed? I thought _REENTRANT was obsolete on macOS.

This was because this patch vas developed back from 2017 (by another person who 
signed the FSF agreements and also expressed his intents to contribute but 
didn’t...), and I was just maintaining it.
Didn’t bother to fix that... will fix that too.

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

Agreed.

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

I vaguely remember that it once emitted an error when compiling... will try 
again.

>> +#include <stdio.h> /* FIXME: Emacs error? message? instead of printf.  */
> 
> Yes, we don't want printfs there.

Can you give some candidates to use? 
I’m actually not that proficient in programming elisp functions in C... :-(

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

Will fix.

>> +#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.

That one was because the macro WEBKIT_CHECK_VERSION is only defined in GTK 
webkit... Last time I looked, I remember being puzzled because C preprocessor 
didn’t do proper short circuiting?̊̈ I’ll try that again.

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