From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Paul Eggert Newsgroups: gmane.emacs.devel Subject: Re: [PATCH] Add xwidget webkit support for macOS Cocoa Date: Sat, 25 May 2019 10:47:20 -0700 Organization: UCLA Computer Science Department Message-ID: References: <20190525132727.94982-1-pcr910303@icloud.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="16322"; mail-complaints-to="usenet@blaine.gmane.org" User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 Cc: emacs-devel@gnu.org To: Sungbin Jo Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sat May 25 19:47:35 2019 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([209.51.188.17]) by blaine.gmane.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hUala-00045o-UW for ged-emacs-devel@m.gmane.org; Sat, 25 May 2019 19:47:35 +0200 Original-Received: from localhost ([127.0.0.1]:44772 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hUalZ-0004mV-FL for ged-emacs-devel@m.gmane.org; Sat, 25 May 2019 13:47:33 -0400 Original-Received: from eggs.gnu.org ([209.51.188.92]:55368) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hUalR-0004mP-O6 for emacs-devel@gnu.org; Sat, 25 May 2019 13:47:26 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1hUalQ-0000yx-L8 for emacs-devel@gnu.org; Sat, 25 May 2019 13:47:25 -0400 Original-Received: from zimbra.cs.ucla.edu ([131.179.128.68]:43922) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1hUalQ-0000yQ-Db for emacs-devel@gnu.org; Sat, 25 May 2019 13:47:24 -0400 Original-Received: from localhost (localhost [127.0.0.1]) by zimbra.cs.ucla.edu (Postfix) with ESMTP id 56953161B58; Sat, 25 May 2019 10:47:22 -0700 (PDT) Original-Received: from zimbra.cs.ucla.edu ([127.0.0.1]) by localhost (zimbra.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10032) with ESMTP id 8HNcZQ9lmYD7; Sat, 25 May 2019 10:47:21 -0700 (PDT) Original-Received: from localhost (localhost [127.0.0.1]) by zimbra.cs.ucla.edu (Postfix) with ESMTP id 5D592161BC4; Sat, 25 May 2019 10:47:21 -0700 (PDT) X-Virus-Scanned: amavisd-new at zimbra.cs.ucla.edu Original-Received: from zimbra.cs.ucla.edu ([127.0.0.1]) by localhost (zimbra.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id PV_pmNVZih1B; Sat, 25 May 2019 10:47:21 -0700 (PDT) Original-Received: from [192.168.1.9] (cpe-23-242-74-103.socal.res.rr.com [23.242.74.103]) by zimbra.cs.ucla.edu (Postfix) with ESMTPSA id 2D506161A53; Sat, 25 May 2019 10:47:21 -0700 (PDT) In-Reply-To: <20190525132727.94982-1-pcr910303@icloud.com> Content-Language: en-US X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 131.179.128.68 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:236985 Archived-At: Thanks for doing all that work! Although I have no expertise in macOS I h= ave a=20 few minor comments about the patch from the perspective of someone who bu= ilds=20 and ports Emacs, and who wants to help navigate this patch through the Em= acs=20 build bureaucracy. First, when I tried to use "git am" to apply the patch= , I got=20 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=20 --with-xwidgets" on Fedora 30, I got the following diagnostics that shoul= d get=20 fixed: xwidget.c:258:1: warning: no previous prototype for =E2=80=98store_xwidge= t_event_string=E2=80=99=20 [-Wmissing-prototypes] 258 | store_xwidget_event_string (struct xwidget *xw, const char *even= tname, | ^~~~~~~~~~~~~~~~~~~~~~~~~~ xwidget.c:272:1: warning: no previous prototype for=20 =E2=80=98store_xwidget_response_callback_event=E2=80=99 [-Wmissing-protot= ypes] 272 | store_xwidget_response_callback_event (struct xwidget *xw, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ xwidget.c:292:1: warning: no previous prototype for=20 =E2=80=98store_xwidget_js_callback_event=E2=80=99 [-Wmissing-prototypes] 292 | store_xwidget_js_callback_event (struct xwidget *xw, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + WEBKIT_CFLAGS=3D"-D_REENTRANT -I/System/Library/Frameworks/WebKit.= framework/Headers" Why is that -D_REENTRANT needed? I thought _REENTRANT was obsolete on mac= OS. > - Does Emacs support Xwidgets (requires gtk3)? ${HAVE_XWIDG= ETS} > + Does Emacs support Xwidgets? ${HAVE_XWIDG= ETS} > + (requires gtk3 or macOS Cocoa) Probably better to omit that second line; that part of the output is gett= ing too=20 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=3D= lucid does not have this problem > #endif /* HAVE_W32NOTIFY */ > #endif /* WINDOWSNT */ > =20 > +#ifdef HAVE_XWIDGETS > + syms_of_xwidget (); > +#endif /* HAVE_XWIDGETS */ > + Why move the call to syms_of_xwidget? And why surround it with "#ifdef", = since=20 syms_of_xwidget is a no-op if HAVE_XWIDGETS is not defined? It's better t= o avoid=20 #if when that's convenient. > +#include /* FIXME: Emacs error? message? instead of printf. = */ Yes, we don't want printfs there. > +#if defined (USE_GTK) > #include > #include > +#elif defined (NS_IMPL_COCOA) > +#include "nsxwidget.h" > +#endif Indent preprocessor directives consistently. No parens needed in "defined= X".=20 "#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.