441636 - Add support for dbus handler apps

Status: RESOLVED FIXED

(Core :: General, defect)

Description

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

Attachment: implment dbus handler

The maemo devices (n800/n810) handle most uri's via the dbus.  Handling them through the local handler app interface would seem to require implementing helper apps.

Comment 1

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

Attachment: updated based on dougt's comments

Comment 2

[Benjamin Smedberg]

Comment on attachment 326606 [details] [diff] [review]
updated based on dougt's comments

I am not the right person to review this patch. I'm not sure who is, but I don't know anything about handler apps or DBUS.

Comment 3

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

Attachment: changes to makefile and dialog.js

I missed these files in the previous patch.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 326778 [details] [diff] [review]
changes to makefile and dialog.js

>diff --git a/uriloader/exthandler/Makefile.in b/uriloader/exthandler/Makefile.in
>--- a/uriloader/exthandler/Makefile.in
>+++ b/uriloader/exthandler/Makefile.in
>@@ -152,6 +152,13 @@
> 	$(OSHELPER) \
> 	$(NULL)
> 
>+ifdef MOZ_ENABLE_DBUS
>+ CPPSRCS += nsDBusHandlerApp.cpp
>+ LOCAL_INCLUDES += -I/usr/include/dbus-1.0/ -I/usr/lib/dbus-1.0/include/ -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include
>+ EXTRA_DSO_LDOPTS += -ldbus-1
>+endif

Hardcoding these paths looks wrong to me.  Shouldn't configure be running pkg-config to populate some tree-global variables for this sort of thing?

>+
> >+ifeq ($(OS_ARCH),WINNT WINCE)
> OS_LIBS		+= shell32.lib
> GARBAGE		+= nsOSHelperAppService.cpp $(srcdir)/nsOSHelperAppService.cpp 
>diff --git a/toolkit/mozapps/handling/content/dialog.js b/toolkit/mozapps/handling/content/dialog.js
>--- a/toolkit/mozapps/handling/content/dialog.js
>+++ b/toolkit/mozapps/handling/content/dialog.js
>@@ -156,16 +156,19 @@ var dialog = {
>           }
>           catch (e) {
>             iconURI = uri.prePath + "/favicon.ico";
>           }
>           elm.setAttribute("image", iconURI);
>         }
>         elm.setAttribute("description", uri.prePath);
>       }
>+      else if(app instanceof Ci.nsIDBusHandlerApp){
               ^ space here, please
>+	  elm.setAttribute("description",app.name);  
                                         ^ and here

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 326606 [details] [diff] [review]
updated based on dougt's comments


>+
>+/**
>+ * nsIDBusHandlerApp is a DBus based local handler
>+ */
>+[scriptable, uuid(1ffc274b-4cbf-4bb5-a635-05ad2cbb6534)]
>+interface nsIDBusHandlerApp : nsIHandlerApp {
>+
>+    /**
>+     * Template used to construct the URI to GET.  Template is expected to have
>+     * a %s in it, and the escaped URI to be handled is inserted in place of 
>+     * that %s, as per the HTML5 spec.
>+     */

The above doxygen commentary is incorrect.

>+    attribute AUTF8String service;
>+    attribute AUTF8String objpath;
>+    attribute AUTF8String dBusInterface;
>+    attribute AUTF8String method;

Each of the above attributes needs doxygen commentary describing what the exact semantics of the attributes are, who should be setting them, what it means to have them unset, exceptions that could be thrown, etc.

Comment 6

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

Attachment: updated based on dmose's comments

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 327194 [details] [diff] [review]
updated based on dmose's comments

This needs an automated test as well; it can probably live in handlerApps.js and be similar to the ones that are already there.  Sorry for not noticing this the first time around.

Comment 8

[Doug Turner (:dougt)]

Comment on attachment 327194 [details] [diff] [review]
updated based on dmose's comments

more eyes; more love:

nit:  + * nsIDBusHandlerApp is a DBus based local handler

what does this mean?  It isn't clear how this interface is used.


nit: dBusInterface -> dbusInterface


+EXTRA_DSO_LDOPTS +=   -ldbus-1 

too many space between = and -, and trailing ws.


dialog.js

Do we need a image to show the user?  Here you are setting the description to be the same as the name.  What is the dialog going to actually look like?


+ LOCAL_INCLUDES   += $(TK_CFLAGS) $(MOZ_DBUS_GLIB_CFLAGS)

Use CXXFLAGS instead.


	

uriloader/exthandler/nsDBusHandlerApp.cpp

Move +#include "libosso.h" into the NS_OSS #ifdef


+// XXX why does nsMIMEInfoImpl have a threadsafe nsISupports?  do we need one 
+// here too?


You tell me.  Does this object get passed between threads?  if no, remove the comment. If yes, fix your critical sections.


+nsDBusHandlerApp::Equals(nsIHandlerApp *aHandlerApp, PRBool *_retval)
+{
+  NS_ENSURE_ARG_POINTER(aHandlerApp);


Also test the _retval for null.


Drop the extra ws:

+  *_retval = method.Equals(mMethod);
+  
+  


Line these up:

+nsDBusHandlerApp::LaunchWithURI(nsIURI *aURI,
+                                 nsIInterfaceRequestor *aWindowContext)


nit:

Add some spacing here separating the variable declarations and actual function calls..  It might just be better to move the variables from where they are, to where they are first used.

+  DBusConnection  *connection;

+  nsCAutoString spec;
+  aURI->GetAsciiSpec(spec);
+  const char* uri = spec.get(); 


What if this fails?
+  aURI->GetAsciiSpec(spec);


Spacing:
+  if (dbus_error_is_set(&err)) { 
+    dbus_error_free(&err); 
+    return NS_ERROR_FAILURE;
+	}
+  if (NULL == connection) { 
+    return NS_ERROR_FAILURE; 
+  }


nit: use nsnull:
+  if (NULL == connection) { 
and other places.



space after the comma:
+  msg = dbus_message_new_method_call (service, objpath, interface,method);


+  dbus_message_set_no_reply (msg, TRUE);
TRUE -> PR_TRUE


+  if (dbus_connection_send (connection, msg, NULL) == TRUE) {

Drop the explict | == TRUE|

Why do we do any work if #ifdef NS_OSSO isn't defined?


Do you really need all of those temporary variables?

+  const char* service = mService.get();
+  const char* method  = mMethod.get();
+  const char* objpath = mObjpath.get();
+  const char* interface = mInterface.get();


Just do the .get() where you need to and avoid the temps.

I haven't used the dbus_* apis, but they look right.  What do you want to do if dbus_message_new_method_call fails and return null?  Right now, we return NS_OK.  Same with other failures (dbus_message_append_args, dbus_connection_send).

Also in LaunchWithURI, your function calls between the function name and the first param.  This is inconsistent with the rest of the file.



+      aHandlerApp.QueryInterface(Ci.nsIDBusHandlerApp);
+      

Not sure why you do this.  I do not think it is needed.

Comment 9

[Doug Turner (:dougt)]

brad, do you have any ideas on testing this?

Comment 10

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

(In reply to comment #8)
> (From update of attachment 327194 [details] [diff] [review])
> more eyes; more love:
> 
> nit:  + * nsIDBusHandlerApp is a DBus based local handler
> 
> what does this mean?  It isn't clear how this interface is used.
> 

hows "nsIDBusHandlerApp represents local applications launched by DBus a command" sound?
> 
> nit: dBusInterface -> dbusInterface
> 

I think dBusInterface is right by convention

> 
> +EXTRA_DSO_LDOPTS +=   -ldbus-1 
> 
> too many space between = and -, and trailing ws.
> 
also probably better to  use $(MOZ_DBUS_GLIB_LIBS) 

> 
> dialog.js
> 
> Do we need a image to show the user?  Here you are setting the description to
> be the same as the name.  What is the dialog going to actually look like?
> 

I don't think an icon is necessary, but perhaps the method name would make a better description.  For example "send_mailto" is the method name for the mailto handler.

> 
> + LOCAL_INCLUDES   += $(TK_CFLAGS) $(MOZ_DBUS_GLIB_CFLAGS)
> 
> Use CXXFLAGS instead.

Using CXXFLAGS doesn't work. I'd love to know why.

> 
> 
> 
> 
> uriloader/exthandler/nsDBusHandlerApp.cpp
> 
> Move +#include "libosso.h" into the NS_OSS #ifdef
> 
> 
> +// XXX why does nsMIMEInfoImpl have a threadsafe nsISupports?  do we need one 
> +// here too?
> 

That comment was in nsLocalHandlerApp, which I based this on. Given that nsLocalHandlerApp has been used for a while without issue, I think we're safe.  But the question should probably be answered for both classes.

> 
> You tell me.  Does this object get passed between threads?  if no, remove the
> comment. If yes, fix your critical sections.
> 
> 
> +nsDBusHandlerApp::Equals(nsIHandlerApp *aHandlerApp, PRBool *_retval)
> +{
> +  NS_ENSURE_ARG_POINTER(aHandlerApp);
> 
> 
> Also test the _retval for null.
> 
> 
> Drop the extra ws:
> 
> +  *_retval = method.Equals(mMethod);
> +  
> +  
> 
> 
> Line these up:
> 
> +nsDBusHandlerApp::LaunchWithURI(nsIURI *aURI,
> +                                 nsIInterfaceRequestor *aWindowContext)
> 
> 
> nit:
> 
> Add some spacing here separating the variable declarations and actual function
> calls..  It might just be better to move the variables from where they are, to
> where they are first used.
> 
> +  DBusConnection  *connection;
> 
> +  nsCAutoString spec;
> +  aURI->GetAsciiSpec(spec);
> +  const char* uri = spec.get(); 
> 
> 
> What if this fails?
> +  aURI->GetAsciiSpec(spec);
> 
> 
> Spacing:
> +  if (dbus_error_is_set(&err)) { 
> +    dbus_error_free(&err); 
> +    return NS_ERROR_FAILURE;
> +       }
> +  if (NULL == connection) { 
> +    return NS_ERROR_FAILURE; 
> +  }
> 
> 
> nit: use nsnull:
> +  if (NULL == connection) { 
> and other places.
> 
> 
> 
> space after the comma:
> +  msg = dbus_message_new_method_call (service, objpath, interface,method);
> 
> 
> +  dbus_message_set_no_reply (msg, TRUE);
> TRUE -> PR_TRUE
> 
> 
> +  if (dbus_connection_send (connection, msg, NULL) == TRUE) {
> 
> Drop the explict | == TRUE|
> 
> Why do we do any work if #ifdef NS_OSSO isn't defined?

We can use the dbus to launch handler apps on systems without osso.

> 
> 
> Do you really need all of those temporary variables?
> 
> +  const char* service = mService.get();
> +  const char* method  = mMethod.get();
> +  const char* objpath = mObjpath.get();
> +  const char* interface = mInterface.get();
> 
> 
> Just do the .get() where you need to and avoid the temps.
> 
> I haven't used the dbus_* apis, but they look right.  What do you want to do if
> dbus_message_new_method_call fails and return null?  Right now, we return
> NS_OK.  Same with other failures (dbus_message_append_args,
> dbus_connection_send).
> 
> Also in LaunchWithURI, your function calls between the function name and the
> first param.  This is inconsistent with the rest of the file.
> 
> 
> 
> +      aHandlerApp.QueryInterface(Ci.nsIDBusHandlerApp);
> +      
> 
> Not sure why you do this.  I do not think it is needed.
> 

perhaps not

Comment 11

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

Attachment: updated based on dougt's comments and implmented a mochitest

LaunchWithURI fails if you use mService.get(), mObjpath.get(), mMethod.get() and mDBusInterface.get() directly.

Comment 12

[Doug Turner (:dougt)]

dmose, can you review again?

Comment 13

[Stuart Parmenter]

Comment on attachment 329226 [details] [diff] [review]
updated based on dougt's comments and implmented a mochitest

this looks OK to me -- would be great to get another review

Comment 14

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

Comment on attachment 329226 [details] [diff] [review]
updated based on dougt's comments and implmented a mochitest

>+++ b/docshell/build/Makefile.in	Sat Jul 12 12:37:38 2008 -0400
>\ No newline at end of file

Please add

>+++ b/netwerk/mime/public/nsIMIMEInfo.idl	Sat Jul 12 12:37:38 2008 -0400
>+ * nsIDBusHandlerApp represents local applications launched by DBus a command

"by a DBus command"? Also it seems that "method" or "request" would match DBus terminology better.

The comment should also mention the expected signature of the invoked method. Looks like it expects a method with a single string argument that is the URI?

>+    attribute AUTF8String objpath;

Can you name this objectPath?

>+    /**
>+     * DBusInterface defines the interface of the dbus service that should 
>+     * handle this protocol. If its not set,  NS_ERROR_FAILURE will be  
>+     * returned by LaunchWithURI
>+     */
>+    attribute AUTF8String dBusInterface;

Please add a newline after this

>+++ b/uriloader/exthandler/Makefile.in	Sat Jul 12 12:37:38 2008 -0400
>+ifdef MOZ_ENABLE_DBUS
>+ CPPSRCS += nsDBusHandlerApp.cpp
>+ LOCAL_INCLUDES   += $(TK_CFLAGS) $(MOZ_DBUS_GLIB_CFLAGS)
>+ EXTRA_DSO_LDOPTS += $(MOZ_DBUS_GLIB_LIBS)
>+endif

Doesn't look like this space at the beginning matches the style of the file

>+++ b/uriloader/exthandler/nsDBusHandlerApp.cpp	Sat Jul 12 12:37:38 2008 -0400
>+ * Contributor(s):
>+ *   Brad Lassey <[e-mail]>

Add a newline after this

>+#include "nsDBusHandlerApp.h"
>+#include "nsIURI.h"
>+#include  <dbus/dbus.h>
>+#include "nsIGenericFactory.h"
>+#include "nsIClassInfoImpl.h"

Please include the dbus header at the beginning. Also remove one of the two spaces.

>+#ifdef NS_OSSO
>+#include "libosso.h"

Should be <libosso.h> right?

>+  if(!_retval)
>+    return NS_ERROR_NULL_POINTER;

No point in checking this

>+  if (NS_FAILED(rv)){

Missing space before {, multiple times

>+  *_retval = service.Equals(mService);
>+  if(!*_retval)
>+    return NS_OK;
>+  *_retval = method.Equals(mMethod);

Why not just write this as:

  *_retval = service.Equals(mService) && method.Equals(mMethod);
  return NS_OK;

>+nsDBusHandlerApp::LaunchWithURI(nsIURI *aURI,
>+                                 nsIInterfaceRequestor *aWindowContext)

Incorrect indentation on the second line here

>+{
>+  DBusMessage* msg;
>+  DBusMessageIter iter;
>+  DBusConnection  *connection;
>+  nsCAutoString spec;
>+  DBusError err;
>+  nsresult rv;

Please declare your variables where you use them instead of all at the beginning of the function

>+  rv = aURI->GetAsciiSpec(spec);
>+  NS_ENSURE_SUCCESS(rv,rv);
>+  const char* uri = spec.get(); 

Can you add a newline after that line?

>+  connection = dbus_bus_get(DBUS_BUS_SESSION, &err);

Shouldn't you call dbus_connection_set_exit_on_disconnect after this? (with an argument of false)

>+  const char* service = mService.get();
>+  const char* objpath = mObjpath.get(); 
>+  const char* interface = mInterface.get();
>+  const char* method = mMethod.get();

I'm not really sure why you use temporary variables for all this but OK...

>+  msg = dbus_message_new_method_call (service, objpath, interface, method);

Remove the space before the (. Also in the rest of the file.

>+  if(!msg){

Add a space before the (

>+  dbus_message_set_no_reply (msg, PR_TRUE);
>+  dbus_message_iter_init_append (msg, &iter);
>+  dbus_message_iter_append_basic (&iter, DBUS_TYPE_STRING, &uri);
>+  
>+  if (dbus_connection_send (connection, msg, NULL)) {

You have to dbus_message_unref the msg here and in the else branch

>+  }else{

Space after } and before {

>+}
>+NS_IMETHODIMP nsDBusHandlerApp::SetService(const nsACString & aService)

Please add a newline between functions, here and in the other places in this file.

>+++ b/uriloader/exthandler/nsDBusHandlerApp.h	Sat Jul 12 12:37:38 2008 -0400
>+#include "nsString.h"
>+#include "nsIMIMEInfo.h"
>+#include "nsIFile.h"
>+class nsDBusHandlerApp : public nsIDBusHandlerApp

Why are you including nsIMIMEInfo and nsIFile, but not nsIDBusHandlerApp?

>diff -r a94113664fc2 uriloader/exthandler/tests/mochitest/handlerApps.js
>+function fakeURI() {}
>+
>+fakeURI.prototype = {
>+    get asciiSpec(){
>+	return "org.freedesktop.Notifications";
>+    }, 

This is not a valid nsIURI object for a lot of reasons. Do not do this.

>+    QueryInterface: function XPCOMUtils_QueryInterface(iid) {

(and this is not a method of XPCOMUtils...)

>+  if(osString =="Linux"){

Missing space before ( and {

>+    //test dbus handler apps
>+    //dbus-send --print-reply   --type=method_call --dest=org.freedesktop.DBus   /org/freedesktop/DBus org.freedesktop.DBus.GetNameOwner   string:'org.freedesktop.Notifications'

The dbus-send comment doesn't seem very useful. There's also a space missing after the //. And I don't really understand what you are testing here. It seems like the test doesn't do anything useful and in fact pretty much always passes.

Comment 15

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

(In reply to comment #11)
> LaunchWithURI fails if you use mService.get(), mObjpath.get(), mMethod.get()
> and mDBusInterface.get() directly.

With what error?

Comment 16

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

(In reply to comment #14)
 
> >+++ b/netwerk/mime/public/nsIMIMEInfo.idl	Sat Jul 12 12:37:38 2008 -0400
> >+ * nsIDBusHandlerApp represents local applications launched by DBus a command
> 
> "by a DBus command"? Also it seems that "method" or "request" would match DBus
> terminology better.
> 
> The comment should also mention the expected signature of the invoked method.
> Looks like it expects a method with a single string argument that is the URI?
> 
* nsIDBusHandlerApp represents local applications launched by DBus a message
* invoking a method taking a single string argument descibing a URI



> 
> >+{
> >+  DBusMessage* msg;
> >+  DBusMessageIter iter;

> >+  rv = aURI->GetAsciiSpec(spec);
> >+  NS_ENSURE_SUCCESS(rv,rv);

> >+  connection = dbus_bus_get(DBUS_BUS_SESSION, &err);
> 
> Shouldn't you call dbus_connection_set_exit_on_disconnect after this? (with an
> argument of false)

very good point

> 
> >+  const char* service = mService.get();
> >+  const char* objpath = mObjpath.get(); 
> >+  const char* interface = mInterface.get();
> >+  const char* method = mMethod.get();
> 
> I'm not really sure why you use temporary variables for all this but OK...

dbus_connection_send returns false when called with mObjPath.get() etc. arguments


> >+  if(!msg){
> 
> Add a space before the (



> 
> >+++ b/uriloader/exthandler/nsDBusHandlerApp.h	Sat Jul 12 12:37:38 2008 -0400
> >+#include "nsString.h"
> >+#include "nsIMIMEInfo.h"
> >+#include "nsIFile.h"
> >+class nsDBusHandlerApp : public nsIDBusHandlerApp
> 
> Why are you including nsIMIMEInfo and nsIFile, but not nsIDBusHandlerApp?

there is no nsIDBusHandlerApp.h, but nsIDBusHandlerApp is defined in nsiMIMEInfo.h

nsIFile.h is unnecessary 

> 
> >diff -r a94113664fc2 uriloader/exthandler/tests/mochitest/handlerApps.js
> >+function fakeURI() {}
> >+
> >+fakeURI.prototype = {
> >+    get asciiSpec(){
> >+	return "org.freedesktop.Notifications";
> >+    }, 
> 
> This is not a valid nsIURI object for a lot of reasons. Do not do this.
> 
> >+    QueryInterface: function XPCOMUtils_QueryInterface(iid) {
> 
> (and this is not a method of XPCOMUtils...)
> 
> >+  if(osString =="Linux"){
> 
> Missing space before ( and {
> 
> >+    //test dbus handler apps
> >+    //dbus-send --print-reply   --type=method_call --dest=org.freedesktop.DBus   /org/freedesktop/DBus org.freedesktop.DBus.GetNameOwner   string:'org.freedesktop.Notifications'
> 
> The dbus-send comment doesn't seem very useful. There's also a space missing
> after the //. And I don't really understand what you are testing here. It seems
> like the test doesn't do anything useful and in fact pretty much always passes.
> 

The test results in a dbus message being sent that is the equivalent of what would be sent by the command in the comment. If you break LaunchWithURI, this test will fail.

wrt "fakeURI,"  the point of the test is to call a dbus method that takes one string argument.  The idea was to call a method that will exist on all Linux systems with dbus.  nsDBusHandlerApp::LaunchWithURI pulls the spec string out of the URI passed in right away and only uses that.

I don't think there is any dbus method that takes a uri argument and is common to all Linux distros.  If you have another idea for a test, let me know.   Otherwise, if you still object to this test, it can just be dropped.

Comment 17

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

Attachment: updated based on christian's comments

Comment 18

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

Comment on attachment 330558 [details] [diff] [review]
updated based on christian's comments

+ * nsIDBusHandlerApp represents local applications launched by DBus a message
+ * invoking a method taking a single string argument descibing a URI

"by invoking a DBus message"?

+  nsCOMPtr <nsIDBusHandlerApp> dbusHandlerApp = do_QueryInterface(aHandlerApp);

no space  before <

+  msg = dbus_message_new_method_call(service, objpath, interface, method);

per our IRC discussion this can be mService.get() etc

+  dbus_message_iter_init_append (msg, &iter);


please remove the space before the ( for all the dbus_* function calls in this method

+++ b/uriloader/exthandler/nsHandlerService.js
+    }else{

should be } else {

> Otherwise, if you still object to this test, it can just be dropped.

Yeah... I do. I don't think this is a valuable test. It does not, for example, ensure that LaunchWithURI does anything at all.

Comment 19

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

Attachment: stores and retrieves dbus handlers correctly

As discussed on irc, the reviewed patch doesn't store and retrieve dbus handers correctly.  This addition fixes that problem

Comment 20

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

Comment on attachment 331326 [details] [diff] [review]
stores and retrieves dbus handlers correctly

+++ b/uriloader/exthandler/nsHandlerService.js
+      handlerApp.service   = service;

use just one space before the equals sign

Comment 21

[Stuart Parmenter]

brad landed this

Comment 22

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

author	Brad Lassey <[e-mail]>
	Wed Sep 10 12:10:46 2008 -0400 (at Wed Sep 10 12:10:46 2008 -0400)
changeset 19110	0230a5162f93
parent 19109	fe6a1600592c
child 19111	78f592a0a109