[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Add user content APIs for WebKit Xwidgets
From: |
Qiantan Hong |
Subject: |
Re: [PATCH] Add user content APIs for WebKit Xwidgets |
Date: |
Sat, 15 Oct 2022 18:29:35 +0000 |
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, 2022/10/14
- Re: [PATCH] Add user content APIs for WebKit Xwidgets, Po Lu, 2022/10/14
- Re: [PATCH] Add user content APIs for WebKit Xwidgets, Qiantan Hong, 2022/10/14
- Re: [PATCH] Add user content APIs for WebKit Xwidgets, Po Lu, 2022/10/14
- Re: [PATCH] Add user content APIs for WebKit Xwidgets, Qiantan Hong, 2022/10/15
- Re: [PATCH] Add user content APIs for WebKit Xwidgets, Qiantan Hong, 2022/10/15
- Re: [PATCH] Add user content APIs for WebKit Xwidgets, Qiantan Hong, 2022/10/15
- Re: [PATCH] Add user content APIs for WebKit Xwidgets, Po Lu, 2022/10/15
- Re: [PATCH] Add user content APIs for WebKit Xwidgets,
Qiantan Hong <=
- Re: [PATCH] Add user content APIs for WebKit Xwidgets, Po Lu, 2022/10/15
- Re: [PATCH] Add user content APIs for WebKit Xwidgets, Qiantan Hong, 2022/10/16
- Re: [PATCH] Add user content APIs for WebKit Xwidgets, Po Lu, 2022/10/16
- Re: [PATCH] Add user content APIs for WebKit Xwidgets, Qiantan Hong, 2022/10/16
- Re: [PATCH] Add user content APIs for WebKit Xwidgets, Po Lu, 2022/10/16
- Re: [PATCH] Add user content APIs for WebKit Xwidgets, Po Lu, 2022/10/16
- Re: [PATCH] Add user content APIs for WebKit Xwidgets, Qiantan Hong, 2022/10/23
- Re: [PATCH] Add user content APIs for WebKit Xwidgets, Po Lu, 2022/10/23
- Re: [PATCH] Add user content APIs for WebKit Xwidgets, Qiantan Hong, 2022/10/23
- Re: [PATCH] Add user content APIs for WebKit Xwidgets, Po Lu, 2022/10/23