From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Po Lu Newsgroups: gmane.emacs.devel Subject: Re: [PATCH] Add user content APIs for WebKit Xwidgets Date: Sat, 15 Oct 2022 19:23:17 +0800 Message-ID: <87ilklnxei.fsf@yahoo.com> References: <763B89A7-AF82-4AAB-A0E9-A04D9958CAE8@stanford.edu> <878rlianx4.fsf@yahoo.com> <9FF9464C-1369-423E-A581-A900D132845F@stanford.edu> <87zgdy97t1.fsf@yahoo.com> <63F00459-018C-4634-9B52-A89A3ED1AA36@stanford.edu> <0F1442C8-45E2-408C-B310-448B4A26496E@stanford.edu> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="3012"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: Lars Ingebrigtsen , "emacs-devel@gnu.org" To: Qiantan Hong Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Sat Oct 15 13:25:14 2022 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ojfI1-0000bY-UW for ged-emacs-devel@m.gmane-mx.org; Sat, 15 Oct 2022 13:25:14 +0200 Original-Received: from localhost ([::1]:52030 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1ojfI0-0002zh-Ar for ged-emacs-devel@m.gmane-mx.org; Sat, 15 Oct 2022 07:25:12 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:60930) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1ojfGS-0002He-3h for emacs-devel@gnu.org; Sat, 15 Oct 2022 07:23:37 -0400 Original-Received: from sonic304-21.consmr.mail.ne1.yahoo.com ([66.163.191.147]:34232) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1ojfGP-00088r-4m for emacs-devel@gnu.org; Sat, 15 Oct 2022 07:23:35 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com; s=s2048; t=1665833010; bh=M0lpYv/7gwoIlAH2WKNeJ8Sb/Rd6/dbFjx2KrxaXDfQ=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From:Subject:Reply-To; b=KeGw5aGk60OcgVQA1jBaS0Y5eTh/oJkFDHzeH9hFoiqcBv4K9HWEI8OQ0ndXHScyou18mqwHndrMmSqwd1rLQHuXMF+iyfEWXBhvZQ57U/QOsyQbEocWMELejMiZ/LjPrR2QmzZVL+VVLRJapf4KcN3CBuBesgfn09fkOJtXHwhcqRUtvY6niJVWSuQZZeZbxecUXdRXWRnY7xpnfXckXV6g62NKbUjXOdpdEihSmbS2eCNPFJszG6KTe/R86QH7Ew4zz+USMmMNEQVkxqKuggMHgEk7/5DuwCsvUDSHwaByEb/NtykvcA152Q+2NO5aHJM7esNAtXckC5d6WxdKww== X-SONIC-DKIM-SIGN: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com; s=s2048; t=1665833010; bh=OFaDRhqXnKnnxboW/dUHghbgT+IoBYkhqZVRTGKPHmh=; h=X-Sonic-MF:From:To:Subject:Date:From:Subject; b=oabbYtin/QQ1bTSYoRDseg0oTUgGcRi56m648Lhij3zGC3ERk0Uua5gitTrJx0QdPiDK+4a755z2Olc7b34awODirqUfy6AmM52vLr3/tbIJZDcCfnBI0pkX20LDzD1Nll9oYQ+X69gAzLtgNfkAh5H3s3wAp714TLeH7/2LElzUwjt8vnxTuKBrKOTGCiw/sOukeSRMOwUDQHWnH2Db7WPmm8pOAv+pEHgY7YYjjy0o9oLb4MAoQCjig4nAN5DWWQgIlT5njfdnE5rZenyXPKWa4HjnMhFjNAJAXKR24eT3ZCMfSvazKXP6fz38brzMRUBQj8qSMqjFB2nr5b5ccw== X-YMail-OSG: 7mzEkcoVM1moTFoNRCHL7mJmg9My.lHBdbElgkUDZbjMBFVlReZSyibSr4s1xth oV5AncCBM23PBoULg5w5aGwzUWkoLV5NepU.3WhhDbpuGNHezIO1GhnOseIMqi1Dg7.vu.cZmKf9 gTy_xDDWhylp7r9ibmyPyiQ6ZI6Dep2Bb0HcleeJ5wbuYDxw18_9ZnFsQNyFkZL22B3Rc719YrCA 0yVi7S3ASSFfKcFHPsMGidUi6u9z3uwbnuvSgtYFGsKQod4Cr_rsSwIp7rMzwefRcTcql1Y5KSnM asuObdFLwCv4ZLGnIHxvnMnS6_N3gQX7M72z8.yc_6qpeo9lF8FrxRdlYiEY3gFcpOWQjjyrFPAF PEPAGsoLQzPMHVPSSCcA_qM0xP9yQIZiBFgmg49UaW26szEGNLnEkdTAOvZvJnVNz8VdyeaGoBYB 9s2POkNKvhGY7EthhXDM12JpEDd8FbCtMScH2.0dkxEvpQTEE_dD4nQv4XVuhQfrXA0ScP8aU1gH Z3Be7m5npVqZocc3S2JMjTrLxVfm1bm2VhL4Z4ROirICOL11ILOfoU758uwdU.1NKx10BNvIMHQl bMQfdQ_TO5_wPqNjPGUoBNghd85QYh9ExDvGkw8xpALRt3u03kA64ab.lkJKXD989lJQ.x1wkG47 Q3bhwWNTbDf5B2ecw3.wn6pfc7C_qm65TlSOe7Csi.W79FmRgpmtJaKZpPxdc57xl2Xb5n5tF6bn HzpZxZluHsnNBC2IjU6XeiCKv55uqVj0dC0FMnuucC8Rw_3xD0R1HuGiNh6daV7KSQSVIMp8rJin KP2edKgHfF3llgm6DMmvYYaw.wFIs.uHhmQQHLX9jA X-Sonic-MF: Original-Received: from sonic.gate.mail.ne1.yahoo.com by sonic304.consmr.mail.ne1.yahoo.com with HTTP; Sat, 15 Oct 2022 11:23:30 +0000 Original-Received: by hermes--production-sg3-785466d859-7l8f2 (Yahoo Inc. Hermes SMTP Server) with ESMTPA ID add1f38e632fe0e199ae5af8508d642a; Sat, 15 Oct 2022 11:23:23 +0000 (UTC) In-Reply-To: <0F1442C8-45E2-408C-B310-448B4A26496E@stanford.edu> (Qiantan Hong's message of "Sat, 15 Oct 2022 07:53:21 +0000") X-Mailer: WebService/1.1.20740 mail.backend.jedi.jws.acl:role.jedi.acl.token.atz.jws.hermes.yahoo Received-SPF: pass client-ip=66.163.191.147; envelope-from=luangruo@yahoo.com; helo=sonic304-21.consmr.mail.ne1.yahoo.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.io gmane.emacs.devel:297773 Archived-At: Qiantan Hong 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 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.