bug#51712: 29.0.50; [PATCH] New function `xwidget-webkit-load-html'

[Eli Zaretskii <eliz@gnu.org>]

> Date: Tue, 09 Nov 2021 18:26:17 +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-load-html xwidget text base-uri
> +Load @var{text} into @var{xwidget}, which should be a WebKit xwidget.
> +@var{text} will be treated as HTML markup, and rendered by
> +@var{xwidget}.

can you rephrase the last sentence not to use passive tense, please?

> +For the purpose of controlling the location where web resources will
> +be found, you can optionally specify a base URI as a string in
> +@var{base-uri}, which defaults to @samp{about:blank}.

But BASE-URI is not an &optional argument above.

And also, this sentence is unnecessarily complex because you state the
reason before the goal.  It is much better to do it the other way
around, and while at that, make 2 simple sentences out of one complex
one:

  Optional argument @var{base-uri}, which should be a string,
  specifies the location of the web resource.  It defaults to
  @samp{about:blank}.

(Of course, now this begs the question: what does it mean "web
resource" for HTML text?  How about answering that in the text?)

> ++++
> +*** New function 'xwidget-webkit-load-html'.
> +This function is used to load HTML text into WebKit xwidgets, without
> +having to create a temporary file or URL.

The second part is either unnecessary or too terse.  if it is
important to explain that, please elaborate how temporary files are
relevant here.

> +DEFUN ("xwidget-webkit-load-html", Fxwidget_webkit_load_html,
> +       Sxwidget_webkit_load_html, 2, 3, 0,
> +       doc: /* Make XWIDGET's WebKit widget render text.
> +XWIDGET should be a WebKit xwidget, that will receive TEXT.  TEXT
> +should be a string that will be displayed by XWIDGET as plain text.

The "plain text" part seems to contradict the text in the manual,
which says it will be rendered as HTML markup?

> +BASE_URI will be a URI that is used to fetch resources, and if not
> +specified, is treated as equivalent to `about:blank'.  */)

"will be" is confusing: why the future tense?

Also, what kind of Lisp type is that?

And I think explaining the importance of "fetching resources" would be
beneficiary here.

And finally, "is treated as equivalent" is better reworded as "and
defaults to ...", which is our style in these cases.

> +  if (NILP (base_uri))
> +    base_uri = build_string ("about:blank");
> +  CHECK_STRING (base_uri);

That last line should better be under 'else', right?

> +  base_uri = ENCODE_UTF_8 (base_uri);

Is it a good idea to produce non-ASCII URIs?

> +  text = ENCODE_UTF_8 (text);
> +  xw = XXWIDGET (xwidget);
> +
> +#ifdef USE_GTK
> +  data = SSDATA (text);
> +  uri = SSDATA (base_uri);
> +  webview = WEBKIT_WEB_VIEW (xw->widget_osr);
> +
> +  block_input ();
> +  webkit_web_view_load_html (webview, data, uri);
> +  unblock_input ();
> +#endif

Hmm... if we only use TEXT and BASE-URI in the GTK build, why do we
encode them in the other builds?  Isn't that a waste of cycles?  IOW,
what does this function do if USE_GTK is not defined?

Thanks.