145860 - BeOS needs implemenation of nsOSHelperAppService

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Paul]

An implementation of nsOSHelperAppService needs to be done for BeOS.  This, I think, should
allow the "Launch" button to work.  From the header of the unix version:

"It contains platform specific code for finding helper applications for a given mime type
in addition to launching those applications."

Comment 1

[Matthias Versen [:Matti]]

I can't fix it...

Comment 2

[Sergei Dolgov]

MAtti,
there was bug (and patch) to implement "Launch" button already
http://bugzilla.mozilla.org/show_bug.cgi?id=145860

Comment 3

[Matthias Versen [:Matti]]

[e-mail]: 
Can you explain that ? I never fix bugs bugs etc...
and why do you put this bug URL in this bug ?

Comment 4

[Sergei Dolgov]

Sorry, Matti, i you were wrong person. Don't bother

Comment 5

[Paul]

taking

Comment 6

[Paul]

Attachment: zip containing beos specific nsOSHelperAppServer impl

Ok, this seems to be called, and gets information from the MIME database
properly.  Now, we must find out why, under BeOS, all files are automatically
saved, and can never be launched.  We don't get the save/open dialog when
clicking on a file

Comment 7

[Paul]

Attachment: patch to Makefile.in

this is needed to build the above code

Comment 8

[Sergei Dolgov]

There are some crash-bugs in code, like:
char *desc;
if (mimeType.GetShortDescription(desc) != B_OK)
    if (mimeType.GetLongDescription(desc) != B_OK
       desc = (char *)aMIMEType;
from BeBook:
file:///boot/beos/documentation/Be%20Book/The%20Storage%20Kit/MimeType.html:
" The Get functions 
copy the string into the argument <B>(which must be allocated)</B>."

so, i'm revising code now

Comment 9

[Sergei Dolgov]

ok, code works, but there are two problems in nsExternalHelperAppService.cpp, in
NS_IMETHODIMP nsExternalAppHandler::OnStartRequest()
First - 
  PRBool alwaysAsk = PR_TRUE;
  mMimeInfo->GetAlwaysAskBeforeHandling(&alwaysAsk);
always results in alwaysAsk == PR_TRUE, inspite of value set in
Preferences->Navigator->Helper Application.

and second, more serious for BeOS - as alwaysAsk == true, the method calls 
rv = mDialog->Show( this, mWindowContext );
where 
    mDialog = do_CreateInstance( NS_IHELPERAPPLAUNCHERDLG_CONTRACTID, &rv );

and main bug is here - 
!!!instead creating this helper dialog, it launches native BeOS nsFilePicker!!!
Sure, that filepicker don't set any preffered action, and by default it is save
to disk.

If is set alwaysAsk to false in if() - all works like a charm

Comment 10

[Sergei Dolgov]

so it should to call FilePicker:
http://lxr.mozilla.org/mozilla/source/embedding/components/ui/helperAppDlg/nsIHelperAppLauncherDialog.idl#83
http://lxr.mozilla.org/mozilla/source/embedding/components/ui/helperAppDlg/nsHelperAppDlg.js#114

So it should create new BeOS port bug -  "HelperAppDlg.cpp needs implementation
for BeOS",
likehttp://lxr.mozilla.org/mozilla/source/embedding/browser/activex/src/control/HelperAppDlg.cpp

Comment 11

[Sergei Dolgov]

Attachment: Patch for nsExternalHelperAppService.cpp

Workaround for missing in BeZIlla AppHelperDlg class.
Uses BAlert.
Patch for Pauls' files (non-allocated chars *) will follow.

Comment 12

[hacker formerly known as [e-mail]]

Comment on attachment 93028 [details] [diff] [review]
patch to Makefile.in

r=cls

Comment 13

[Paul]

Attachment: updated beos/nsOSHelperAppService code, as per Sergei's comments

Ok, the "desc" is now allocated.

Problems:
- The "Launch" button in the BAlert downloads the file, but always fails to
actually launch, with a "file could not be opened, because an unknown error
occurred ... Sorry about that.	Try saving to disk first and then openning the
file." message
- The "Launch" button in the Save/Download dialog is always disabled.

Comment 14

[Sergei Dolgov]

Paul, appSig should be also allocated.
Maybe this is your problem.
It all works here fine, so i'll send  my workfiles by e-mail, to see if there is
some unnoticed difference. (there are lot of debug frointfs there, though).
Sure, my version of nsExternalHelperAppService.cpp is older than your, but i
doubt that that is the reason.

Comment 15

[Paul]

Attachment: updated beos/nsOSHelperAppService.[cpp|h]

Thanks to Sergei, this now works (forgot to allocate the appSig char array as
well, oops).

Still, the "Save" dialog has a disabled "Launch" button, and is never enabled,
but, the "Launch" button from the new BAlert now works :)

Comment 16

[Sergei Dolgov]

Yeah, Launch button in downloader is different case. Didn't investigated it yet.
Maybe it will be another bug. Problem is that i don;t figure how i can force to
appear that Dialog, because i have that list-view download manager window
instead, which never launches final dialog for me (at least i can force it to be
shown) - so some clue/advice needed).

As for current problem, still needs investigation - why
mMimeInfo->GetAlwaysAskBeforeHandling(&alwaysAsk) works so suspiciosly and where
we can set it. (there are is outcommented code in nsExternalHelperAppService.cpp
- that alwaysAsk isn't now in rdf).

Also, as unimplemented HelperAppDlg.cpp belongs to embedding code, on which Paul
(i hope!) continue working) - it is worth implementing in future.

Though, i think this current solution should be checked in. (heh, who may be
superreviewer, especially for nsExternalHelperAppService.cpp change?)

Comment 17

[Sergei Dolgov]

Comment on attachment 99306 [details]
updated beos/nsOSHelperAppService.[cpp|h]

r=[e-mail]

Comment 18

[Paul]

Comment on attachment 99231 [details] [diff] [review]
Patch for nsExternalHelperAppService.cpp

r=[e-mail]

Comment 19

[Sergei Dolgov]

>More rant.
Mozilla seems loosing consistency.
Nobody never calls SetAlwaysAskBeforeHandling() now,
but calls GetAlwaysAskBeforeHandling() which checks preferences for two options.
Couldn't find any components setting those preferences, though. Hanging options
without handler.

More, exthandler relies more on those helper dialogs in embedding/ui.
But generic version of those dialogs seems half-broken and again relies on other
dialogs like FilePickers.
>End generic rant.

Now about "Launch" button in download/progress dialogs. Problem is MS-related
paranoia. For enabling "Launch" code checks !File.IsExecutable.
And now to BeOS. It seems that this check always returns IsExecutable for those
cases.

I think that nsLocalFileUnix.cpp implementation of IsExecutable is not the best
for BeOS. Or higher-level function which uses that call are flaky - probably
don't check error status returned by 
NS_IMETHODIMP
nsLocalFile::IsExecutable(PRBool *_retval)
{
    CHECK_mPath();
    NS_ENSURE_ARG_POINTER(_retval);

    *_retval = (access(mPath.get(), X_OK) == 0);
    if (*_retval || errno == EACCES)
        return NS_OK;
    return NSRESULT_FOR_ERRNO();
}

Comment 20

[Sergei Dolgov]

Good news and bad news.
Bad - BeOS POSIX call access() is buggy.
Good - with correctly working IsExecutable we don't need hack for
nsExternalHelperAppService.cpp - because standard dialog seem start appearing
and working.

It seems we have need to fork nsLocalFileUnix to nsLocalFileBeOS.

Comment 21

[Paul]

We had talked about making our own version of nsLocalFile before for
internationalization issues, so, maybe now is  a good time to do it.

Comment 22

[Sergei Dolgov]

NS_IMETHODIMP
nsLocalFile::IsExecutable(PRBool *_retval)
Yeah, Paul. To get the taste, you can replace IsExecutable() code in
nsLocalFileUnix.cpp with following code and enjoy what happens. Some new dialogs
which we never seen appears, in addition to enabled "Launch" button.
{
struct stat buf;
 CHECK_mPath();
 NS_ENSURE_ARG_POINTER(_retval);
stat(mPath.get(), &buf);
*_retval = (S_ISREG(buf.st_mode) && (buf.st_mode & 0111));
CHECK_mPath();
    NS_ENSURE_ARG_POINTER(_retval);
 
    if (*_retval)
        return NS_OK;
    return NSRESULT_FOR_ERRNO();
}

Comment 23

[Sergei Dolgov]

code was messed up with copy/paste and Mozilla issues:
trying again.

NS_IMETHODIMP
nsLocalFile::IsExecutable(PRBool *_retval)
{
struct stat buf;
 CHECK_mPath();
 NS_ENSURE_ARG_POINTER(_retval);
stat(mPath.get(), &buf);
*_retval = (S_ISREG(buf.st_mode) && (buf.st_mode & 0111));
    if (*_retval)
        return NS_OK;
    return NSRESULT_FOR_ERRNO();
}

Comment 24

[Sergei Dolgov]

Comment on attachment 99231 [details] [diff] [review]
Patch for nsExternalHelperAppService.cpp

should work without this hack after fix of bug 169506

Comment 25

[Sergei Dolgov]

I'm still in doubts about given problem,
maybe i should just propose to replace general access()-based FileUnix code for
Is*() functions with VALIDATE_STAT_CACHE() based. As it seems reliable for all
"unices" inc. BeOS  and that VALIDATE_STAT_CACHE() is used already widely in code

Comment 26

[Paul]

The tree is currently closed, but blocker bug#169506 should be checked in this
weekend.  I would like to check this for the 1.2 beta, as well.  The tree will
be closing for that on tuesday.  This patch allows enables the open/save dialog
for the first time under BeOS, and will now open a file, using the BeOS MIME
database.
This patch is BeOS specific, and does not affect any other platforms.

Scott/Seth, could you please review the change, and give an sr= for this bug,
so I can check it in before tuesday?  

Comment 27

[Boris Zbarsky [:bzbarsky]]

> tmpdesc.AssignWithConversion(desc);

Is "desc" always ASCII?  If not, this is wrong.  AssignWithConversion assumes
ASCII input.

same for appPath.

> // check to see if the given MIME type is registerred

This comment seems out of place in GetFromExtension...  And please fix the
spelling of "registered".

> // if the extension is null, return immediately

This comment seems out of place in GetFromMIMEType

GetFileTokenForPath, as written, will only work with ASCII paths.  I doubt
that's what you want... Wouldn't using CStrings throughout there and using
InitWithNativePath instead of InitWithPath be better?

Otherwise looks pretty good!

Comment 28

[Paul]

Boris, thanks for the comments!

I will fix the comments in the code.

I don't want to work with only ASCII paths, correct.  I took the code from GTK/Windows,
so, they are both wrong, as well.  

So, I should use a nsCString instead of nsCAutoString or nsAutoString for both of the 
places you mentioned, correct?

Comment 29

[Boris Zbarsky [:bzbarsky]]

The "gtk" code is correct; the files it's working with only usefully support
ASCII for now... ;)

In your case...  For the description, you'll need to get UCS2 out of your char*.
 Do you know what encoding that char* is in?  Can you assume UTF8 or could it be
just some "system encoding"?

For the nsIFile stuff, I think you could just use nsCString/nsCAutoString
throughout and work in whatever the native encoding happens to be all the way
through (using InitWithNativePath).  But then for SetApplicationDescription you
again need to convert that to UCS2.  The best way may be to get the leafname off
the nsIFile; that has accessors to return UCS2 path chunks.

Comment 30

[Paul]

BeOS uses UTF8 always.  Trying to read through the String classes in mozilla really gives
me a headache :)  I see the headers are documented.  Can documentation be made from them?
If so, how?  It would be much easier to read/follow.

Aren't the nsCString/nsCAutoString classes "obsolete"?  If so, should I use something
different?  What is the best way to convert UTF8 to USC2 and vice versa?

Oh, and gtk and windows both use InitWithPath, but, I have chnaged the BeOS impl. to use
InitWithNativePath.

Comment 31

[Paul]

Attachment: updated beos/nsOSHelperAppService.[cpp|h] as per Boris

Zip file containing the beos implementation files.

I think I have done the String code properly, now.  It's not so bad when you
use the "obsolete" string functions.  I wasn't really looking in there, since
it had the name, obsolete.

Comment 32

[Sergei Dolgov]

Paul,  documentation here was quite good for me
http://unstable.elemental.com/mozilla/ - but site seems broken now.

Comment 33

[Boris Zbarsky [:bzbarsky]]

There's some decent documentation on strings at
http://www.mozilla.org/projects/xpcom/string-guide.html

nsC(Auto)String is not obsolete.  ;)  Or rather it's obsolete as an interface
but not as a concrete implementation.

The best way to convert UTF8 to UCS2 and back are the NS_ConvertUCS2toUTF8 and
NS_ConvertUTF8toUCS2 classes.  The former implements nsACString, the latter
implements nsAString.  Typical usage:

CallSomeFuncNeedingUTF8(NS_ConvertUCS2toUTF8(myUCS2String));

Like I said, the gtk code is pretty dumb at the moment.  Windows is most likely
using the UCS2 system APIs...

The patch is still doing:

		nsAutoString appPath;
		appPath.AssignWithConversion(path.Path());

Which is probably not what you want.  ;)  It's also using AssignWithConvertion
for the path leaf.....

I just realized that you can't pass a CString to GetFileTokenForPath (this
should be fixed at some point; I see no reason for GetFileTokenForPath to live
on the superclass as opposed to being implemented usefully by the child classes;
no one but the child classes should be calling it anyway).  In that case, you
should just use NS_ConvertUTF8toUCS2 on your path, pass .get() on the result to
GetFileTokenForPath and use InitWithPath...

Sorry for all the string confusion.

Comment 34

[Paul]

Attachment: mozilla/uriloader/exthandler/beos/nsOSHelperAppService.h

adding files separately to make it easier to view from the bug page

Comment 35

[Paul]

Attachment: mozilla/uriloader/exthandler/beos/nsOSHelperAppService.cpp

Added separately to make for easier viewing from the bug page.

- comments are fixed
- description code in SetMIMEInforForType now used UTF8 <-> UCS2 conversion
classes
- GetFileTokenForPath now uses the UTF8 <-> UCS2 conversion classes.  Also, I
am using the InitWithPath instead of InitWithNativePath, as per Boris's
comments.

Sergei, can you test the latest changes as well?  They work for me :)

Comment 36

[Sergei Dolgov]

Have problems here.
First seems related to patch, second to code outside this patch scope.
1)Outside problem - something happened with Mozilla dialog which asks about
preffered action - no OK button there - see:
http://www.fi.tartu.ee/~sergei_d/MozDiag2.png

2) Seems patch problem - look at
http://www.fi.tartu.ee/~sergei_d/MozDiag1.png

I clicked on link *.rtf file on beunited.org.
Previously, with old sources and older version of patch, it proposed correct
application - BeBasics Writer for *.rtf. Now it proposes misterious #1 or such.
Here i just suspect this strings and encodings problem.
Btw, month ago or so, when i started to dig this problem, i tried in your code
both InitWithPath and UC2-UTF8 conversion (without Boris advice, and i have
feeling that it worked:) Should look in archive, if it is still on my disk.

Comment 37

[Sergei Dolgov]

Using such "tricks" as

NS_ConvertUTF8toUCS2 tmpStr(desc);
mimeInfo->SetDescription(tmpStr.get());

seems suspicious for me, Paul.

Changing it to:
nsAutoString tmpStr;
*******
tmpStr.Assign(NS_ConvertUTF8toUCS2(desc));

solved problem 2) for me.

(Too much Java coding, ehh?)

Comment 38

[Sergei Dolgov]

Attachment: nsOSHelperAppService.cpp - my version

Hello, this is my version of nsOSHelperAppService.cpp.
As this isn't patch, for convinience i didn't removed changed code, but put it
in comments.

Paul, also you forgot to replace AssignWithConversion with NS_ConvertUTF8toUCS2
in two other places. Approach should be persistent.

Hope Boris will look at this version again.

Comment 39

[Boris Zbarsky [:bzbarsky]]

> Using such "tricks" as

Actually, that's a perfectly valid usage.  NS_ConvertUTF8toUCS2 is a subclass of
nsAutoString.  If you're having to make that extra string copy with tmpStr,
that's a bug in the string library (and should be filed on the string library).
 Could you please check on that?  I suspect the AssignWithConverion() calls were
a more likely culprit than the NS_ConvertUTF8toUCS2.  Also, as I said, you can
avoid having an named temporary entirely if you want and let the compiler handle
it (see comment 33).

Problem #1 is due to the long app path causing line wrapping, most likely... 
There's an existing bug on that issue.

Comment 40

[Sergei Dolgov]

Hello Boris.
If "NS_ConvertUTF8toUCS2 is a subclass of nsAutoString" and "you can
avoid having an named temporary entirely",
maybe best solution is something like this:
mimeInfo->SetDescription(NS_ConvertUTF8toUCS2(desc).get());
???

Comment 41

[Sergei Dolgov]

btw, maybe i'm tired a bit, and it was discussed before - but what we doing?
Getting char *, supposing it is UTF8, convering to UC2, but then converting back
to char * (using get()) - but now in ASCII. Or even if get() performs conversion
to UTF8, it seems still overhead for me:
GetShortDescription(desc) - desc is char[]
NS_ConvertUTF8toUCS2 tmpStr(desc) - char[] to UC2[]
mimeInfo->SetDescription(tmpStr.get() - UC2[] to char *

Comment 42

[Boris Zbarsky [:bzbarsky]]

> maybe best solution is something like this:

That's what I was saying, yes.

> then converting back to char * (using get())

.get() on an nsAFlatString (which is what nsAutoString et. al. are) returns a
PRUnichar*, not a char*.

Comment 43

[Sergei Dolgov]

".get() on an nsAFlatString (which is what nsAutoString et. al. are) returns a
PRUnichar*, not a char*."
so mimeInfo->GetShortDescription() provides char *
but 
mimeInfo->SetDescription() accepts PRUnichar * ???

Wonderful. Very consistent.

Also about InitWithPath(), UTF8<->UCS2 etc.

seems all overhead.
Explaining. we converring path from UTF8 to UCS2.
Then passing it to nsLocalFile functions which calls macro, which converts it
using NS_CopyUnicodeToNative to to UTF8 (!!! - yeah, it is hardcoded for BeOS in
nsNativeCharsetUtils) - and passsong it to InitWithnativePath.
What the heck...here and there and back again

Comment 44

[Sergei Dolgov]

Sorry, was wrong about consistency.

But still investgating "Way Of Path"

Comment 45

[Boris Zbarsky [:bzbarsky]]

Yes. The overhead is dumb.  See what I said about GetFileTokenForPath in comment
33 toward the end. We should fix that.  Either as part of this patch or as a
separate bug, whichever you guys prefer.

Comment 46

[Sergei Dolgov]

I have feeling that there are lot of such "two-way route"  overheads in Mozilla
code for BeOS especially, as it is "pure UTF8 OS".
But cleanup maybe so huge work - i recall size of patches for Win32 platform for
case #ifdef MOZ_UNICODE or what it was.

Comment 47

[Sergei Dolgov]

http://lxr.mozilla.org/mozilla/source/uriloader/exthandler/nsExternalHelperAppService.h#90
// GetFileTokenForPath must be implemented by each platform. 
// platformAppPath --> a platform specific path to an application that we got
out of the 
//                     rdf data source. This can be a mac file spec, a unix path
or a windows path depending on the platform
// aFile --> an nsIFile representation of that platform application path.

As you said, unfortunately it is declared above port implementations. Sure,
ideal for BeOS port code is GetFileTokenForPath(const char*
 

Comment 48

[Boris Zbarsky [:bzbarsky]]

> unfortunately it is declared above port implementations.

Yeah... The code in the base class uses it when parsing mimeTypes.rdf.  Feel
free to file a bug on me regarding that; I may be changing that code anyway.

Comment 49

[Sergei Dolgov]

Boris,
as per comments ## 36-42
Tested it again.
Usage of both forms
NS_Convert*to* foo1(foo2); foo3(foo1);
and 
foo3(NS_Convert*to*(foo2));

produces crap on various stages, depending on where i use it - for mime
description, short description or Leaf name.

I know you are Mozilla strings guru, and i cannot pretend to any competence in
this field, but this is how it works (or don't) here.

Seems that life scope is unsufficient,
maybe even this is BeOS-only problem, related to Thread-Per-Window nature of
BeOS applications. Dunno.

Comment 50

[Boris Zbarsky [:bzbarsky]]

> I know you are Mozilla strings guru

You have me confused with scc or dbaron.  This _does_ sound like a BeOS only 
problem, yes.  It works fine on other platforms.  Do what works in the patch, 
add a comment saying why so people don't "fix" it, and file a bug on the String 
component.

Comment 51

[Sergei Dolgov]

Attachment: last version of nsOSHelperAppService.cpp

explicit declarations of ns*String everywhere,
also no more ascii.

Paul, now it is your turn to test.

Comment 52

[Paul]

Comment on attachment 102063 [details]
last version of nsOSHelperAppService.cpp

r=arougthopher

There are two lines that use spaces & tabs for indenting, but, that can be
cleaned up before I check it in, once I get an sr=

Comment 53

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 102063 [details]
last version of nsOSHelperAppService.cpp

sr=bzbarsky if you fix those whitespace things and file a bug on strings about
the issues you were having with inline temporaries (and cc me, ok?  I'm
curious)

Comment 54

[Sergei Dolgov]

Attachment: nsOSHelperAppService.cpp

Rollback. Using form from comment #40.
As said my first computer/electronics guru "There are no wonders, there are
f*ng contacts" ("Chudes ne byvajet, byvajut tolko hujovyje kontakty" -russian
original).
After rebuilng cleanly libnecko.so (nsMIMEInfoImpl is there) - all started to
work as expected.

Comment 55

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 102155 [details]
nsOSHelperAppService.cpp

Heh.  sr=bzbarsky

Comment 56

[Paul]

Comment on attachment 102155 [details]
nsOSHelperAppService.cpp

r=arougthopher

I'll check this in ASAP

Comment 57

[Paul]

this has been checked in

Comment 58

[Sergei Dolgov]

Got recently bug report from user about 25-Oct-2002 nightly build.
Bug is same as in http://bugzilla.mozilla.org/show_bug.cgi?id=145860#c10
So, this string error seems rising again