455509 - Implement system integration via dbus (open_mime) in Fennec

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Doug Turner (:dougt)]

On Maemo devices, application developers can setup a "install file" so that they can be added to the application catalog on device could via a "single click".  Basically the user goes to a website, they see a "Click to Install" button.  When they click, the browser redirects to a file that ends in .install (mime type?).  The browser passes this to the application catalog, and the user is allowed to install the software.

Comment 1

[Antonio Gomes (tonikitoo)]

Attachment: fennec install file

stuart pointed ftp://ftp.mozilla.org/pub/mozilla.org/mobile/dists/chinook/release/binary-armel/ as the place where fennec releases are going to live in. So I've made a fennec.install file accordingly to pick debs from that directory (attachment). As packages.gz living there have not being uploaded since m7, that is what is going to be installable.

I've also uploaded it to maemo.org/downloads, so a temporary .install file can be accessible by https://maemo.org/downloads/product/raw/OS2008/mozilla-fennec?get_installfile

please see https://maemo.org/downloads/product/OS2008/mozilla-fennec/

Comment 2

[timeless]

Comment on attachment 339009 [details]
fennec install file

use http:, not ftp:

Comment 3

[Doug Turner (:dougt)]

this bug was more about allowing/enabling fennec to handle *.install files.

Comment 4

[Antonio Gomes (tonikitoo)]

(In reply to comment #2)
> (From update of attachment 339009 [details])
> use http:, not ftp:

josh , why ? afaiu their .deb's are going to live in ftp.

(In reply to comment #3)
> this bug was more about allowing/enabling fennec to handle *.install files.

dougt, it is enabled whenever .deb's and packages.gz are are properly created.

Comment 5

[Stuart Parmenter]

(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 339009 [details] [details])
> > use http:, not ftp:
> 
> josh , why ? afaiu their .deb's are going to live in ftp.
> 

http:// is a mirror network, ftp:// is a single box

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

File is live:
ftp://ftp.mozilla.org/pub/mobile/mozilla-fennec.install

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

as Doug said, this bug was about the browser handling the .install file.  I suspect this was taken care of by the libhildonmime though, so I'm not going to reopen.

Comment 8

[Antonio Gomes (tonikitoo)]

it opens download dialog, but does not integrate as doug described (like microb does, for example). I will take a look.

Comment 9

[Antonio Gomes (tonikitoo)]

Attachment: make fennec to ask dbus for a default handler for an unsupported file type.

Patch makes possible to the query maemo system for candidate applications to open a spefic file (application registered to handle a given mime type), and call this candidate app through DBus passing a "mime_open" message (default in the platform for such circumstances).

Comment 10

[Antonio Gomes (tonikitoo)]

Attachment: sshot of the result of patch 0.1

Comment 11

[Antonio Gomes (tonikitoo)]

Attachment: sshot of the result of patch 0.1 w/ third part apps that implement mime_open

for mfinkle to like the fix :)

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 367499 [details] [diff] [review]
make fennec to ask dbus for a default handler for an unsupported file type.

>+#if defined(MOZ_PLATFORM_HILDON)
>+  nsCAutoString realName (dgettext("maemo-af-desktop", PromiseFlatCString(name).get()));
>+  mimeInfo->SetDefaultDescription(NS_ConvertUTF8toUTF16(realName));
>+
>+#else
>   mimeInfo->SetDefaultDescription(NS_ConvertUTF8toUTF16(name));
>+#endif


as discussed on irc, please confirm that the api has different behavior on maemo than desktop and add a comment describing the difference

otherwise, this looks good.  Be sure to get a review from a peer though.



Also, as discussed on irc, perhaps its time to add this sort of functionality to the nsDBusService.

Comment 13

[Antonio Gomes (tonikitoo)]

Attachment: v0.2 - make fennec to ask dbus for a default handler for an unsupported file type

same patch as previous, but added explicative comments as per blassey suggestion.

Comment 14

[Benjamin Smedberg]

Comment on attachment 368047 [details] [diff] [review]
v0.2 - make fennec to ask dbus for a default handler for an unsupported file type

Not my area. This kind of code is pretty security-sensitive, perhaps this should have a security review?

Comment 15

[Boris Zbarsky [:bzbarsky]]

The comments say that errors mean falling back to gnome-vfs, but the code just returns on errors.

Can you point me to the docs for hildon_mime_open_file_with_mime_type?

Comment 16

[Shawn Wilsher :sdwilsh]

Comment on attachment 368047 [details] [diff] [review]
v0.2 - make fennec to ask dbus for a default handler for an unsupported file type

>+#ifdef MOZ_PLATFORM_HILDON
>+  // on maemo/hildon gnome_vfs_mime_application_get_name function returns
>+  // a localized name for the app. We pass that and the default maemo domain-name
>+  // to dgettext as an attempt to translate a text string into the userâs native
Looks like bugzilla fails at encoding this, but could you just use ' instead please?

>+  nsCAutoString realName (dgettext("maemo-af-desktop", PromiseFlatCString(name).get()));
So, "maemo-af-desktop" is a magic string that doesn't really tell me anything, and the comment above this block doesn't help at all.

In fact, this comment doesn't make much sense to me in general.  If GetName returns a localized name, why do you need to get another localized name with dgettext?

>+#ifdef MOZ_PLATFORM_HILDON
>+  // hildon/maemo applications usually offer a default named dbus method
>+  // (called 'mime_open') available for others to invoke it, so we first
>+  // try to launch the default handler for current file/mime through
>+  // dbug via |mime_open| and fallback to gnome_vfs if it does not work
typo?  dbus?

>+  // for some reason.
>+  DBusError err;
>+  dbus_error_init(&err);
>+
>+  DBusConnection *connection = dbus_bus_get(DBUS_BUS_SESSION, &err);
>+  if (dbus_error_is_set(&err)) {
>+    dbus_error_free(&err);
>+    return NS_ERROR_FAILURE;
>+  }
Your code appears to fail and not fallback like the comment says.

>+  if (nsnull == connection)
>+    return NS_ERROR_FAILURE;
ditto

Comment 17

[Antonio Gomes (tonikitoo)]

Attachment: v0.3 - make fennec to ask dbus for a default handler for an unsupported file type

v0.3



@sdwilsh:
> >+  // to dgettext as an attempt to translate a text string into the userâs native
> Looks like bugzilla fails at encoding this, but could you just use ' instead
> please?

done.

> >+  nsCAutoString realName (dgettext("maemo-af-desktop", PromiseFlatCString(name).get()));
> So, "maemo-af-desktop" is a magic string that doesn't really tell me anything,
> and the comment above this block doesn't help at all.

as per |man pages| specify the .po and .mo files that dgettext will look at maps the untranslated string to a "user string". Having that said, I am using 'maemo-af-desktop' according to (see below)

/home/user # find / -name maemo-af-desktop.mo
/usr/share/locale/da_DK/LC_MESSAGES/maemo-af-desktop.mo
/usr/share/locale/de_DE/LC_MESSAGES/maemo-af-desktop.mo
/usr/share/locale/en_GB/LC_MESSAGES/maemo-af-desktop.mo
/usr/share/locale/en_US/LC_MESSAGES/maemo-af-desktop.mo
/usr/share/locale/es_ES/LC_MESSAGES/maemo-af-desktop.mo
/usr/share/locale/es_MX/LC_MESSAGES/maemo-af-desktop.mo
/usr/share/locale/fi_FI/LC_MESSAGES/maemo-af-desktop.mo
/usr/share/locale/fr_CA/LC_MESSAGES/maemo-af-desktop.mo
/usr/share/locale/fr_FR/LC_MESSAGES/maemo-af-desktop.mo
/usr/share/locale/it_IT/LC_MESSAGES/maemo-af-desktop.mo
/usr/share/locale/nl_NL/LC_MESSAGES/maemo-af-desktop.mo
/usr/share/locale/no_NO/LC_MESSAGES/maemo-af-desktop.mo
/usr/share/locale/pt_BR/LC_MESSAGES/maemo-af-desktop.mo
/usr/share/locale/pt_PT/LC_MESSAGES/maemo-af-desktop.mo
/usr/share/locale/ru_RU/LC_MESSAGES/maemo-af-desktop.mo
/usr/share/locale/sv_SE/LC_MESSAGES/maemo-af-desktop.mo


> In fact, this comment doesn't make much sense to me in general.  If GetName
> returns a localized name, why do you need to get another localized name with
> dgettext?

ohh, my bad. I mis-wrote the comment. On gnome_vfs desktop GetName returns a localized string, but not on maemo. It is like:
* on desktop -> GetName would return "Application Manager"
* on maemo -> GetName would return "maemo_app_manager"

> >+#ifdef MOZ_PLATFORM_HILDON
> >+  // hildon/maemo applications usually offer a default named dbus method
> >+  // (called 'mime_open') available for others to invoke it, so we first
> >+  // try to launch the default handler for current file/mime through
> >+  // dbug via |mime_open| and fallback to gnome_vfs if it does not work
> typo?  dbus?

fixed.

> >+  // for some reason.
> >+  DBusError err;
> >+  dbus_error_init(&err);
> >+
> >+  DBusConnection *connection = dbus_bus_get(DBUS_BUS_SESSION, &err);
> >+  if (dbus_error_is_set(&err)) {
> >+    dbus_error_free(&err);
> >+    return NS_ERROR_FAILURE;
> >+  }
> Your code appears to fail and not fallback like the comment says.

fixed.

> >+  if (nsnull == connection)
> >+    return NS_ERROR_FAILURE;
> ditto

fixed.

@bz:

> Can you point me to the docs for hildon_mime_open_file_with_mime_type?

see comment in the method header:

* http://timeless.justdave.net/mxr-test/chinook/source/libhildonmime-1.10.0/libhildonmime/hildon-mime-open.c#274

it calls

* http://timeless.justdave.net/mxr-test/chinook/source/libhildonmime-1.10.0/libhildonmime/hildon-mime-open.c#274

that invokes dbus mime_open method.

Comment 18

[Shawn Wilsher :sdwilsh]

Comment on attachment 368270 [details] [diff] [review]
v0.3 - make fennec to ask dbus for a default handler for an unsupported file type   

>   nsCAutoString name;
>   handlerApp->GetName(name);
>+#ifdef MOZ_PLATFORM_HILDON
>+  // on maemo/hildon gnome_vfs_mime_application_get_name function returns
>+  // a non-localized name for the app. We pass that as well as the default 
>+  // maemo domain-name to dgettext as an attempt to translate a text string 
>+  // into the user native language.
So, I don't really like this comment.  You mention gnome_vfs_mime_application_get_name, but it's not clear that we call that in this method at all.  Additionally, it doesn't really return a non-localized name, but rather a message id.  I think you should really say something like this:
On Maemo/Hildon, GetName ends up calling gnome_vfs_mime_application_get_name, which happens to return a non-localized message-id for the application.  To get the localized name for the application, we have to call dgettext with the default maemo domain-name to try and translate the string into the operating system's native language.

But, I guess the real question is why aren't you setting the proper name in nsMIMEInfoUnix::GetPossibleApplicationHandlers where the name is set?  That way anyone who ends up getting ahold of this application via public API's gets the right name.

>+  nsCAutoString realName (dgettext("maemo-af-desktop", PromiseFlatCString(name).get()));
regardless, I want maemo-af-desktop pulled out into a constant to make this more clear

Is it possible to write a test for this?

> nsresult
> nsMIMEInfoUnix::LaunchDefaultWithFile(nsIFile *aFile)
> {
>   nsCAutoString nativePath;
>   aFile->GetNativePath(nativePath);
>+
>+#ifdef MOZ_PLATFORM_HILDON
>+  // Before trying to launch the associated default handler for the given
>+  // mime/file through gnome_vfs, lets try to invoke the it via hildon 
>+  // specific APIs (in this case hildon_mime_open_file* which are essecially 
typo: essentially

>+  if (dbus_error_is_set(&err)) {
>+    dbus_error_free(&err);
>+    goto fallback;
I don't like using goto's here.  Can we pull this code out into a new method and just have it return?  The code would be a bit cleaner and easier to follow then...

>+  dbus_connection_set_exit_on_disconnect(connection, false);
A comment explaining this would be nice.  Based on the documentation, it sounds like either we'd have _exit called on us, or the other application, when we disconnect, but it's not totally clear what is going on.

>+  result = hildon_mime_open_file_with_mime_type(connection,
>+                                                PromiseFlatCString(nativePath).get(),
>+                                                mType.get());
>+  if (result == 1)
Oh yay, their success code is simply one.  Can you make a constant (something like HILDON_SUCCESS) and use that instead please

Comment 19

[Boris Zbarsky [:bzbarsky]]

> that invokes dbus mime_open method.

Is that guaranteed to dispatch on MIME type and not on things like filename?

Comment 20

[Antonio Gomes (tonikitoo)]

Attachment: v0.4 - make fennec to ask dbus for a default handler for an unsupported file type

thanks for reviewing. next round:

> >   nsCAutoString name;
> >   handlerApp->GetName(name);
> >+#ifdef MOZ_PLATFORM_HILDON
> >+  // on maemo/hildon gnome_vfs_mime_application_get_name function returns
> >+  // a non-localized name for the app. We pass that as well as the default 
> >+  // maemo domain-name to dgettext as an attempt to translate a text string 
> >+  // into the user native language.
> So, I don't really like this comment.  You mention
> gnome_vfs_mime_application_get_name, but it's not clear that we call that in
> this method at all.  Additionally, it doesn't really return a non-localized
> name, but rather a message id.  I think you should really say something like
> this:
> On Maemo/Hildon, GetName ends up calling gnome_vfs_mime_application_get_name,
> which happens to return a non-localized message-id for the application.  To get
> the localized name for the application, we have to call dgettext with the
> default maemo domain-name to try and translate the string into the operating
> system's native language.

replaced, really better.

> But, I guess the real question is why aren't you setting the proper name in
> nsMIMEInfoUnix::GetPossibleApplicationHandlers where the name is set?  That way
> anyone who ends up getting ahold of this application via public API's gets the
> right name.

::GetPossibleApplicationHandlers is not even being called here. It is called for unsupported schemes (e.g. tel:// or mailto:// ), but not mimes, so I do not see why to add it there.

> >+  nsCAutoString realName (dgettext("maemo-af-desktop", PromiseFlatCString(name).get()));
> regardless, I want maemo-af-desktop pulled out into a constant to make this
> more clear

right added 

const char kDefaultTextDomain [] = "maemo-af-desktop";

locally to the method. is that ok ?

> > nsresult
> > nsMIMEInfoUnix::LaunchDefaultWithFile(nsIFile *aFile)
> > {
> >   nsCAutoString nativePath;
> >   aFile->GetNativePath(nativePath);
> >+
> >+#ifdef MOZ_PLATFORM_HILDON
> >+  // Before trying to launch the associated default handler for the given
> >+  // mime/file through gnome_vfs, lets try to invoke the it via hildon 
> >+  // specific APIs (in this case hildon_mime_open_file* which are essecially 
> typo: essentially

fixed.

> >+  if (dbus_error_is_set(&err)) {
> >+    dbus_error_free(&err);
> >+    goto fallback;
> I don't like using goto's here.  Can we pull this code out into a new method
> and just have it return?  The code would be a bit cleaner and easier to follow
> then...

ok. no 'goto'. added "nsresult nsMIMEInfoUnix::LaunchDefaultWithDBus(const char *aFilePath" . Is it ok ?

> >+  dbus_connection_set_exit_on_disconnect(connection, false);
> A comment explaining this would be nice.  Based on the documentation, it sounds
> like either we'd have _exit called on us, or the other application, when we
> disconnect, but it's not totally clear what is going on.

removed. It was not needed.

> >+  result = hildon_mime_open_file_with_mime_type(connection,
> >+                                                PromiseFlatCString(nativePath).get(),
> >+                                                mType.get());
> >+  if (result == 1)
> Oh yay, their success code is simply one.  Can you make a constant (something
> like HILDON_SUCCESS) and use that instead please

Ok, added "const PRInt32 kHILDON_SUCCESS = 1;" locally to nsMIMEInfoUnix::LaunchDefaultWithDBus . is it ok ?
 
> Is it possible to write a test for this?

Yeah, do you know about any other similar test to this I can look at ?

Comment 21

[Shawn Wilsher :sdwilsh]

Comment on attachment 368426 [details] [diff] [review]
v0.4 - make fennec to ask dbus for a default handler for an unsupported file type 

OK, this looks good to me.
r=sdwilsh

Comment 22

[Antonio Gomes (tonikitoo)]

(In reply to comment #19)
> > that invokes dbus mime_open method.
> 
> Is that guaranteed to dispatch on MIME type and not on things like filename?

boris, if I got the question right, we cover both mime type and filename "approaches" on the patch (look at the hildon* method calls below):

(...)
+  result = hildon_mime_open_file_with_mime_type(connection,
+                                                aFilePath,
+                                                mType.get());
+  if (result != kHILDON_SUCCESS)
+    if (hildon_mime_open_file(connection, aFilePath) != kHILDON_SUCCESS)
+      return NS_ERROR_FAILURE;
+
(...)

both methods rely on the mime_open dbus method as you can see here

http://timeless.justdave.net/mxr-test/chinook/source/libhildonmime-1.10.0/libhildonmime/hildon-mime-open.c#289

Comment 23

[Boris Zbarsky [:bzbarsky]]

I don't think you got the question right.  The question is this: if I pass a filepath of /tmp/foo.pl and a MIME type of text/plain, will the result be that the data is handled as text or as Perl?  Or does it depend on some other configuration somewhere?

If you can point me to the documentation for the dbus mime_open method, it might answer this question, of course.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Still waiting for a response to comment 23.

Comment 25

[Antonio Gomes (tonikitoo)]

bz: sorry for taking long to reply.

(In reply to comment #23)

> If you can point me to the documentation for the dbus mime_open method, it
> might answer this question, of course.

best documentation I know of about it are comments in the header of each mentioned methods (hildon_mime_open_file and hildon_mime_open_file_with_mime_type).

361 /**
362  * hildon_mime_open_file:
363  * @con: The D-BUS connection to use.
364  * @file: A %const @gchar pointer URI to be opened (UTF-8). 
365  *
366  * This function opens a file in the application that has
367  * registered as the handler for the mime type of the @file.
368  *
369  * The @file parameter must be a full URI, that is to say that it must
370  * be in the form of 'file:///etc/hosts'.  
371  *
372  * The mapping from mime type to D-BUS service is done by looking up the
373  * application for the mime type and in the desktop file for that application
374  * the X-Osso-Service field is used to get the D-BUS service.
375  * (...)

AND

/**
518  * hildon_mime_open_file_with_mime_type:
519  * @con: The D-BUS connection that we want to use.
520  * @file: A %const @gchar pointer URI to be opened (UTF-8). 
521  * @mime_type: A %const @gchar pointer MIME-type to be used (UTF-8). 
522  *
523  * This function opens @file in the application that has
524  * registered as the handler for @mime_type.
525  *
526  * The @file is optional. If it is omitted, the @mime_type is used
527  * to know which application to launch.
528  * 
529  * For more information on opening files, see hildon_mime_open_file().

PS: for comments below, see http://timeless.justdave.net/mxr-test/chinook/source/libhildonmime-1.10.0/libhildonmime/hildon-mime-open.c for a reference source code.

It is possible to see that both methods bodies are really similar, differing in the way one gets the proper service_name to be passed in to DBUS. See:

* http://timeless.justdave.net/mxr-test/chinook/source/libhildonmime-1.10.0/libhildonmime/hildon-mime-open.c#408
AND 
* http://timeless.justdave.net/mxr-test/chinook/source/libhildonmime-1.10.0/libhildonmime/hildon-mime-open.c#566

*however* internally both called methods get the service off of the |mime open|, and NO FILE NAME / EXTENSION. see

* http://timeless.justdave.net/mxr-test/chinook/source/libhildonmime-1.10.0/libhildonmime/hildon-mime-open.c#197
AND 
* http://timeless.justdave.net/mxr-test/chinook/source/libhildonmime-1.10.0/libhildonmime/hildon-mime-open.c#159

> I don't think you got the question right.  The question is this: if I pass a
> filepath of /tmp/foo.pl and a MIME type of text/plain, will the result be that
> the data is handled as text or as Perl?  Or does it depend on some other
> configuration somewhere?

boris, as shown above it will definitely launch the handler app registered in the system as the default handler for the MIME type. So in this case, NOTES application (it is the default one for text/plain). This is a default system behavior, not something settable ...

Comment 26

[Antonio Gomes (tonikitoo)]

bz: due to the lack of documentation available about mime_open itself, I can try to do a quickly overview.

0) most of the hildonized applications in maemo have a .desktop file.
0.1) two of the optional fields supported by a .desktop file are 'X-Osso-Service' and 'MimeType' (see complete list at http://wiki.maemo.org/Desktop_file_format for reference). They specify the service_name (for other apps to invoke it via dbus) and the mime_tipe's this application is supposed to handle. e.g.:

X-Osso-Service=Mozilla.Prism
MimeType=application/x-webapp

1) (now mime_open comes to scene) an application has to register itself (with osso_mime_set_cb) passing in a callback to be called when dbus invokes its service w/ mime_open parameter ...

2) other applications invoke it indirectly by calling hildon_mime_open* methods from previous comments.

hope that helps...

Comment 27

[Antonio Gomes (tonikitoo)]

thanks blassy, sdwilsh, bz 

26724:395e11e49392