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

[Qiantan Hong <qthong@stanford.edu>]

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.
>