273524 - Helper applications with parameters don't work ( GTK2/Gnome )

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect, P2)

Description

[RamaSubbu SK]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:1.7.5) Gecko/20041110 Firefox/1.0
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:1.7.5) Gecko/20041110 Firefox/1.0

I have compiled the firefox with GTK2 (--enable-default-toolkit=gtk2). When I
download any download any application from Internet, It is not populating the
default handler properly, whatever I have associated in GNOME.


Reproducible: Always
Steps to Reproduce:
1. Associate a .txt file in gnome with gedit but with a parameter --new-window 
   'gedit --new-window'. By make the default action as custom and 'Program to
Run' gedit --new-window
2. Now, in the firefox try to download a file with .txt extension then the file
handler will not come.


Actual Results:  
file handler list will be empty

Expected Results:  
it should come what ever we have associated in GNOME.

Comment 1

[Wolfgang Rosenauer [:wolfiR]]

seems that nsGNOMERegistry can't deal with parameters in handlers.

Comment 2

[RamaSubbu SK]

Attachment: create new nsMimeInfoGnome class to support the parameter types.

Comment 3

[timeless]

Comment on attachment 168132 [details] [diff] [review]
create new nsMimeInfoGnome class to support the parameter types.

please use cvs diff -uN (use cvsutils/cvsdo add to add files if you don't have
cvs write access)

>+++ mozilla.work/uriloader/exthandler/Makefile.in	2004-12-07 18:16:12.290564280 +0530
> ifeq ($(MOZ_WIDGET_TOOLKIT),gtk2)
>+OSHELPER	+= nsMIMEInfoGnome.cpp
> OSHELPER	+= nsGNOMERegistry.cpp
normally we do
  OSHELPER	+= nsGNOMERegistry.cpp \
		   nsMIMEInfoGnome.cpp \
		   $(NULL)
or something like that

>+++ mozilla.work/uriloader/exthandler/nsMIMEInfoGnome.cpp	2004-12-07 19:08:57.733343832 +0530
>+ * The contents of this file are subject to the Netscape Public License
new files really shouldn't be NPL

>+ * Portions created by the Initial Developer are Copyright (C) 1998
nor should they be (c) 1998

>+nsMIMEInfoGnome::nsMIMEInfoGnome() {
>+    m_parameters=NULL;
>+}
please use:
nsMIMEInfoGnome::nsMIMEInfoGnome()
  : mParameters(nsnull)
{
}

1. nsnull instead of NULL
2. member initialization for constructors instead of assignment
3. mFoo/kFoo/sFoo/gFoo - member, konstant, static, [javascript] global

>+void nsMIMEInfoGnome::SetApplicationParameter( const char * parameter) {
>+    if(parameter==NULL) 
#1, also perhaps just (!parameter)
>+	m_parameters=NULL;
#1
4. you leak mParameters, that's bad :)
5. don't use tabs
>+    else {
>+	g_free(m_parameters);
6. i don't trust random free impls to be null safe, please provide a url
indicating it is
>+	m_parameters = ( char *) g_malloc(strlen(parameter));
7. null check allocations, they can fail
8. since failing to allocate would cause this function to fail, it should
return a result code instead of being void
>+	g_stpcpy(m_parameters,parameter);
>+    }
>+}
>+
>+nsresult 
>+nsMIMEInfoGnome::LaunchWithIProcessP(nsIFile* aApp, nsIFile* aFile) {
i don't like this method name.
>+    NS_ASSERTION(aApp && aFile, "Unexpected null pointer, fix caller");
NS_PRECONDITION?
note that you'll still crash ..., which is ok, although if you're going to
crash why bother with the assertion?
>+    nsCAutoString path;
>+    aApp->GetNativePath(path);
>+    
>+    nsresult rv;
>+    nsCOMPtr<nsIProcess> process = do_CreateInstance(NS_PROCESS_CONTRACTID, &rv);
>+    
>+    if (NS_FAILED(rv))
>+    return rv;
9. indentation is off
>+    
>+    aApp->GetNativePath(path);
why do you keep calling this? you've now called it twice
>+    
>+    if (NS_FAILED(rv = process->Init(aApp)))
>+	return rv;
>+    
>+    aFile->GetNativePath(path);
that's the third assignment to path, the other two are unused. and you aren't
checking for a failure code from GetNativePath
>+    PRUint32 pid;  
>+
>+    if(m_parameters==NULL) { // If there is no parameter, then call run application as normal method.
#1
>+	const char * strPath = path.get();
>+	return process->Run(PR_FALSE, &strPath, 1, &pid);
>+    }
>+    
>+    // Constructs parameter including the file names in a double pointer
>+    // can call the application with that constructed parameter.
>+
this parsing looks pretty careless
what happens if i want to do:
/usr/bin/perl ~timeless/bin/magical.pl -mode "hello world" %1
>+    char ** parameter = g_strsplit( g_strdup_printf("%s %s",m_parameters,path.get())," ", 0);
10. i'm 99% certain you leak g_strdup_printf
#7
>+    int count=0;
>+    while(m_parameters[count++]); //count how number of parameters are there.
>+    nsresult nv= process->Run(PR_FALSE, (const char **)parameter, count, &pid); 
11. use rv, not rv2, not result, not nv, ...
>+    g_strfreev(parameter);
>+    return nv;
>+}

>+++ mozilla.work/uriloader/exthandler/nsMIMEInfoGnome.h	2004-12-07 19:07:54.787912992 +0530

>+      * This is the virutal function must be over loaded.
'that must be'

>+     // This method is to set the extra parameter for application handler file.
/**
 *
 */, please :)
>+     void  SetApplicationParameter(const char *parameter);

>+++ mozilla.work/uriloader/exthandler/unix/nsGNOMERegistry.cpp	2004-12-07 19:36:46.688624064 +0530

>+  gchar **originalCommand = g_strsplit( nativeCommand, " ", 2); 
#7

>+++ mozilla.work/uriloader/exthandler/unix/nsOSHelperAppService.cpp	2004-12-07 18:49:33.079398144 +0530
>+  // Moved this code up, to give first preference to GNOME association.

imo this change should be done as a distinct bug so people can fight about it
without affecting the rest of your changes.

Comment 4

[Wolfgang Rosenauer [:wolfiR]]

The patch in nsOSHelperAppService.cpp is not related to this bugreport, is it?
With this change you give precedence to the GNOME settings. That might be useful
in special circumstances but it's not needed here IMHO.

Comment 5

[timeless]

Comment on attachment 168132 [details] [diff] [review]
create new nsMimeInfoGnome class to support the parameter types.

>+    nsresult nv= process->Run(PR_FALSE, (const char **)parameter, count, &pid); 
while i'm here, why did you have a cast here ^^^ ?

Comment 6

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

Comment on attachment 168132 [details] [diff] [review]
create new nsMimeInfoGnome class to support the parameter types.

firstly, thank you for creating this patch.

note that in the Makefile.in you could put nsMIMEInfoGnome.cpp on the same line
as nsGNOMERegistry.cpp

Why bother with the if in  
 uriloader/exthandler/unix/nsGNOMERegistry.cpp? it seems like you'll always
just pass originalCommand[1] anyway. I don't really see the point of
SetApplicationParameter(NULL) anyway.

there isn't really a need to copy the comments into nsMIMEInfoGnome...

it seems like you are using a really complex way of launching this application.
does glib not provide anything simpler?

I suspect nsMIMEInfoGnome is including way more headers than it needs.

Comment 7

[RamaSubbu SK]

ok thanks for all , for the comments. 
B'coz I got educated a bit from the comments.
Will attach another patch soon, after doing all the changes.

Comment 8

[RamaSubbu SK]

I'm done with changes, for those two new files what PL ( Public License ) I have
to add like NPL,MPL,GPL,...xPL 

Comment 9

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

http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c

Comment 10

[RamaSubbu SK]

the change in this I made /uriloader/exthandler/unix/nsOSHelperAppService.cpp
not only for the presidence to GTK2. The following is the original code.

  LookUpExtensionsAndDescription(majorType,
                                 minorType,
                                 extensions,
                                 mime_types_description);


  nsAutoString mailcap_description, handler, mozillaFlags;
  DoLookUpHandlerAndDescription(majorType,
                                minorType,
                                typeOptions,
                                handler,
                                mailcap_description,
                                mozillaFlags,
                                PR_TRUE);


  if (handler.IsEmpty() && extensions.IsEmpty() &&
      mailcap_description.IsEmpty() && mime_types_description.IsEmpty()) {
    // No useful data yet

#ifdef MOZ_WIDGET_GTK2
    LOG(("Looking in GNOME registry\n"));
    nsMIMEInfoBase *gnomeInfo = nsGNOMERegistry::GetFromType(aMIMEType).get();
    if (gnomeInfo) {
      LOG(("Got MIMEInfo from GNOME registry\n"));
      return gnomeInfo;
    }
#endif
}

In the above code, for the .gz extension file type,
LookUpExtensionsAndDescription function give only extension and the mime
description will be empty. Then the if condition fails, hence the file
association is not populating properly. That is why I moved up the code.

Comment 11

[RamaSubbu SK]

Attachment: patch -p1: Again attaching the patch with all the recommended changes. thanks

Comment 12

[timeless]

Comment on attachment 168304 [details] [diff] [review]
patch -p1: Again attaching the patch with all the recommended changes. thanks

>--- mozilla/uriloader/exthandler/nsMIMEInfoGnome.cpp	1970-01-01 05:30:00.000000000 +0530
>+	mParameters = ( char *) g_malloc(strlen(parameter));
>+	if(!mParameters)
this is backwards!
if the allocation fails, you'll do the strcpy, if it fails, you'll crash!
>+	    g_stpcpy(mParameters,parameter);
>+	else
>+	    return NS_ERROR_OUT_OF_MEMORY;
don't ^^ use tabs :)

instead, write it this way:
>+  if(!mParameters)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  g_stpcpy(mParameters,parameter);

please be consistent about indentation, either use 4space or 2space, don't do
4-2 space

note that i don't use an else after the return.

>+    if (NS_FAILED(rv))
>+    return rv;
      ^ not indented

you don't check this for failure:
>+    aFile->GetNativePath(path);
>+    PRUint32 pid;  
>+
>+    if(mParameters==NULL) { // If there is no parameter, then call run application as normal method.
this is evil:
>+	const char * strPath = path.get();
>+	return process->Run(PR_FALSE, &strPath, 1, &pid);

if you needed to pass a single argument, you should have done:
	const char args[] = { path.get() };
	return process->Run(PR_FALSE, args, 1, &pid);

but, in this case you should just do:
	return process->Run(PR_FALSE, nsnull, 0, &pid);

>+    gchar * singleString = g_strdup_printf("%s %s",mParameters,path.get()); 
you still didn't check to see if this ^ fails...

>+    char ** parameter = g_strsplit( singleString," ", 0);
or this ^...
		       ^^^
>+    g_free(singleString);
>+    int count=0;
	      ^^^
>+    while(parameter[count++]); //count how number of parameters are there.
>+    rv= process->Run(PR_FALSE, (const char **)parameter, count, &pid); 
       ^^^
please be consistent about spaces around operators (=)

>+    g_strfreev(parameter);
aren't you leaking singleString?
>+    return rv;

>+nsMIMEInfoGnome::LaunchDefaultWithFile(nsIFile* aFile)
>+{
>+  
^ empty line ?
>+  if (!mDefaultApplication)
>+    return NS_ERROR_FILE_NOT_FOUND;
NS_ERROR_NOT_INITIALIZED ?

>+nsMIMEInfoGnome::~nsMIMEInfoGnome()
>+{
>+    if(!mParameters)
	 ^ logic reversed
>+	g_free(mParameters);

>--- mozilla/uriloader/exthandler/nsMIMEInfoGnome.h	1970-01-01 05:30:00.000000000 +0530
>+class nsMIMEInfoGnome : public nsMIMEInfoImpl
>+{
>+ public:
>+  private :
   ^	 ^ ^ please be consistent about whitespace :)
			  v
>+      char * mParameters ; // a member variable to store the extra parameters.

>--- mozilla/uriloader/exthandler/unix/nsGNOMERegistry.cpp	2004-03-06 20:11:00.000000000 +0530
>+  gchar **originalCommand = g_strsplit( nativeCommand, " ", 2); 
check for failure?
>+ 
>   g_free(nativeCommand);
> 
>+  gchar *commandPath = g_find_program_in_path(originalCommand[0]);
>+
>   if (!commandPath) {
>     _gnome_vfs_mime_application_free(handlerApp);
don't leak things
>     return nsnull;
>   }
> 
>+  if(originalCommand[1]!=nsnull) { // Storing parameter in the mimeGnomeInfo.
>+    if(mimeInfo->SetApplicationParameter(originalCommand[1])!=NS_OK)
don't leak things
>+      return nsnull;
>+  }
>+  else 
>+    mimeInfo->SetApplicationParameter(NULL);
>+
>+  g_strfreev(originalCommand); //Freeing the allocated memory

>--- mozilla/uriloader/exthandler/unix/nsOSHelperAppService.cpp	2004-10-20 19:02:38.000000000 +0530
i'd still rather you did this in a different bug.

Comment 13

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

OK,
gnomevfs provides this nice method:
http://developer.gnome.org/doc/API/2.0/gnome-vfs-2.0/gnome-vfs-20-gnome-vfs-mime-database.html#gnome-vfs-mime-application-launch
it would simplify this code a lot.

Can we require gnomevfs 2.4? or is that too new?

Comment 14

[RamaSubbu SK]

Attachment: CVS diff ( cvsdo )

Attaching another patch with all timeless, comments and sugesstion implemented.

Timeless: Thanks for the comments.
and the changes in nsOSHelperAppService.cpp has been moved out.

Comment 15

[RamaSubbu SK]

Comment on attachment 168304 [details] [diff] [review]
patch -p1: Again attaching the patch with all the recommended changes. thanks

it has lot of memory leaks :(

Comment 16

[Marco Pesenti Gritti]

I havent looked at the patch in detail, I just wanted to add some comments about
the native part of it. I think there is no way to make this work in a
satisfactory way on GNOME >= 2.4 without depending on gnome-vfs 2.4. We had a
complete backend replacement and part of the apis have been deprecated. The
deprecated api is sort of working with the new backend but it was not possible
to keep full compatibility.

So I'd suggest to depend on gnome-vfs 2.4 and use the
gnome_vfs_mime_application_launch API. This will work well with both the old and
the new backend. There is really no way to use the old api without introducing
bugs :(

Comment 17

[RamaSubbu SK]

Yeah!! you have valid reason. ok.. I will add another patch soon with the
gnome-vfs 2.4 apis. Thanks

(In reply to comment #16)
> I havent looked at the patch in detail, I just wanted to add some comments about
> the native part of it. I think there is no way to make this work in a
> satisfactory way on GNOME >= 2.4 without depending on gnome-vfs 2.4. We had a
> complete backend replacement and part of the apis have been deprecated. The
> deprecated api is sort of working with the new backend but it was not possible
> to keep full compatibility.
> 
> So I'd suggest to depend on gnome-vfs 2.4 and use the
> gnome_vfs_mime_application_launch API. This will work well with both the old and
> the new backend. There is really no way to use the old api without introducing
> bugs :(

Comment 18

[RamaSubbu SK]

Sorry!! for late reply. I was into some other issue. Will invest more time in this.

Marco :
Can you pin-point me, which is deprecated api, I'm very much new to GNOME-VFS
and as well mozilla code. It will be easy for me to fix quickly. 


(In reply to comment #16)
> I havent looked at the patch in detail, I just wanted to add some comments about
> the native part of it. I think there is no way to make this work in a
> satisfactory way on GNOME >= 2.4 without depending on gnome-vfs 2.4. We had a
> complete backend replacement and part of the apis have been deprecated. The
> deprecated api is sort of working with the new backend but it was not possible
> to keep full compatibility.
> 

Comment 19

[Marco Pesenti Gritti]

Here is the non deprecated API:

http://cvs.gnome.org/viewcvs/gnome-vfs/libgnomevfs/gnome-vfs-mime-handlers.h?rev=1.44&view=markup

You can also see which part of the GnomeVFSApplication has been deprecated.

Here is the stuff that has been deprecated:

http://cvs.gnome.org/viewcvs/gnome-vfs/libgnomevfs/gnome-vfs-mime-deprecated.h?rev=1.2&view=markup

The problem with mozilla code is that it's using the ->command field of the
GnomeVFSMimeApplication structure. It's deprecated and only partially compatible
with the new database.

The only way I can think to make this work with gnome-vfs >= 2.4 (so with both
the new and the old database) is to use gnome_vfs_mime_application_launch. You
will need a way to pass the GnomeVFSMimeApplication structure to the launching
code though.

Feel free to ask, if I'm not being clear enough.

Comment 20

[Wolfgang Rosenauer [:wolfiR]]

rsubbu, what the status with this? Any news or would you like to get your last
patch reviewed?

Comment 21

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch, uses newer GNOME APIs

I independently came up with the solution outlined by marco in comment 19
before noticing this.  Anyway, here's the work I've done for Fedora to make
this work properly.

Comment 22

[Boris Zbarsky [:bzbarsky]]

So is that patch ready for review?  If so, you may want to ask biesi and bryner
to review it...

Comment 23

[Wolfgang Rosenauer [:wolfiR]]

Chris, we should get this included. But I do repeat my comment #4 for your patch ;-)

Would you please take this one out and ask for review?

Comment 24

[Walter Meinl]

Attachment: patch corrected for the trunk after checkin of bug 183156

after the checkin for https://bugzilla.mozilla.org/show_bug.cgi?id=183156 the patch causes a compilation error for the trunk due to the line
+      mDefaultAppDescription = NS_ConvertUTF8toUCS2(name);
I've corrected it to
+      mDefaultAppDescription = NS_ConvertUTF8toUTF16(name);
and removed the patch for nsOSHelperAppService.cpp as requested in comment #4
Hoping that this is not taken as an offense but as a chance to revive this bug

Comment 25

[Wolfgang Rosenauer [:wolfiR]]

Comment on attachment 210748 [details] [diff] [review]
patch corrected for the trunk after checkin of bug 183156

per Boris' comment, requesting review from bryner
Walter, I guess the patch is tested already by you?

Comment 26

[Walter Meinl]

(In reply to comment #25)
> (From update of attachment 210748 [details] [diff] [review] [edit])
> per Boris' comment, requesting review from bryner
> Walter, I guess the patch is tested already by you?
> 

Yes, actually running trunk with that patch
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060205 Firefox/1.6a1
As from comment 21 Christopher uses the patch https://bugzilla.mozilla.org/attachment.cgi?id=185501 in fedora for firefox from 1.1 versions on. Gentoo uses it also even for aviary. I could convince them to use it for 1.5 omitting the nsOSHelperAppService.cpp parts as you suggested. So it should be thoroughly tested for several versions and platforms.

Comment 27

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

(In reply to comment #24)
> I've corrected it to
> +      mDefaultAppDescription = NS_ConvertUTF8toUTF16(name);

That should actually be:
  CopyUTF8toUTF16(name, mDefaultAppDescription);

Comment 28

[Walter Meinl]

Attachment: patch for the trunk updated to biesies comment

updated the patch to Christians last comment and to comment #4. Works as expected here. As Wolfgang suggested I'll ask bryner for review.

Comment 29

[Brian Ryner (not reading)]

Comment on attachment 220285 [details] [diff] [review]
patch for the trunk updated to biesies comment

Looks good.

Comment 30

[Walter Meinl]

(In reply to comment #29)
> (From update of attachment 220285 [details] [diff] [review] [edit])
> Looks good.
> 

Fine, anybody here able to check it in, i don't have the right to do so, thanks

Comment 31

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

why duplicate the function pointer getting in nsGNOMERegistry and here?

+    gchar *uri = _gnome_vfs_make_uri_from_input(nativePath.get());
+
+    GList *uris = NULL;
+    uris = g_list_append(uris, uri);

why not merge the two uris lines, like the uri one?

given this change:
-typedef struct {
+struct GnomeVFSMimeApplication {

why does nsMIMEInfoUnix.h have things like:
+typedef enum {
+  GNOME_VFS_OK // there's more but we don't care about them.
+} GnomeVFSResult;

shouldn't this mime info class be called nsMIMEInfoGnome?

(And why map all errors to the same NS_ERROR_FAILURE?)

Comment 32

[Mike Hommey [:glandium]]

A huge part of the nsMIMEInfoUnix.cpp file seems useless since it is
already done in nsGNOMERegistry.cpp...

It would also be interesting to see if it's not "simply" possible to put
all that in the mozgnome component. It wouldn't need all the hackish
dlopen thing.

Comment 33

[Mike Hommey [:glandium]]

Actually, the nsGNOMERegistry could be rewritten to use the nsGConfService and the nsGnomeVFSService provided in the mozgnome component. That would require some additions to nsGnomeVFSService, but all in all it would make the code cleaner, and the dynamic loading of libraries useless.

Comment 34

[Mike Hommey [:glandium]]

Attachment: fixes issues with nsIMIMEService.getFromTypeAndExtension

Attachment #220285 [details] [diff] leads to bugs such as http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=414008 and bug #370386, which can be fixed by replacing it by this new patch.

I'm also working on reimplementing the gnome mime stuff following my advices from comment #32 and #33, which is not complete yet, but works as expected, except I don't know the "supported way" to force the headers to be generated from toolkit/components/gnome (a make export there is needed before building in uriloader/exthandler, but it doesn't happen due to uriloader/exthandler being tier_9 and toolkit/components/gnome being tier_50 ; i wonder if i should just make -C $(DEPTH)/toolkit/components/gnome export conditionally in the rules in uriloader/exthandler)

Comment 35

[Boris Zbarsky [:bzbarsky]]

Note that I object to the change in ordering wrt mailcap as it's done in this patch.  A user's _private_ mailcap data (from ~/.mailcap) should override the _system_default_ GNOME registry (but not the user's local changes to the registry).  That's not what the code does now, but that's what it _should_ do instead of what it does now or what this patch is doing.  This keeps coming up every time someone touches this code, and I'm tired of having to point this out.  So this patch is not acceptable as is, no matter what the parts that are trying to fix this bug look like.

This is also why we do separate patches for separate issues, of course...  I realize the earlier patches in this bug have the same problem as well, and it should have been caught by reviews back then.

Comment 36

[Mike Hommey [:glandium]]

AFAIK, Gnome doesn't use mailcap entries *at all*. So basically, that pretty much depends on how integrated with gnome you want to be... looking-up in mailcap first is definitely not doing the gnome way.

Comment 37

[Boris Zbarsky [:bzbarsky]]

I care more about things working the way the user wants than conforming to The GNOME Way.  Users who just started using Linux yesterday won't have a ~/.mailcap so won't be affected, and there's no reason to break things that have worked for users for years if they set up a ~/.mailcap at some point (e.g. by running Netscape 4 and setting up helper applications, which is not an uncommon state of being for long-time Linux users from what I've seen).

Comment 38

[Mike Hommey [:glandium]]

if you care so much about mailcap "working", how come bug #83305 is not fixed yet ?

Comment 39

[Boris Zbarsky [:bzbarsky]]

Because I've had higher priority work items and because it's working in 80% or better of cases.  And quite frankly, because that bug doesn't affect me, nor anyone I know personally; if it did I'd probably put more effort into it.

Comment 40

[Mike Hommey [:glandium]]

Personally, I know a lot of persons who got "less" as choice for text/plain files, because it's the first entry in /etc/mailcap, and though it has a needsterminal in the entry, it is not launched this way. Putting gnome first did indeed fix this. Anyways, this has nothing to do with helper applications. Do you know if there is a bug about putting gnome first ?

Comment 41

[Boris Zbarsky [:bzbarsky]]

Not without doing a bugzilla search, no.

Comment 42

[Boris Zbarsky [:bzbarsky]]

> That's not what the code does now,

Ignore that.  The code does do this, as of bug 221042.  Which means that GNOME settings take precedence over /etc/mailcap.

Comment 43

[Mike Hommey [:glandium]]

hum, without the reordering, /etc/mailcap takes precedence over gnome at the moment, on 1.8.0 branch at least.

Comment 44

[Boris Zbarsky [:bzbarsky]]

Odd.  It really shouldn't.  Please file a bug on me and attach the results of an NSPR_LOG_MODULES=HelperAppService:5 log of a minimal-ish browsing session that shows the problem?  I'll look into it when I get back.

Comment 46

[Phil Ringnalda (:philor)]

Comment on attachment 258000 [details] [diff] [review]
fixes issues with nsIMIMEService.getFromTypeAndExtension

bryner's pretty much out of the review business, but biesi said he'd take the review.

Comment 47

[Wolfgang Rosenauer [:wolfiR]]

So how about driving this a bit forward?
Mike Hommey, do you have any objections on the patch from
https://bugzilla.mozilla.org/attachment.cgi?id=220285 ?
It already has r+ from bryner, seems to work correctly and is almost identical to your patch but has no change in the odering for app resolution.
If you agree I would ask for sr on that one and we can discuss and maybe change the ordering in another bug. (Do we already have a bug as asked in comment #44?)
Let get this bug resolved now and handle the rest elsewhere.

Comment 48

[Mike Hommey [:glandium]]

(In reply to comment #47)
> So how about driving this a bit forward?
> Mike Hommey, do you have any objections on the patch from
> https://bugzilla.mozilla.org/attachment.cgi?id=220285 ?
> It already has r+ from bryner, seems to work correctly and is almost identical
> to your patch but has no change in the odering for app resolution.

I have no objection, except what I said in comments #32 and #33. A first step in this way is in bug #373397, and the patch for this bug could be rewritten much simpler (but with additions to the mozgnome component) following that. Note I already have code for this too.
I would also object because of the ordering problem being handled elsewhere, see below.

> If you agree I would ask for sr on that one and we can discuss and maybe change
> the ordering in another bug. (Do we already have a bug as asked in comment
> #44?)

That's bug #373955, which already has a patch and is likely to be landed first. The patch here would need update.

Comment 49

[Wolfgang Rosenauer [:wolfiR]]

I was just about requesting sr and approval for 1.9 but figured that bug 385065 clashes the r+ patch here.
Anyone time to get https://bugzilla.mozilla.org/attachment.cgi?id=220285 ported to current trunk?

Comment 50

[Reed Loden [:reed]]

Mike, think you could update the patch against current trunk?

Comment 51

[Mike Hommey [:glandium]]

I'd prefer to know what is going to happen regarding bug #373397 before updating the patch here.

Comment 52

[Phil Ringnalda (:philor)]

Comment on attachment 258000 [details] [diff] [review]
fixes issues with nsIMIMEService.getFromTypeAndExtension

Well, apparently biesi didn't actually *mean* he would review it, and I'm getting tired of seeing the request in my request queue, and I can't even follow the discussion of which patch in which bug that's doing approximately the same thing should be the one true patch, so... good luck!

Comment 53

[Reed Loden [:reed]]

Comment on attachment 258000 [details] [diff] [review]
fixes issues with nsIMIMEService.getFromTypeAndExtension

chpe / biesi: Can you please try to review this soon?

Comment 54

[Christopher Aillon (sabbatical, not receiving bugmail)]

Nominating for blocking.

<blizzard> caillon: nominate?
<blizzard> caillon: I'm not in the blocker/non-blocker path anymore
<blizzard> caillon: but if right now ff3 doesn't do the right thing when you click on a file, it should get fixed

Comment 55

[Christopher Blizzard (:blizzard)]

Talking with Beltzner a little bit on this.  Not sure if it's actually blocking, but it sure should be resolved by FF3.  Most people who are using Linux have gotten FF via their distro and it sounds like most distros to date have carried some version of this patch and we should get something in that makes this work.

Comment 56

[Mike Hommey [:glandium]]

Attachment: Much simpler patch, part 1

This is a new implementation of the fix, much simpler, and using the gnomevfs component. This patch doesn't depend on bug #373397.

Comment 57

[Mike Hommey [:glandium]]

Attachment: Much simpler patch, part 2

This complements attachment #299314 [details] [diff] [review]. This patch is against trunk and doesn't depend on bug #373397.

Comment 58

[Mike Hommey [:glandium]]

Attachment: Much simpler patch, part 2, after bug #373397

Same as attachment #299315 [details] [diff] [review], except it is to be applied after patch from bug #373397, that removes the _gnome_vfs_mime_application_free calls.

Comment 59

[Mike Schroepfer]

Yes - let's try and get this resolved one way or the other

Comment 60

[Walter Meinl]

Comment on attachment 220285 [details] [diff] [review]
patch for the trunk updated to biesies comment

marking this as obsolete as it doesn't apply anymore and Mike's patches will solve the problem

Comment 61

[Reed Loden [:reed]]

Comment on attachment 299315 [details] [diff] [review]
Much simpler patch, part 2

Does that mean we don't need this patch anymore?

Comment 62

[Mike Hommey [:glandium]]

Comment on attachment 299315 [details] [diff] [review]
Much simpler patch, part 2

> Does that mean we don't need this patch anymore?

Since the patch for bug #373397 has landed, yes.

Comment 63

[Mike Hommey [:glandium]]

Are there any chances to see this landed before beta 3 ?

Comment 64

[Reed Loden [:reed]]

(In reply to comment #63)
> Are there any chances to see this landed before beta 3 ?

Not beta 3, no. Beta 3 builds were made early last week and are scheduled for release this Tuesday. If chpe and/or biesi can review this soon, it's possible to get this in for beta 4, though.

Comment 65

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

Comment on attachment 299316 [details] [diff] [review]
Much simpler patch, part 2, after bug #373397

I don't think we should just ignore the commandline arguments

Comment 66

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

Comment on attachment 299316 [details] [diff] [review]
Much simpler patch, part 2, after bug #373397

oh... misread. next time could you just make one patch file for the two parts?

Comment 67

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

Comment on attachment 299314 [details] [diff] [review]
Much simpler patch, part 1

+    nsIGnomeVFSMimeApp *app;
+    if (NS_SUCCEEDED(vfs->GetAppForMimeType(mType, &app)) && app)
+      return vfs->ShowURIForInput(nativePath);

this leaks the app, use an nsCOMPtr for it

Comment 68

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

Comment on attachment 299316 [details] [diff] [review]
Much simpler patch, part 2, after bug #373397

So, why is this patch needed? Wouldn't it be better to implement GetHasDefaultHandler on nsMIMEInfoUnix and make it return true if a gnome-vfs handler exists?

Comment 69

[Mike Hommey [:glandium]]

(In reply to comment #68)
> (From update of attachment 299316 [details] [diff] [review])
> So, why is this patch needed? Wouldn't it be better to implement
> GetHasDefaultHandler on nsMIMEInfoUnix and make it return true if a gnome-vfs
> handler exists?

It would, but the value would still be necessary to feed SetDefaultDescription, though I really don't get what is supposed to be the value for that. I also don't get what is supposed to be the difference between the default description and the description...

Comment 70

[Mike Hommey [:glandium]]

Oh I got it, the variable name just sucks... default description is default *application* description, which description is *mime type* description.

Comment 71

[Mike Hommey [:glandium]]

And I have eyes full of crap, the command is not used for the default description.

Comment 72

[Mike Hommey [:glandium]]

Attachment: Patch

Following comments #67 and #68.

Comment 73

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 305694 [details] [diff] [review]
Patch

a=beltzner

Comment 74

[Reed Loden [:reed]]

Checking in uriloader/exthandler/unix/nsGNOMERegistry.cpp;
/cvsroot/mozilla/uriloader/exthandler/unix/nsGNOMERegistry.cpp,v  <--  nsGNOMERegistry.cpp
new revision: 1.19; previous revision: 1.18
done
Checking in uriloader/exthandler/unix/nsMIMEInfoUnix.cpp;
/cvsroot/mozilla/uriloader/exthandler/unix/nsMIMEInfoUnix.cpp,v  <--  nsMIMEInfoUnix.cpp
new revision: 1.3; previous revision: 1.2
done
Checking in uriloader/exthandler/unix/nsMIMEInfoUnix.h;
/cvsroot/mozilla/uriloader/exthandler/unix/nsMIMEInfoUnix.h,v  <--  nsMIMEInfoUnix.h
new revision: 1.3; previous revision: 1.2
done

Comment 75

[Wolfgang Rosenauer [:wolfiR]]

If I read the code correctly this caused bug 421879