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: Sun, 23 Oct 2022 18:58:10 +0800 Message-ID: <874jvu6c3h.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> <87ilklnxei.fsf@yahoo.com> <0583C9C0-9953-414E-9F51-2AEFDF225BD9@stanford.edu> <87edv8mlr5.fsf@yahoo.com> <264D17C0-1FD6-44BA-B8BE-28249374F7A2@stanford.edu> <877d10mfsw.fsf@yahoo.com> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="3688"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: Andrew De Angelis , "emacs-devel@gnu.org" To: Qiantan Hong Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Mon Oct 24 04:14:37 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 1ommz7-0000kH-0J for ged-emacs-devel@m.gmane-mx.org; Mon, 24 Oct 2022 04:14:37 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1ommCN-0005CT-CB for ged-emacs-devel@m.gmane-mx.org; Sun, 23 Oct 2022 21:24:15 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1omYgV-0007tG-KP for emacs-devel@gnu.org; Sun, 23 Oct 2022 06:58:30 -0400 Original-Received: from sonic305-20.consmr.mail.ne1.yahoo.com ([66.163.185.146]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1omYgS-0003PF-S5 for emacs-devel@gnu.org; Sun, 23 Oct 2022 06:58:27 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com; s=s2048; t=1666522699; bh=xzpLd+N2jYUzxnhKwHqpN0sa5hdilMzhAnkU+m96lq4=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From:Subject:Reply-To; b=mBe6pqfrKYRx9VmWu1rhPLQfo+pL4pl3Ndq/9CwusWZl7DanbTZpLsIgEtjiiPO9hSV2ujtmuyeSadvtwGSRSHqij+X7DKyqqsTpiqtC+rmoGKJAv5tx7F934AKXBJKybNmHKimjkfjV8XDOejjVLfMVsrulfwwq8FxBLwtV5uRh88ehYDbY8edl1mWnGqy7EyeSkJjYyqXXP3ryoIe0tnptBk8e7W85WZ2rrOj17DWzieWh0oiYtzgNSzmKmAi5UD1jXI8+ANKQoUFW7juYbNYVDf0tMraGYccbs0GzPUoHQ/IAkqs5+wVO3lxL5eJlqqc56ehRiL+PnfZQewGesg== X-SONIC-DKIM-SIGN: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com; s=s2048; t=1666522699; bh=3O4ii8ZSUcikDO8WSm6sOKRTLnFLImdKQevEi3qAQw4=; h=X-Sonic-MF:From:To:Subject:Date:From:Subject; b=BjPT5zbo3x3398Bu9y6JGFD+WuVsmsz3Lg1IeKqxa9lkb9xh27QPFkk2u1sbmt84W9RG2/FHzp7EqmlUNBN3dpDMyxoUBxtq7uRSIWioCvpT6IfvNNndBzJSec6zq/9onz0+8bc6lt/PehwqyqEZAzH2T8uNTs6YwWTpZ8JUER7CUM6AuQtnG4eRlTVAgt4C37g/wbYUEaY1wCFuaOooqYTg3b2BILH7ND0xIkqMdOx37/RF3glxrs9XKyB3hT9099KLTYJZqi5yxOVdSCEG2U4eeuO5Sz1AyfvrfJoavF4/SZRWwhwXq9Qu/TCAi+8isDWTaJMWGiHFyCKtH6rHxw== X-YMail-OSG: j1SslxwVM1nO0h5n4uE3mk9yDP7vkGIcrg69AkyO1652ILxsGqAr9OF6yiBZtpJ D8Azzw_FU7.fqTo9fGaa8VMPKoCtGk9fsRNNYC5o.6jgRBGwUfGeJPJXRYjbw210BF1709kfa1hF HjcdvPPjNVphjYGfF5PiW.hDDArDDj5QaV7UlygB.FezzvU9Xqv4oAFDZyzhySwStvBzqgG89ZZ9 jC_LNiM.eD44ybfphLQ4cYWmRItKT0NZYGmg8nFbcRYj4y7_6BBDMX30E06l9sbkBShut30VBawg g38jKnJmDtvU1_3ck3oMXkDQd952SEZybXGmzOpVSO8egwrl5_cEutQH4U3QDbrbUaXuuf54rDXY AoQTzUhdlv3fY28OSJ4o85tw_nni3ACZUYrlHo_H6yCwfRJrRnEQzy3.VQKcz5GzquvjRlt0kWK_ LYqz6ocMBnXy9C0U8qpOX5bbl9OEjppoU9c9qLVcPMWJQtU4tTs.wJ74bBaukovgplqwe1_dyAfU mQfMjBb8LvMcFVAynkh1UyNs4T458r6fgp4LhNMd5BPBMbznITRh7pdvRddAw3DsNhS1LGk8qbGY srmGCZrBCG33lSJQnF7lM__AZvEsR3vJSBGNLC3X1Q.ldpCs7mhl1A.q74JEeVjguMSHbawEHFcy QOmjoo_t.kKhTcSk6u8Izv1XMSRNQY6emAq0WBKyjcI40xUKf2A1tfVj0gNxNxD3CfG8rDcCEV0H .Qn3SWEI.RlYrH9jYjN3bf.xSQnev2aWumKN.C.veU9iGQYA4hX24cufoLy9xTW4Zoqdbdxs4wAh lTF91xmiM2ICRiyeXWm2VmWRcsQ26DQYxy8oukUG_0 X-Sonic-MF: Original-Received: from sonic.gate.mail.ne1.yahoo.com by sonic305.consmr.mail.ne1.yahoo.com with HTTP; Sun, 23 Oct 2022 10:58:19 +0000 Original-Received: by hermes--production-sg3-74fb94585-582mk (Yahoo Inc. Hermes SMTP Server) with ESMTPA ID 8e98edd40b8f756461582ef3496cc23e; Sun, 23 Oct 2022 10:58:16 +0000 (UTC) In-Reply-To: (Qiantan Hong's message of "Sun, 23 Oct 2022 09:11:39 +0000") X-Mailer: WebService/1.1.20754 mail.backend.jedi.jws.acl:role.jedi.acl.token.atz.jws.hermes.yahoo Received-SPF: pass client-ip=66.163.185.146; envelope-from=luangruo@yahoo.com; helo=sonic305-20.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, RCVD_IN_MSPIKE_H2=-0.001, 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:298286 Archived-At: Qiantan Hong 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?