On Thu, 30 Dec 2021 19:18:57 +0800 Po Lu <luangruo@yahoo.com> wrote:

>> 2021-12-30  Stephen Berman  <stephen.berman@gmx.net>
>>
>> 	Fix handling of webkit xwidget bookmarks
>>
>> 	Make jumping to a bookmarked webkit xwidget in another window or
>> 	another frame show the xwidget only in that window or frame.
>> 	Adjust xwidget.el code accordingly.  Make xwidget-webkit-clone-*
>> 	commands work. (Bug#52856)
>>
>> 	* lisp/xwidget.el (xwidget-webkit-browse-url): Remove
>> 	`new-session' parameter, since the bookmark handler, which was its
>> 	only user, no longer calls this function.  Remove superfluous
>> 	`browse-url' require from interactive spec and discard the unused
>> 	flag returned by `browse-url-interactive-arg'.  Incorporate the
>> 	code of the now removed `xwidget-webkit-goto-url', since this
>> 	function was its only caller.
>
> While that might be the case in-tree, external packages and users might
> be using that parameter -- it should stay.

Ok.  (The superfluous `browse-url' require can be removed by future code
cleanup.)

>> 	(xwidget-webkit-goto-url): Remove.
>
> The same goes for this.  I think it's certain that some external code
> will be using this function, as the name is rather tempting.

Ok.

> I also have some further comments below:
>
>> +        (progn
>> +          (xwidget-webkit-goto-uri (xwidget-webkit-current-session) url)
>> +          (switch-to-buffer (xwidget-buffer (xwidget-webkit-current-session))))
>
> Since you already define a replacement for `xwidget-webkit-new-session'
> below that also behaves like this, is there any reason you can't use
> that here instead?

This is the code from the not-to-be-removed `xwidget-webkit-goto-url'
for reusing the current xwidget-webkit session instead of creating a new
session.  This is the only occurrence of this sexp, so encapsulating it
in a function seems like overkill.

>> +(defun xwidget-webkit-bookmark-jump-handler (bmk)
>> +  "Function for jumping to the webkit xwidget bookmarked by BMK.
>> +If `xwidget-webkit-bookmark-jump-new-session' is non-nil, create
>> +a new xwidget-webkit session, otherwise use an existing session."
>
> I think it would be clearer to say "Jump to the page described by the
> bookmark record BMK."
>
> But I'm not good at writing documentation, so I won't insist.
>
> Also, `bmk' should be named `bookmark', or something else to that
> effect.

How's this: "Jump to the web page bookmarked by the bookmark record
BOOKMARK."?

>> +  "Return current xwidget webkit URL and display it in a message.
>> +Also place it on the `kill-ring'."
>
> I don't understand why this user command has to be called
> programmatically, and why it should return the URL.  Can't you use
> `xwidget-webkit-uri' instead?

Sure.

> And thanks for the work you've been doing.  I really appreciate it.

Thanks.  Here's the latest version:


2021-12-30  Stephen Berman  <stephen.berman@gmx.net>

	Fix handling of webkit xwidget bookmarks

	Make jumping to a bookmarked webkit xwidget in another window or
	another frame show the xwidget only in that window or frame and
	refactor new session code used by the bookmark code (bug#52856).
	In addition, make xwidget-webkit-clone-* commands work.

	* lisp/xwidget.el (xwidget-webkit-clone-and-split-below)
	(xwidget-webkit-clone-and-split-right): Unbreak these functions by
	passing just the URL to them, not the message displaying it.
	(xwidget-webkit-bookmark-jump-new-session): Adjust doc string,
	rephrasing and removing a customization suggestion that cannot
	take effect.
	(xwidget-webkit-bookmark-jump-handler): New autoloaded function.
	(xwidget-webkit-bookmark-make-record): Use it.
	(xwidget-webkit--create-new-session-buffer): New function
	extracted from `xwidget-webkit-new-session'.
	(xwidget-webkit-new-session): Use it.