bug#60703: Patches to xwidget code

[Andrew De Angelis]

Thank you both.

@Po Lu, I will work on the changes you mentioned.

Can you clarify the C++-style comments vs C style ones? When should we use "/* */" and when should we use "//"?

I looked around the code and it doesn't seem very consistent, but I can help clean up the xwidget-related files.

@Eli,

Are we sure this will not affect any important aspects
of the build, except where we already have problems?  (I don't see any
description of the problems and their solution in the commit log or
the comments.)

This should only affect builds on Mac machines, and makes it so the xwidget feature doesn't leak memory anymore. Importantly, I do not have access to any Linux machine, so I can't be 100% sure they're not affected. But as far as I can tell, the changes are limited to Mac builds using the "-with-xwidgets" feature.

I'll add a better explanation in the upcoming Change Log, but essentially, the problems were simply memory leaks. The leaked memory wasn't a lot, so it seldom caused noticeable issues (at least in my experience).

I'm sending my answers to the copyright questionnaire to assign@gnu.org. This is my first contribution, so let me know if I'm missing anything.

Best,

Andrew

On Tue, Jan 10, 2023 at 8:40 AM Eli Zaretskii <eliz@gnu.org> wrote:

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org,  60703@debbugs.gnu.org
> Date: Tue, 10 Jan 2023 17:59:04 +0800
>
> > --- a/lisp/xwidget.el
> > +++ b/lisp/xwidget.el
> > @@ -924,8 +924,9 @@ xwidget-webkit-reload
> >  (defun xwidget-webkit-current-url ()
> >    "Display the current xwidget webkit URL and place it on the `kill-ring'."
> >    (interactive nil xwidget-webkit-mode)
> > -  (let ((url (xwidget-webkit-uri (xwidget-webkit-current-session))))
> > -    (message "URL: %s" (kill-new (or url "")))))
> > +  (let ((url (or (xwidget-webkit-uri (xwidget-webkit-current-session)) "")))
> > +    (kill-new url)
> > +    (message "URL: %s" url)))
> > 
> >  (defun xwidget-webkit-browse-history ()
> >    "Display a buffer containing the history of page loads."
>
> This change is fine for Emacs 29.1.  Eli, WDYT?

Fine by me.

> These changes are also fine for Emacs 29, but the TODO seems excessive.
> Eli, any comments here?

I don't have an opinion, so if it's OK in your opinion, I'm fine with
adding this.  Are we sure this will not affect any important aspects
of the build, except where we already have problems?  (I don't see any
description of the problems and their solution in the commit log or
the comments.)

> All in all, all your changes are fine for Emacs 29 by me.  What they
> need is a proper commit message.  See the node "Style of Change Logs" in
> the GNU Coding Standards for examples of how to do this:
>
>   https://www.gnu.org/prep/standards/standards.html#Style-of-Change-Logs

I don't see copyright assignment for Andrew, so this should be settled
before accepting the contribution of this magnitude.

> Thanks again for working on Emacs.

Seconded.

P.S. Please don't cross-post to emacs-devel.