Re: Subject: [PATCH] * lisp/xwidget.el (xwidget-webkit-browse-url): Remove space prefix of url.

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> Is this a replacement for eww--dwim-expand-url?  Or is this in
> addition to it?

It's a replacement for `eww--dwim-expand-url', but in url-utils, because
it is too generally useful to warrant staying internal to eww.

>> (defun url-dwim-expand-url (url local-regex search-prefix)
>>   "Canonicalize URL.

> First line of a doc string should mention all the mandatory
> arguments.  (But see below.)

>> Try to determine if URL is an incomplete URL or a search query, and
>> return the canonical form of URL.
>> SEARCH-PREFIX is the prefix to be prepended to URL if it is a search query.

> Can the doc string explain what does "search query" mean in this
> context?

>> match is successful, then URL is treated as an address."

> This begs the question: and if it doesn't match, then what?  And what
> does "address" mean in this context, i.e. what does "treated as an
> address" means in practice?

>>   (cond ((string-match-p "\\`file:/" url))
>>      ;; Don't mangle file: URLs at all.

> This comment should be above the line that handles file:// URLs.
> Btw, should other URLs be exempt from "mangling"?  AFAIK, there are
> many protocols whose syntax we don't really understand in url*.el
> code, so shouldn't they all be left alone?

This is code I extracted from eww.el; Lars should know the reasoning for
that better.

>>         ((string-match-p "\\`ftp://"; url)
>>          (user-error "FTP is not supported"))

> I can understand this in EWW, but why should FTP be unsupported in
> url-util?

Hmm, makes sense.  See below.

>>       ;; Anything that starts with something that vaguely looks
>>       ;; like a protocol designator is interpreted as a full URL.
>>          (if (or (string-match "\\`[A-Za-z]+:" url)

> This will match Windows-style d:/foo/bar absolute file names.  Is that
> what we want?

No, but in this case eww should have to be fixed too.

>>               (and (= (length (split-string url)) 1)

> You are using split-string here to verify that URL has no SPC
> characters?  

That's what the original code in eww.el did.  I didn't dare change it,
because I didn't know why it was there.

>>                    (or (and (not (string-match-p "\\`[\"'].*[\"']\\'" url))
>>                             (> (length (split-string url "[.:]")) 1))

> It would be good to have a comment here explaining what do these
> conditions test.

Lars, do you know what this test is for?  Thanks.

>>                        (string-match local-regex url))))

> This sole use of LOCAL-REGEX hints that maybe it should be an optional
> argument.
>
>>              (progn
>>                (unless (string-match-p "\\`[a-zA-Z][-a-zA-Z0-9+.]*://" url)
>>                  (setq url (concat "http://"; url)))
>
> "http", not "https"?  I think the default nowadays is the latter.
>
>>            (setq url (concat search-prefix
>>                              (mapconcat
>>                               #'url-hexify-string (split-string url) 
>> "+"))))))
>>   url)

> Doesn't this part mean a search query is expected to be in some
> specific format?  If so, that format should be documented in the doc
> string.

I don't know: Lars, do you know why this is here?  Thanks in advance.

> Thanks.

Thanks.  Here's a better docstring, but I don't want to change any of
the code until I know precisely what it does.

Canonicalize URL, with SEARCH-PREFIX if URL seems to be a search query.

Try to determine if URL is an address or a query for a search engine,
and return canonicalized URL, with SEARCH-PREFIX prepended before
cannibalisation if it seems to be such a query.  Optional argument
LOCAL-REGEX is a regular expression that URL is matched against.  If the
match is successful, then URL is treated as an address, and not a search
query.