Re: [PATCH v5] Enable xwidgets on macOS

[Eli Zaretskii <eliz@gnu.org>]

> From: 조성빈 <pcr910303@icloud.com>
> Date: Fri, 26 Jul 2019 03:51:11 +0900
> Cc: alan@idiocy.org,
>  emacs-devel@gnu.org
> 
> Hello, I have prepared three patches to apply on master.

Thanks.  Alan, could you please review the NS specific parts?

> diff --git a/etc/NEWS b/etc/NEWS
> index 5313270411..52b49bab75 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -2440,6 +2440,18 @@ was able to 'set' modifiers without the knowledge of the user.
>  ** On NS multicolor font display is enabled again since it is also
>  implemented in Emacs on free operating systems via Cairo drawing.
>  
> +** On macOS, Xwidget is now supported.
> +If Emacs was built with xwidget support, you can access the embedded
> +webkit browser with 'M-x xwidget-webkit-browse-url'.  Viewing two
> +instances of xwidget webkit is not supported.
> +
> +*** New functions for xwidget-webkit mode
> +'xwidget-webkit-clone-and-split-below',
> +'xwidget-webkit-clone-and-split-right'.
> +
> +*** New variable 'xwidget-webkit-enable-plugins'.

The last two entries are not specific to macOS, are they?  If so, they
should be in the general sections of NEWS.

> diff --git a/etc/TODO b/etc/TODO
> index a065763933..bda1cf8f9e 100644
> --- a/etc/TODO
> +++ b/etc/TODO
> @@ -640,15 +640,6 @@ from the emacsclient process.
>  
>  This sections contains features found in other official Emacs ports.
>  
> -**** Support for xwidgets
> -
> -Emacs 25 has support for xwidgets, a system to include operating
> -system components into an Emacs buffer.  The components range from
> -simple buttons to webkit (effectively, a web browser).
> -
> -Currently, xwidgets works only for the gtk+ framework but it is
> -designed to be compatible with multiple Emacs ports.

The MS-Windows and non-GTK builds of Emacs on X still don't support
xwidgets, so I think this entry should not be deleted in its entirety,
it should still say that some configurations don't support xwidgets.

> +(defun xwidget-webkit-clone-and-split-below ()
> +  "Clone current URL into a new widget place in new window below.
> +Get the URL of current session, then browse to the URL
> +in `split-window-below' with a new xwidget webkit session."

The second sentence should be reworded:

  Get the URL of current session, then browse that URL in another
  window after splitting the selected window horizontally.

> +(defun xwidget-webkit-clone-and-split-right ()
> +  "Clone current URL into a new widget place in new window right.
> +Get the URL of current session, then browse to the URL
> +in `split-window-right' with a new xwidget webkit session."

Same here (but use "vertically" instead of "horizontally").

>  (defvar bookmark-make-record-function)
> +(when (memq window-system '(mac ns))
> +  (defvar xwidget-webkit-enable-plugins nil
> +    "Enable plugins for xwidget webkit.
> +If non-nil, plugins are enabled.  Otherwise, disabled."))

It is better to have this defvar unconditionally, and tell in the doc
string that it only has effect on macOS.

> @@ -228,13 +247,14 @@ offscreen_damage_event (GtkWidget *widget, GdkEvent *event,
>    if (GTK_IS_WIDGET (xv_widget))
>      gtk_widget_queue_draw (GTK_WIDGET (xv_widget));
>    else
> -    printf ("Warning, offscreen_damage_event received invalid xv pointer:%p\n",
> -            xv_widget);
> +    message ("Warning, offscreen_damage_event received invalid xv pointer:%p\n",
> +             xv_widget);

Why replace printf by message?

> +#elif defined NS_IMPL_COCOA
> +              nsxwidget_resize_view(xv, xw->width, xw->height);
                                      ^
Space before the opening parenthesis is missing.

> +*** Functions 'xwidget-webkit-scroll-up', 'xwidget-webkit-scroll-down'
> +now supports scrolling arbitrary pixel values.  It now treats the
       ^^^^^^^^           ^^^^^^^^^^^^^^^^^^^^^^   ^^^^^^^^^^^^^
"support" (plural) "by arbitrary pixel values".  "They now treat" (plural).

> +(defun xwidget-webkit-scroll-up-line (&optional n)
> +  "Scroll webkit up by N lines.
> +The height of line is calculated with `window-font-height'.
> +Stop if the bottom edge of the page is reached.
> +If N is omitted or nil, scroll up by one line."
> +  (interactive "p")
> +  (xwidget-webkit-scroll-up (* n (window-font-height))))
> +
> +(defun xwidget-webkit-scroll-down-line (&optional n)
> +  "Scroll webkit down by N lines.
> +The height of line is calculated with `window-font-height'.
> +Stop if the top edge of the page is reached.
> +If N is omitted or nil, scroll down by one line."
> +  (interactive "p")
> +  (xwidget-webkit-scroll-down (* n (window-font-height))))
> +
> +(defun xwidget-webkit-scroll-forward (&optional n)
> +  "Scroll webkit horizontally by N chars.
> +The width of char is calculated with `window-font-width'.
> +If N is ommited or nil, scroll forwards by one char."
> +  (interactive "p")
>    (xwidget-webkit-execute-script
>     (xwidget-webkit-current-session)
> -   "window.scrollBy(50, 0);"))
> -
> -(defun xwidget-webkit-scroll-backward ()
> -  "Scroll webkit backwards."
> -  (interactive)
> +   (format "window.scrollBy(%d, 0);"
> +           (* n (window-font-width)))))
> +
> +(defun xwidget-webkit-scroll-backward (&optional n)
> +  "Scroll webkit back by N chars.
> +The width of char is calculated with `window-font-width'.
> +If N is ommited or nil, scroll backwards by one char."
> +  (interactive "p")

These commands should say in their doc strings that interactively N is
the prefix numeric argument.

> diff --git a/etc/NEWS b/etc/NEWS
> index f9a42f73be..c7f980f212 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -2469,6 +2469,10 @@ instances of xwidget webkit is not supported.
>  
>  *** New variable 'xwidget-webkit-enable-plugins'.
>  
> +** On macOS, downloading files from xwidget-webkit is supported.
> +
> +*** New variable 'xwidget-webkit-download-dir'.

The macOS-specific part of this should be in the non-free OS part of
NEWS.

> +(defun xwidget-webkit-save-as-file (url mime-type file-name)
> +  "For XWIDGET webkit, save URL of MIME-TYPE to location specified by user.
> +FILE-NAME combined with `xwidget-webkit-download-dir' is the default file name
> +of the prompt when reading.  When the file name the user specified is a
> +directory, URL is saved at the specified directory as FILE-NAME."

The last sentence is unclear: what will be the directory where the URL
will be saved, and what will be the name of the saved file in that
directory?