254320 - Add protocols to ExternalProtocolHandlerExists

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Mike Kaply [:mkaply]]

In bug 241966, we added custom support for certain protocols to launch apps that
are specified in the INI file.

Unfortunately, we forgot to add the code into ExternalProtcolHandlerExists so
that when Mozilla asks if we have handlers for those protocols, we say "yes".

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Fix

I didn't make this as complex as the other code - i.e. if we get a ftp URL, we
don't check for a default browser.

This is probably how the other code should look as well.

Comment 2

[timeless]

Comment on attachment 156810 [details] [diff] [review]
Fix

>+  nsDependentCString protocol = nsDependentCString(aProtocolScheme);

i think:

nsDependentCString protocol(aProtocolScheme);

or

nsDependentCString &protocol = nsDependentCString(aProtocolScheme);

is more common...

Comment 3

[Peter Weilbacher]

Comment on attachment 156810 [details] [diff] [review]
Fix

Just one thing:
Although I hardly understand anything in the "Mozilla String Guide" but
nsDependentCString does not know EqualsLiteral (that seems to be restricted to
AutoStrings?)

At first I thought it might be shorter and more readable to use the return
value of PrfQueryProfileString, e.g. 
   *aHandlerExists = (PrfQueryProfileString(...) > 0);
but then if also returns true when the INI-key only contains '\0'. So testing
for that -- as the patch currently does -- may indeed make more sense.

Comment 4

[Peter Weilbacher]

Played around with the patch a bit. It _does_ work when the
protocol.EqualsLiteral() is replaced by protocol.Equals(). But strangely, it
does not work for the "news" protocol, there I still get the alert that says
"news is not a registered protocol"

Btw, it might be a good idea to use "snews" there as well. There might not be
many Newsreaders on OS/2 to support that protocol, but it is used on
<http://www.mozilla.org/support/>. The same page also raised the question what
to do with irc: URIs. I will ask Aaron if he is willing to add it to ConfigApps.

Comment 5

[Peter Weilbacher]

Attachment: New version for the protocol handler

OK, I made some changes to Mike's original patch:
- add an extra check for *aHandlerExists before returning NS_OK
- changed all protocol.EqualsLiteral() to protocol.Equals()
- added "snews" protocol in addition to "news"
- added the new IRC options from Aaron's preliminary ConfigApps 1.1.0
- deleted support for the non-standard "DefaultBrowserParameters" key. I also
asked Aaron to correct this error in his little app

This patch compiles and when added some debug output it really reacts the way
we want. What is still missing for complete support are the different
parameters that ConfigApps suggests.

Comment 6

[Peter Weilbacher]

Comment on attachment 160414 [details] [diff] [review]
New version for the protocol handler

It's probably a bit unusual to ask for review from someone who made the first
version of a patch, but as this is platform specific...

Comment 7

[Aaron Lawrence]

Greetings. Peter mailed me about this, and suggested some new named placeholder
parameters in place of my previous suggestions for %1 %2 etc. I agree that named
parameters are nicer, and I will update my ConfigApps readme to match.

Peters suggestions:
For browser:
  %url%    - insert URL here
Mail
  %url%    - complete email address
News
  %group%  - group name
  %host%   - server name
  %mid%    - message ID (if relevant)
FTP
  %url%    - server address, including dir 
IRC
  %url%    - full server+channel URL

%mid% is unrecognisable to me, I would suggest %msgid% or %messageid%. 

I'm happy to add snews (secure news?) if you really think it is worthwhile, but
it seems a bit pointless...

Comment 8

[Aaron Lawrence]

It seems to me we should separate the FTP server from directory. I don't
understand the argument not to. Making little wrapper batch files drives me batty...
Anyway, I updated ConfigApps to fix the browser parameters key
http://aaronlawrence.fastmail.fm/ConfigApps1_1_1.zip

Comment 9

[Peter Weilbacher]

Well, in principle I agree with you on decomposing the URLs into subunits (don't
like batch files either), but from the code I have seen in
nsOSHelperAppService.cpp it's not _that_ easy to separate the various parts of a
URL unless they were given in user.js/prefs.js. But that may just be my limited
understanding of the C++ classes and their relations, so I will look into this a
bit more... The inetapps.dll library you talked about and that you still have to
write (?) could be helpful there, although it would only confuse users more if
Mozilla/FF would be dependent on it.

For the %messageid% string: it there an example of a page where this is used or
was your original idea to just use this in MailNews/Thunderbird?

snews could be the same handler as for normal news, right? That's how I added it
to the patch. Even though I haven't see any (OS/2) program apart from
Mozilla/Thunderbird that can handle those URLs...

Comment 10

[Aaron Lawrence]

True it would be bad to have Mozilla dependent upon some obscure little addon dll.
Regarding messageid, I thought I've seen some use of this in places. Could be
wrong. It's not a priority, for sure.
You say that it could be difficult to separate the parts of the URL - I guess
you are right - but that would be FAR harder to do in a batch file than in
Mozilla itself, with all its handy libraries!
I guess we should add port, user and password to ftp....

Comment 11

[Peter Weilbacher]

Attachment: new patch, full support for ConfigApps parameters

Actually, it was not that difficult, once I had worked out how all these string
types and URI methods are supposed to work.

This patch now supports all the replacement variables that were suggested in
the ConfigApps readme. I tested this on a Seamonkey trunk build without
MailNews or Chatzilla and it works fine for the news/snews, mailto, irc
schemes. Someone has to test the remaining http/https/ftp schemes with
Thunderbird. Especially, I am not sure if GetPath() really gives me the path
including the filename, but GetFilePath() was not recognized...

From this patch, I would suggest the following text for ConfigApps (you don't
have to change the application for that, do you?).

--------------------------------------------
Application    URL-Scheme
Web Browser    "http:" and "https:"
Email	       "mailto:"
Newsgroups     "news:" and "snews:"
FTP	       "ftp:"
IRC	       "irc:"

The replacement variable 
  %url%      - server address, including scheme, dir and everything
can be used for all application types.

URLs for web browsers and FTP applications follow this general template
  ftp://username:password@host:port/path
where instead of ftp: also http: and https: are in use. Hence 
applications should support these replacements:
  %username% - username for authentication
  %password% - password for authentication
  %host%     - only the server address
  %port%     - TCP/IP port of the server
  %path%     - path to the file (including filename)

For Email, this variable should be used
  %email%    - the email address from the link

Usenet News URLs can take three basic forms
   news:group
   news://host/group
   news:msgid
where news: could also be replaced by snews: for encrypted news feeds.
Applications should hence support these variables:
   %group%  - group name
   %host%   - server name
   %msgid%  - message ID

For IRC, the URL usually takes a form like
   irc://server/channel
Apps should support this variable in addition to the full URL:
   %host%    - the server name
   %channel% - the name of the IRC channel

Comment 12

[Aaron Lawrence]

I have put these into the readme. There's no need to change the application, as
you say.
Arguably, it would be nicer if it had some online help or other guide telling
you what to put in the parameters, as most programs would require this done
manually. But that is a luxury.

Comment 13

[Aaron Lawrence]

New ConfigApps is on Hobbes now, including IRC settings and the new text Peter
suggested.
http://hobbes.nmsu.edu/pub/incoming/configapps1_1_1.zip
later
http://hobbes.nmsu.edu/pub/os2/apps/misc/configapps1_1_1.zip

Comment 14

[Peter Weilbacher]

Attachment: One more try

Sigh... Before, I forgot the case where someone uses the WPS object and doesn't
know about the ConfigApps variables like %url%. Now the full URL now just gets
appended to the parameters if no replacement happened.

This patch has the drawback that if someone sets up a handler using ConfigApps
but has something like network.protocol-handler.external.mailto etc in the
prefs,	he might get something different than expected, becaue the prefs case
takes precedence. I don't see a way around that but then it is the user's own
fault, isn't it?

Comment 15

[Peter Weilbacher]

Attachment: Patch for aviary branch

Mike, should this not get into Firefox before the 1.0 release for OS/2 and
especially before Thunderbird 1.0? This is the same patch adapted for the
aviary branch.

Comment 16

[Peter Weilbacher]

Comment on attachment 164016 [details] [diff] [review]
One more try

The trunk patch is now superseded by attachment 165214 [details] [diff] [review] of bug 264647...

Comment 17

[Mike Kaply [:mkaply]]

This fix is in on 1.7 and aviary.

We'll handle the trunk in bug 264647