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 wrote: > > From: Po Lu > > 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. >