Re: [PATCH] Add user content APIs for WebKit Xwidgets

Re: [PATCH] Add user content APIs for WebKit Xwidgets

[Qiantan Hong]

Hi,

Is there any chance to get this merged now?

Best,
Qiantan

Re: [PATCH] Add user content APIs for WebKit Xwidgets

[Po Lu]

Qiantan Hong <qthong@stanford.edu> writes:

> Hi,
>
> Is there any chance to get this merged now?
>
> Best,
> Qiantan

I don't see the patch.  :(

Re: [PATCH] Add user content APIs for WebKit Xwidgets

[Qiantan Hong]

Ah sorry, I’m replying to a really ancient thread in 2020, the patch is at
https://lists.gnu.org/archive/html/emacs-devel/2020-08/txtNrEQRJNtgd.txt

I hope the patch is still applicable… If it’s not I’ll find sometime this weekend to update it.

Best,
Qiantan



> On Oct 14, 2022, at 12:01 AM, Po Lu <luangruo@yahoo.com> wrote:
> 
> Qiantan Hong <qthong@stanford.edu> writes:
> 
>> Hi,
>> 
>> Is there any chance to get this merged now?
>> 
>> Best,
>> Qiantan
> 
> I don't see the patch.  :(

Re: [PATCH] Add user content APIs for WebKit Xwidgets

[Po Lu]

Qiantan Hong <qthong@stanford.edu> writes:

> Ah sorry, I’m replying to a really ancient thread in 2020, the patch is at
> https://lists.gnu.org/archive/html/emacs-devel/2020-08/txtNrEQRJNtgd.txt
>
> I hope the patch is still applicable… If it’s not I’ll find sometime this weekend to update it.

Thanks.  Please do that, and also fix the coding style to comply with
our coding guidelines.

Re: [PATCH] Add user content APIs for WebKit Xwidgets

[Qiantan Hong]

[-- Attachment #1.1: Type: text/plain, Size: 553 bytes --]

Hi,

Here’s the updated patch.


Best,
Qiantan



> On Oct 14, 2022, at 12:35 AM, Po Lu <luangruo@yahoo.com> wrote:
>
> Qiantan Hong <qthong@stanford.edu> writes:
>
>> Ah sorry, I’m replying to a really ancient thread in 2020, the patch is at
>> https://lists.gnu.org/archive/html/emacs-devel/2020-08/txtNrEQRJNtgd.txt
>>
>> I hope the patch is still applicable… If it’s not I’ll find sometime this weekend to update it.
>
> Thanks.  Please do that, and also fix the coding style to comply with
> our coding guidelines.


[-- Attachment #1.2: Type: text/html, Size: 1168 bytes --]

[-- Attachment #2: 0001-Implment-some-user-content-APIs-for-WebKit-Xwidgets.txt --]
[-- Type: text/plain, Size: 18985 bytes --]

From d03d1b813c016e805fe8902500cdd20ade39ed53 Mon Sep 17 00:00:00 2001
From: Qiantan Hong <qhong@alum.mit.edu>
Date: Fri, 14 Oct 2022 14:08:40 -0700
Subject: [PATCH] Implment some user content APIs for WebKit Xwidgets

Implement WebKit user scripts and script message handlers.
* src/xwidget.h (store_xwidget_script_message_event): store script
  message event into event queue
* src/xwidget.c (store_xwidget_script_message_event, make-xwidget,
  webkit_script_message_cb, xwidget-webkit-add-user-script,
  xwidget-webkit-remove-all-user-scripts,
  xwidget-webkit-register-script-message,
  xwidget-webkit-unregister-script-message): Implement user script
  and script message handler primitives.
* src/nsxwidget.c (nsxwidget_webkit_add_user_script,
  nsxwidget_webkit_remove_all_user_scripts,
  nsxwidget_webkit_register_script_message,
  nsxwidget_webkit_unregister_script_message,
  initWithFrame, initialize, userContentController): NS
  implementation. Changed naming of a previous used  script message
  handler to avoid namespace pollution.
* src/nsxwidget.h (nsxwidget_webkit_add_user_script,
  nsxwidget_webkit_remove_all_user_scripts,
  nsxwidget_webkit_register_script_message,
  nsxwidget_webkit_unregister_script_message): NS implementation
* lisp/xwidget.el (xwidget-webkit-callback,
  xwidget-webkit-add-script-message-handler,
  xwidget-webkit-remove-script-message-handler):
  let lisp recognize and dispatch script message events
---
 lisp/xwidget.el |  32 +++++++++
 src/nsxwidget.h |   5 ++
 src/nsxwidget.m |  80 +++++++++++++++++++--
 src/xwidget.c   | 181 ++++++++++++++++++++++++++++++++++++++++++++++++
 src/xwidget.h   |   4 ++
 5 files changed, 295 insertions(+), 7 deletions(-)

diff --git a/lisp/xwidget.el b/lisp/xwidget.el
index 41a1190c64..02412a8808 100644
--- a/lisp/xwidget.el
+++ b/lisp/xwidget.el
@@ -485,8 +485,40 @@ xwidget-webkit-callback
            (let ((proc (nth 3 last-input-event))
                  (arg  (nth 4 last-input-event)))
              (funcall proc arg)))
+          ((eq xwidget-event-type 'script-message)
+           (let ((name (nth 3 last-input-event))
+                 (value (nth 4 last-input-event)))
+             (let ((handler-pair (assq name (xwidget-get xwidget 'script-message-handlers))))
+               (if handler-pair
+                   (funcall (cdr handler-pair) xwidget value)
+                 (xwidget-log "unhandled script message:%s" name)))))
           (t (xwidget-log "unhandled event:%s" xwidget-event-type)))))
 
+(defun xwidget-webkit-add-script-message-handler (xwidget name handler)
+  "Associate HANDLER with script messages under symbol NAME for Webkit XWIDGET.
+
+HANDLER will be called with two arguments: the Webkit XWIDGET and
+the javascript object passed from the script message converted to
+a Lisp object."
+  (xwidget-webkit-register-script-message (xwidget-webkit-current-session) name)
+  (xwidget-put xwidget 'script-message-handlers
+               (cons (cons name handler) (xwidget-get xwidget 'script-message-handlers)))
+  name)
+
+(defun xwidget-webkit-remove-script-message-handler (xwidget name)
+  "Remove a handler associated with symbol NAME for Webkit XWIDGET.
+
+Returns the removed (NAME . HANDLER) pair, or NIL if such handler
+is not found."
+  (let* ((old-alist (xwidget-get xwidget 'script-message-handlers))
+         (handler-pair (assq name old-alist)))
+    (when handler-pair
+      (let ((new-alist (delq handler-pair old-alist)))
+        (xwidget-put xwidget 'script-message-handlers new-alist)
+        (unless (assq name new-alist)
+          (xwidget-webkit-unregister-script-message (xwidget-webkit-current-session) name)))
+      handler-pair)))
+
 (defvar bookmark-make-record-function)
 (when (memq window-system '(mac ns))
   (defcustom xwidget-webkit-enable-plugins nil
diff --git a/src/nsxwidget.h b/src/nsxwidget.h
index 666509744a..cd1dc1bc2c 100644
--- a/src/nsxwidget.h
+++ b/src/nsxwidget.h
@@ -40,6 +40,11 @@ #define NSXWIDGET_H_INCLUDED
 void nsxwidget_webkit_execute_script (struct xwidget *xw, const char *script,
                                       Lisp_Object fun);
 
+void nsxwidget_webkit_add_user_script (struct xwidget *xw, const char *script,
+                                       int injection_time_start, int main_frame_only);
+void nsxwidget_webkit_remove_all_user_scripts (struct xwidget *xw);
+Lisp_Object nsxwidget_webkit_register_script_message (struct xwidget *xw, const char *name);
+void nsxwidget_webkit_unregister_script_message (struct xwidget *xw, const char *name);
 /* Functions for xwidget model.  */
 
 #ifdef __OBJC__
diff --git a/src/nsxwidget.m b/src/nsxwidget.m
index be0eba0bcb..e09759f754 100644
--- a/src/nsxwidget.m
+++ b/src/nsxwidget.m
@@ -88,7 +88,7 @@ - (id)initWithFrame:(CGRect)frame
         @"Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6)"
         @" AppleWebKit/603.3.8 (KHTML, like Gecko)"
         @" Version/11.0.1 Safari/603.3.8";
-      [scriptor addScriptMessageHandler:self name:@"keyDown"];
+      [scriptor addScriptMessageHandler:self name:@"__xwidget_internal_keyDown"];
       [scriptor addUserScript:[[WKUserScript alloc]
                                 initWithSource:xwScript
                                  injectionTime:
@@ -275,23 +275,34 @@ + (void)initialize
       @"}"
       @"function xwKeyDown(event) {"
       @"  if (event.ctrlKey && event.key == 'g') {"
-      @"    window.webkit.messageHandlers.keyDown.postMessage('C-g');"
+      @"    window.webkit.messageHandlers.__xwidget_internal_keyDown.postMessage('C-g');"
       @"  }"
       @"}"
       @"document.addEventListener('keydown', xwKeyDown);"
       ;
 }
 
+static Lisp_Object js_to_lisp (id value);
+
 /* Confirming to WKScriptMessageHandler, listens concerning keyDown in
    webkit. Currently 'C-g'.  */
 - (void)userContentController:(WKUserContentController *)userContentController
       didReceiveScriptMessage:(WKScriptMessage *)message
 {
-  if ([message.body isEqualToString:@"C-g"])
+  if ([message.name isEqualToString:@"__xwidget_internal_keyDown"])
     {
-      /* Just give up focus, no relay "C-g" to emacs, another "C-g"
-         follows will be handled by emacs.  */
-      [self.window makeFirstResponder:self.xw->xv->emacswindow];
+      if ([message.body isEqualToString:@"C-g"])
+        {
+          /* Just give up focus, no relay "C-g" to emacs, another "C-g"
+             follows will be handled by emacs.  */
+          [self.window makeFirstResponder:self.xw->xv->emacswindow];
+        }
+    }
+  else
+    {
+      store_xwidget_script_message_event (self.xw,
+                                          message.name.UTF8String,
+                                          js_to_lisp (message.body));
     }
 }
 
@@ -437,6 +448,61 @@ - (void)userContentController:(WKUserContentController *)userContentController
     }];
 }
 
+void
+nsxwidget_webkit_add_user_script (struct xwidget *xw, const char *script,
+                                  int injection_time_start, int main_frame_only)
+{
+  XwWebView *xwWebView = (XwWebView *) xw->xwWidget;
+  WKUserContentController *scriptor = xwWebView.configuration.userContentController;
+
+  NSString *javascriptString = [NSString stringWithUTF8String:script];
+  WKUserScriptInjectionTime injectionTime = injection_time_start?
+    WKUserScriptInjectionTimeAtDocumentStart : WKUserScriptInjectionTimeAtDocumentEnd;
+  WKUserScript *userScript = [[WKUserScript alloc]
+                               initWithSource: javascriptString
+                                injectionTime: injectionTime
+                               forMainFrameOnly: main_frame_only];
+  [scriptor addUserScript: userScript];
+}
+
+void
+nsxwidget_webkit_remove_all_user_scripts (struct xwidget *xw)
+{
+  XwWebView *xwWebView = (XwWebView *) xw->xwWidget;
+  WKUserContentController *scriptor = xwWebView.configuration.userContentController;
+
+  [scriptor removeAllUserScripts];
+}
+
+Lisp_Object
+nsxwidget_webkit_register_script_message (struct xwidget *xw, const char *name)
+{
+  XwWebView *xwWebView = (XwWebView *) xw->xwWidget;
+  WKUserContentController *scriptor = xwWebView.configuration.userContentController;
+
+  NSString *messageName = [NSString stringWithUTF8String:name];
+
+  @try
+    {
+      [scriptor addScriptMessageHandler:xwWebView name:messageName];
+    }
+  @catch (NSException *e)
+    {
+      return Qnil;
+    }
+  return Qt;
+}
+
+void
+nsxwidget_webkit_unregister_script_message (struct xwidget *xw, const char *name)
+{
+  XwWebView *xwWebView = (XwWebView *) xw->xwWidget;
+  WKUserContentController *scriptor = xwWebView.configuration.userContentController;
+
+  NSString *messageName = [NSString stringWithUTF8String:name];
+  [scriptor removeScriptMessageHandlerForName:messageName];
+}
+
 /* Window containing an xwidget.  */
 
 @implementation XwWindow
@@ -469,7 +535,7 @@ - (BOOL)isFlipped { return YES; }
       WKUserContentController *scriptor =
         ((XwWebView *) xw->xwWidget).configuration.userContentController;
       [scriptor removeAllUserScripts];
-      [scriptor removeScriptMessageHandlerForName:@"keyDown"];
+      [scriptor removeScriptMessageHandlerForName:@"__xwidget_internal_keyDown"];
       [scriptor release];
       if (xw->xv)
         xw->xv->model = Qnil; /* Make sure related view stale.  */
diff --git a/src/xwidget.c b/src/xwidget.c
index 8bdfab02fd..ea19264a18 100644
--- a/src/xwidget.c
+++ b/src/xwidget.c
@@ -226,6 +226,15 @@ xw_forward_event_from_view (GtkWidget *widget, GdkEvent *event,
   /* Don't propagate this event further.  */
   return TRUE;
 }
+
+struct webkit_script_message_cb_data
+{
+  struct xwidget *xw;
+  char name[0];
+};
+static void webkit_script_message_cb (WebKitUserContentManager *,
+                                          WebKitJavascriptResult *,
+                                          gpointer);
 #endif
 
 #ifdef HAVE_X_WINDOWS
@@ -380,6 +389,9 @@ DEFUN ("make-xwidget",
 	  settings = webkit_web_view_get_settings (WEBKIT_WEB_VIEW (xw->widget_osr));
 	  g_object_set (G_OBJECT (settings), "enable-developer-extras", TRUE, NULL);
 	}
+          WebKitUserContentManager *scriptor = webkit_user_content_manager_new ();
+          xw->widget_osr = webkit_web_view_new_with_user_content_manager (scriptor);
+        }
 
       gtk_widget_set_size_request (GTK_WIDGET (xw->widget_osr), xw->width,
                                    xw->height);
@@ -2308,6 +2320,21 @@ store_xwidget_js_callback_event (struct xwidget *xw,
   kbd_buffer_store_event (&event);
 }
 
+void
+store_xwidget_script_message_event (struct xwidget *xw,
+                                         const char *name,
+                                         Lisp_Object body)
+{
+  struct input_event event;
+  Lisp_Object xwl;
+  XSETXWIDGET (xwl, xw);
+  EVENT_INIT (event);
+  event.kind = XWIDGET_EVENT;
+  event.frame_or_window = Qnil;
+  event.arg = list4 (intern ("script-message"), xwl, intern (name), body);
+  kbd_buffer_store_event (&event);
+}
+
 
 #ifdef USE_GTK
 static void
@@ -2622,6 +2649,17 @@ webkit_decide_policy_cb (WebKitWebView *webView,
   }
 }
 
+static void webkit_script_message_cb (WebKitUserContentManager *scriptor,
+                                          WebKitJavascriptResult *js_result,
+                                          gpointer data)
+{
+  JSCValue *value = webkit_javascript_result_get_js_value (js_result);
+  struct webkit_script_message_cb_data *arg = data;
+
+  Lisp_Object lisp_value = webkit_js_to_lisp (value);
+  store_xwidget_script_message_event (arg->xw, arg->name, lisp_value);
+}
+
 static gboolean
 webkit_script_dialog_cb (WebKitWebView *webview,
 			 WebKitScriptDialog *script_dialog,
@@ -2717,6 +2755,7 @@ xwidget_init_view (struct xwidget *xww,
   XSETWINDOW (xv->w, s->w);
   XSETXWIDGET (xv->model, xww);
 
+
 #ifdef HAVE_X_WINDOWS
   xv->dpy = FRAME_X_DISPLAY (s->f);
 
@@ -3198,6 +3237,140 @@ DEFUN ("xwidget-webkit-execute-script",
   return Qnil;
 }
 
+DEFUN ("xwidget-webkit-add-user-script",
+       Fxwidget_webkit_add_user_script, Sxwidget_webkit_add_user_script,
+       4, 4, 0,
+       doc: /* Add user SCRIPT to the Webkit XWIDGET.
+INJECTION-TIME is a symbol which can take one of the following values:
+
+- start: SCRIPT is injected when document starts loading
+- end: SCRIPT is injected when document finishes loading
+
+If MAIN_FRAME_ONLY is nil, SCRIPT is injected to all frames.
+Otherwise, SCRIPT is only injected to top frames.*/)
+  (Lisp_Object xwidget, Lisp_Object script,
+   Lisp_Object injection_time, Lisp_Object main_frame_only)
+{
+  WEBKIT_FN_INIT ();
+  CHECK_STRING (script);
+  CHECK_SYMBOL (injection_time);
+
+  script = ENCODE_SYSTEM(script);
+
+  int injection_time_start, mfo;
+  mfo = !NILP (main_frame_only);
+  if (EQ (injection_time, Qstart))
+    injection_time_start = 1;
+  else if (EQ (injection_time, Qend))
+    injection_time_start = 0;
+  else
+    error ("Unknown Xwidget Webkit user script injection time: %s",
+           SDATA (SYMBOL_NAME (injection_time)));
+
+#ifdef USE_GTK
+  WebKitWebView *wkwv = WEBKIT_WEB_VIEW (xw->widget_osr);
+  WebKitUserContentManager *scriptor = webkit_web_view_get_user_content_manager (wkwv);
+
+  int webkit_injected_frames = mfo?
+    WEBKIT_USER_CONTENT_INJECT_TOP_FRAME : WEBKIT_USER_CONTENT_INJECT_ALL_FRAMES;
+  int webkit_injection_time = injection_time_start?
+    WEBKIT_USER_SCRIPT_INJECT_AT_DOCUMENT_START : WEBKIT_USER_SCRIPT_INJECT_AT_DOCUMENT_END;
+  WebKitUserScript *userScript = webkit_user_script_new (SSDATA (script),
+                                                         webkit_injected_frames,
+                                                         webkit_injection_time,
+                                                         NULL, NULL);
+  webkit_user_content_manager_add_script (scriptor, userScript);
+  webkit_user_script_unref (userScript);
+#elif defined NS_IMPL_COCOA
+  nsxwidget_webkit_add_user_script (xw, SSDATA (script), injection_time_start, mfo);
+#endif
+  return Qnil;
+}
+
+DEFUN ("xwidget-webkit-remove-all-user-scripts",
+       Fxwidget_webkit_remove_all_user_scripts, Sxwidget_webkit_remove_all_user_scripts,
+       1, 1, 0,
+       doc: /* Remove all user scripts from XWIDGET.  */)
+  (Lisp_Object xwidget)
+{
+  WEBKIT_FN_INIT ();
+
+#ifdef USE_GTK
+  WebKitWebView *wkwv = WEBKIT_WEB_VIEW (xw->widget_osr);
+  WebKitUserContentManager *scriptor = webkit_web_view_get_user_content_manager (wkwv);
+
+  webkit_user_content_manager_remove_all_scripts (scriptor);
+#elif defined NS_IMPL_COCOA
+  nsxwidget_webkit_remove_all_user_scripts(xw);
+#endif
+  return Qnil;
+}
+
+DEFUN ("xwidget-webkit-register-script-message",
+       Fxwidget_webkit_register_script_message, Sxwidget_webkit_register_script_message,
+       2, 2, 0,
+       doc: /* Register script message with symbol NAME in Webkit XWIDGET.
+Returns T if the operation is successful, NIL otherwise.
+The cause of failure is usually that NAME has already been registered for XWIDGET.  */)
+  (Lisp_Object xwidget, Lisp_Object name)
+{
+  WEBKIT_FN_INIT ();
+  CHECK_SYMBOL (name);
+  const char *sname = SDATA( SYMBOL_NAME (name));
+
+#ifdef USE_GTK
+  WebKitWebView *wkwv = WEBKIT_WEB_VIEW (xw->widget_osr);
+  WebKitUserContentManager *scriptor = webkit_web_view_get_user_content_manager (wkwv);
+
+  gchar *signal_name = g_strconcat ("script-message-received::", sname, NULL);
+  size_t name_length = strlen (sname) + 1;
+  struct webkit_script_message_cb_data *arg = malloc (sizeof *arg + name_length);
+  arg->xw = xw;
+  g_strlcpy (arg->name, sname, name_length);
+  g_signal_connect_data(scriptor, signal_name, G_CALLBACK (webkit_script_message_cb),
+                        arg, (GClosureNotify)free, 0);
+  g_free (signal_name);
+  if (webkit_user_content_manager_register_script_message_handler (scriptor, sname))
+    {
+      return Qt;
+    }
+  else
+    {
+      g_signal_handlers_disconnect_matched  (scriptor,
+                                             G_SIGNAL_MATCH_DATA,
+                                             0, 0, 0, 0, arg);
+      return Qnil;
+    }
+#elif defined NS_IMPL_COCOA
+  return nsxwidget_webkit_register_script_message(xw, sname);
+#endif
+}
+
+DEFUN ("xwidget-webkit-unregister-script-message",
+       Fxwidget_webkit_unregister_script_message, Sxwidget_webkit_unregister_script_message,
+       2, 2, 0,
+       doc: /* Unregister script message with symbol NAME in Webkit XWIDGET.  */)
+  (Lisp_Object xwidget, Lisp_Object name)
+{
+  WEBKIT_FN_INIT ();
+  CHECK_SYMBOL (name);
+  const char *sname = SSDATA( SYMBOL_NAME (name));
+
+#ifdef USE_GTK
+  WebKitWebView *wkwv = WEBKIT_WEB_VIEW (xw->widget_osr);
+  WebKitUserContentManager *scriptor = webkit_web_view_get_user_content_manager (wkwv);
+
+  webkit_user_content_manager_unregister_script_message_handler (scriptor, sname);
+  g_signal_handlers_disconnect_matched  (scriptor,
+                                         G_SIGNAL_MATCH_FUNC,
+                                         0, g_quark_from_string (sname), 0,
+                                         G_CALLBACK (webkit_script_message_cb), 0);
+#elif defined NS_IMPL_COCOA
+  nsxwidget_webkit_unregister_script_message(xw, sname);
+#endif
+  return Qnil;
+}
+
 DEFUN ("xwidget-resize", Fxwidget_resize, Sxwidget_resize, 3, 3, 0,
        doc: /* Resize XWIDGET to NEW_WIDTH, NEW_HEIGHT.  */ )
   (Lisp_Object xwidget, Lisp_Object new_width, Lisp_Object new_height)
@@ -3919,6 +4092,14 @@ syms_of_xwidget (void)
   defsubr (&Sxwidget_webkit_execute_script);
   DEFSYM (Qwebkit, "webkit");
 
+  defsubr (&Sxwidget_webkit_add_user_script);
+  DEFSYM (Qstart, "start");
+  DEFSYM (Qend, "end");
+  defsubr (&Sxwidget_webkit_remove_all_user_scripts);
+
+  defsubr (&Sxwidget_webkit_register_script_message);
+  defsubr (&Sxwidget_webkit_unregister_script_message);
+
   defsubr (&Sxwidget_size_request);
   defsubr (&Sdelete_xwidget_view);
 
diff --git a/src/xwidget.h b/src/xwidget.h
index 502beb6765..e835f700bc 100644
--- a/src/xwidget.h
+++ b/src/xwidget.h
@@ -203,6 +203,10 @@ #define XG_XWIDGET_VIEW "emacs_xwidget_view"
                                       Lisp_Object proc,
                                       Lisp_Object argument);
 
+void store_xwidget_script_message_event (struct xwidget *xw,
+                                         const char *name,
+                                         Lisp_Object value);
+
 extern struct xwidget *xwidget_from_id (uint32_t id);
 
 #ifdef HAVE_X_WINDOWS
-- 
2.32.1 (Apple Git-133)

Re: [PATCH] Add user content APIs for WebKit Xwidgets

[Qiantan Hong]

Ah sorry for the noise, please don’t merge yet. I only tested it on NS before
and only tried it on GTK just now. It is broken on GTK. Will fix soon.

Best,
Qiantan



> On Oct 14, 2022, at 2:13 PM, Qiantan Hong <qthong@stanford.edu> wrote:
> 
> Hi,
> 
> Here’s the updated patch.
> <0001-Implment-some-user-content-APIs-for-WebKit-Xwidgets.txt>
> 
> Best,
> Qiantan
> 
> 
> 
>> On Oct 14, 2022, at 12:35 AM, Po Lu <luangruo@yahoo.com> wrote:
>> 
>> Qiantan Hong <qthong@stanford.edu> writes:
>> 
>>> Ah sorry, I’m replying to a really ancient thread in 2020, the patch is at
>>> https://lists.gnu.org/archive/html/emacs-devel/2020-08/txtNrEQRJNtgd.txt
>>> 
>>> I hope the patch is still applicable… If it’s not I’ll find sometime this weekend to update it.
>> 
>> Thanks.  Please do that, and also fix the coding style to comply with
>> our coding guidelines.
> 

Re: [PATCH] Add user content APIs for WebKit Xwidgets

[Qiantan Hong]

[-- Attachment #1.1: Type: text/plain, Size: 1072 bytes --]

Here it goes, tested on GTK!



Best,
Qiantan



> On Oct 14, 2022, at 6:37 PM, Qiantan Hong <qthong@stanford.edu> wrote:
>
> Ah sorry for the noise, please don’t merge yet. I only tested it on NS before
> and only tried it on GTK just now. It is broken on GTK. Will fix soon.
>
> Best,
> Qiantan
>
>
>
>> On Oct 14, 2022, at 2:13 PM, Qiantan Hong <qthong@stanford.edu> wrote:
>>
>> Hi,
>>
>> Here’s the updated patch.
>> <0001-Implment-some-user-content-APIs-for-WebKit-Xwidgets.txt>
>>
>> Best,
>> Qiantan
>>
>>
>>
>>> On Oct 14, 2022, at 12:35 AM, Po Lu <luangruo@yahoo.com> wrote:
>>>
>>> Qiantan Hong <qthong@stanford.edu> writes:
>>>
>>>> Ah sorry, I’m replying to a really ancient thread in 2020, the patch is at
>>>> https://lists.gnu.org/archive/html/emacs-devel/2020-08/txtNrEQRJNtgd.txt
>>>>
>>>> I hope the patch is still applicable… If it’s not I’ll find sometime this weekend to update it.
>>>
>>> Thanks.  Please do that, and also fix the coding style to comply with
>>> our coding guidelines.
>>
>


[-- Attachment #1.2: Type: text/html, Size: 1987 bytes --]

[-- Attachment #2: 0002-Implment-some-user-content-APIs-for-WebKit-Xwidgets.txt --]
[-- Type: text/plain, Size: 19000 bytes --]

From aac249c4f0a3e0f21aae81e0a26c362b67336af2 Mon Sep 17 00:00:00 2001
From: Qiantan Hong <qhong@alum.mit.edu>
Date: Fri, 14 Oct 2022 14:08:40 -0700
Subject: [PATCH] Implment some user content APIs for WebKit Xwidgets

Implement WebKit user scripts and script message handlers.
* src/xwidget.h (store_xwidget_script_message_event): store script
  message event into event queue
* src/xwidget.c (store_xwidget_script_message_event, make-xwidget,
  webkit_script_message_cb, xwidget-webkit-add-user-script,
  xwidget-webkit-remove-all-user-scripts,
  xwidget-webkit-register-script-message,
  xwidget-webkit-unregister-script-message): Implement user script
  and script message handler primitives.
* src/nsxwidget.c (nsxwidget_webkit_add_user_script,
  nsxwidget_webkit_remove_all_user_scripts,
  nsxwidget_webkit_register_script_message,
  nsxwidget_webkit_unregister_script_message,
  initWithFrame, initialize, userContentController): NS
  implementation. Changed naming of a previous used  script message
  handler to avoid namespace pollution.
* src/nsxwidget.h (nsxwidget_webkit_add_user_script,
  nsxwidget_webkit_remove_all_user_scripts,
  nsxwidget_webkit_register_script_message,
  nsxwidget_webkit_unregister_script_message): NS implementation
* lisp/xwidget.el (xwidget-webkit-callback,
  xwidget-webkit-push-script-message-handler,
  xwidget-webkit-pop-script-message-handler):
  let lisp recognize and dispatch script message events

Acked-by: Qiantan Hong <qhong@mit.edu>
---
 lisp/xwidget.el |  32 +++++++++
 src/nsxwidget.h |   5 ++
 src/nsxwidget.m |  80 +++++++++++++++++++--
 src/xwidget.c   | 181 ++++++++++++++++++++++++++++++++++++++++++++++++
 src/xwidget.h   |   4 ++
 5 files changed, 295 insertions(+), 7 deletions(-)

diff --git a/lisp/xwidget.el b/lisp/xwidget.el
index 41a1190c64..79c9cc2afd 100644
--- a/lisp/xwidget.el
+++ b/lisp/xwidget.el
@@ -485,8 +485,40 @@ xwidget-webkit-callback
            (let ((proc (nth 3 last-input-event))
                  (arg  (nth 4 last-input-event)))
              (funcall proc arg)))
+          ((eq xwidget-event-type 'script-message)
+           (let ((name (nth 3 last-input-event))
+                 (value (nth 4 last-input-event)))
+             (let ((handler-pair (assq name (xwidget-get xwidget 'script-message-handlers))))
+               (if handler-pair
+                   (funcall (cdr handler-pair) xwidget value)
+                 (xwidget-log "unhandled script message:%s" name)))))
           (t (xwidget-log "unhandled event:%s" xwidget-event-type)))))
 
+(defun xwidget-webkit-push-script-message-handler (xwidget name handler)
+  "Associate HANDLER with script messages under symbol NAME for Webkit XWIDGET.
+
+HANDLER will be called with two arguments: the Webkit XWIDGET and
+the javascript object passed from the script message converted to
+a Lisp object."
+  (xwidget-webkit-register-script-message (xwidget-webkit-current-session) name)
+  (xwidget-put xwidget 'script-message-handlers
+               (cons (cons name handler) (xwidget-get xwidget 'script-message-handlers)))
+  name)
+
+(defun xwidget-webkit-pop-script-message-handler (xwidget name)
+  "Remove a handler associated with symbol NAME for Webkit XWIDGET.
+
+Returns the removed handler function, or NIL if such handler is
+not found."
+  (let* ((old-alist (xwidget-get xwidget 'script-message-handlers))
+         (handler-pair (assq name old-alist)))
+    (when handler-pair
+      (let ((new-alist (delq handler-pair old-alist)))
+        (xwidget-put xwidget 'script-message-handlers new-alist)
+        (unless (assq name new-alist)
+          (xwidget-webkit-unregister-script-message (xwidget-webkit-current-session) name)))
+      (cdr handler-pair))))
+
 (defvar bookmark-make-record-function)
 (when (memq window-system '(mac ns))
   (defcustom xwidget-webkit-enable-plugins nil
diff --git a/src/nsxwidget.h b/src/nsxwidget.h
index 666509744a..cd1dc1bc2c 100644
--- a/src/nsxwidget.h
+++ b/src/nsxwidget.h
@@ -40,6 +40,11 @@ #define NSXWIDGET_H_INCLUDED
 void nsxwidget_webkit_execute_script (struct xwidget *xw, const char *script,
                                       Lisp_Object fun);
 
+void nsxwidget_webkit_add_user_script (struct xwidget *xw, const char *script,
+                                       int injection_time_start, int main_frame_only);
+void nsxwidget_webkit_remove_all_user_scripts (struct xwidget *xw);
+Lisp_Object nsxwidget_webkit_register_script_message (struct xwidget *xw, const char *name);
+void nsxwidget_webkit_unregister_script_message (struct xwidget *xw, const char *name);
 /* Functions for xwidget model.  */
 
 #ifdef __OBJC__
diff --git a/src/nsxwidget.m b/src/nsxwidget.m
index be0eba0bcb..e09759f754 100644
--- a/src/nsxwidget.m
+++ b/src/nsxwidget.m
@@ -88,7 +88,7 @@ - (id)initWithFrame:(CGRect)frame
         @"Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6)"
         @" AppleWebKit/603.3.8 (KHTML, like Gecko)"
         @" Version/11.0.1 Safari/603.3.8";
-      [scriptor addScriptMessageHandler:self name:@"keyDown"];
+      [scriptor addScriptMessageHandler:self name:@"__xwidget_internal_keyDown"];
       [scriptor addUserScript:[[WKUserScript alloc]
                                 initWithSource:xwScript
                                  injectionTime:
@@ -275,23 +275,34 @@ + (void)initialize
       @"}"
       @"function xwKeyDown(event) {"
       @"  if (event.ctrlKey && event.key == 'g') {"
-      @"    window.webkit.messageHandlers.keyDown.postMessage('C-g');"
+      @"    window.webkit.messageHandlers.__xwidget_internal_keyDown.postMessage('C-g');"
       @"  }"
       @"}"
       @"document.addEventListener('keydown', xwKeyDown);"
       ;
 }
 
+static Lisp_Object js_to_lisp (id value);
+
 /* Confirming to WKScriptMessageHandler, listens concerning keyDown in
    webkit. Currently 'C-g'.  */
 - (void)userContentController:(WKUserContentController *)userContentController
       didReceiveScriptMessage:(WKScriptMessage *)message
 {
-  if ([message.body isEqualToString:@"C-g"])
+  if ([message.name isEqualToString:@"__xwidget_internal_keyDown"])
     {
-      /* Just give up focus, no relay "C-g" to emacs, another "C-g"
-         follows will be handled by emacs.  */
-      [self.window makeFirstResponder:self.xw->xv->emacswindow];
+      if ([message.body isEqualToString:@"C-g"])
+        {
+          /* Just give up focus, no relay "C-g" to emacs, another "C-g"
+             follows will be handled by emacs.  */
+          [self.window makeFirstResponder:self.xw->xv->emacswindow];
+        }
+    }
+  else
+    {
+      store_xwidget_script_message_event (self.xw,
+                                          message.name.UTF8String,
+                                          js_to_lisp (message.body));
     }
 }
 
@@ -437,6 +448,61 @@ - (void)userContentController:(WKUserContentController *)userContentController
     }];
 }
 
+void
+nsxwidget_webkit_add_user_script (struct xwidget *xw, const char *script,
+                                  int injection_time_start, int main_frame_only)
+{
+  XwWebView *xwWebView = (XwWebView *) xw->xwWidget;
+  WKUserContentController *scriptor = xwWebView.configuration.userContentController;
+
+  NSString *javascriptString = [NSString stringWithUTF8String:script];
+  WKUserScriptInjectionTime injectionTime = injection_time_start?
+    WKUserScriptInjectionTimeAtDocumentStart : WKUserScriptInjectionTimeAtDocumentEnd;
+  WKUserScript *userScript = [[WKUserScript alloc]
+                               initWithSource: javascriptString
+                                injectionTime: injectionTime
+                               forMainFrameOnly: main_frame_only];
+  [scriptor addUserScript: userScript];
+}
+
+void
+nsxwidget_webkit_remove_all_user_scripts (struct xwidget *xw)
+{
+  XwWebView *xwWebView = (XwWebView *) xw->xwWidget;
+  WKUserContentController *scriptor = xwWebView.configuration.userContentController;
+
+  [scriptor removeAllUserScripts];
+}
+
+Lisp_Object
+nsxwidget_webkit_register_script_message (struct xwidget *xw, const char *name)
+{
+  XwWebView *xwWebView = (XwWebView *) xw->xwWidget;
+  WKUserContentController *scriptor = xwWebView.configuration.userContentController;
+
+  NSString *messageName = [NSString stringWithUTF8String:name];
+
+  @try
+    {
+      [scriptor addScriptMessageHandler:xwWebView name:messageName];
+    }
+  @catch (NSException *e)
+    {
+      return Qnil;
+    }
+  return Qt;
+}
+
+void
+nsxwidget_webkit_unregister_script_message (struct xwidget *xw, const char *name)
+{
+  XwWebView *xwWebView = (XwWebView *) xw->xwWidget;
+  WKUserContentController *scriptor = xwWebView.configuration.userContentController;
+
+  NSString *messageName = [NSString stringWithUTF8String:name];
+  [scriptor removeScriptMessageHandlerForName:messageName];
+}
+
 /* Window containing an xwidget.  */
 
 @implementation XwWindow
@@ -469,7 +535,7 @@ - (BOOL)isFlipped { return YES; }
       WKUserContentController *scriptor =
         ((XwWebView *) xw->xwWidget).configuration.userContentController;
       [scriptor removeAllUserScripts];
-      [scriptor removeScriptMessageHandlerForName:@"keyDown"];
+      [scriptor removeScriptMessageHandlerForName:@"__xwidget_internal_keyDown"];
       [scriptor release];
       if (xw->xv)
         xw->xv->model = Qnil; /* Make sure related view stale.  */
diff --git a/src/xwidget.c b/src/xwidget.c
index 8bdfab02fd..e4250065b9 100644
--- a/src/xwidget.c
+++ b/src/xwidget.c
@@ -126,6 +126,16 @@ webkit_decide_policy_cb (WebKitWebView *,
 };
 
 static void find_widget (GtkWidget *t, struct widget_search_data *);
+
+struct webkit_script_message_cb_data
+{
+  struct xwidget *xw;
+  char name[0];
+};
+static void webkit_script_message_cb (WebKitUserContentManager *,
+                                          WebKitJavascriptResult *,
+                                          gpointer);
+
 #endif
 
 #ifdef HAVE_PGTK
@@ -380,6 +390,8 @@ DEFUN ("make-xwidget",
 	  settings = webkit_web_view_get_settings (WEBKIT_WEB_VIEW (xw->widget_osr));
 	  g_object_set (G_OBJECT (settings), "enable-developer-extras", TRUE, NULL);
 	}
+      WebKitUserContentManager *scriptor = webkit_user_content_manager_new ();
+      xw->widget_osr = webkit_web_view_new_with_user_content_manager (scriptor);
 
       gtk_widget_set_size_request (GTK_WIDGET (xw->widget_osr), xw->width,
                                    xw->height);
@@ -2308,6 +2320,21 @@ store_xwidget_js_callback_event (struct xwidget *xw,
   kbd_buffer_store_event (&event);
 }
 
+void
+store_xwidget_script_message_event (struct xwidget *xw,
+                                         const char *name,
+                                         Lisp_Object body)
+{
+  struct input_event event;
+  Lisp_Object xwl;
+  XSETXWIDGET (xwl, xw);
+  EVENT_INIT (event);
+  event.kind = XWIDGET_EVENT;
+  event.frame_or_window = Qnil;
+  event.arg = list4 (intern ("script-message"), xwl, intern (name), body);
+  kbd_buffer_store_event (&event);
+}
+
 
 #ifdef USE_GTK
 static void
@@ -2622,6 +2649,17 @@ webkit_decide_policy_cb (WebKitWebView *webView,
   }
 }
 
+static void webkit_script_message_cb (WebKitUserContentManager *scriptor,
+                                          WebKitJavascriptResult *js_result,
+                                          gpointer data)
+{
+  JSCValue *value = webkit_javascript_result_get_js_value (js_result);
+  struct webkit_script_message_cb_data *arg = data;
+
+  Lisp_Object lisp_value = webkit_js_to_lisp (value);
+  store_xwidget_script_message_event (arg->xw, arg->name, lisp_value);
+}
+
 static gboolean
 webkit_script_dialog_cb (WebKitWebView *webview,
 			 WebKitScriptDialog *script_dialog,
@@ -2717,6 +2755,7 @@ xwidget_init_view (struct xwidget *xww,
   XSETWINDOW (xv->w, s->w);
   XSETXWIDGET (xv->model, xww);
 
+
 #ifdef HAVE_X_WINDOWS
   xv->dpy = FRAME_X_DISPLAY (s->f);
 
@@ -3198,6 +3237,140 @@ DEFUN ("xwidget-webkit-execute-script",
   return Qnil;
 }
 
+DEFUN ("xwidget-webkit-add-user-script",
+       Fxwidget_webkit_add_user_script, Sxwidget_webkit_add_user_script,
+       4, 4, 0,
+       doc: /* Add user SCRIPT to the Webkit XWIDGET.
+INJECTION-TIME is a symbol which can take one of the following values:
+
+- start: SCRIPT is injected when document starts loading
+- end: SCRIPT is injected when document finishes loading
+
+If MAIN_FRAME_ONLY is nil, SCRIPT is injected to all frames.
+Otherwise, SCRIPT is only injected to top frames.*/)
+  (Lisp_Object xwidget, Lisp_Object script,
+   Lisp_Object injection_time, Lisp_Object main_frame_only)
+{
+  WEBKIT_FN_INIT ();
+  CHECK_STRING (script);
+  CHECK_SYMBOL (injection_time);
+
+  script = ENCODE_SYSTEM(script);
+
+  int injection_time_start, mfo;
+  mfo = !NILP (main_frame_only);
+  if (EQ (injection_time, Qstart))
+    injection_time_start = 1;
+  else if (EQ (injection_time, Qend))
+    injection_time_start = 0;
+  else
+    error ("Unknown Xwidget Webkit user script injection time: %s",
+           SDATA (SYMBOL_NAME (injection_time)));
+
+#ifdef USE_GTK
+  WebKitWebView *wkwv = WEBKIT_WEB_VIEW (xw->widget_osr);
+  WebKitUserContentManager *scriptor = webkit_web_view_get_user_content_manager (wkwv);
+
+  int webkit_injected_frames = mfo?
+    WEBKIT_USER_CONTENT_INJECT_TOP_FRAME : WEBKIT_USER_CONTENT_INJECT_ALL_FRAMES;
+  int webkit_injection_time = injection_time_start?
+    WEBKIT_USER_SCRIPT_INJECT_AT_DOCUMENT_START : WEBKIT_USER_SCRIPT_INJECT_AT_DOCUMENT_END;
+  WebKitUserScript *userScript = webkit_user_script_new (SSDATA (script),
+                                                         webkit_injected_frames,
+                                                         webkit_injection_time,
+                                                         NULL, NULL);
+  webkit_user_content_manager_add_script (scriptor, userScript);
+  webkit_user_script_unref (userScript);
+#elif defined NS_IMPL_COCOA
+  nsxwidget_webkit_add_user_script (xw, SSDATA (script), injection_time_start, mfo);
+#endif
+  return Qnil;
+}
+
+DEFUN ("xwidget-webkit-remove-all-user-scripts",
+       Fxwidget_webkit_remove_all_user_scripts, Sxwidget_webkit_remove_all_user_scripts,
+       1, 1, 0,
+       doc: /* Remove all user scripts from XWIDGET.  */)
+  (Lisp_Object xwidget)
+{
+  WEBKIT_FN_INIT ();
+
+#ifdef USE_GTK
+  WebKitWebView *wkwv = WEBKIT_WEB_VIEW (xw->widget_osr);
+  WebKitUserContentManager *scriptor = webkit_web_view_get_user_content_manager (wkwv);
+
+  webkit_user_content_manager_remove_all_scripts (scriptor);
+#elif defined NS_IMPL_COCOA
+  nsxwidget_webkit_remove_all_user_scripts(xw);
+#endif
+  return Qnil;
+}
+
+DEFUN ("xwidget-webkit-register-script-message",
+       Fxwidget_webkit_register_script_message, Sxwidget_webkit_register_script_message,
+       2, 2, 0,
+       doc: /* Register script message with symbol NAME in Webkit XWIDGET.
+Returns T if the operation is successful, NIL otherwise.
+The cause of failure is usually that NAME has already been registered for XWIDGET.  */)
+  (Lisp_Object xwidget, Lisp_Object name)
+{
+  WEBKIT_FN_INIT ();
+  CHECK_SYMBOL (name);
+  const char *sname = SSDATA( SYMBOL_NAME (name));
+
+#ifdef USE_GTK
+  WebKitWebView *wkwv = WEBKIT_WEB_VIEW (xw->widget_osr);
+  WebKitUserContentManager *scriptor = webkit_web_view_get_user_content_manager (wkwv);
+
+  gchar *signal_name = g_strconcat ("script-message-received::", sname, NULL);
+  size_t name_length = strlen (sname) + 1;
+  struct webkit_script_message_cb_data *arg = malloc (sizeof *arg + name_length);
+  arg->xw = xw;
+  g_strlcpy (arg->name, sname, name_length);
+  g_signal_connect_data(scriptor, signal_name, G_CALLBACK (webkit_script_message_cb),
+                        arg, (GClosureNotify)free, 0);
+  g_free (signal_name);
+  if (webkit_user_content_manager_register_script_message_handler (scriptor, sname))
+    {
+      return Qt;
+    }
+  else
+    {
+      g_signal_handlers_disconnect_matched  (scriptor,
+                                             G_SIGNAL_MATCH_DATA,
+                                             0, 0, 0, 0, arg);
+      return Qnil;
+    }
+#elif defined NS_IMPL_COCOA
+  return nsxwidget_webkit_register_script_message(xw, sname);
+#endif
+}
+
+DEFUN ("xwidget-webkit-unregister-script-message",
+       Fxwidget_webkit_unregister_script_message, Sxwidget_webkit_unregister_script_message,
+       2, 2, 0,
+       doc: /* Unregister script message with symbol NAME in Webkit XWIDGET.  */)
+  (Lisp_Object xwidget, Lisp_Object name)
+{
+  WEBKIT_FN_INIT ();
+  CHECK_SYMBOL (name);
+  const char *sname = SSDATA( SYMBOL_NAME (name));
+
+#ifdef USE_GTK
+  WebKitWebView *wkwv = WEBKIT_WEB_VIEW (xw->widget_osr);
+  WebKitUserContentManager *scriptor = webkit_web_view_get_user_content_manager (wkwv);
+
+  webkit_user_content_manager_unregister_script_message_handler (scriptor, sname);
+  g_signal_handlers_disconnect_matched  (scriptor,
+                                         G_SIGNAL_MATCH_FUNC | G_SIGNAL_MATCH_DETAIL,
+                                         0, g_quark_from_string (sname), 0,
+                                         G_CALLBACK (webkit_script_message_cb), 0);
+#elif defined NS_IMPL_COCOA
+  nsxwidget_webkit_unregister_script_message(xw, sname);
+#endif
+  return Qnil;
+}
+
 DEFUN ("xwidget-resize", Fxwidget_resize, Sxwidget_resize, 3, 3, 0,
        doc: /* Resize XWIDGET to NEW_WIDTH, NEW_HEIGHT.  */ )
   (Lisp_Object xwidget, Lisp_Object new_width, Lisp_Object new_height)
@@ -3919,6 +4092,14 @@ syms_of_xwidget (void)
   defsubr (&Sxwidget_webkit_execute_script);
   DEFSYM (Qwebkit, "webkit");
 
+  defsubr (&Sxwidget_webkit_add_user_script);
+  DEFSYM (Qstart, "start");
+  DEFSYM (Qend, "end");
+  defsubr (&Sxwidget_webkit_remove_all_user_scripts);
+
+  defsubr (&Sxwidget_webkit_register_script_message);
+  defsubr (&Sxwidget_webkit_unregister_script_message);
+
   defsubr (&Sxwidget_size_request);
   defsubr (&Sdelete_xwidget_view);
 
diff --git a/src/xwidget.h b/src/xwidget.h
index 502beb6765..e835f700bc 100644
--- a/src/xwidget.h
+++ b/src/xwidget.h
@@ -203,6 +203,10 @@ #define XG_XWIDGET_VIEW "emacs_xwidget_view"
                                       Lisp_Object proc,
                                       Lisp_Object argument);
 
+void store_xwidget_script_message_event (struct xwidget *xw,
+                                         const char *name,
+                                         Lisp_Object value);
+
 extern struct xwidget *xwidget_from_id (uint32_t id);
 
 #ifdef HAVE_X_WINDOWS
-- 
2.26.2

Re: [PATCH] Add user content APIs for WebKit Xwidgets

[Po Lu]

Qiantan Hong <qthong@stanford.edu> writes:

> Implement WebKit user scripts and script message handlers.
> * src/xwidget.h (store_xwidget_script_message_event): store script
>   message event into event queue
> * src/xwidget.c (store_xwidget_script_message_event, make-xwidget,
>   webkit_script_message_cb, xwidget-webkit-add-user-script,
>   xwidget-webkit-remove-all-user-scripts,
>   xwidget-webkit-register-script-message,
>   xwidget-webkit-unregister-script-message): Implement user script
>   and script message handler primitives.
> * src/nsxwidget.c (nsxwidget_webkit_add_user_script,
>   nsxwidget_webkit_remove_all_user_scripts,
>   nsxwidget_webkit_register_script_message,
>   nsxwidget_webkit_unregister_script_message,
>   initWithFrame, initialize, userContentController): NS
>   implementation. Changed naming of a previous used  script message
>   handler to avoid namespace pollution.
> * src/nsxwidget.h (nsxwidget_webkit_add_user_script,
>   nsxwidget_webkit_remove_all_user_scripts,
>   nsxwidget_webkit_register_script_message,
>   nsxwidget_webkit_unregister_script_message): NS implementation
> * lisp/xwidget.el (xwidget-webkit-callback,
>   xwidget-webkit-push-script-message-handler,
>   xwidget-webkit-pop-script-message-handler):
>   let lisp recognize and dispatch script message events

Thanks; this is not how we write commit messages.  It should be:

* src/xwidget.h (store_xwidget_script_message_event):
* src/xwidget.c (store_xwidget_script_message_event):
(xwidget_script_message_cb):
(Fxwidget_webkit_add_user_script): Store script message event
into event queue.

and so on.  Pay attention to how the name of the C function is written
for defuns, how continuing lines in each message are not indented, and
how brackets are placed around functions.

> Acked-by: Qiantan Hong <qhong@mit.edu>

Please remove this extraneous line from the comment message.

> +          ((eq xwidget-event-type 'script-message)
> +           (let ((name (nth 3 last-input-event))
> +                 (value (nth 4 last-input-event)))
> +             (let ((handler-pair (assq name (xwidget-get xwidget 'script-message-handlers))))
> +               (if handler-pair
> +                   (funcall (cdr handler-pair) xwidget value)
> +                 (xwidget-log "unhandled script message:%s" name)))))

Please place a space between : and " ".

> +(defun xwidget-webkit-push-script-message-handler (xwidget name handler)
> +  "Associate HANDLER with script messages under symbol NAME for Webkit XWIDGET.
> +
> +HANDLER will be called with two arguments: the Webkit XWIDGET and
> +the javascript object passed from the script message converted to
> +a Lisp object."
> +  (xwidget-webkit-register-script-message (xwidget-webkit-current-session) name)

80 columns alert!  That aside, this does not explain what "script
messages" are.  "Webkit" and "javascript" should also be capitalized
"WebKit" and "JavaScript" respectively.

> +(defun xwidget-webkit-pop-script-message-handler (xwidget name)
> +  "Remove a handler associated with symbol NAME for Webkit XWIDGET.
> +
> +Returns the removed handler function, or NIL if such handler is
> +not found."
> +  (let* ((old-alist (xwidget-get xwidget 'script-message-handlers))
> +         (handler-pair (assq name old-alist)))
> +    (when handler-pair
> +      (let ((new-alist (delq handler-pair old-alist)))
> +        (xwidget-put xwidget 'script-message-handlers new-alist)
> +        (unless (assq name new-alist)
> +          (xwidget-webkit-unregister-script-message (xwidget-webkit-current-session) name)))

80 columns alert!  What I said about documentation above applies here as
well.

> +  @try
> +    {
> +      [scriptor addScriptMessageHandler:xwWebView name:messageName];
> +    }
> +  @catch (NSException *e)
> +    {
> +      return Qnil;
> +    }

Please remove the unnecessary braces here.

> diff --git a/src/xwidget.c b/src/xwidget.c
> index 8bdfab02fd..e4250065b9 100644
> --- a/src/xwidget.c
> +++ b/src/xwidget.c
> @@ -126,6 +126,16 @@ webkit_decide_policy_cb (WebKitWebView *,
>  };
>  
>  static void find_widget (GtkWidget *t, struct widget_search_data *);
> +
> +struct webkit_script_message_cb_data
> +{
> +  struct xwidget *xw;
> +  char name[0];
> +};
> +static void webkit_script_message_cb (WebKitUserContentManager *,
> +                                          WebKitJavascriptResult *,
> +                                          gpointer);
> +

Zero-length arrays are not portable, and it is bad style to store string
data in arrays that way.  Please allocate a separate string, and free it
manually in the GDestroyNotify.  In addition, please write a comment
above each field describing its meaning.

> +      WebKitUserContentManager *scriptor = webkit_user_content_manager_new ();
> +      xw->widget_osr = webkit_web_view_new_with_user_content_manager (scriptor);

scriptor is leaked, as WebKitUserContentManager not GInitiallyUnowned.
Did you forget a g_object_unref?

> +void
> +store_xwidget_script_message_event (struct xwidget *xw,
> +                                         const char *name,
> +                                         Lisp_Object body)
> +{
> +  struct input_event event;
> +  Lisp_Object xwl;
> +  XSETXWIDGET (xwl, xw);
> +  EVENT_INIT (event);
> +  event.kind = XWIDGET_EVENT;
> +  event.frame_or_window = Qnil;
> +  event.arg = list4 (intern ("script-message"), xwl, intern (name), body);
> +  kbd_buffer_store_event (&event);
> +}

Calling intern with a static string is bad style.  Please write this as
a DEFSYM instead, in syms_of_xwidget:

  DEFSYM (Qscript_message, "script-message");

and use Qscript_message.

> +static void webkit_script_message_cb (WebKitUserContentManager *scriptor,
> +                                          WebKitJavascriptResult *js_result,
> +                                          gpointer data)
> +{
> +  JSCValue *value = webkit_javascript_result_get_js_value (js_result);
> +  struct webkit_script_message_cb_data *arg = data;
> +
> +  Lisp_Object lisp_value = webkit_js_to_lisp (value);
> +  store_xwidget_script_message_event (arg->xw, arg->name, lisp_value);
> +}

Please add a newline after "static void".  It is also very ugly to put a
newline between "arg" and "lisp_value"; I would prefer this be written:

  JSCValue *value;
  struct webkit_script_message_cb_data *arg;
  Lisp_Object lisp_value;

  value = webkit_javascript...

In addition, don't you have to call webkit_javascript_result_unref?

> +
>  #ifdef HAVE_X_WINDOWS
>    xv->dpy = FRAME_X_DISPLAY (s->f);
>  

Please remove this whitespace change.

> +       doc: /* Add user SCRIPT to the Webkit XWIDGET.

This should be read "Add the user script SCRIPT to the WebKit XWIDGET.",
followed by a short summary of what userscripts are, and what the script
argument should be.

> +INJECTION-TIME is a symbol which can take one of the following values:

"WHEN is one of the following values, and specifies when the script is run:"

> +- start: SCRIPT is injected when document starts loading
> +- end: SCRIPT is injected when document finishes loading

"  - `start', which means the script is run upon first loading a document.
   - `end', which means the script is run after a document loads."

> +If MAIN_FRAME_ONLY is nil, SCRIPT is injected to all frames.

"If MAIN-FRAME-ONLY is non-nil, run SCRIPT for all frames."  (Followed
by a description of what "frames" are.)

> +Otherwise, SCRIPT is only injected to top frames.

What are top frames?

> +  script = ENCODE_SYSTEM(script);

Are you sure that is the right coding system?  WebKit APIs usually take
UTF-8 strings.

In any case, this should be:

  script = ENCODE_SYSTEM (script);

> +  int injection_time_start, mfo;
> +  mfo = !NILP (main_frame_only);

What is the purpose of those two variables?  In either case, `mfo'
should be named something else, as that name carries no meaning.

When chosing variable names, try to avoid truncating words, or worse,
simply making up acronyms.  Consider the following:

  Bool window_data_exists;

  window_data_exists = (XGetWindowProperty ....

or even

  Bool data_exists;

  data_exists = (XGetWindowProperty ....

and note how much clearer it is compared to:

  Bool wde;

  wde = (XGetWindowProperty ...

or

  Bool dat_p;

  dat_p = (XGetWindowProperty ...

> +  if (EQ (injection_time, Qstart))
> +    injection_time_start = 1;
> +  else if (EQ (injection_time, Qend))
> +    injection_time_start = 0;
> +
> +    error ("Unknown Xwidget Webkit user script injection time: %s",
> +           SDATA (SYMBOL_NAME (injection_time)));

Since there are only two values, how about replacing `injection-time'
with a single boolean argument `start', which determines whether to run
the user script at or after page load?

> +  int webkit_injected_frames = mfo?
> +    WEBKIT_USER_CONTENT_INJECT_TOP_FRAME : WEBKIT_USER_CONTENT_INJECT_ALL_FRAMES;

This is not our coding style.  It should be:

  (!NILP (main_frame_only)
   ? WEBKIT_USER_CONTENT_INJECT_TOP_FRAME
   : WEBKIT_USER_CONTENT_INJECT_ALL_FRAMES)

further more, the type is not int, it is enum
WebKitUserContentInjectedFrames.

> +  int webkit_injection_time = injection_time_start?
> +    WEBKIT_USER_SCRIPT_INJECT_AT_DOCUMENT_START : WEBKIT_USER_SCRIPT_INJECT_AT_DOCUMENT_END;

Likewise.  enum WebKitUserScriptInjectionTime.

> +  WebKitUserScript *userScript = webkit_user_script_new (SSDATA (script),
> +                                                         webkit_injected_frames,
> +                                                         webkit_injection_time,
> +                                                         NULL, NULL);

80 columns alert!  Please name the variable "user_script" instead; this
piece of code should probably be written as follows:

  WebKitUserScript *user_script
    = webkit_user_script_new (SSDATA (script),
			      webkit_injected_frames,
			      webkit_injection_time,
			      NULL, NULL);

> +DEFUN ("xwidget-webkit-register-script-message",
> +       Fxwidget_webkit_register_script_message, Sxwidget_webkit_register_script_message,
> +       2, 2, 0,
> +       doc: /* Register script message with symbol NAME in Webkit
> XWIDGET.

What was said about capitalization above also applies here.  Please fill
everything to 80 columns as well.

> +Returns T if the operation is successful, NIL otherwise.

Emacs Lisp is not Common Lisp, and does not capitalize symbol names by
default.

> +The cause of failure is usually that NAME has already been registered for XWIDGET.  */)

This belongs in the Lisp reference manual, and is likely redundant in
any case.  Also, the function should signal an error if NAME has already
been used, instead of simply returning nil.

> +  const char *sname = SSDATA( SYMBOL_NAME (name));

This is a pointer to string data.  It is not safe to assume GC does not
happen and relocate it below, due to the multitude of GLib callbacks
installed that can call GC.  Consider using xlispstrdup instead.

In addition to that, please make "name" a string; I see no reason for it
to be a symbol.

> +#ifdef USE_GTK
> +  WebKitWebView *wkwv = WEBKIT_WEB_VIEW (xw->widget_osr);

Please name this variable view, not "wkwv", for the reasons explained
above.

> +  gchar *signal_name = g_strconcat ("script-message-received::", sname, NULL);

Please consider using alloca or SAFE_ALLOCA to allocate this buffer, and
then using strncpy or lispstpcpy to copy the data over.

SAFE_ALLOCA is used like this, when there are no conflicting specbinds:

void
function_using_safe_alloca (void)
{
  char *mybuffer;

  USE_SAFE_ALLOCA;
  mybuffer = SAFE_ALLOCA (size_of_buffer);

  /* Do stuff with mybuffer... Finally: */

  SAFE_FREE ();
}

> +  struct webkit_script_message_cb_data *arg = malloc (sizeof *arg + name_length);
> +  arg->xw = xw;
> +  g_strlcpy (arg->name, sname, name_length);

Recall what I said about zero-length or variable length arrays being bad
style here.  Please allocate arg->name separately, probably with
lispstpcpy...

> +  g_signal_connect_data(scriptor, signal_name, G_CALLBACK (webkit_script_message_cb),
> +                        arg, (GClosureNotify)free, 0);
                                                ^^^^

and free it in the destroy_data callback.  In addition, please put a
space between the cast and the type, and use G_CONNECT_DEFAULT as
opposed to 0.

In addition, xw is a Lisp object.  It should be marked for garbage
collection as long as the signal handler is attached, as making
assumptions about when signals are run is not safe.

> +  if (webkit_user_content_manager_register_script_message_handler (scriptor, sname))
> +    {
> +      return Qt;
> +    }
> +  else
> +    {
> +      g_signal_handlers_disconnect_matched  (scriptor,
                                             ^

Extra whitespace.  Please also remove the redundant braces above.

> +  WebKitWebView *wkwv = WEBKIT_WEB_VIEW (xw->widget_osr);

"WebKitWebView *view = WEBKIT_WEB_VIEW (..."

> +  WebKitUserContentManager *scriptor = webkit_web_view_get_user_content_manager (wkwv);

How about naming all the "scriptor" variables "content_manager", or
"manager" instead?  Also, please fill this function to 80 columns as
well.

> +  g_signal_handlers_disconnect_matched  (scriptor,
> +                                         G_SIGNAL_MATCH_FUNC | G_SIGNAL_MATCH_DETAIL,
> +                                         0, g_quark_from_string (sname), 0,
> +                                         G_CALLBACK (webkit_script_message_cb), 0);

There is extra whitespace here.  

> +
>  DEFUN ("xwidget-resize", Fxwidget_resize, Sxwidget_resize, 3, 3, 0,
>         doc: /* Resize XWIDGET to NEW_WIDTH, NEW_HEIGHT.  */ )
>    (Lisp_Object xwidget, Lisp_Object new_width, Lisp_Object new_height)
> @@ -3919,6 +4092,14 @@ syms_of_xwidget (void)
>    defsubr (&Sxwidget_webkit_execute_script);
>    DEFSYM (Qwebkit, "webkit");
>  
> +  defsubr (&Sxwidget_webkit_add_user_script);
> +  DEFSYM (Qstart, "start");
> +  DEFSYM (Qend, "end");
> +  defsubr (&Sxwidget_webkit_remove_all_user_scripts);
> +
> +  defsubr (&Sxwidget_webkit_register_script_message);
> +  defsubr (&Sxwidget_webkit_unregister_script_message);
> +
>    defsubr (&Sxwidget_size_request);
>    defsubr (&Sdelete_xwidget_view);

Why are none of the new xwidget event and all of these new functions
documented in the Lisp reference manual?

I did not look at the NS code very closely, but I also can't see where
anything is deallocated there.  If you wrote that code assuming
Objective-C automatic reference counting is used in Emacs, it will have
to be rewritten to use manual memory management, as Objective-C features
not supported by GCC are not allowed in Emacs.

Re: [PATCH] Add user content APIs for WebKit Xwidgets

[Qiantan Hong]

Thanks for the detailed comments! A few questions:

>> +void
>> +store_xwidget_script_message_event (struct xwidget *xw,
>> +                                         const char *name,
>> +                                         Lisp_Object body)
>> +{
>> +  struct input_event event;
>> +  Lisp_Object xwl;
>> +  XSETXWIDGET (xwl, xw);
>> +  EVENT_INIT (event);
>> +  event.kind = XWIDGET_EVENT;
>> +  event.frame_or_window = Qnil;
>> +  event.arg = list4 (intern ("script-message"), xwl, intern (name), body);
>> +  kbd_buffer_store_event (&event);
>> +}
> 
> Calling intern with a static string is bad style.  Please write this as
> a DEFSYM instead, in syms_of_xwidget:
> 
>  DEFSYM (Qscript_message, "script-message");
> 
> and use Qscript_message.

I see similar intern usage for "xwidget-internal”, "download-callback”
and "javascript-callback” in xwidget.c. I was trying to be consistent.
If I’m changing to Qscript_message should those be changed too?

> In addition to that, please make "name" a string; I see no reason for it
> to be a symbol.
name is used as a symbol/identifier in a namespace in JavaScript. 
It is used in the form of “messageHandlers.name”. Therefore I wanted
to map it to something conceptually similar in Lisp, i.e. a Lisp symbol.
That way we get efficient comparison and EQ lookup (useful for identifiers!)
by sacrificing string-manipulating operations (not that often for identifiers!).
For example, I need to lookup a handler in an ALIST for every script-message.
Does that make sense?

Best,
Qiantan



> On Oct 15, 2022, at 4:23 AM, Po Lu <luangruo@yahoo.com> wrote:
> 
> Qiantan Hong <qthong@stanford.edu> writes:
> 
>> Implement WebKit user scripts and script message handlers.
>> * src/xwidget.h (store_xwidget_script_message_event): store script
>>  message event into event queue
>> * src/xwidget.c (store_xwidget_script_message_event, make-xwidget,
>>  webkit_script_message_cb, xwidget-webkit-add-user-script,
>>  xwidget-webkit-remove-all-user-scripts,
>>  xwidget-webkit-register-script-message,
>>  xwidget-webkit-unregister-script-message): Implement user script
>>  and script message handler primitives.
>> * src/nsxwidget.c (nsxwidget_webkit_add_user_script,
>>  nsxwidget_webkit_remove_all_user_scripts,
>>  nsxwidget_webkit_register_script_message,
>>  nsxwidget_webkit_unregister_script_message,
>>  initWithFrame, initialize, userContentController): NS
>>  implementation. Changed naming of a previous used  script message
>>  handler to avoid namespace pollution.
>> * src/nsxwidget.h (nsxwidget_webkit_add_user_script,
>>  nsxwidget_webkit_remove_all_user_scripts,
>>  nsxwidget_webkit_register_script_message,
>>  nsxwidget_webkit_unregister_script_message): NS implementation
>> * lisp/xwidget.el (xwidget-webkit-callback,
>>  xwidget-webkit-push-script-message-handler,
>>  xwidget-webkit-pop-script-message-handler):
>>  let lisp recognize and dispatch script message events
> 
> Thanks; this is not how we write commit messages.  It should be:
> 
> * src/xwidget.h (store_xwidget_script_message_event):
> * src/xwidget.c (store_xwidget_script_message_event):
> (xwidget_script_message_cb):
> (Fxwidget_webkit_add_user_script): Store script message event
> into event queue.
> 
> and so on.  Pay attention to how the name of the C function is written
> for defuns, how continuing lines in each message are not indented, and
> how brackets are placed around functions.
> 
>> Acked-by: Qiantan Hong <qhong@mit.edu>
> 
> Please remove this extraneous line from the comment message.
> 
>> +          ((eq xwidget-event-type 'script-message)
>> +           (let ((name (nth 3 last-input-event))
>> +                 (value (nth 4 last-input-event)))
>> +             (let ((handler-pair (assq name (xwidget-get xwidget 'script-message-handlers))))
>> +               (if handler-pair
>> +                   (funcall (cdr handler-pair) xwidget value)
>> +                 (xwidget-log "unhandled script message:%s" name)))))
> 
> Please place a space between : and " ".
> 
>> +(defun xwidget-webkit-push-script-message-handler (xwidget name handler)
>> +  "Associate HANDLER with script messages under symbol NAME for Webkit XWIDGET.
>> +
>> +HANDLER will be called with two arguments: the Webkit XWIDGET and
>> +the javascript object passed from the script message converted to
>> +a Lisp object."
>> +  (xwidget-webkit-register-script-message (xwidget-webkit-current-session) name)
> 
> 80 columns alert!  That aside, this does not explain what "script
> messages" are.  "Webkit" and "javascript" should also be capitalized
> "WebKit" and "JavaScript" respectively.
> 
>> +(defun xwidget-webkit-pop-script-message-handler (xwidget name)
>> +  "Remove a handler associated with symbol NAME for Webkit XWIDGET.
>> +
>> +Returns the removed handler function, or NIL if such handler is
>> +not found."
>> +  (let* ((old-alist (xwidget-get xwidget 'script-message-handlers))
>> +         (handler-pair (assq name old-alist)))
>> +    (when handler-pair
>> +      (let ((new-alist (delq handler-pair old-alist)))
>> +        (xwidget-put xwidget 'script-message-handlers new-alist)
>> +        (unless (assq name new-alist)
>> +          (xwidget-webkit-unregister-script-message (xwidget-webkit-current-session) name)))
> 
> 80 columns alert!  What I said about documentation above applies here as
> well.
> 
>> +  @try
>> +    {
>> +      [scriptor addScriptMessageHandler:xwWebView name:messageName];
>> +    }
>> +  @catch (NSException *e)
>> +    {
>> +      return Qnil;
>> +    }
> 
> Please remove the unnecessary braces here.
> 
>> diff --git a/src/xwidget.c b/src/xwidget.c
>> index 8bdfab02fd..e4250065b9 100644
>> --- a/src/xwidget.c
>> +++ b/src/xwidget.c
>> @@ -126,6 +126,16 @@ webkit_decide_policy_cb (WebKitWebView *,
>> };
>> 
>> static void find_widget (GtkWidget *t, struct widget_search_data *);
>> +
>> +struct webkit_script_message_cb_data
>> +{
>> +  struct xwidget *xw;
>> +  char name[0];
>> +};
>> +static void webkit_script_message_cb (WebKitUserContentManager *,
>> +                                          WebKitJavascriptResult *,
>> +                                          gpointer);
>> +
> 
> Zero-length arrays are not portable, and it is bad style to store string
> data in arrays that way.  Please allocate a separate string, and free it
> manually in the GDestroyNotify.  In addition, please write a comment
> above each field describing its meaning.
> 
>> +      WebKitUserContentManager *scriptor = webkit_user_content_manager_new ();
>> +      xw->widget_osr = webkit_web_view_new_with_user_content_manager (scriptor);
> 
> scriptor is leaked, as WebKitUserContentManager not GInitiallyUnowned.
> Did you forget a g_object_unref?
> 
>> +void
>> +store_xwidget_script_message_event (struct xwidget *xw,
>> +                                         const char *name,
>> +                                         Lisp_Object body)
>> +{
>> +  struct input_event event;
>> +  Lisp_Object xwl;
>> +  XSETXWIDGET (xwl, xw);
>> +  EVENT_INIT (event);
>> +  event.kind = XWIDGET_EVENT;
>> +  event.frame_or_window = Qnil;
>> +  event.arg = list4 (intern ("script-message"), xwl, intern (name), body);
>> +  kbd_buffer_store_event (&event);
>> +}
> 
> Calling intern with a static string is bad style.  Please write this as
> a DEFSYM instead, in syms_of_xwidget:
> 
>  DEFSYM (Qscript_message, "script-message");
> 
> and use Qscript_message.
> 
>> +static void webkit_script_message_cb (WebKitUserContentManager *scriptor,
>> +                                          WebKitJavascriptResult *js_result,
>> +                                          gpointer data)
>> +{
>> +  JSCValue *value = webkit_javascript_result_get_js_value (js_result);
>> +  struct webkit_script_message_cb_data *arg = data;
>> +
>> +  Lisp_Object lisp_value = webkit_js_to_lisp (value);
>> +  store_xwidget_script_message_event (arg->xw, arg->name, lisp_value);
>> +}
> 
> Please add a newline after "static void".  It is also very ugly to put a
> newline between "arg" and "lisp_value"; I would prefer this be written:
> 
>  JSCValue *value;
>  struct webkit_script_message_cb_data *arg;
>  Lisp_Object lisp_value;
> 
>  value = webkit_javascript...
> 
> In addition, don't you have to call webkit_javascript_result_unref?
> 
>> +
>> #ifdef HAVE_X_WINDOWS
>>   xv->dpy = FRAME_X_DISPLAY (s->f);
>> 
> 
> Please remove this whitespace change.
> 
>> +       doc: /* Add user SCRIPT to the Webkit XWIDGET.
> 
> This should be read "Add the user script SCRIPT to the WebKit XWIDGET.",
> followed by a short summary of what userscripts are, and what the script
> argument should be.
> 
>> +INJECTION-TIME is a symbol which can take one of the following values:
> 
> "WHEN is one of the following values, and specifies when the script is run:"
> 
>> +- start: SCRIPT is injected when document starts loading
>> +- end: SCRIPT is injected when document finishes loading
> 
> "  - `start', which means the script is run upon first loading a document.
>   - `end', which means the script is run after a document loads."
> 
>> +If MAIN_FRAME_ONLY is nil, SCRIPT is injected to all frames.
> 
> "If MAIN-FRAME-ONLY is non-nil, run SCRIPT for all frames."  (Followed
> by a description of what "frames" are.)
> 
>> +Otherwise, SCRIPT is only injected to top frames.
> 
> What are top frames?
> 
>> +  script = ENCODE_SYSTEM(script);
> 
> Are you sure that is the right coding system?  WebKit APIs usually take
> UTF-8 strings.
> 
> In any case, this should be:
> 
>  script = ENCODE_SYSTEM (script);
> 
>> +  int injection_time_start, mfo;
>> +  mfo = !NILP (main_frame_only);
> 
> What is the purpose of those two variables?  In either case, `mfo'
> should be named something else, as that name carries no meaning.
> 
> When chosing variable names, try to avoid truncating words, or worse,
> simply making up acronyms.  Consider the following:
> 
>  Bool window_data_exists;
> 
>  window_data_exists = (XGetWindowProperty ....
> 
> or even
> 
>  Bool data_exists;
> 
>  data_exists = (XGetWindowProperty ....
> 
> and note how much clearer it is compared to:
> 
>  Bool wde;
> 
>  wde = (XGetWindowProperty ...
> 
> or
> 
>  Bool dat_p;
> 
>  dat_p = (XGetWindowProperty ...
> 
>> +  if (EQ (injection_time, Qstart))
>> +    injection_time_start = 1;
>> +  else if (EQ (injection_time, Qend))
>> +    injection_time_start = 0;
>> +
>> +    error ("Unknown Xwidget Webkit user script injection time: %s",
>> +           SDATA (SYMBOL_NAME (injection_time)));
> 
> Since there are only two values, how about replacing `injection-time'
> with a single boolean argument `start', which determines whether to run
> the user script at or after page load?
> 
>> +  int webkit_injected_frames = mfo?
>> +    WEBKIT_USER_CONTENT_INJECT_TOP_FRAME : WEBKIT_USER_CONTENT_INJECT_ALL_FRAMES;
> 
> This is not our coding style.  It should be:
> 
>  (!NILP (main_frame_only)
>   ? WEBKIT_USER_CONTENT_INJECT_TOP_FRAME
>   : WEBKIT_USER_CONTENT_INJECT_ALL_FRAMES)
> 
> further more, the type is not int, it is enum
> WebKitUserContentInjectedFrames.
> 
>> +  int webkit_injection_time = injection_time_start?
>> +    WEBKIT_USER_SCRIPT_INJECT_AT_DOCUMENT_START : WEBKIT_USER_SCRIPT_INJECT_AT_DOCUMENT_END;
> 
> Likewise.  enum WebKitUserScriptInjectionTime.
> 
>> +  WebKitUserScript *userScript = webkit_user_script_new (SSDATA (script),
>> +                                                         webkit_injected_frames,
>> +                                                         webkit_injection_time,
>> +                                                         NULL, NULL);
> 
> 80 columns alert!  Please name the variable "user_script" instead; this
> piece of code should probably be written as follows:
> 
>  WebKitUserScript *user_script
>    = webkit_user_script_new (SSDATA (script),
> 			      webkit_injected_frames,
> 			      webkit_injection_time,
> 			      NULL, NULL);
> 
>> +DEFUN ("xwidget-webkit-register-script-message",
>> +       Fxwidget_webkit_register_script_message, Sxwidget_webkit_register_script_message,
>> +       2, 2, 0,
>> +       doc: /* Register script message with symbol NAME in Webkit
>> XWIDGET.
> 
> What was said about capitalization above also applies here.  Please fill
> everything to 80 columns as well.
> 
>> +Returns T if the operation is successful, NIL otherwise.
> 
> Emacs Lisp is not Common Lisp, and does not capitalize symbol names by
> default.
> 
>> +The cause of failure is usually that NAME has already been registered for XWIDGET.  */)
> 
> This belongs in the Lisp reference manual, and is likely redundant in
> any case.  Also, the function should signal an error if NAME has already
> been used, instead of simply returning nil.
> 
>> +  const char *sname = SSDATA( SYMBOL_NAME (name));
> 
> This is a pointer to string data.  It is not safe to assume GC does not
> happen and relocate it below, due to the multitude of GLib callbacks
> installed that can call GC.  Consider using xlispstrdup instead.
> 
> In addition to that, please make "name" a string; I see no reason for it
> to be a symbol.
> 
>> +#ifdef USE_GTK
>> +  WebKitWebView *wkwv = WEBKIT_WEB_VIEW (xw->widget_osr);
> 
> Please name this variable view, not "wkwv", for the reasons explained
> above.
> 
>> +  gchar *signal_name = g_strconcat ("script-message-received::", sname, NULL);
> 
> Please consider using alloca or SAFE_ALLOCA to allocate this buffer, and
> then using strncpy or lispstpcpy to copy the data over.
> 
> SAFE_ALLOCA is used like this, when there are no conflicting specbinds:
> 
> void
> function_using_safe_alloca (void)
> {
>  char *mybuffer;
> 
>  USE_SAFE_ALLOCA;
>  mybuffer = SAFE_ALLOCA (size_of_buffer);
> 
>  /* Do stuff with mybuffer... Finally: */
> 
>  SAFE_FREE ();
> }
> 
>> +  struct webkit_script_message_cb_data *arg = malloc (sizeof *arg + name_length);
>> +  arg->xw = xw;
>> +  g_strlcpy (arg->name, sname, name_length);
> 
> Recall what I said about zero-length or variable length arrays being bad
> style here.  Please allocate arg->name separately, probably with
> lispstpcpy...
> 
>> +  g_signal_connect_data(scriptor, signal_name, G_CALLBACK (webkit_script_message_cb),
>> +                        arg, (GClosureNotify)free, 0);
>                                                ^^^^
> 
> and free it in the destroy_data callback.  In addition, please put a
> space between the cast and the type, and use G_CONNECT_DEFAULT as
> opposed to 0.
> 
> In addition, xw is a Lisp object.  It should be marked for garbage
> collection as long as the signal handler is attached, as making
> assumptions about when signals are run is not safe.
> 
>> +  if (webkit_user_content_manager_register_script_message_handler (scriptor, sname))
>> +    {
>> +      return Qt;
>> +    }
>> +  else
>> +    {
>> +      g_signal_handlers_disconnect_matched  (scriptor,
>                                             ^
> 
> Extra whitespace.  Please also remove the redundant braces above.
> 
>> +  WebKitWebView *wkwv = WEBKIT_WEB_VIEW (xw->widget_osr);
> 
> "WebKitWebView *view = WEBKIT_WEB_VIEW (..."
> 
>> +  WebKitUserContentManager *scriptor = webkit_web_view_get_user_content_manager (wkwv);
> 
> How about naming all the "scriptor" variables "content_manager", or
> "manager" instead?  Also, please fill this function to 80 columns as
> well.
> 
>> +  g_signal_handlers_disconnect_matched  (scriptor,
>> +                                         G_SIGNAL_MATCH_FUNC | G_SIGNAL_MATCH_DETAIL,
>> +                                         0, g_quark_from_string (sname), 0,
>> +                                         G_CALLBACK (webkit_script_message_cb), 0);
> 
> There is extra whitespace here.  
> 
>> +
>> DEFUN ("xwidget-resize", Fxwidget_resize, Sxwidget_resize, 3, 3, 0,
>>        doc: /* Resize XWIDGET to NEW_WIDTH, NEW_HEIGHT.  */ )
>>   (Lisp_Object xwidget, Lisp_Object new_width, Lisp_Object new_height)
>> @@ -3919,6 +4092,14 @@ syms_of_xwidget (void)
>>   defsubr (&Sxwidget_webkit_execute_script);
>>   DEFSYM (Qwebkit, "webkit");
>> 
>> +  defsubr (&Sxwidget_webkit_add_user_script);
>> +  DEFSYM (Qstart, "start");
>> +  DEFSYM (Qend, "end");
>> +  defsubr (&Sxwidget_webkit_remove_all_user_scripts);
>> +
>> +  defsubr (&Sxwidget_webkit_register_script_message);
>> +  defsubr (&Sxwidget_webkit_unregister_script_message);
>> +
>>   defsubr (&Sxwidget_size_request);
>>   defsubr (&Sdelete_xwidget_view);
> 
> Why are none of the new xwidget event and all of these new functions
> documented in the Lisp reference manual?
> 
> I did not look at the NS code very closely, but I also can't see where
> anything is deallocated there.  If you wrote that code assuming
> Objective-C automatic reference counting is used in Emacs, it will have
> to be rewritten to use manual memory management, as Objective-C features
> not supported by GCC are not allowed in Emacs.
> 

Re: [PATCH] Add user content APIs for WebKit Xwidgets

[Qiantan Hong]

More questions:

> 
>> + WebKitWebView *wkwv = WEBKIT_WEB_VIEW (xw->widget_osr);
> 
> "WebKitWebView *view = WEBKIT_WEB_VIEW (…"

Some existing code in xwidget.c use this naming as well.
Should I make a separate patch to fix those?
Should I also include the fix to some capitalization error
and the runtime symbol usage in that same patch?

> In addition, xw is a Lisp object. It should be marked for garbage
> collection as long as the signal handler is attached, as making
> assumptions about when signals are run is not safe.
How do I mark it?

> I did not look at the NS code very closely, but I also can't see where
> anything is deallocated there. If you wrote that code assuming
> Objective-C automatic reference counting is used in Emacs, it will have
> to be rewritten to use manual memory management, as Objective-C features
> not supported by GCC are not allowed in Emacs.
I’m imitating the usage pattern of what is already in nsxwidget.m.
I think if I’m leaking memory then the current code must also have been
(which could be the case)...

Best,
Qiantan

Re: [PATCH] Add user content APIs for WebKit Xwidgets

[Po Lu]

Qiantan Hong <qthong@stanford.edu> writes:

> I see similar intern usage for "xwidget-internal”, "download-callback”
> and "javascript-callback” in xwidget.c. I was trying to be consistent.
> If I’m changing to Qscript_message should those be changed too?

Sure.

> name is used as a symbol/identifier in a namespace in JavaScript. 
> It is used in the form of “messageHandlers.name”. Therefore I wanted
> to map it to something conceptually similar in Lisp, i.e. a Lisp symbol.
> That way we get efficient comparison and EQ lookup (useful for identifiers!)
> by sacrificing string-manipulating operations (not that often for identifiers!).
> For example, I need to lookup a handler in an ALIST for every script-message.
> Does that make sense?

assoc is fast unless you have lots of handlers, in which case I'd be
more worried about using an alist.

Anyway, you proceed to copy the symbol name into a string and intern it
later.  That is not correct, since the symbol given may be uninterned.

Re: [PATCH] Add user content APIs for WebKit Xwidgets

[Po Lu]

Qiantan Hong <qthong@stanford.edu> writes:

> Some existing code in xwidget.c use this naming as well.
> Should I make a separate patch to fix those?

No, I'd rather avoid extraneous style changes to existing contents of
the file.  At the very least it makes vc-annotate a pain.

> Should I also include the fix to some capitalization error
> and the runtime symbol usage in that same patch?

BTW, there are many other problems with your patch, aside from
capitalization and interning symbols with a static string.  Please read
through the entirety of my reply.

> How do I mark it?

I suggest linking each callback data struct into a list, each element of
which is then marked in a "mark_xwidget" function that should be called
by garbage_collect in alloc.c.

> I’m imitating the usage pattern of what is already in nsxwidget.m.
> I think if I’m leaking memory then the current code must also have been
> (which could be the case)...

I guess that should be fixed.

Thanks.

Re: [PATCH] Add user content APIs for WebKit Xwidgets

[Qiantan Hong]

> BTW, there are many other problems with your patch, aside from
> capitalization and interning symbols with a static string.  Please read
> through the entirety of my reply.
Yes I’m aware. I was fixing them one by one.

>> How do I mark it?
> 
> I suggest linking each callback data struct into a list, each element of
> which is then marked in a "mark_xwidget" function that should be called
> by garbage_collect in alloc.c.
It seems that existing code is connecting signal without adding the marking
code, guess that should be fixed too...

> 
>> I’m imitating the usage pattern of what is already in nsxwidget.m.
>> I think if I’m leaking memory then the current code must also have been
>> (which could be the case)...
> 
> I guess that should be fixed.

Those two problems above, I don’t have any existing code to imitate, so it
would take a while to learn the codebase for me to be able to fix them.
I’m also not sure if I should just make my part conforming or also fix all existing code.
Is it acceptable that I submit a patch (with other problems fixed) first,
and work on fixing these two classes of problems later?

Best,
Qiantan

Re: [PATCH] Add user content APIs for WebKit Xwidgets

[Po Lu]

Qiantan Hong <qthong@stanford.edu> writes:

> Yes I’m aware. I was fixing them one by one.

Thanks.

> It seems that existing code is connecting signal without adding the marking
> code, guess that should be fixed too...

Where?  Existing code should not connect signals to anything other than
`widget_osr' which is destroyed upon the xwidget being killed, a
prerequisite for being marked by GC.

> Those two problems above, I don’t have any existing code to imitate, so it
> would take a while to learn the codebase for me to be able to fix them.
> I’m also not sure if I should just make my part conforming or also fix all existing code.
> Is it acceptable that I submit a patch (with other problems fixed) first,
> and work on fixing these two classes of problems later?

I'd rather not install code with known memory management issues,
especially not this close to the Emacs 29 release.

Re: [PATCH] Add user content APIs for WebKit Xwidgets

[Po Lu]

Po Lu <luangruo@yahoo.com> writes:

> Where?  Existing code should not connect signals to anything other than
> `widget_osr' which is destroyed upon the xwidget being killed, a
> prerequisite for being marked by GC.
                         ^^^^^^

That should've been "destroyed", sorry.

Re: [PATCH] Add user extension APIs for WebKit Xwidgets

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

While we're installing this, please remove the word "content"
from everything except the external interface symbol names where
its use is unavoidable.  Let's not get sucked into spreading the idea
that everything people publish or communicate is merely stuff to
fill a box.

See https://gnu.org/philosophy/words-to-avoid.html#Content.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: [PATCH] Add user extension APIs for WebKit Xwidgets

[Alan Mackenzie]

Hello, Richard.

On Sun, Oct 16, 2022 at 16:51:34 -0400, Richard Stallman wrote:
> [[[ To any NSA and FBI agents reading my email: please consider    ]]]
> [[[ whether defending the US Constitution against all enemies,     ]]]
> [[[ foreign or domestic, requires you to follow Snowden's example. ]]]

> While we're installing this, please remove the word "content"
> from everything except the external interface symbol names where
> its use is unavoidable.  Let's not get sucked into spreading the idea
> that everything people publish or communicate is merely stuff to
> fill a box.

Now is the autumn of our discontent.  :-)

> See https://gnu.org/philosophy/words-to-avoid.html#Content.

> -- 
> Dr Richard Stallman (https://stallman.org)
> Chief GNUisance of the GNU Project (https://gnu.org)
> Founder, Free Software Foundation (https://fsf.org)
> Internet Hall-of-Famer (https://internethalloffame.org)

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: [PATCH] Add user extension APIs for WebKit Xwidgets

[Eli Zaretskii]

> From: Richard Stallman <rms@gnu.org>
> Cc: luangruo@yahoo.com, larsi@gnus.org, emacs-devel@gnu.org
> Date: Sun, 16 Oct 2022 16:51:34 -0400
> 
> See https://gnu.org/philosophy/words-to-avoid.html#Content.

That section fails to suggest good replacements for this term,
AFAICT.  Which makes that part of the article less useful than it
could be, perhaps.

And, of course, we frequently say stuff like "file's contents" or
"buffer contents", which is perfectly OK, IMO.  This usage is not
mentioned in that article, so someone, especially non-native speakers,
could think such uses are part of the problem -- perhaps another minor
issue that should be mentioned there, but isn't.

Re: [PATCH] Add user extension APIs for WebKit Xwidgets

[Jean Louis]

* Eli Zaretskii <eliz@gnu.org> [2022-10-17 08:33]:
> > From: Richard Stallman <rms@gnu.org>
> > Cc: luangruo@yahoo.com, larsi@gnus.org, emacs-devel@gnu.org
> > Date: Sun, 16 Oct 2022 16:51:34 -0400
> > 
> > See https://gnu.org/philosophy/words-to-avoid.html#Content.
> 
> That section fails to suggest good replacements for this term,
> AFAICT.  Which makes that part of the article less useful than it
> could be, perhaps.
> 
> And, of course, we frequently say stuff like "file's contents" or
> "buffer contents", which is perfectly OK, IMO.  This usage is not
> mentioned in that article, so someone, especially non-native speakers,
> could think such uses are part of the problem -- perhaps another minor
> issue that should be mentioned there, but isn't.

The description on
https://gnu.org/philosophy/words-to-avoid.html#Content speaks of works
and publications, thus it does not relate to file contents or buffer
contents which is something else but work or publication. Just the
Emacs Info manual has many mentionings of "contents", in their correct
context -- which is correct, as those are smaller pieces of
information or data not referring to works or publications as whole.


English Wordnet dictionary:

* Overview of noun content

The noun content has 7 senses (first 6 from tagged texts)
1. (12) content -- (everything that is included in a collection and that is held or included in something; "he emptied the contents of his pockets"; "the two groups were similar in content")
2. (8) message, content, subject matter, substance -- (what a communication that is about something is about)
3. (3) content -- (the proportion of a substance that is contained in a mixture or alloy etc.)
4. (2) capacity, content -- (the amount that can be contained; "the gas tank has a capacity of 12 gallons")
5. (2) content, cognitive content, mental object -- (the sum or range of what has been perceived, discovered, or learned)
6. (1) contentedness, content -- (the state of being contented with your situation in life; "he relaxed in sleepy contentedness"; "they could read to their heart's content")
7. subject, content, depicted object -- (something (a person or object or scene) selected by an artist or photographer for graphic representation; "a moving picture of a train is more dramatic than a still picture of the same subject")



--
Jean

Take action in Free Software Foundation campaigns:
https://www.fsf.org/campaigns

In support of Richard M. Stallman
https://stallmansupport.org/

Re: [PATCH] Add user extension APIs for WebKit Xwidgets

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > Now is the autumn of our discontent.  :-)

You know I'm a malcontent provider ;-}.
That's the root of the Free Software Movement.
-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: [PATCH] Add user extension APIs for WebKit Xwidgets

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > The description on
  > https://gnu.org/philosophy/words-to-avoid.html#Content speaks of works
  > and publications, thus it does not relate to file contents or buffer
  > contents which is something else but work or publication.

That's correct.  Indeed, these are two different usages: one is
"content" and the other is "contents".  "Contents" is used in regard
to one specific work, and there is no problem with that usage.

"Content" is an abstraction use to generalkize and equate _all_
publications and communications, of whatever kind of medium.
The range of generalization is what makes it a problem.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: [PATCH] Add user extension APIs for WebKit Xwidgets

[Eli Zaretskii]

> From: Richard Stallman <rms@gnu.org>
> Cc: eliz@gnu.org, emacs-devel@gnu.org
> Date: Wed, 19 Oct 2022 13:04:21 -0400
> 
>   > The description on
>   > https://gnu.org/philosophy/words-to-avoid.html#Content speaks of works
>   > and publications, thus it does not relate to file contents or buffer
>   > contents which is something else but work or publication.
> 
> That's correct.  Indeed, these are two different usages: one is
> "content" and the other is "contents".  "Contents" is used in regard
> to one specific work, and there is no problem with that usage.
> 
> "Content" is an abstraction use to generalkize and equate _all_
> publications and communications, of whatever kind of medium.
> The range of generalization is what makes it a problem.

IME, non-native English speakers frequently don't distinguish between
"content" and "contents", and use the former when they mean the
latter.  That's why I made my comment, and that's why I also think the
article should take that into account, so it avoids one possible
confusion.

Re: [PATCH] Add user extension APIs for WebKit Xwidgets

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > That section fails to suggest good replacements for this term,
  > AFAICT.

It suggests "works" and "publications."

I will add "messages", because in some context that is the category
of things that people are referring to.

  > And, of course, we frequently say stuff like "file's contents" or
  > "buffer contents", which is perfectly OK, IMO.  This usage is not
  > mentioned in that article, so someone, especially non-native speakers,
  > could think such uses are part of the problem -- perhaps another minor
  > issue that should be mentioned there, but isn't.

I will add text about that in
https://gnu.org/philosophy/words-to-avoid.html.  Thanks.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: [PATCH] Add user extension APIs for WebKit Xwidgets

[Eli Zaretskii]

> From: Richard Stallman <rms@gnu.org>
> Cc: emacs-devel@gnu.org
> Date: Thu, 20 Oct 2022 15:46:30 -0400
> 
>   > That section fails to suggest good replacements for this term,
>   > AFAICT.
> 
> It suggests "works" and "publications."
> 
> I will add "messages", because in some context that is the category
> of things that people are referring to.

Thanks.  But I think this doesn't cover well the electronic media:
audio, video, and images.

Re: [PATCH] Add user extension APIs for WebKit Xwidgets

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks.  But I think this doesn't cover well the electronic media:
> audio, video, and images.

Here, "content" refers to the contents of the web view widget.  That is,
the contents of the relevant system and graphics memory.

I fail to see how the "content" in "content manager" has any kind of
semantic implication about when not to use the word "content".

Re: [PATCH] Add user content APIs for WebKit Xwidgets

[Qiantan Hong]

[-- Attachment #1.1: Type: text/plain, Size: 1543 bytes --]

Hi Po,

I tried to fix these problems, and I've attached my updated patch.

Some remaining questions/issues:

1. I think that connecting signal to scriptor (now called manager)
doesn't require xw to be marked, because it is also destroyed when xw
is killed? Suppose that now I reference count correctly, manager
should be owned by xw, just like the WebKitWebContext at line 363.
2. I didn't fix the ARC assumption in NS port. I hope it can be fixed
in one batch by Andrew. Thank you Andrew!

Best,
Qiantan


> On Oct 15, 2022, at 11:41 PM, Po Lu <luangruo@yahoo.com> wrote:
>
> Qiantan Hong <qthong@stanford.edu> writes:
>
>> Yes I’m aware. I was fixing them one by one.
>
> Thanks.
>
>> It seems that existing code is connecting signal without adding the marking
>> code, guess that should be fixed too...
>
> Where?  Existing code should not connect signals to anything other than
> `widget_osr' which is destroyed upon the xwidget being killed, a
> prerequisite for being marked by GC.
>
>> Those two problems above, I don’t have any existing code to imitate, so it
>> would take a while to learn the codebase for me to be able to fix them.
>> I’m also not sure if I should just make my part conforming or also fix all existing code.
>> Is it acceptable that I submit a patch (with other problems fixed) first,
>> and work on fixing these two classes of problems later?
>
> I'd rather not install code with known memory management issues,
> especially not this close to the Emacs 29 release.


[-- Attachment #1.2: Type: text/html, Size: 2196 bytes --]

[-- Attachment #2: 0001-Implement-WebKit-user-scripts-and-script-message-han.txt --]
[-- Type: text/plain, Size: 20864 bytes --]

From fb375e5e3582a5c9cfe3d11c1036fe9affe1b12f Mon Sep 17 00:00:00 2001
From: Qiantan Hong <qhong@alum.mit.edu>
Date: Sun, 23 Oct 2022 01:55:53 -0700
Subject: [PATCH] Implement WebKit user scripts and script message handlers.

* src/xwidget.h (store_xwidget_script_message_event): store script
message event into event queue
* src/xwidget.c (store_xwidget_script_message_event):
(Fxwidget_webkit_add_user_script):
(Fxwidget_webkit_remove_all_user_scripts): user script primitives
(Fxwidget_webkit_register_script_message):
(Fxwidget_webkit_unregister_script_message): script message handler
primitives.
* src/nsxwidget.c (nsxwidget_webkit_add_user_script):
(nsxwidget_webkit_remove_all_user_scripts):
(nsxwidget_webkit_register_script_message):
(nsxwidget_webkit_unregister_script_message): NS implementation of
user script and script message handler. Changed naming of a previous
used script message handler to avoid namespace pollution.
* src/nsxwidget.h (nsxwidget_webkit_add_user_script):
(nsxwidget_webkit_remove_all_user_scripts):
(nsxwidget_webkit_register_script_message):
(nsxwidget_webkit_unregister_script_message):
* lisp/xwidget.el (xwidget-webkit-callback):
(xwidget-webkit-push-script-message-handler):
(xwidget-webkit-pop-script-message-handler): let lisp recognize and
dispatch script message events
* doc/lispref/display.texi: document user script and script message
handler
---
 doc/lispref/display.texi |  52 +++++++++++
 lisp/xwidget.el          |  36 +++++++-
 src/nsxwidget.h          |   5 +
 src/nsxwidget.m          |  80 ++++++++++++++--
 src/xwidget.c            | 192 +++++++++++++++++++++++++++++++++++++++
 src/xwidget.h            |   4 +
 6 files changed, 361 insertions(+), 8 deletions(-)

diff --git a/doc/lispref/display.texi b/doc/lispref/display.texi
index 15cd5518d9..48b38ff837 100644
--- a/doc/lispref/display.texi
+++ b/doc/lispref/display.texi
@@ -7279,6 +7279,58 @@ Xwidgets
 @var{default} was omitted.
 @end defun
 
+@defun xwidget-webkit-add-user-script xwidget script when main-frame-only
+This function adds the specified user script @code{script} to the
+browser widget specified by @var{xwidget}.
+
+User scripts are custom JavaScript code that is automatically run in
+the pages of the browser widget.  The @var{when} argument specifies
+when @var{script} is run, it can be one of the following:
+
+@table @code
+@item start
+@var{script} is run upon first loading a document.
+@item end
+@var{script} is run after a document loads.
+@end table
+
+The @var{main-frame-only} argument specifies which HTML frames run
+@var{script}.  If @var{main-frame-only} is @code{nil}, @var{script} is
+run in all HTML frames.  Otherwise, @var{script} is only run in
+top-level HTML frames.
+@end defun
+
+@defun xwidget-webkit-remove-all-user-scripts xwidget
+This functions removes all user scripts from the browser widget
+specified by @var{xwidget}.
+@end defun
+
+@defun xwidget-webkit-push-script-message-handler xwidget name handler
+This function registers @var{handler} to handle script messages with
+namespace specified by string @var{name} for the browser widget
+specified by @var{xwidget}.
+
+Script message enables JavaScript code run in browser widgets to pass
+data to Emacs.  After @var{handler} is registered with @var{name},
+running the following JavaScript code
+
+@example
+window.webkit.messageHandlers.@var{name}.postMessage(@var{JSObject});
+@end example
+
+@noindent causes @var{handler} to be called with two arguments: @var{xwidget}
+and @var{JSObject} converted to a Lisp object.
+@end defun
+
+@defun xwidget-webkit-pop-script-message-handler
+This functions removes a handler registered with namespace specified
+by string @var{name} for the browser widget specified by
+@var{xwidget}.
+
+The returned value is the removed handler function, or @code{nil} if
+no such handler is found.
+@end defun
+
 @defun xwidget-webkit-get-title xwidget
 This function returns the title of @var{xwidget} as a string.
 @end defun
diff --git a/lisp/xwidget.el b/lisp/xwidget.el
index 41a1190c64..418a9f3acc 100644
--- a/lisp/xwidget.el
+++ b/lisp/xwidget.el
@@ -485,7 +485,41 @@ xwidget-webkit-callback
            (let ((proc (nth 3 last-input-event))
                  (arg  (nth 4 last-input-event)))
              (funcall proc arg)))
-          (t (xwidget-log "unhandled event:%s" xwidget-event-type)))))
+          ((eq xwidget-event-type 'script-message)
+           (let ((name (nth 3 last-input-event))
+                 (value (nth 4 last-input-event)))
+             (let ((handler-pair (assoc name (xwidget-get xwidget 'script-message-handlers))))
+               (if handler-pair
+                   (funcall (cdr handler-pair) xwidget value)
+                 (xwidget-log "unhandled script message: %s" name)))))
+          (t (xwidget-log "unhandled event: %s" xwidget-event-type)))))
+
+(defun xwidget-webkit-push-script-message-handler (xwidget name handler)
+  "Register HANDLER to handle script messages with NAME for WebKit XWIDGET.
+
+HANDLER will be called with two arguments: the WebKit XWIDGET and
+the JavaScript object passed from the script message converted to
+a Lisp object."
+  (let ((old-alist (xwidget-get xwidget 'script-message-handlers)))
+    (unless (assoc name old-alist)
+      (xwidget-webkit-register-script-message (xwidget-webkit-current-session) name))
+    (xwidget-put xwidget 'script-message-handlers
+                 (cons (cons name handler) old-alist)))
+  name)
+
+(defun xwidget-webkit-pop-script-message-handler (xwidget name)
+  "Remove a handler registered with NAME for WebKit XWIDGET.
+
+Return the removed handler function, or NIL if no such handler is
+found."
+  (let* ((old-alist (xwidget-get xwidget 'script-message-handlers))
+         (handler-pair (assoc name old-alist)))
+    (when handler-pair
+      (let ((new-alist (delq handler-pair old-alist)))
+        (xwidget-put xwidget 'script-message-handlers new-alist)
+        (unless (assoc name new-alist)
+          (xwidget-webkit-unregister-script-message (xwidget-webkit-current-session) name)))
+      (cdr handler-pair))))
 
 (defvar bookmark-make-record-function)
 (when (memq window-system '(mac ns))
diff --git a/src/nsxwidget.h b/src/nsxwidget.h
index 666509744a..cd1dc1bc2c 100644
--- a/src/nsxwidget.h
+++ b/src/nsxwidget.h
@@ -40,6 +40,11 @@ #define NSXWIDGET_H_INCLUDED
 void nsxwidget_webkit_execute_script (struct xwidget *xw, const char *script,
                                       Lisp_Object fun);
 
+void nsxwidget_webkit_add_user_script (struct xwidget *xw, const char *script,
+                                       int injection_time_start, int main_frame_only);
+void nsxwidget_webkit_remove_all_user_scripts (struct xwidget *xw);
+Lisp_Object nsxwidget_webkit_register_script_message (struct xwidget *xw, const char *name);
+void nsxwidget_webkit_unregister_script_message (struct xwidget *xw, const char *name);
 /* Functions for xwidget model.  */
 
 #ifdef __OBJC__
diff --git a/src/nsxwidget.m b/src/nsxwidget.m
index be0eba0bcb..901277f9a3 100644
--- a/src/nsxwidget.m
+++ b/src/nsxwidget.m
@@ -88,7 +88,7 @@ - (id)initWithFrame:(CGRect)frame
         @"Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6)"
         @" AppleWebKit/603.3.8 (KHTML, like Gecko)"
         @" Version/11.0.1 Safari/603.3.8";
-      [scriptor addScriptMessageHandler:self name:@"keyDown"];
+      [scriptor addScriptMessageHandler:self name:@"__xwidget_internal_keyDown"];
       [scriptor addUserScript:[[WKUserScript alloc]
                                 initWithSource:xwScript
                                  injectionTime:
@@ -275,23 +275,34 @@ + (void)initialize
       @"}"
       @"function xwKeyDown(event) {"
       @"  if (event.ctrlKey && event.key == 'g') {"
-      @"    window.webkit.messageHandlers.keyDown.postMessage('C-g');"
+      @"    window.webkit.messageHandlers.__xwidget_internal_keyDown.postMessage('C-g');"
       @"  }"
       @"}"
       @"document.addEventListener('keydown', xwKeyDown);"
       ;
 }
 
+static Lisp_Object js_to_lisp (id value);
+
 /* Confirming to WKScriptMessageHandler, listens concerning keyDown in
    webkit. Currently 'C-g'.  */
 - (void)userContentController:(WKUserContentController *)userContentController
       didReceiveScriptMessage:(WKScriptMessage *)message
 {
-  if ([message.body isEqualToString:@"C-g"])
+  if ([message.name isEqualToString:@"__xwidget_internal_keyDown"])
     {
-      /* Just give up focus, no relay "C-g" to emacs, another "C-g"
-         follows will be handled by emacs.  */
-      [self.window makeFirstResponder:self.xw->xv->emacswindow];
+      if ([message.body isEqualToString:@"C-g"])
+        {
+          /* Just give up focus, no relay "C-g" to emacs, another "C-g"
+             follows will be handled by emacs.  */
+          [self.window makeFirstResponder:self.xw->xv->emacswindow];
+        }
+    }
+  else
+    {
+      store_xwidget_script_message_event (self.xw,
+                                          message.name.UTF8String,
+                                          js_to_lisp (message.body));
     }
 }
 
@@ -437,6 +448,61 @@ - (void)userContentController:(WKUserContentController *)userContentController
     }];
 }
 
+void
+nsxwidget_webkit_add_user_script (struct xwidget *xw, const char *script,
+                                  int injection_time_start, int main_frame_only)
+{
+  XwWebView *xwWebView = (XwWebView *) xw->xwWidget;
+  WKUserContentController *scriptor = xwWebView.configuration.userContentController;
+
+  NSString *javascriptString = [NSString stringWithUTF8String:script];
+  WKUserScriptInjectionTime injectionTime = injection_time_start?
+    WKUserScriptInjectionTimeAtDocumentStart : WKUserScriptInjectionTimeAtDocumentEnd;
+  WKUserScript *userScript = [[WKUserScript alloc]
+                               initWithSource: javascriptString
+                                injectionTime: injectionTime
+                               forMainFrameOnly: main_frame_only];
+  [scriptor addUserScript: userScript];
+}
+
+void
+nsxwidget_webkit_remove_all_user_scripts (struct xwidget *xw)
+{
+  XwWebView *xwWebView = (XwWebView *) xw->xwWidget;
+  WKUserContentController *scriptor = xwWebView.configuration.userContentController;
+
+  [scriptor removeAllUserScripts];
+}
+
+Lisp_Object
+nsxwidget_webkit_register_script_message (struct xwidget *xw, const char *name)
+{
+  XwWebView *xwWebView = (XwWebView *) xw->xwWidget;
+  WKUserContentController *scriptor = xwWebView.configuration.userContentController;
+
+  NSString *messageName = [NSString stringWithUTF8String:name];
+
+  @try
+    {
+      [scriptor addScriptMessageHandler:xwWebView name:messageName];
+    }
+  @catch (NSException *e)
+    {
+      error ("Failed to register WebKit script message handler");
+    }
+  return Qnil;
+}
+
+void
+nsxwidget_webkit_unregister_script_message (struct xwidget *xw, const char *name)
+{
+  XwWebView *xwWebView = (XwWebView *) xw->xwWidget;
+  WKUserContentController *scriptor = xwWebView.configuration.userContentController;
+
+  NSString *messageName = [NSString stringWithUTF8String:name];
+  [scriptor removeScriptMessageHandlerForName:messageName];
+}
+
 /* Window containing an xwidget.  */
 
 @implementation XwWindow
@@ -469,7 +535,7 @@ - (BOOL)isFlipped { return YES; }
       WKUserContentController *scriptor =
         ((XwWebView *) xw->xwWidget).configuration.userContentController;
       [scriptor removeAllUserScripts];
-      [scriptor removeScriptMessageHandlerForName:@"keyDown"];
+      [scriptor removeScriptMessageHandlerForName:@"__xwidget_internal_keyDown"];
       [scriptor release];
       if (xw->xv)
         xw->xv->model = Qnil; /* Make sure related view stale.  */
diff --git a/src/xwidget.c b/src/xwidget.c
index 8bdfab02fd..3cdd95f970 100644
--- a/src/xwidget.c
+++ b/src/xwidget.c
@@ -126,6 +126,11 @@ webkit_decide_policy_cb (WebKitWebView *,
 };
 
 static void find_widget (GtkWidget *t, struct widget_search_data *);
+
+static void webkit_script_message_cb (WebKitUserContentManager *,
+                                          WebKitJavascriptResult *,
+                                          gpointer);
+
 #endif
 
 #ifdef HAVE_PGTK
@@ -380,6 +385,9 @@ DEFUN ("make-xwidget",
 	  settings = webkit_web_view_get_settings (WEBKIT_WEB_VIEW (xw->widget_osr));
 	  g_object_set (G_OBJECT (settings), "enable-developer-extras", TRUE, NULL);
 	}
+      WebKitUserContentManager *manager = webkit_user_content_manager_new ();
+      xw->widget_osr = webkit_web_view_new_with_user_content_manager (manager);
+      g_object_unref (manager);
 
       gtk_widget_set_size_request (GTK_WIDGET (xw->widget_osr), xw->width,
                                    xw->height);
@@ -2308,6 +2316,21 @@ store_xwidget_js_callback_event (struct xwidget *xw,
   kbd_buffer_store_event (&event);
 }
 
+void
+store_xwidget_script_message_event (struct xwidget *xw,
+				    const char *name,
+				    Lisp_Object body)
+{
+  struct input_event event;
+  Lisp_Object xwl;
+  XSETXWIDGET (xwl, xw);
+  EVENT_INIT (event);
+  event.kind = XWIDGET_EVENT;
+  event.frame_or_window = Qnil;
+  event.arg = list4 (Qscript_message, xwl, build_string (name), body);
+  kbd_buffer_store_event (&event);
+}
+
 
 #ifdef USE_GTK
 static void
@@ -2622,6 +2645,21 @@ webkit_decide_policy_cb (WebKitWebView *webView,
   }
 }
 
+static void
+webkit_script_message_cb (WebKitUserContentManager *manager,
+			  WebKitJavascriptResult *js_result,
+			  gpointer data)
+{
+  struct xwidget *xw = g_object_get_data (G_OBJECT (manager),
+					  XG_XWIDGET);
+  JSCValue *value = webkit_javascript_result_get_js_value (js_result);
+  const char *arg = data;
+  Lisp_Object lisp_value = webkit_js_to_lisp (value);
+  store_xwidget_script_message_event (xw, arg, lisp_value);
+
+  webkit_javascript_result_unref (js_result);
+}
+
 static gboolean
 webkit_script_dialog_cb (WebKitWebView *webview,
 			 WebKitScriptDialog *script_dialog,
@@ -3198,6 +3236,151 @@ DEFUN ("xwidget-webkit-execute-script",
   return Qnil;
 }
 
+DEFUN ("xwidget-webkit-add-user-script",
+       Fxwidget_webkit_add_user_script, Sxwidget_webkit_add_user_script,
+       4, 4, 0,
+       doc: /* Add the user script SCRIPT to the WebKit XWIDGET.
+
+WHEN is one of the following values, and specifies when the script is
+run:
+- `start', which means the script is run upon first loading a document.
+- `end', which means the script is run after a document loads.
+
+If MAIN_FRAME_ONLY is nil, SCRIPT is run in all HTML frames.
+Otherwise, SCRIPT is only run in top-level HTML frames.  */)
+  (Lisp_Object xwidget, Lisp_Object script,
+   Lisp_Object when, Lisp_Object main_frame_only)
+{
+  WEBKIT_FN_INIT ();
+  CHECK_STRING (script);
+  CHECK_SYMBOL (when);
+
+  script = ENCODE_UTF_8 (script);
+
+  Bool start;
+  if (EQ (when, Qstart))
+    start = true;
+  else if (EQ (when, Qend))
+    start = false;
+  else
+    error ("Unknown Xwidget Webkit user script injection time: %s",
+           SDATA (SYMBOL_NAME (when)));
+
+#ifdef USE_GTK
+  WebKitWebView *view = WEBKIT_WEB_VIEW (xw->widget_osr);
+  WebKitUserContentManager *manager = webkit_web_view_get_user_content_manager (view);
+
+  WebKitUserContentInjectedFrames webkit_injected_frames
+    = (!NILP (main_frame_only))
+    ? WEBKIT_USER_CONTENT_INJECT_TOP_FRAME
+    : WEBKIT_USER_CONTENT_INJECT_ALL_FRAMES;
+  WebKitUserScriptInjectionTime webkit_injection_time = start
+    ? WEBKIT_USER_SCRIPT_INJECT_AT_DOCUMENT_START
+    : WEBKIT_USER_SCRIPT_INJECT_AT_DOCUMENT_END;
+  WebKitUserScript *user_script
+    = webkit_user_script_new (SSDATA (script),
+			      webkit_injected_frames,
+			      webkit_injection_time,
+			      NULL, NULL);
+  webkit_user_content_manager_add_script (manager, user_script);
+  webkit_user_script_unref (user_script);
+#elif defined NS_IMPL_COCOA
+  nsxwidget_webkit_add_user_script (xw, SSDATA (script), injection_time_start, mfo);
+#endif
+  return Qnil;
+}
+
+DEFUN ("xwidget-webkit-remove-all-user-scripts",
+       Fxwidget_webkit_remove_all_user_scripts, Sxwidget_webkit_remove_all_user_scripts,
+       1, 1, 0,
+       doc: /* Remove all user scripts from WebKit XWIDGET.  */)
+  (Lisp_Object xwidget)
+{
+  WEBKIT_FN_INIT ();
+
+#ifdef USE_GTK
+  WebKitWebView *view = WEBKIT_WEB_VIEW (xw->widget_osr);
+  WebKitUserContentManager *manager = webkit_web_view_get_user_content_manager (view);
+
+  webkit_user_content_manager_remove_all_scripts (manager);
+#elif defined NS_IMPL_COCOA
+  nsxwidget_webkit_remove_all_user_scripts(xw);
+#endif
+  return Qnil;
+}
+
+DEFUN ("xwidget-webkit-register-script-message",
+       Fxwidget_webkit_register_script_message, Sxwidget_webkit_register_script_message,
+       2, 2, 0,
+       doc: /* Register script message with NAME in WebKit XWIDGET.
+
+This operation fails if NAME has already been registered for XWIDGET.  */)
+  (Lisp_Object xwidget, Lisp_Object name)
+{
+  WEBKIT_FN_INIT ();
+  CHECK_STRING (name);
+  char *sname = xlispstrdup (name);
+
+#ifdef USE_GTK
+  WebKitWebView *view = WEBKIT_WEB_VIEW (xw->widget_osr);
+  WebKitUserContentManager *manager = webkit_web_view_get_user_content_manager (view);
+
+  USE_SAFE_ALLOCA;
+  gchar *signal_name = SAFE_ALLOCA (strlen ("script-message-received::") + strlen (sname) + 1);
+  stpcpy (stpcpy (signal_name, "script-message-received::"), sname);
+  g_signal_connect_data (manager, signal_name, G_CALLBACK (webkit_script_message_cb),
+			 sname, (GClosureNotify) xfree, 0);
+  SAFE_FREE ();
+
+  if (webkit_user_content_manager_register_script_message_handler (manager, sname))
+    {
+      g_object_set_data (G_OBJECT (manager), XG_XWIDGET, xw);
+    }
+  else
+    {
+      g_signal_handlers_disconnect_matched (manager,
+					    G_SIGNAL_MATCH_DATA,
+					    0, 0, NULL, NULL, sname);
+      error ("Failed to register WebKit script message handler");
+    }
+#elif defined NS_IMPL_COCOA
+  nsxwidget_webkit_register_script_message (xw, sname);
+  xfree (sname);
+#endif
+  return Qnil;
+}
+
+DEFUN ("xwidget-webkit-unregister-script-message",
+       Fxwidget_webkit_unregister_script_message, Sxwidget_webkit_unregister_script_message,
+       2, 2, 0,
+       doc: /* Unregister script message with NAME in Webkit XWIDGET.  */)
+  (Lisp_Object xwidget, Lisp_Object name)
+{
+  WEBKIT_FN_INIT ();
+  CHECK_STRING (name);
+
+  USE_SAFE_ALLOCA;
+  char *sname;
+  SAFE_ALLOCA_STRING (sname, name);
+
+#ifdef USE_GTK
+  WebKitWebView *view = WEBKIT_WEB_VIEW (xw->widget_osr);
+  WebKitUserContentManager *manager
+    = webkit_web_view_get_user_content_manager (view);
+
+  webkit_user_content_manager_unregister_script_message_handler (manager, sname);
+  g_signal_handlers_disconnect_matched (manager,
+					G_SIGNAL_MATCH_FUNC | G_SIGNAL_MATCH_DETAIL,
+					0, g_quark_from_string (sname), 0,
+					G_CALLBACK (webkit_script_message_cb), 0);
+#elif defined NS_IMPL_COCOA
+  nsxwidget_webkit_unregister_script_message(xw, sname);
+#endif
+
+  SAFE_FREE();
+  return Qnil;
+}
+
 DEFUN ("xwidget-resize", Fxwidget_resize, Sxwidget_resize, 3, 3, 0,
        doc: /* Resize XWIDGET to NEW_WIDTH, NEW_HEIGHT.  */ )
   (Lisp_Object xwidget, Lisp_Object new_width, Lisp_Object new_height)
@@ -3919,6 +4102,15 @@ syms_of_xwidget (void)
   defsubr (&Sxwidget_webkit_execute_script);
   DEFSYM (Qwebkit, "webkit");
 
+  defsubr (&Sxwidget_webkit_add_user_script);
+  DEFSYM (Qstart, "start");
+  DEFSYM (Qend, "end");
+  defsubr (&Sxwidget_webkit_remove_all_user_scripts);
+
+  DEFSYM (Qscript_message, "script-message");
+  defsubr (&Sxwidget_webkit_register_script_message);
+  defsubr (&Sxwidget_webkit_unregister_script_message);
+
   defsubr (&Sxwidget_size_request);
   defsubr (&Sdelete_xwidget_view);
 
diff --git a/src/xwidget.h b/src/xwidget.h
index 502beb6765..e835f700bc 100644
--- a/src/xwidget.h
+++ b/src/xwidget.h
@@ -203,6 +203,10 @@ #define XG_XWIDGET_VIEW "emacs_xwidget_view"
                                       Lisp_Object proc,
                                       Lisp_Object argument);
 
+void store_xwidget_script_message_event (struct xwidget *xw,
+                                         const char *name,
+                                         Lisp_Object value);
+
 extern struct xwidget *xwidget_from_id (uint32_t id);
 
 #ifdef HAVE_X_WINDOWS
-- 
2.32.1 (Apple Git-133)

Re: [PATCH] Add user content APIs for WebKit Xwidgets

[Po Lu]

Qiantan Hong <qthong@stanford.edu> writes:

> 1. I think that connecting signal to scriptor (now called manager)
> doesn't require xw to be marked, because it is also destroyed when xw
> is killed? Suppose that now I reference count correctly, manager
> should be owned by xw, just like the WebKitWebContext at line 363.

No, because webkit_web_view_new_with_related_view is used to create
WebKitWebView objects when an existing live xwidget is specified as the
related argument to `make-xwidget', and those will hold further
references to the content manager beyond the lifetime of the
WebKitWebView holding the initial reference to said content manager.  In
fact, won't userscripts apply to every WebKitWebView with the same
content manager?

I think I would prefer documenting and implementing userscripts as
applying to all related xwidgets: you could link each related xwidget
onto a circular queue, designate one as the "head", and hold a reference
to the head, replacing it with a different xwidget on the queue each
time the head is killed, until there are no more xwidgets on that queue,
at which point it becomes safe to free the userscript data.

> 2. I didn't fix the ARC assumption in NS port. I hope it can be fixed
> in one batch by Andrew. Thank you Andrew!

Sure, but please fix the redundant braces here:

> +      if ([message.body isEqualToString:@"C-g"])
> +        {
> +          /* Just give up focus, no relay "C-g" to emacs, another "C-g"
> +             follows will be handled by emacs.  */
> +          [self.window makeFirstResponder:self.xw->xv->emacswindow];
> +        }
> +    }
> +  else
> +    {
> +      store_xwidget_script_message_event (self.xw,
> +                                          message.name.UTF8String,
> +                                          js_to_lisp (message.body));
>      }

And some more comments below:

> +User scripts are custom JavaScript code that is automatically run in

This should read "User scripts are custom JavaScript code automatically
run in designated WebKit xwidgets as pages are being loaded".

> +void nsxwidget_webkit_add_user_script (struct xwidget *xw, const char *script,
> +                                       int injection_time_start, int main_frame_only);

I have a feeling the types here are also not supposed to be "int".

> +  WKUserScriptInjectionTime injectionTime = injection_time_start?
> +    WKUserScriptInjectionTimeAtDocumentStart : WKUserScriptInjectionTimeAtDocumentEnd;

Coding style warning: this should probably be

  (injection_time_start
   ? WKUserScriptInjectionTimeAtDocumentStart
   : WKUserScriptInjectionTimeAtDocumentEnd)

> +  Bool start;

Why this X library type, and not `bool'?

> +  if (EQ (when, Qstart))
> +    start = true;
> +  else if (EQ (when, Qend))
> +    start = false;

If you are going to use Bool, then the boolean constants used should be
True and False, but since Bool is only available on X Windows, that is
likely not what you want.

> +  else
> +    error ("Unknown Xwidget Webkit user script injection time: %s",
> +           SDATA (SYMBOL_NAME (when)));

"Xwidget Webkit" is not the correct capitalization.  The error message
should probably also be reworded, as I think I explained earlier.  Also,
since there are only two possible values here, this should be a boolean.

> +    = (!NILP (main_frame_only))
> +    ? WEBKIT_USER_CONTENT_INJECT_TOP_FRAME
> +    : WEBKIT_USER_CONTENT_INJECT_ALL_FRAMES;

This should be:

  = (!NILP (main_frame_only)
     ? WEBKIT_USER_CONTENT_INJECT_TOP_FRAME
     : WEBKIT_USER_CONTENT_INJECT_ALL_FRAMES)

> +  WebKitUserScriptInjectionTime webkit_injection_time = start
> +    ? WEBKIT_USER_SCRIPT_INJECT_AT_DOCUMENT_START
> +    : WEBKIT_USER_SCRIPT_INJECT_AT_DOCUMENT_END;

Likewise.  (!NILP (start)
            ? WEBKIT_USER_SCRIPT_INJECT_AT_DOCUMENT_START
            : WEBKIT_USER_SCRIPT_INJECT_AT_DOCUMENT_END)

> +#elif defined NS_IMPL_COCOA
> +  nsxwidget_webkit_add_user_script (xw, SSDATA (script), injection_time_start, mfo);
> +#endif

80 columns alert; I thought I also explained why not to use variable
names such as "mfo".

> +#elif defined NS_IMPL_COCOA
> +  nsxwidget_webkit_remove_all_user_scripts(xw);
> +#endif

Please put a space between the "ts" and "(".

> +This operation fails if NAME has already been registered for XWIDGET.  */)

Fails?  How?  This should mention that an error is signalled under that
situation.

> +  stpcpy (stpcpy (signal_name, "script-message-received::"), sname);

Maybe lispstpcpy is what you want here.

> +  if (webkit_user_content_manager_register_script_message_handler (manager, sname))
> +    {
> +      g_object_set_data (G_OBJECT (manager), XG_XWIDGET, xw);
> +    }

The braces here are redundant.  Please remove them.

> +DEFUN ("xwidget-webkit-unregister-script-message",
> +       Fxwidget_webkit_unregister_script_message, Sxwidget_webkit_unregister_script_message,
> +       2, 2, 0,
> +       doc: /* Unregister script message with NAME in Webkit XWIDGET.  */)
> +  (Lisp_Object xwidget, Lisp_Object name)

"Unregister the script message named NAME from the given WebKit widget..."

The library we use is capitalized "WebKit", not "Webkit".

> +  nsxwidget_webkit_unregister_script_message(xw, sname);

As I said earlier, there should be a space here.

> +  SAFE_FREE();

And here.

> +  defsubr (&Sxwidget_webkit_add_user_script);
> +  DEFSYM (Qstart, "start");
> +  DEFSYM (Qend, "end");
> +  defsubr (&Sxwidget_webkit_remove_all_user_scripts);
> +
> +  DEFSYM (Qscript_message, "script-message");
> +  defsubr (&Sxwidget_webkit_register_script_message);
> +  defsubr (&Sxwidget_webkit_unregister_script_message);

Why not make it so the DEFSYMS are placed together, away from the
defsubrs?

Re: [PATCH] Add user extension APIs for WebKit Xwidgets

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > Thanks.  But I think this doesn't cover well the electronic media:
  > audio, video, and images.

The words "works" and "publications" include all kinds of media,
includinmg audio and video.  "Works" is a normal term for artists'
paintings and photos, and all sorts of music.

I added a few more suggestions that might fit certain circumstances.
I also explained that "contents" is a fine word to use because
it means something different.  I checked in the changes just now in
https://gnu.org/philosophy/words-to-avoid.html.

  > Here, "content" refers to the contents of the web view widget.  That is,
  > the contents of the relevant system and graphics memory.

This would be a place to use the word "contents".

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: [PATCH] Add user content APIs for WebKit Xwidgets

[Qiantan Hong]

>> 1. I think that connecting signal to scriptor (now called manager)
>> doesn't require xw to be marked, because it is also destroyed when xw
>> is killed? Suppose that now I reference count correctly, manager
>> should be owned by xw, just like the WebKitWebContext at line 363.
> 
> No, because webkit_web_view_new_with_related_view is used to create
> WebKitWebView objects when an existing live xwidget is specified as the
> related argument to `make-xwidget', and those will hold further
> references to the content manager beyond the lifetime of the
> WebKitWebView holding the initial reference to said content manager.  In
> fact, won't userscripts apply to every WebKitWebView with the same
> content manager?
> 
> I think I would prefer documenting and implementing userscripts as
> applying to all related xwidgets: you could link each related xwidget
> onto a circular queue, designate one as the "head", and hold a reference
> to the head, replacing it with a different xwidget on the queue each
> time the head is killed, until there are no more xwidgets on that queue,
> at which point it becomes safe to free the userscript data.

Applying to all related Xwidgets sound like a good idea. I think we should
formalize the "group of related Xwidgets" as some kind of object? Maybe
a record type implemented in Lisp, or a pseudo vector (which I don't know
much about how to implement). An alternative is to expose content manager
as a pseudovector, which will in fact have 1-to-1 correspondence with "related
group", but I like related group more because it feels like a nicer abstraction.

What do you think?

If that is implemented I believe we also don't need extra marking, as the
manager will hold a reference to the related group object, but the related
group object will be collected only all related xwdigets are already killed.

Best,
Qiantan

Re: [PATCH] Add user content APIs for WebKit Xwidgets

[Po Lu]

Qiantan Hong <qthong@stanford.edu> writes:

> Applying to all related Xwidgets sound like a good idea. I think we should
> formalize the "group of related Xwidgets" as some kind of object? Maybe
> a record type implemented in Lisp, or a pseudo vector (which I don't know
> much about how to implement). An alternative is to expose content manager
> as a pseudovector, which will in fact have 1-to-1 correspondence with "related
> group", but I like related group more because it feels like a nicer abstraction.

I don't think that is necessary.  There should at most be a function
that returns a list of all related xwidgets.

Pseudovector types are a limited resource.  There can only be 64 at any
given time, so it would be a good idea not to waste them on such
trivialities.

Re: [PATCH] Add user content APIs for WebKit Xwidgets

[Qiantan Hong]

Currently, I store the ALIST of handlers on xwidget-plist.  If we're
going to have it for groups of related Xwidgets, what is the best
place to store it? On the xwidget-plist of the head xwidget? Then we
have to inspect the xwidget-plist in the C function kill_xwidget and
migrate a specific subset of properties...

Or we should add another slot and provide xwidget-group-plist and
set-xwidget-group-plist, which will only have non-NULL value on the
head xwidget and is migrated automatically?

Best,
Qiantan



> On Oct 23, 2022, at 5:30 PM, Po Lu <luangruo@yahoo.com> wrote:
> 
> Qiantan Hong <qthong@stanford.edu> writes:
> 
>> Applying to all related Xwidgets sound like a good idea. I think we should
>> formalize the "group of related Xwidgets" as some kind of object? Maybe
>> a record type implemented in Lisp, or a pseudo vector (which I don't know
>> much about how to implement). An alternative is to expose content manager
>> as a pseudovector, which will in fact have 1-to-1 correspondence with "related
>> group", but I like related group more because it feels like a nicer abstraction.
> 
> I don't think that is necessary.  There should at most be a function
> that returns a list of all related xwidgets.
> 
> Pseudovector types are a limited resource.  There can only be 64 at any
> given time, so it would be a good idea not to waste them on such
> trivialities.

Re: [PATCH] Add user content APIs for WebKit Xwidgets

[Po Lu]

Qiantan Hong <qthong@stanford.edu> writes:

> Currently, I store the ALIST of handlers on xwidget-plist.  If we're
> going to have it for groups of related Xwidgets, what is the best
> place to store it? On the xwidget-plist of the head xwidget?

In a C data structure (not the plist!) attached to the "head" xwidget.
It does not have to be accessible to Lisp.

> Or we should add another slot and provide xwidget-group-plist and
> set-xwidget-group-plist, which will only have non-NULL value on the
> head xwidget and is migrated automatically?

What slot?

I don't think it has to be exposed to Lisp at all.  It's just an
implementation detail, after all.

Re: [PATCH] Add user content APIs for WebKit Xwidgets

[Qiantan Hong]

>> Currently, I store the ALIST of handlers on xwidget-plist.  If we're
>> going to have it for groups of related Xwidgets, what is the best
>> place to store it? On the xwidget-plist of the head xwidget?
> 
> In a C data structure (not the plist!) attached to the "head" xwidget.
> It does not have to be accessible to Lisp.
> 
>> Or we should add another slot and provide xwidget-group-plist and
>> set-xwidget-group-plist, which will only have non-NULL value on the
>> head xwidget and is migrated automatically?
> 
> What slot?
> 
> I don't think it has to be exposed to Lisp at all.  It's just an
> implementation detail, after all.

Sure. In that case, would it be appropriate for the script_message_cb
to call the handler directly? Or should it emit an event and let
xwidget-webkit-callback call it on Lisp side? I'm not sure how well
does the former approach (which is more direct) interact with error
handling/Lisp debugger.

Re: [PATCH] Add user content APIs for WebKit Xwidgets

[Po Lu]

Qiantan Hong <qthong@stanford.edu> writes:

> Sure. In that case, would it be appropriate for the script_message_cb
> to call the handler directly?

No, it is not safe to call Lisp there.

> Or should it emit an event and let xwidget-webkit-callback call it on
> Lisp side?

Yes.

> I'm not sure how well does the former approach (which is more direct)
> interact with error handling/Lisp debugger.

Badly, unless you use safe_call, but that's not the place for safe_call.