264647 - Implement nsOSHelperAppService::GetApplicationDescription for OS/2

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

to be able to show an application description in the confirmation dialog for
external protocols, nsOSHelperAppService::GetApplicationDescription should be
implemented.

this bug is about the OS/2 implementation.

Comment 1

[Peter Weilbacher]

What kind of description did you think of? What do Windows or Linux give me if I
lauch a link like mailto: with a script runmymailprogram.cmd or if I run a ssh:
link with putty.exe?
The windows code seems to look something up in the registry, but I have no idea
what is in there...

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

the windows code, as well as the linux and gnome code, use the leaf name of the
application as description. (windows will be changed to use the application's
description from its resource, in bug 252189).

os/2 currently shows <Unknown>, I think; which is somewhat undesirable..

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

oops. actually, windows seems to use the full pathname to the application.
rather, whatever the application stored there... (full pathname seems common)

Comment 4

[Peter Weilbacher]

OK, thanks. Full pathname should be possible, don't know what a leaf name is,
anyway... Although it would be nice to show something more descriptive, I don't
think that's easy to get on OS/2. Well, some EXE files have some kind of
application description embedded, perhaps that can be used, although it does not
seem to be very standardized. I will try to use that.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

given c:\foo\bar.exe, leaf name would be bar.exe (i.e. just the filename w/o
directory information)

Comment 6

[Peter Weilbacher]

Hmm, for OS/2 this would mean to go through all the stuff that we now do to
determine the handling app including the internet app(s) as set in the URL
object and/or ConfigApps, see bug 254320. Either we duplicate a lot of code to
create something like the GetHandlerAppFromPrefs that is used for this on Unix
(and we already have duplicate stuff in ExternalProtocolHandlerExists and
LoadUriInternal). Or we invent a new method/function that handles at least the
case where the handler is drawn from the INI file. As the platforms already have
wildly different methods in nsOSHelperAppService.cpp anyway, I guess the latter
approach is the one to go for...

Comment 7

[Peter Weilbacher]

Attachment: patch adding GetApplicationDescription for OS/2

OK, a first try to implement this. I moved the application from OS2.INI stuff
to a new helper method ::GetApplicationAndParametersFromINI and then used this
and stuff from ::LoadUriInternal for the new method
::GetApplicationDescription. This now shows either the application as given,
or, if that cannot be accessed like that, searches the PATH for it and if found
shows the full path+leaf name. If the PATH lookup fails I just took what is
there...

Because it is difficult to disentangle this from the work going on in bug
254320 I include the new stuff from there as well, so that this patch
supersedes attachment 164016 [details] [diff] [review].

Comment 8

[Peter Weilbacher]

Comment on attachment 165214 [details] [diff] [review]
patch adding GetApplicationDescription for OS/2

Javier, do you still do OS/2 reviews?

Comment 9

[jhp (no longer active)]

Comment on attachment 165214 [details] [diff] [review]
patch adding GetApplicationDescription for OS/2

+  /* unsupported protocol scheme */
+  else {
+    return NS_ERROR_FAILURE;
+  }

Is it worth putting an assertion or warning here, so we know when the function
gets called for an unsupported protocol?  

+  /* application string in INI was empty */
+  if (strcmp(app, "\0") == 0)

You could just do "if (app[0] == '\0')".

+  if (GetApplicationAndParametersFromINI(nsDependentCString(aProtocolScheme),
+					  szAppFromINI, sizeof(szAppFromINI),
+					  szParamsFromINI,
sizeof(szParamsFromINI))
+      == NS_OK) {
+    *aHandlerExists = PR_TRUE;


Don't like this lengthy function call inside the if statement.	I think it
would look cleaner if you just saved the return value and checked it on a
different line.  You do this elsewhere in the patch, also.

At the end of the function GetApplicationDescription(), if
NS_NewNativeLocalFile and DosSearchPath both fail, then you would still return
NS_OK.	Is that your intent?

Comment 10

[Peter Weilbacher]

(In reply to comment #9)
> At the end of the function GetApplicationDescription(), if
> NS_NewNativeLocalFile and DosSearchPath both fail, then you would still return
> NS_OK.	Is that your intent?

Actually, yes. We then still have whatever will be passed to the OS and this
should be good enough to display in the warning message. That the call will most
likely fail later in ::LoadUriInternal is something else.

Will post a new patch with the other requested changes, thanks!

Comment 11

[Peter Weilbacher]

Attachment: better patch, changed following comments

Comment 12

[Mike Kaply [:mkaply]]

Comment on attachment 165343 [details] [diff] [review]
better patch, changed following comments

sr=blizzard (platform specific)
a=mkaply for 1.8a5

Comment 13

[Mike Kaply [:mkaply]]

Fix checked in to trunk.