bug#52856: 29.0.50; Problematic handling of webkit xwidget bookmarks

[Po Lu]

Stephen Berman <stephen.berman@gmx.net> writes:

> Thanks for the suggestion, see the patch below.  Since the previous
> bookmark handler was the only user of `xwidget-webkit-browse-url''s
> `new-session' parameter, I removed that, and took the opportunity to
> make some other cleanups to that function, see the commit message.
> While testing the refactored `xwidget-webkit-new-session' I found that
> the xwidget-webkit-clone-* commands were broken because
> `xwidget-webkit-current-url' returned the message instead of the URL, so
> I fixed that.  And I removed from the doc string of
> `xwidget-webkit-bookmark-jump-new-session' the suggestion to customize
> `xwidget-webkit-last-session-buffer', since, for one thing, it's not a
> defcustom, but more importantly, changing it has no effect, since it
> gets set by `xwidget-webkit--create-new-session-buffer' (previously by
> `xwidget-webkit-new-session'), overriding any user change to its
> value.

Thanks, but there are a few problems with this change:

> 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.

>       (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.

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?

> +(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.

> +  "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?

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