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.