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?