242254 - Implement nsIShellService methods for GNOME

Status: VERIFIED FIXED

(Firefox :: Shell Integration, defect, P1)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

Need to implement a nsGNOMEShellService for Firefox, especially
isDefaultBrowser, setDefaultBrowser, and shouldCheckDefaultBrowser to allow
Firefox to register with gconf as the default handler for the http protocol
(allowing other apps to send it links) and to offer a UI to users to allow them
to set this within the browser.

Other methods are desirable but not so urgent.

Currently people are using shell scripts and/or other hacks to allow Firefox to
handle links, which is less than optimal.

Comment 1

[Brian Ryner (not reading)]

Attachment: patch (against AVIARY_1_0_20040515_BRANCH)

Ok.  Here are some design notes on what I've done:

- Rather than deal with dlopen or splitting out the shell service into its own
component, I implemented GConf and Gnome-VFS interfaces as XPCOM services.  The
GNOME component will fail to load if GNOME is not on the system but that will
not be fatal.  I plan to make use of this later in uriloader, the icon decoder,
etc to reduce code duplication.  At present the services implement the minimum
needed to implement the shell service.

- The XPCOM service lives in toolkit/components/gnome.	In preparation for the
service being used by all of our applications, I added toolkit to the list of
directories that are always pulled.

- I folded in the latest start script from bug 177996, with minor changes to
un-hardcode a couple of things.

And some known bugs, though none that I feel need to block this:

- SetDesktopBackground does not handle 8-bit image data.  I didn't feel like
trying to figure out how to write out a color map.  From my tests, what I have
works fine on a 24-bit display regardless of the image depth.  It also doesn't
do anything with alpha.

- SetDesktopBackground sets the registry keys and assumes that Nautilus is
watching those keys.  If the user is not using nautilus, this probably doesn't
work.

- OpenPreferredApplication does not handle applications that require a terminal
to run.  It shouldn't be overly difficult to add, but I'm fine with this
landing without that suport.

Looking for feedback, particularly anything that doesn't work on default setups
(I tested against Fedora Core 1), bugs, and suggestions for the IDL interfaces.

Comment 2

[Brian Ryner (not reading)]

cc'ing shaver, the XPCOM interface for gconf/gnome-vfs was his suggestion and he
might have some feedback.

Comment 3

[Brian Ryner (not reading)]

Comment on attachment 152395 [details] [diff] [review]
patch (against AVIARY_1_0_20040515_BRANCH)

new patch coming up

Comment 4

[Brian Ryner (not reading)]

Attachment: patch (against AVIARY_1_0_20040515_BRANCH)

Added setting of the shell icon for the MIME types that we handle.  A few other
misc cleanups.	I just realized I didn't add shellservice.xpt and
nsSetDefaultBrowser.js to the packages-unix file... doing so in my tree but I'm
not going to post a new patch for that.

Comment 5

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

Comment on attachment 152493 [details] [diff] [review]
patch (against AVIARY_1_0_20040515_BRANCH)

-ifdef MOZ_XUL_APP
 FASTUPDATE_MOZTOOLKIT := fast_update $(CVSCO_MOZTOOLKIT)

hm...

Comment 6

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

Comment on attachment 152493 [details] [diff] [review]
patch (against AVIARY_1_0_20040515_BRANCH)

oops, didn't mean to hit submit.

+	     if test "$MOZ_ENABLE_GCONF" = "force"
+	     if test "$MOZ_ENABLE_LIBGNOME" = "force"

since you never set it to force (no configure argument), I think you don't need
this check

are the script changesi n browser/app supposed to be part of this patch?

(I'm not going to review firefox xul/js changes)

+ifneq (,$(filter windows gtk2, $(MOZ_WIDGET_TOOLKIT)))
+DEFINES += -DHAVE_SHELL_SERVICE=1

any reason not to do this in configure?

+interface nsIGConfService : nsISupports

Hmm. Maybe this should be in some shared location, and be implemented both for
the windows registry and for gconf.

+interface nsIGnomeVFSMimeApp : nsISupports

this could use some documentation :)

+  void 	      setAppStringKey(in string id,

can I just make up an id? how do they look like?

maybe it would be better to have an own function for each thing to set.

+  /* Commit application registry changes to disk. */
+  void 	      syncAppRegistry();

must this be called so that these changes are saved, or do I only have to call
it if I want to apply the changes right now?

you are being inconsistent with ACString vs string...

I suggest some intl-aware string type for extensions

+  /* Set the icon for a MIME type */

is that just a file path to some image file?
maybe nsIFile would be a good idea.

how do I _get_ the icon for a given file?


+  /* Add a MIME type to the list of types that the application can handle */
+  void 	      addMimeType(in string id,
+				  in string mimeType);

+   * Set the list of extensions for a given MIME type.
+   * Should be passed a space separated list of extensions with no
+   * dot, i.e. "gif jpg png".

well, consistency is overrated I guess.

+  /* Open the given URL in the default application */
+  void 	      showURL(in string url);

nsIURI?

toolkit/components/gnome/nsIGnomeVFSService.idl~

doesn't look like it should be part of this patch



+#ifdef HAVE_SHELL_SERVICE
     <command id="Browser:ReadMail" oncommand="MailIntegration.readMail();"/>
     <command id="Browser:ReadNews" oncommand="MailIntegration.readNews();"/>
-#ifdef XP_WIN
+#ifdef HAVE_SHELL_SERVICE
	 <toolbarbutton id="mail-button" type="menu" class="toolbarbutton-1"
			label="&mailButton.label;" orient="vertical"
			onmouseover="MailIntegration.updateUnreadCount();"
So... what if gnome is actually not available?
why not make this a runtime check?

Comment 7

[Brian Ryner (not reading)]

(In reply to comment #6)
> are the script changesi n browser/app supposed to be part of this patch?
> 

They can be separate if you want; as I said it's just the latest script from bug
177996.

> +ifneq (,$(filter windows gtk2, $(MOZ_WIDGET_TOOLKIT)))
> +DEFINES += -DHAVE_SHELL_SERVICE=1
> 
> any reason not to do this in configure?
> 

Well, it's specific to firefox, defining it globally seemed unnecessary.

> +interface nsIGConfService : nsISupports
> 
> Hmm. Maybe this should be in some shared location, and be implemented both for
> the windows registry and for gconf.
> 

It could be, but I have no plans to implement it on windows.

> +  void 	      setAppStringKey(in string id,
> 
> can I just make up an id? how do they look like?
> 

Yes you can just use any string. I'll document that.

> maybe it would be better to have an own function for each thing to set.
> 

I had that at one point but decided this was easier.

> +  /* Commit application registry changes to disk. */
> +  void 	      syncAppRegistry();
> 
> must this be called so that these changes are saved, or do I only have to call
> it if I want to apply the changes right now?

It must be called to save the changes.  I'll document this as well.

> you are being inconsistent with ACString vs string...

I tried to avoid nsACString parameters because I didn't want to have to use the
PromiseFlatCString stuff on each call.  I did use ACStrings for out parameters
to make it easier on the caller.

> I suggest some intl-aware string type for extensions
> 

The extensions are UTF-8 encoded, that should be intl-aware enough.

> +  /* Set the icon for a MIME type */
> 
> is that just a file path to some image file?
> maybe nsIFile would be a good idea.
> 

It's not exactly a path, no.  It's basically a raw filename that is searched for
in the icon search path.  So it doesn't point to a particular file on disk.

> how do I _get_ the icon for a given file?
> 

I didn't add that because I didn't need it.

> 
> +  /* Add a MIME type to the list of types that the application can handle */
> +  void 	      addMimeType(in string id,
> +				  in string mimeType);
> 
> +   * Set the list of extensions for a given MIME type.
> +   * Should be passed a space separated list of extensions with no
> +   * dot, i.e. "gif jpg png".
> 
> well, consistency is overrated I guess.
> 

These are the easiest methods to map onto the underlying gnome-vfs interface.

> +  /* Open the given URL in the default application */
> +  void 	      showURL(in string url);
> 
> nsIURI?
> 

Why?

> +#ifdef HAVE_SHELL_SERVICE
> 	 <toolbarbutton id="mail-button" type="menu" class="toolbarbutton-1"
> 			label="&mailButton.label;" orient="vertical"
> 			onmouseover="MailIntegration.updateUnreadCount();"
> So... what if gnome is actually not available?
> why not make this a runtime check?
> 

I'll have to take a look at see if that's feasible.

Comment 8

[Brendan Eich [:brendan]]

biesi: there's no reason to demand a Windows-capable interface here.  The only
requirement is to support dynamic loading of GNOME stuff, no compile-time deps.

/be

Comment 9

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

(In reply to comment #7)
> They can be separate if you want; as I said it's just the latest script from bug
> 177996.

ah - I don't really know these scripts well, I'd prefer it if someone else would
review this.

> > +ifneq (,$(filter windows gtk2, $(MOZ_WIDGET_TOOLKIT)))
> > +DEFINES += -DHAVE_SHELL_SERVICE=1
> > 
> > any reason not to do this in configure?
> 
> Well, it's specific to firefox, defining it globally seemed unnecessary.

hm, yeah... this way, you have to define it in two files though...

> > you are being inconsistent with ACString vs string...
> 
> I tried to avoid nsACString parameters because I didn't want to have to use the
> PromiseFlatCString stuff on each call.  I did use ACStrings for out parameters
> to make it easier on the caller.

ah... ok.

> > I suggest some intl-aware string type for extensions
> 
> The extensions are UTF-8 encoded, that should be intl-aware enough.

(as mentioned on IRC, |string| does not support non-ascii values when called
from js)

+  void               setAppStringKey(in string id,
+                                     in long   key,  /* see enums above */
+                                     in string value);

I'm just noticing that that's a problem here as well - the name or command of
the app may contain non-ascii chars

> It's not exactly a path, no.  It's basically a raw filename that is searched for
> in the icon search path.  So it doesn't point to a particular file on disk.

hm... ok... so to use this, I first have to put my icon into that search path?

> > how do I _get_ the icon for a given file?
(that should mention "mime type", of course)
> I didn't add that because I didn't need it.

ok. the gnome moz-icon channel could make use of it, but it can also continue as
before.

> These are the easiest methods to map onto the underlying gnome-vfs interface.

glad to see that other software likes inconsistency as well :)

> > +  /* Open the given URL in the default application */
> > +  void 	      showURL(in string url);
> > 
> > nsIURI?
> > 
> 
> Why?

Well, uris in mozilla are usually passed as nsIURI objects (to preserve their
charset, for uris with non-ascii characters)

a string is probably fine as well; for IDN support it should probably be an
AUTF8String or wstring, though...


(In reply to comment #8)
> biesi: there's no reason to demand a Windows-capable interface here.  The only
> requirement is to support dynamic loading of GNOME stuff, no compile-time deps.

brendan, I wasn't "demanding" a windows-capable interface here. I just thought
that this interface could be made a bit more general, and be used in other
places as well, that want to access the windows registry. I did not suggest that
bryner implement that, my suggestion was to consider moving the interface into a
more general place.
this bug is indeed only about dynamic loading of gnome stuff, you noticed that
well, but code lives longer than the bug which created it, and having good APIs
does generally seem like a good thing to me.

Comment 10

[Brian Ryner (not reading)]

> (as mentioned on IRC, |string| does not support non-ascii values when called
> from js)

I've changed all these to AUTF8String locally.

> hm... ok... so to use this, I first have to put my icon into that search path?

Yes.

> ok. the gnome moz-icon channel could make use of it, but it can also continue as
> before.

Oh, I didn't realize that.  I'll add it if the icon code needs it.  I intended
this to be used by all of our GNOME related code (though isn't there a problem
where the API is only available on new-ish libgnome-vfs versions? Need to
investigate that for this patch as well.)


> bryner implement that, my suggestion was to consider moving the interface into a
> more general place.

Well, I could make it just be nsISystemRegistry or some such; the get/set
methods are pretty generic.  The app/protocol stuff on nsIGConfService is pretty
specific to the implementation living in GConf.  My personal opinion is that any
code that would be calling this needs to be equally platform-specific (for the
keys and values), so trying to funnel through a common interface seems like it's
unneeded.

Comment 11

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

(In reply to comment #10)
> Oh, I didn't realize that.  I'll add it if the icon code needs it.  I intended
> this to be used by all of our GNOME related code (though isn't there a problem
> where the API is only available on new-ish libgnome-vfs versions? Need to
> investigate that for this patch as well.)

oh hm, this is just the icon from gnome-vfs? then nevermind, this is not what
the icon code uses, iirc (it asks libgnomeui)

> Well, I could make it just be nsISystemRegistry or some such; the get/set
> methods are pretty generic.  The app/protocol stuff on nsIGConfService is pretty
> specific to the implementation living in GConf. 

hm, yeah. the reason for the suggestion was: the methods did seem generic; and
I've seen people ask for windows registry access from javascript occasionally.

> My personal opinion is that any
> code that would be calling this needs to be equally platform-specific (for the
> keys and values), so trying to funnel through a common interface seems like it's
> unneeded.

this is a good point though. feel free to leave this interface where it is.

Comment 12

[sairuh (rarely reading bugmail)]

hey Brian, what QA would you need for this one? would it be a matter of testing
links in other linux apps and to see if firefox will launch (if not already
open) and load them? or something else?

Comment 13

[Brian Ryner (not reading)]

(In reply to comment #11)
> oh hm, this is just the icon from gnome-vfs? then nevermind, this is not what
> the icon code uses, iirc (it asks libgnomeui)
> 

Right.  What I figured out eventually is that the libgnomeui code first consults
gnome-vfs for an icon name but has fallback code in the icon theme for handling
different types of files.  So the level we set the icon at isn't the same one we
ask for it at.

I'm just about finished making the UI elements hide themselves at runtime when
GNOME isn't present, I'll post a new patch when that's done.

sairuh, here is the functionality that would need to be tested for this:
- clicking links in other (GNOME/Mozilla) apps launches firefox
- double clicking HTML files in nautilus launches firefox
- firefox launches your preferred mail reader from the mail toolbar icon
- firefox associates its icon with HTML files in nautilus
- set as wallpaper

Comment 14

[Brian Ryner (not reading)]

Attachment: new patch

This should address all of biesi's comments, notably that the IDL types are
changed to AUTF8String and the interfaces are better-documented.  I also
rewrote SetDesktopBackground code to use GdkPixbuf to write a png rather than
attempting to write out a bitmap file.	The code is smaller and also supports 1
and 8 bit alpha. (though the alpha part won't work correctly until bug 250531
is fixed)

Comment 15

[Mike Shaver (:shaver emeritus)]

(In reply to comment #11)
> hm, yeah. the reason for the suggestion was: the methods did seem generic; and
> I've seen people ask for windows registry access from javascript occasionally.

I think extensions/winhooks already provides that.  Something does; I've done
registry access from JS in a test extension recently.

Comment 16

[Brian Ryner (not reading)]

Comment on attachment 152725 [details] [diff] [review]
new patch

shaver offered to take a look at this

Comment 17

[Mike Shaver (:shaver emeritus)]

Comment on attachment 152725 [details] [diff] [review]
new patch

>+  var shell;

You check this for truth-value below, possibly without any intervening
assignment; initialize to null here?

>+    var shouldCheck = shell.shouldCheckDefaultBrowser;
>+    if (shouldCheck && !shell.isDefaultBrowser(true)) {

Edumacate me: why would the shell tell us that we should check the default
browser if we're already the default browser?  I'm trying to imagine when you
would use shouldCheck without immediately calling isDefault that way.

>+        // Set As Wallpaper depends on whether an image was clicked on, and only works on Windows/GNOME.

s|on Windows/GNOME|if we have a shell service|

>+          var color;
>+          try {
>+	    var shell = Components.classes["@mozilla.org/browser/shell-service;1"]
>+                                .getService(Components.interfaces.nsIShellService);
>+            color = shell.desktopBackgroundColor;
>+          } catch (e) {}

This is a pretty common pattern; worth commonizing somewhere?

>+#elif defined(MOZ_WIDGET_GTK2)
> #include "nsGNOMEShellService.h"

Do we really depend on GTK2?  Can we not do GNOME integration with GTK1?

>+static const ProtocolAssociation appProtocols[] = {
>+  { "http",   PR_TRUE  },
>+  { "https",  PR_TRUE  },
>+  { "ftp",    PR_FALSE },
>+  { "gopher", PR_FALSE },
>+  { "chrome", PR_FALSE }
>+};

jar? (about: =) )

>+  // The string will be something of the form: [/path/to/]browser "%s"
>+  // We want to remove all parameters, which is anything after the first
>+  // unescaped space.

What about quoted spaces?

>+  nsCOMPtr<nsIGConfService> gconf = do_GetService(NS_GCONFSERVICE_CONTRACTID);

Check for failure?

>+#ifdef DEBUG
>+  if (aForAllUsers)
>+    NS_WARNING("Setting the default browser for all users is not yet supported");
>+#endif

Should we return an error?  I find myself thinking so.

>+  if (bpp != 24)
>+    return NS_ERROR_FAILURE;

Some better error code?  INVALID_ARG or some such?

>+    if (format == gfxIFormats::RGB_A1)
>+      alphaDepth = 1;
>+    else
>+      alphaDepth = 8;

switch instead?  I can trivially imagine a post-SVG world in which someone adds
another format.

>+      // If using 1-bit alpha, we must expand it to 8-bit
>+      PRUint32 bitPos = 7;
>+
>+      for (PRInt32 x = 0; x < width; ++x) {
>+        if (alphaDepth == 1) {
>+          pixbufPixel[pixbufChannels - 1] = ((*maskPixel >> bitPos) & 1) ? 255 : 0;
>+          if (bitPos-- == 0) { // wrapped around, move forward a byte
>+            ++maskPixel;
>+            bitPos = 7;
>+          }
>+        } else {
>+          pixbufPixel[pixbufChannels - 1] = *maskPixel++;
>+        }

Shame that libpr0n doesn't have some function to do all this munging for you...

>+  gdk_pixbuf_save(alphaPixbuf ? alphaPixbuf : pixbuf, aPath.get(),
>+                  "png", NULL, NULL);

Can this fail?	I bet it can, and we should report if it does, I think.

>+  nsCOMPtr<nsIDOMHTMLImageElement> imgElement(do_QueryInterface(aElement));
>+  if (!imgElement) {
>+    // XXX write background loading stuff!

Please to be filing a bug on that!  (But if we don't enable the
set-as-background menu item until it's loaded, maybe we just return error here
and forget about it?)

> extern "C" NS_GFX_(PRBool) NS_HexToRGB(const nsString& aBuf, nscolor* aResult);
>+extern "C" NS_GFX_(PRBool) NS_ASCIIHexToRGB(const nsCString& aBuf,
>+                                            nscolor* aResult);
> 
> // Translate a hex string to a color. Return true if it parses ok,
> // otherwise return false.
>@@ -96,6 +96,8 @@ extern "C" NS_GFX_(PRBool) NS_LooseHexTo
> // Translate a color to a hex string and prepend a '#'.
> // The returned string is always 7 characters including a '#' character.
> extern "C" NS_GFX_(void) NS_RGBToHex(nscolor aColor, nsAString& aResult);
>+extern "C" NS_GFX_(void) NS_RGBToASCIIHex(nscolor aColor,
>+                                          nsAFlatCString& aResult);

(It's totally nuts that we need to manually strip the # for hex-to-rgb, when
it's prepended by rgb-to-hex, BTW.)

>+extern "C" NS_GFX_(void) NS_RGBToASCIIHex(nscolor aColor,
>+                                          nsAFlatCString& aResult)
>+{
>+  aResult.SetLength(7);
>+  char *buf = aResult.BeginWriting();
>+  PR_snprintf(buf, 8, "#%02x%02x%02x",
>+              NS_GET_R(aColor), NS_GET_G(aColor), NS_GET_B(aColor));
>+}

Is SetLength(7) guaranteed to give us room to write the NUL byte that we'll get
from PR_snprintf (by my casual reading)?

sr=shaver with the corrections above, etc.

Comment 18

[Brian Ryner (not reading)]

(In reply to comment #17)
> (From update of attachment 152725 [details] [diff] [review])
> >+  var shell;
> 
> You check this for truth-value below, possibly without any intervening
> assignment; initialize to null here?

Fixed.

> 
> >+    var shouldCheck = shell.shouldCheckDefaultBrowser;
> >+    if (shouldCheck && !shell.isDefaultBrowser(true)) {
> 
> Edumacate me: why would the shell tell us that we should check the default
> browser if we're already the default browser?  I'm trying to imagine when you
> would use shouldCheck without immediately calling isDefault that way.
> 

This is how the shell service API works currently, I'm not looking to change
that here.

> >+        // Set As Wallpaper depends on whether an image was clicked on, and
only works on Windows/GNOME.
> 
> s|on Windows/GNOME|if we have a shell service|
> 

fixed.

> >+          var color;
> >+          try {
> >+	    var shell = Components.classes["@mozilla.org/browser/shell-service;1"]
> >+                               
.getService(Components.interfaces.nsIShellService);
> >+            color = shell.desktopBackgroundColor;
> >+          } catch (e) {}
> 
> This is a pretty common pattern; worth commonizing somewhere?
> 

done.

> >+#elif defined(MOZ_WIDGET_GTK2)
> > #include "nsGNOMEShellService.h"
> 
> Do we really depend on GTK2?  Can we not do GNOME integration with GTK1?

Let's not worry about it.  I think there are some differences in the GObject API
and I don't think it's worth the effort to verify that it works (we don't even
do GTK1 nightlies anymore).

> >+static const ProtocolAssociation appProtocols[] = {
> >+  { "http",   PR_TRUE  },
> >+  { "https",  PR_TRUE  },
> >+  { "ftp",    PR_FALSE },
> >+  { "gopher", PR_FALSE },
> >+  { "chrome", PR_FALSE }
> >+};
> 
> jar? (about: =) )

Let's not just now.

> 
> >+  // The string will be something of the form: [/path/to/]browser "%s"
> >+  // We want to remove all parameters, which is anything after the first
> >+  // unescaped space.
> 
> What about quoted spaces?
> 

changed to g_shell_parse_argv which handles this.

> >+  nsCOMPtr<nsIGConfService> gconf = do_GetService(NS_GCONFSERVICE_CONTRACTID);
> 
> Check for failure?
> 

No need, GS on the service will have failed if Init() failed to get the GConf
service, and the service manager will therefore still be holding onto it.

> >+#ifdef DEBUG
> >+  if (aForAllUsers)
> >+    NS_WARNING("Setting the default browser for all users is not yet
supported");
> >+#endif
> 
> Should we return an error?  I find myself thinking so.

I don't think so for this.  "all users" is additive; we should continue to do it
for the current user even if we don't support doing it for all users.

> 
> >+  if (bpp != 24)
> >+    return NS_ERROR_FAILURE;
> 
> Some better error code?  INVALID_ARG or some such?

It's really an unexpected condition, as I understand it, we always store the
image bits in nsImageGtk as 24bpp image data.

> 
> >+    if (format == gfxIFormats::RGB_A1)
> >+      alphaDepth = 1;
> >+    else
> >+      alphaDepth = 8;
> 
> switch instead?  I can trivially imagine a post-SVG world in which someone adds
> another format.
> 

fixed (I thought I remembered switch with an IDL enum generating a compiler
warning but it doesn't seem to be for this).

> Shame that libpr0n doesn't have some function to do all this munging for you...

gfx, rather... but as far as our internal representation we never deal with
combining the image and alpha channels.

> 
> >+  gdk_pixbuf_save(alphaPixbuf ? alphaPixbuf : pixbuf, aPath.get(),
> >+                  "png", NULL, NULL);
> 
> Can this fail?	I bet it can, and we should report if it does, I think.
> 

fixed.

> >+  nsCOMPtr<nsIDOMHTMLImageElement> imgElement(do_QueryInterface(aElement));
> >+  if (!imgElement) {
> >+    // XXX write background loading stuff!
> 
> Please to be filing a bug on that!  (But if we don't enable the
> set-as-background menu item until it's loaded, maybe we just return error here
> and forget about it?)
> 

I'm not sure; I copied this straight from nsWindowsShellService.

> > extern "C" NS_GFX_(PRBool) NS_HexToRGB(const nsString& aBuf, nscolor* aResult);
> >+extern "C" NS_GFX_(PRBool) NS_ASCIIHexToRGB(const nsCString& aBuf,
> >+                                            nscolor* aResult);
> > 
> > // Translate a hex string to a color. Return true if it parses ok,
> > // otherwise return false.
> >@@ -96,6 +96,8 @@ extern "C" NS_GFX_(PRBool) NS_LooseHexTo
> > // Translate a color to a hex string and prepend a '#'.
> > // The returned string is always 7 characters including a '#' character.
> > extern "C" NS_GFX_(void) NS_RGBToHex(nscolor aColor, nsAString& aResult);
> >+extern "C" NS_GFX_(void) NS_RGBToASCIIHex(nscolor aColor,
> >+                                          nsAFlatCString& aResult);
> 
> (It's totally nuts that we need to manually strip the # for hex-to-rgb, when
> it's prepended by rgb-to-hex, BTW.)

Kind of silly.  I'll poke and see how many call sites would have to change to
make it sane.

> 
> >+extern "C" NS_GFX_(void) NS_RGBToASCIIHex(nscolor aColor,
> >+                                          nsAFlatCString& aResult)
> >+{
> >+  aResult.SetLength(7);
> >+  char *buf = aResult.BeginWriting();
> >+  PR_snprintf(buf, 8, "#%02x%02x%02x",
> >+              NS_GET_R(aColor), NS_GET_G(aColor), NS_GET_B(aColor));
> >+}
> 
> Is SetLength(7) guaranteed to give us room to write the NUL byte that we'll get
> from PR_snprintf (by my casual reading)?

darin tells me yes.

Comment 19

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

Comment on attachment 152725 [details] [diff] [review]
new patch

sorry for taking so long...

+	  var shell =
Components.classes["@mozilla.org/browser/shell-service;1"]
+		       .getService(Components.interfaces.nsIShellService);


if ("@mozilla.org/browser/shell-service;1" in Components.classes) would work
w/o having to catch exceptions, I think

Index: browser/components/shell/src/nsGNOMEShellService.cpp

+  // Get the path we were launched from.

would be kinda nice if the directoryserviceprovider offered this... ah well

+    handler.Truncate(0);

.Truncate()

+    schemeList.Append("file");

so this only makes firefox tell nautilus about file:, not all the other
supported protocols?

+static const MimeTypeAssociation appTypes[] = {

hm... should xml be added to the list?

+WriteImage(const nsCString& aPath, gfxIImageFrame* aImage)

this could maybe make use of bug 245684, once that's fixed

+  nsCOMPtr<nsIDOMHTMLImageElement> imgElement(do_QueryInterface(aElement));
+  if (!imgElement) {

why this QI, why not just the nsIImageLoadingContnet one you do later?

+  gconf->SetString(NS_LITERAL_CSTRING(kDesktopImageKey),
+		    NS_LITERAL_CSTRING(""));
+  gconf->SetString(NS_LITERAL_CSTRING(kDesktopImageKey), filePath);

is there a reason for setting this to an empty string first?

Index: gfx/public/nsColor.h

bah, extern C functions, so you can't use overloading...

toolkit/components/gnome/nsGConfService.cpp

+  NS_ENSURE_TRUE(client, NS_ERROR_FAILURE);

NS_ERROR_NOT_AVAILABLE?


should the GConfClient maybe be stored as a member variable, so it doesn't need
to be gotten each time?

toolkit/components/gnome/nsGnomeVFSService.cpp

+    if (!array->AppendCString(nsDependentCString((char*) list->data)))
+      return NS_ERROR_OUT_OF_MEMORY;

failure case leaks the array


+nsGnomeVFSService::GetAppForMimeType
that function seems to sometimes return a null app with NS_OK result... why not
return an error?
similarly for GetMimeTypeFromExtension, and GetDescriptionForMimeType

actually, what if (say) gnome_vfs_mime_get_description	returns null? doesn't
.Assign crash, then?

+  gnome_vfs_application_registry_add_mime_type(PromiseFlatCString(aID).get(),
+					      
PromiseFlatCString(aType).get());


missing space on second line

+    gnome_vfs_mime_set_extensions_list(PromiseFlatCString(aMimeType).get(),
+				    
PromiseFlatCString(aExtensionsList).get());

missing spaces on second line


Do GConf/gnome-vfs not require some kind of initialization?

Comment 20

[Brian Ryner (not reading)]

(In reply to comment #19)
> if ("@mozilla.org/browser/shell-service;1" in Components.classes) would work
> w/o having to catch exceptions, I think
> 

I factored this out into a getter that returns the shell service or null,
without throwing an exception.

> +    schemeList.Append("file");
> 
> so this only makes firefox tell nautilus about file:, not all the other
> supported protocols?
> 

It says "we support file:" but it does not say "we are the default handler for
file:" as we do for http.  That seemed likely to cause problems.

> +static const MimeTypeAssociation appTypes[] = {
> 
> hm... should xml be added to the list?
> 

We could... it's kind of touchy since xml can be so many different types of data.

> +  nsCOMPtr<nsIDOMHTMLImageElement> imgElement(do_QueryInterface(aElement));
> +  if (!imgElement) {
> 
> why this QI, why not just the nsIImageLoadingContnet one you do later?

changed.

> 
> +  gconf->SetString(NS_LITERAL_CSTRING(kDesktopImageKey),
> +		    NS_LITERAL_CSTRING(""));
> +  gconf->SetString(NS_LITERAL_CSTRING(kDesktopImageKey), filePath);
> 
> is there a reason for setting this to an empty string first?

There is, it's to force a nautilus refresh if you already had
Firefox_wallpaper.png as your background.  I added a comment in the code.

> toolkit/components/gnome/nsGConfService.cpp
> 
> +  NS_ENSURE_TRUE(client, NS_ERROR_FAILURE);
> 
> NS_ERROR_NOT_AVAILABLE?
> 

changed.

> 
> should the GConfClient maybe be stored as a member variable, so it doesn't need
> to be gotten each time?

yeah I guess I could do that...

> 
> toolkit/components/gnome/nsGnomeVFSService.cpp
> 
> +    if (!array->AppendCString(nsDependentCString((char*) list->data)))
> +      return NS_ERROR_OUT_OF_MEMORY;
> 
> failure case leaks the array
> 

fixed.

> 
> +nsGnomeVFSService::GetAppForMimeType
> that function seems to sometimes return a null app with NS_OK result... why not
> return an error?
> similarly for GetMimeTypeFromExtension, and GetDescriptionForMimeType
> 
> actually, what if (say) gnome_vfs_mime_get_description	returns null? doesn't
> .Assign crash, then?
> 
> +  gnome_vfs_application_registry_add_mime_type(PromiseFlatCString(aID).get(),
> +					      
> PromiseFlatCString(aType).get());
> 
> 
> missing space on second line

intentional, to keep it under 80 columns.

> 
> +    gnome_vfs_mime_set_extensions_list(PromiseFlatCString(aMimeType).get(),
> +				    
> PromiseFlatCString(aExtensionsList).get());
> 
> missing spaces on second line

same.

> Do GConf/gnome-vfs not require some kind of initialization?
> 

Good call.  I'll add that.

Comment 21

[Brian Ryner (not reading)]

Attachment: final patch to be checked in

Comment 22

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

>It says "we support file:" but it does not say "we are the default handler for
>file:" as we do for http.  That seemed likely to cause problems.

ah, saying we are the default handler means we don't have to say we can handle
http? ok, I guess that makes sense :)

Comment 23

[Brian Ryner (not reading)]

(In reply to comment #22)
> >It says "we support file:" but it does not say "we are the default handler for
> >file:" as we do for http.  That seemed likely to cause problems.
> 
> ah, saying we are the default handler means we don't have to say we can handle
> http? ok, I guess that makes sense :)
> 

No, everything that we do for file we do for http.  For file, http, https, ftp,
gopher, and chrome we say "we support this protocol".  For all of those _except_
file we also say "we are the default handler for this protocol".

Comment 24

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

+    schemeList.Append("file");

Ahhhh. you are right. Sorry, I had misread that as .Assign.

Comment 25

[Darin Fisher]

So, this broke the Minimo build, apparently because it does not have GConf-2.  I
think the check in Makefile.in to build toolkit/components/gnome needs to be
conditional on more than just MOZ_ENABLE_GTK2.  The Minimo build system has
GTK2, but it does not have GNOME2 (and friends).

My first reaction was to make it conditional on MOZ_XUL_APP, and that's what I
just checked in to fix the bustage.  But, now I realize that that is not quite
what we want.  I didn't realize that toolkit was now going to be used by more
than just the "birds."

I think a better fix would be to test MOZ_ENABLE_GTK2, MOZ_ENABLE_GCONF,
MOZ_ENABLE_LIBGNOME, and MOZ_ENABLE_GNOMEVFS.  It looks like we'll need to add
more defines to autoconf.mk.in for these.  I won't have time to do so tonight.

By the way, I'm glad to see that we can now make extensions/gnomevfs utilize
nsIGnomeVFSService and no longer require extensions/gnomevfs to be built as a
standalone component DSO :-)

Comment 26

[Adam D. Moss]

I guess that's bug 251703 then...

Comment 27

[Adam D. Moss]

Darin, did that semifix hit AVIARY?

Comment 28

[Brian Ryner (not reading)]

marking FIXED.

Comment 29

[sairuh (rarely reading bugmail)]

this works nicely on fedora core 1 and 2, but doesn't quite work on rh9. rather
than reopening this, I've spun off bug 252527.

Comment 30

[sairuh (rarely reading bugmail)]

vrfy'd fixed with recent aviary1.0 (branch) builds (7/20-7/22) on fedora core 1
and 2. thanks a lot for implementing this, Brian!

Comment 31

[Iain MacDonnell]

The fix for this has broken builds on GTKv1 - see bug 252207.