bug#51716: 29.0.50; [PATCH] Expose xwidget navigation history to Lisp code

[Eli Zaretskii <eliz@gnu.org>]

> Date: Tue, 09 Nov 2021 20:15:06 +0800
> From:  Po Lu via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> +@defun xwidget-webkit-goto-history xwidget rel-pos
> +Make @var{xwidget}, a WebKit widget, load the @code{rel-pos}th element
                                                 ^^^^^
This should be @var, not @code.

> +in its navigation history.
> +
> +If @var{rel-pos} is zero, the current page will be reloaded instead.
> +@end defun
> +
> +@defun xwidget-webkit-back-forward-list xwidget &optional limit

Hmm... given that we have xwidget-webkit-goto-history, why do we need
this new function as well?  Or why do we need both?

> +The returned value is a list of the form @code{(@var{back} @var{here}
> +@var{forward})}

This list should be wrapped in @w{..}, so that it is kept unbroken
between 2 lines.

> where @var{here} is the current navigation item,
> +while @var{back} is a list of items containing the history behind the
> +current navigation item, and @var{forward} is a list of items in front
> +of the current navigation item.

The notions of "behind" and "in front" are not well-defined in this
context.  The text should make more clear what does each one of those
mean.

>                                  @var{back}, @var{here} and
> +@var{forward} can all be @code{nil}.

What is the meaning of each one being nil?  The text leaves that
unsaid.

> +Navigation items are themselves lists of the form @code{(@var{idx}
> +@var{title} @var{uri})}.

Please use @w{..} here as well.  basically you should use it for any
form that has embedded whitespace, and can therefore be split between
two lines when Texinfo fills and justifies the text.

>                           In these lists, @var{idx} is an index that
> +can be passed to @code{xwidget-webkit-goto-history}, @var{title} is
> +the human-readable title of the item, and @var{uri} is the URI of the
> +item.

URI, not URL?

> +DEFUN ("xwidget-webkit-back-forward-list", Fxwidget_webkit_back_forward_list,
> +       Sxwidget_webkit_back_forward_list, 1, 2, 0,
> +       doc: /* Return the navigation history of XWIDGET, a WebKit xwidget.
> +
> +The history is returned as a list of the form (BACK HERE FORWARD),
   ^^^^^^^^^^^^^^^^^^^^^^^
Passive tense alert!

> +where HERE is the current navigation item, while BACK and FORWARD are
> +lists of navigation items, which are lists of the form (IDX TITLE

"lists of ... , which are lists" uses "lists" twice, which is
redundant.  Better say

   ... are lists of elements of the form (IDX TITLE URI) ...

> +               The user should normally have no reason to load URI
> +manually to reach a specific history item.  Instead, he should pass
> +IDX as an index to `xwidget-webkit-goto-history'.

This belongs to the manual, not a doc string.  (And try to rephrase to
avoid using "he".)

> +BACK and FORWARD will not contain more elements than LIMIT.

Each one or both together? the text is ambiguous about that.

> +#ifdef USE_GTK
> +  webview = WEBKIT_WEB_VIEW (xw->widget_osr);

I guess this again means this function is a no-op without GTK?  Then
let's not define it except in the GTK build.

> +  list = webkit_web_view_get_back_forward_list (webview);
> +  item = webkit_back_forward_list_get_current_item (list);
> +  lim = XFIXNAT (limit);
> +
> +  if (item)
> +    {
> +      item_title = webkit_back_forward_list_item_get_title (item);
> +      item_uri = webkit_back_forward_list_item_get_uri (item);
> +      here = list3 (make_fixnum (0),
> +		    build_string (item_title ? item_title : ""),
> +		    build_string (item_uri ? item_uri : ""));
> +    }
> +  parent = webkit_back_forward_list_get_back_list_with_limit (list, lim);
> +
> +  if (parent)
> +    {
> +      for (i = 1, tem = parent; parent; parent = parent->next, ++i)
> +	{
> +	  item = tem->data;
> +	  item_title = webkit_back_forward_list_item_get_title (item);
> +	  item_uri = webkit_back_forward_list_item_get_uri (item);
> +	  title = build_string (item_title ? item_title : "");
> +	  uri = build_string (item_uri ? item_uri : "");

build_string can produce either multibyte or unibyte strings.  Which
ones do we want?  And shouldn't we decode the strings that come from
WebKit?

> +	  back = Fcons (list3 (make_fixnum (-i), title, uri), back);
> +	}
> +    }
> +
> +  g_list_free (parent);
> +
> +  parent = webkit_back_forward_list_get_forward_list_with_limit (list, lim);
> +
> +  if (parent)
> +    {
> +      for (i = 1, tem = parent; parent; parent = parent->next, ++i)
> +	{
> +	  item = tem->data;
> +	  item_title = webkit_back_forward_list_item_get_title (item);
> +	  item_uri = webkit_back_forward_list_item_get_uri (item);
> +	  title = build_string (item_title ? item_title : "");
> +	  uri = build_string (item_uri ? item_uri : "");
> +	  forward = Fcons (list3 (make_fixnum (i), title, uri), forward);
> +	}

This generates the lists in the reverse order, doesn't it?

Thanks.