Re: [PATCH v3] Add xwidget webkit support for macOS Cocoa

[Stefan Monnier]

Thanks, looks good for the part I can review.
See additional nitpicks in case nobody comes up with more
substantial comments.

> +(defun xwidget-webkit-split-below ()
> +  "Clone current URL into a new widget place in new window below.

Great.

> +(defun xwidget-webkit-split-right ()
> +  "Get the URL of current session, then browse to the URL \
> +in `split-window-right' with a new xwidget webkit session."

You missed this one, tho.

> +   (format "window.scrollBy(0, %d);"
> +           (or n (xwidget-window-inside-pixel-height (selected-window))))))

Great.

> -   "window.scrollBy(0, -50);"))
> +   (cond ((null n)
> +          (format "window.scrollBy(0, %d);"
> +                  (- (xwidget-window-inside-pixel-height 
> (selected-window)))))
> +         (t (format "window.scrollBy(0, %d);" (- n))))))

You missed this one, tho.

> +(defcustom xwidget-webkit-scroll-line-height 50
> +  "Default line height in pixels for scroll xwidget webkit."
> +  :type 'integer
> +  :group 'xwidget)

Those `:group 'xwidget` are redundant since it defaults to the last
`defgroup` in the buffer anyway.

> +            ;; TODO: Response handling other than download.
> +            ((eq xwidget-event-type 'response-callback)
> +             (let ((url  (nth 3 last-input-event))
> +                   (mime-type (nth 4 last-input-event))
> +                   (file-name (nth 5 last-input-event)))
> +               (xwidget-webkit-save-as-file url mime-type file-name)))

BTW, where does the "response-callback" name come from?
According to the above code, this is used in the case of downloads, so
why is it not called "download-callback"?

> +(defun xwidget-webkit-save-as-file (url mime-type &optional file-name)
> +  "For XWIDGET webkit, save URL resource of MIME-TYPE as FILE-NAME."
> +  (let ((save-name (read-file-name
> +                    (format "Save '%s' file as: " mime-type)
> +                    xwidget-webkit-download-dir
> +                    (expand-file-name
> +                     file-name
> +                     xwidget-webkit-download-dir) nil file-name)))

Again, please don't pass `file-name` as INITIAL arg: leave it nil.

> -         `((page     . ,(xwidget-webkit-current-url))
> -           (handler  . (lambda (bmk) (browse-url
> -                                 (bookmark-prop-get bmk 'page)))))))
> -
> +         `((page . ,(xwidget-webkit-current-url))
> +           (handler  . (lambda (bmk)
> +                         (browse-url
> +                          (bookmark-prop-get bmk 'filename)
> +                          xwidget-webkit-bookmark-jump-new-session)
> +                         (switch-to-buffer
> +                          (xwidget-buffer 
> (xwidget-webkit-last-session))))))))

I see you reverted to `page`, which is fine, but then the
(bookmark-prop-get bmk 'filename) needs to be changed back to
(bookmark-prop-get bmk 'page) as well, shouldn't it?

Also, I don't really understand the new code:
- how do we know that `browse-url` will use the xwidget browser rather than
  any other?  I think we should either call `browse-url` and presume it
  can be *any* browser, or call xwidget-webkit-browse-url instead.
- Why do we need the switch-to-buffer?  Why not rely on
  (xwidget-webkit-)browse-url to Do The Right Thing?
- switch-to-buffer fails miserably in configs such as mine
  (separate minibuffer frame and dedicated windows), so better avoid it
  in Elisp and use things like pop-to-buffer or pop-to-buffer-same-window.

> +    ;; Insert invisible url, good default for next `g' to browse url.
> +    (let ((start (point)))
> +      (insert url)
> +      (put-text-property start (+ start (length url)) 'invisible t))
>      (setq xw (xwidget-insert 1 'webkit bufname

Please use `start` rather than 1.


        Stefan