From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: [Emacs-diffs] xwidget updated (1d8b8a2 -> 5f46725) Date: Sun, 28 Dec 2014 18:08:31 +0200 Message-ID: <831tnjlpts.fsf@gnu.org> References: <20141226164113.11620.38682@vcs.savannah.gnu.org> Reply-To: Eli Zaretskii NNTP-Posting-Host: plane.gmane.org X-Trace: ger.gmane.org 1419782952 3372 80.91.229.3 (28 Dec 2014 16:09:12 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sun, 28 Dec 2014 16:09:12 +0000 (UTC) Cc: monnier@iro.umontreal.ca, emacs-devel@gnu.org To: joakim@verona.se Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sun Dec 28 17:09:05 2014 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1Y5GOu-00073H-16 for ged-emacs-devel@m.gmane.org; Sun, 28 Dec 2014 17:09:04 +0100 Original-Received: from localhost ([::1]:58762 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y5GOt-0005k1-9T for ged-emacs-devel@m.gmane.org; Sun, 28 Dec 2014 11:09:03 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:57534) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y5GOk-0005hJ-Pv for emacs-devel@gnu.org; Sun, 28 Dec 2014 11:09:00 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y5GOf-0003YR-L7 for emacs-devel@gnu.org; Sun, 28 Dec 2014 11:08:54 -0500 Original-Received: from mtaout20.012.net.il ([80.179.55.166]:50498) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y5GOf-0003YH-8b for emacs-devel@gnu.org; Sun, 28 Dec 2014 11:08:49 -0500 Original-Received: from conversion-daemon.a-mtaout20.012.net.il by a-mtaout20.012.net.il (HyperSendmail v2007.08) id <0NHA00100VCSL500@a-mtaout20.012.net.il> for emacs-devel@gnu.org; Sun, 28 Dec 2014 18:08:47 +0200 (IST) Original-Received: from HOME-C4E4A596F7 ([87.69.4.28]) by a-mtaout20.012.net.il (HyperSendmail v2007.08) with ESMTPA id <0NHA001CPVINJ120@a-mtaout20.012.net.il>; Sun, 28 Dec 2014 18:08:47 +0200 (IST) In-reply-to: X-012-Sender: halo1@inter.net.il X-detected-operating-system: by eggs.gnu.org: Solaris 10 X-Received-From: 80.179.55.166 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 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.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:180756 Archived-At: > From: joakim@verona.se > Date: Sat, 27 Dec 2014 16:48:44 +0100 > Cc: emacs-devel@gnu.org > > Stefan Monnier writes: > > > Hi Joakim, > > > > BTW, what's the status of this branch (w.r.t to its mergeability into master)? > > Short answer: I'm trying to figure that out now. > > Longer answer: > > I think its pretty okay. Theres a problem with automatic > resizing of webkit that sometimes get so big emacs crashe I would like > to fix though. > > I'm not sure how to do the actual merge. I think cherry picking or > something would be better than a plain merge. > > Also, since I'm pretty blind to the flaws the code has by now, it would > be nice with some maintainer criticism. Thank you for your work. Please find a few comments and questions below, all based solely on reading the source. 1) In dispextern.h:'struct it' you made this addition to the iterator structure: @@ -500,6 +504,9 @@ struct glyph /* Image ID for image glyphs (type == IMAGE_GLYPH). */ int img_id; +#ifdef HAVE_XWIDGETS + struct xwidget* xwidget; +#endif /* Sub-structure for type == STRETCH_GLYPH. */ struct { This might be a problem, because several places in the display engine make local copies of the 'struct it' object, which will duplicate the pointer you added, so you will have 2 or more pointers to the same object. If one of the copies of the pointer is used to modify the 'struct xwidget' object, or free it, the other copies will be affected, which the code doesn't expect. Note that images, for example, store only their numerical ID in the iterator structure, not a direct pointer to an image. Also, you added a similar pointer to the iterator stack entry: @@ -2379,6 +2396,13 @@ struct it struct { Lisp_Object object; } stretch; +#ifdef HAVE_XWIDGETS + /* method == GET_FROM_XWIDGET */ + struct { + Lisp_Object object; + struct xwidget* xwidget; + } xwidget; +#endif } u; But that pointer seems to be unused, so I guess it should be deleted. 2) In dispnew.c:update_window you added a call to xwidget_end_redisplay. I think this call should be made before we call update_window_end_hook, because when that call is made, the redisplay interface implementation assumes the window is already up to date, whereas xwidget_end_redisplay still manipulates portions of the display (AFAIU). 3) A few places (for example, xdisp.c:handle_single_display_spec) process xwidget display elements even on non-GUI frames -- does that mean xwidget.c will be compiled even in --without-x configurations of Emacs? If not, you need to condition that code on HAVE_WINDOW_SYSTEM, like we do with images, for example. 4) xdisp.c:produce_xwidget_glyph seems to need some cleanup: it's basically a copy of produce_image_glyph, and at least some of the code there is not needed with xwidgets, I think. OTOH, if indeed xwidgets are very similar to images, perhaps we should have only one method that handles both. 5) xdisp.c:produce_xwidget_glyph needs to account for bidirectional display in the same way produce_image_glyph does: swao the left and right box edges, and populate the bidi members of struct glyph. 6) Did you test what happens with xwidgets when the lines are truncated, and only part of the xwidget fits on the line? Are the results reasonable? I see that produce_xwidget_glyph does attempt to crop the xwidget to fit in the line, but then display_line should handle xwidget glyphs the same as it does with image glyphs, when it decides how to go about truncation/continuation. 7) xwidget.c:make-xwidget seems to support xwidgets only in a buffer. What about strings? If strings aren't going to be supported, then the 'object' member of the iterator stack entry for xwidgets is not needed. 8) Do we really need to expose xwgir-require-namespace? Can't something like that be done automatically under the hood? 9) xwgir-xwidget-call-method needs the method as a string. Wouldn't it be better to use a symbol here? Strings are more expensive to compare, e.g. if some code needs to manage methods. 10) Several places in xwidget.c use Lisp string data without first verifying it's a string. Examples include xwidget-webkit-execute-script and xwidget-webkit-goto-uri. 11) The doc strings of functions exposed to Lisp that are defined on xwidget.c are not yet finished. 12) A question about configuration: are xwidgets only supported in a GTK3 compiled Emacs, or also in other builds? 13) A minor stylistic nit: the code style is somewhat different from the GNU Coding Standard: no space between the function name and the left parentheses that follows, opening brace of a block at the end of a line rather than on the next line, comments that don't end with a period, etc. 14) Finally, there are a lot of places in the code with FIXME's, TODO's, fragments that are commented out, debugging printf's, and other left-overs that I suggest to clean up before the merge. Thanks again for working on this.