bug#60703: Patches to xwidget code

[Po Lu]

Andrew De Angelis <bobodeangelis@gmail.com> writes:

> Hello everyone and thanks for all your work!
>
> I have some fixes to the xwidget code that I'd like to contribute.
>
> These should achieve three things 1) fix `xwidget-webkit-current-url'
> in xwidget.el so that it actually displays the current URL 2) fix the
> Objective-C code in nsxwidget.m to eliminate memory leaks 3) implement
> the function `xwidget-webkit-estimated-load-progress' in Objective-C,
> making this functionality available for macOS as well
>
> Regarding 2), I have tested my changes with the Instruments app within
> the XCode dev tools. I was able to test them on two different
> machines: a MacBook Air M2 running macOS Ventura 13, and an Intel
> processor running macOS Big Sur 11.6.7.  When testing with
> Instruments, it appears that Emacs isn't leaking memory anymore. I am
> still seeing some leaks, but according to Instruments, the responsible
> libraries are system libraries, and not Emacs itself.  If possible, I
> would appreciate it if other people can test/profile these changes as
> well, and share their feedback.
>
> Assuming the changes are correct and accepted, would it be possible to
> include them in the upcoming Emacs 29.1 release?  Technically, none of
> these are new features; it's just fixes to existing features.
>
> I intend to keep working on xwidget support for macOS (fixing
> remaining issues and implementing missing functionalities), but I'm
> not sure when I'll be able to get to it; for now I'd like to start by
> contributing these first major fixes.
>
> Thanks in advance for your feedback; let me know if you need anything
> else from me.

Andrew, thanks for working on Emacs.

>
> diff --git a/lisp/xwidget.el b/lisp/xwidget.el
> index abbda29081..8095fa9db5 100644
> --- 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?

>  @implementation XwWebView : WKWebView
>  
> +- (void)dealloc
> +{
> +  [super dealloc];
> +}

Thanks.  Please put a space between the part of the Objective-C method
that looks like a cast (I don't know what it's called, sorry) and the
name of the method.

But, is this really necessary?  Without this being defined, the runtime
will just call the super method, correct?

>  - (id)initWithFrame:(CGRect)frame
>        configuration:(WKWebViewConfiguration *)configuration
>              xwidget:(struct xwidget *)xw
>  {
>    /* Script controller to add script message handler and user script.  */
> -  WKUserContentController *scriptor = [[WKUserContentController alloc] init];
> +  WKUserContentController *scriptor = [[[WKUserContentController alloc] init]
> +                                        autorelease];
>    configuration.userContentController = scriptor;
>  
>    /* Enable inspect element context menu item for debugging.  */
> @@ -81,7 +87,8 @@ - (id)initWithFrame:(CGRect)frame
>    if (self)
>      {
>        self.xw = xw;
> -      self.urlScriptBlocked = [[NSMutableDictionary alloc] init];
> +      self.urlScriptBlocked = [[[NSMutableDictionary alloc] init]
> +                                autorelease];
>        self.navigationDelegate = self;
>        self.UIDelegate = self;
>        self.customUserAgent =
> @@ -89,11 +96,13 @@ - (id)initWithFrame:(CGRect)frame
>          @" AppleWebKit/603.3.8 (KHTML, like Gecko)"
>          @" Version/11.0.1 Safari/603.3.8";
>        [scriptor addScriptMessageHandler:self name:@"keyDown"];
> -      [scriptor addUserScript:[[WKUserScript alloc]
> -                                initWithSource:xwScript
> -                                 injectionTime:
> -                                  WKUserScriptInjectionTimeAtDocumentStart
> -                                forMainFrameOnly:NO]];
> +      WKUserScript *userScript = [[[WKUserScript alloc]
> +                                    initWithSource:xwScript
> +                                     injectionTime:
> +                                      
> WKUserScriptInjectionTimeAtDocumentStart
> +                                    forMainFrameOnly:NO]
> +                                   autorelease];
> +      [scriptor addUserScript:userScript];
>      }
>    return self;
>  }
> @@ -102,7 +111,27 @@ - (void)webView:(WKWebView *)webView
>  didFinishNavigation:(WKNavigation *)navigation
>  {
>    if (EQ (Fbuffer_live_p (self.xw->buffer), Qt))
> -    store_xwidget_event_string (self.xw, "load-changed", "");
> +    store_xwidget_event_string (self.xw, "load-changed", "load-finished");
> +}
> +
> +- (void)webView:(WKWebView *)webView 
> didStartProvisionalNavigation:(WKNavigation *)navigation
> +{
> +  if (EQ (Fbuffer_live_p (self.xw->buffer), Qt))
> +    store_xwidget_event_string (self.xw, "load-changed", "load-started");
> +}
> +
> +- (void)webView:(WKWebView *)webView
> +didReceiveServerRedirectForProvisionalNavigation:(WKNavigation *)navigation
> +{
> +  if (EQ (Fbuffer_live_p (self.xw->buffer), Qt))
> +    store_xwidget_event_string (self.xw, "load-changed", "load-redirected");
> +}
> +
> +// Start loading WKWebView
> +- (void)webView:(WKWebView *)webView didCommitNavigation:(WKNavigation 
> *)navigation
> +{
> +  if (EQ (Fbuffer_live_p (self.xw->buffer), Qt)) // what exactly is this 
> test for
> +    store_xwidget_event_string (self.xw, "load-changed", "load-committed");
>  }

These are also fine for Emacs 29.  However, please replace the C++-style
comments with C style ones, add spaces after the cast-like parts in the
method definition, and fill everything to column 80.  I believe the test
is that the xwidget's buffer has not been killed, and could easily be
written:

  !NILP (Fbuffer_live_p (self.xw->buffer))

as well.

>  - (void)webView:(WKWebView *)webView
> @@ -343,6 +372,20 @@ - (void)userContentController:(WKUserContentController 
> *)userContentController
>    }
>  }
>  
> +double
> +nsxwidget_webkit_estimated_load_progress(struct xwidget *xw)
> +{
> +  XwWebView *xwWebView = (XwWebView *) xw->xwWidget;
> +  return xwWebView.estimatedProgress;
> +}

Please place a space between "nsxwidget_webkit_estimated_load_progress"
and the parameter list.

> +void
> +nsxwidget_webkit_stop_loading (struct xwidget *xw)
> +{
> +  XwWebView *xwWebView = (XwWebView *) xw->xwWidget;
> +  [xwWebView stopLoading];
> +}
> +
>  void
>  nsxwidget_webkit_zoom (struct xwidget *xw, double zoom_change)
>  {
> @@ -452,7 +495,8 @@ - (BOOL)isFlipped { return YES; }
>    NSRect rect = NSMakeRect (0, 0, xw->width, xw->height);
>    xw->xwWidget = [[XwWebView alloc]
>                     initWithFrame:rect
> -                   configuration:[[WKWebViewConfiguration alloc] init]
> +                   configuration:[[[WKWebViewConfiguration alloc] init]
> +                                   autorelease]
>                           xwidget:xw];
>    xw->xwWindow = [[XwWindow alloc]
>                     initWithFrame:rect];
> @@ -470,16 +514,18 @@ - (BOOL)isFlipped { return YES; }
>          ((XwWebView *) xw->xwWidget).configuration.userContentController;
>        [scriptor removeAllUserScripts];
>        [scriptor removeScriptMessageHandlerForName:@"keyDown"];
> -      [scriptor release];
> +
>        if (xw->xv)
>          xw->xv->model = Qnil; /* Make sure related view stale.  */
>  
>        /* This stops playing audio when a xwidget-webkit buffer is
> -         killed.  I could not find other solution.  */
> +         killed.  I could not find other solution.
> +         TODO: improve this */
>        nsxwidget_webkit_goto_uri (xw, "about:blank");
>  
>        [((XwWebView *) xw->xwWidget).urlScriptBlocked release];
>        [xw->xwWidget removeFromSuperviewWithoutNeedingDisplay];
> +
>        [xw->xwWidget release];
>        [xw->xwWindow removeFromSuperviewWithoutNeedingDisplay];
>        [xw->xwWindow release];

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

> diff --git a/src/xwidget.c b/src/xwidget.c
> index efe2705562..b9550da460 100644
> --- a/src/xwidget.c
> +++ b/src/xwidget.c
> @@ -54,6 +54,7 @@ Copyright (C) 2011-2023 Free Software Foundation, Inc.
>  
>  #include <math.h>
>  
> +

Please undo this extraneous whitespace change.

>  
> +DEFUN ("xwidget-webkit-estimated-load-progress",
> +       Fxwidget_webkit_estimated_load_progress, 
> Sxwidget_webkit_estimated_load_progress,
> +       1, 1, 0, doc: /* Get the estimated load progress of XWIDGET, a WebKit 
> widget.
> +Return a value ranging from 0.0 to 1.0, based on how close XWIDGET
> +is to completely loading its page.  */)
> +  (Lisp_Object xwidget)
> +{
> +  struct xwidget *xw;
> +#ifdef USE_GTK
> +  WebKitWebView *webview;
> +#endif
> +  double value;
> +
> +  CHECK_LIVE_XWIDGET (xwidget);
> +  xw = XXWIDGET (xwidget);
> +  CHECK_WEBKIT_WIDGET (xw);
> +
> +  block_input ();
> +#ifdef USE_GTK
> +  webview = WEBKIT_WEB_VIEW (xw->widget_osr);
> +  value = webkit_web_view_get_estimated_load_progress (webview);
> +#elif defined NS_IMPL_COCOA
> +  value = nsxwidget_webkit_estimated_load_progress (xw);
> +#endif
> +
> +  unblock_input ();
> +
> +  return make_float (value);
> +}
> +
>  DEFUN ("xwidget-webkit-goto-uri",
>         Fxwidget_webkit_goto_uri, Sxwidget_webkit_goto_uri,
>         2, 2, 0,
> @@ -3810,28 +3841,6 @@ DEFUN ("xwidget-webkit-back-forward-list", 
> Fxwidget_webkit_back_forward_list,
>    return list3 (back, here, forward);
>  }
>  
> -DEFUN ("xwidget-webkit-estimated-load-progress",
> -       Fxwidget_webkit_estimated_load_progress, 
> Sxwidget_webkit_estimated_load_progress,
> -       1, 1, 0, doc: /* Get the estimated load progress of XWIDGET, a WebKit 
> widget.
> -Return a value ranging from 0.0 to 1.0, based on how close XWIDGET
> -is to completely loading its page.  */)
> -  (Lisp_Object xwidget)
> -{
> -  struct xwidget *xw;
> -  WebKitWebView *webview;
> -  double value;
> -
> -  CHECK_LIVE_XWIDGET (xwidget);
> -  xw = XXWIDGET (xwidget);
> -  CHECK_WEBKIT_WIDGET (xw);
> -
> -  block_input ();
> -  webview = WEBKIT_WEB_VIEW (xw->widget_osr);
> -  value = webkit_web_view_get_estimated_load_progress (webview);
> -  unblock_input ();
> -
> -  return make_float (value);
> -}
>  #endif
>  
>  DEFUN ("xwidget-webkit-set-cookie-storage-file",
> @@ -3874,19 +3883,23 @@ DEFUN ("xwidget-webkit-stop-loading", 
> Fxwidget_webkit_stop_loading,
>  XWIDGET as part of loading a page.  */)
>    (Lisp_Object xwidget)
>  {
> -#ifdef USE_GTK
>    struct xwidget *xw;
> +#ifdef USE_GTK
>    WebKitWebView *webview;
> +#endif
>  
>    CHECK_LIVE_XWIDGET (xwidget);
>    xw = XXWIDGET (xwidget);
>    CHECK_WEBKIT_WIDGET (xw);
>  
>    block_input ();
> +#ifdef USE_GTK
>    webview = WEBKIT_WEB_VIEW (xw->widget_osr);
>    webkit_web_view_stop_loading (webview);
> -  unblock_input ();
> +#elif defined NS_IMPL_COCOA
> +  nsxwidget_webkit_stop_loading (xw);
>  #endif
> +  unblock_input ();
>  
>    return Qnil;
>  }
> @@ -3936,8 +3949,9 @@ syms_of_xwidget (void)
>  #ifdef USE_GTK
>    defsubr (&Sxwidget_webkit_load_html);
>    defsubr (&Sxwidget_webkit_back_forward_list);
> -  defsubr (&Sxwidget_webkit_estimated_load_progress);
>  #endif
> +
> +  defsubr (&Sxwidget_webkit_estimated_load_progress);
>    defsubr (&Skill_xwidget);
>  
>    DEFSYM (QCxwidget, ":xwidget");

None of this seems to affect anything other than the NS build, so it's
fine by me.

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

Thanks again for working on Emacs.