Re: [Emacs-diffs] xwidget updated (1d8b8a2 -> 5f46725)

[Eli Zaretskii]

> From: address@hidden
> Date: Sat, 27 Dec 2014 16:48:44 +0100
> Cc: address@hidden
> 
> Stefan Monnier <address@hidden> 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.