Chris Coulson Assignee
	Description • 14 years ago

With the GNOME 3.0 release approaching, there are a few changes that affect the integration of Firefox in the desktop. The notable ones are:

- GIO no longer delegates the lookup of protocol handlers to gvfs (which used to just get them from /desktop/gnome/url_handlers in GConf). Instead, GIO uses the existing mimetype mechanism for looking up protocol handlers. This breaks the ability of Firefox to set itself as the default browser, as the settings it uses are ignored in the latest GLib version from GIT master.

- GConf is being deprecated in favour of GSettings. Firefox is currently using GConf for setting the desktop background. Whilst there will be a GConf<->GSettings bridge during the transition, it only propagates settings from GSettings to GConf, for the benefit of applications that are just watching settings. This means Firefox won't be able to set the desktop background.

Patches coming.....

	Chris Coulson Assignee
	Comment 1 • 14 years ago

This patch isn't entirely related to this bug and I was going to report it separately, but it touches code that the following patches also touch.

This just implements bug 593948 (but for Firefox) and means that distributors who are currently source patching Firefox to hard-code /usr/bin/firefox (including us) will be able to stop doing this.

	Chris Coulson Assignee
	Comment 2 • 14 years ago

This fixes some issues with the current implementation of nsGIOService:

- In nsGIOMimeApp::GetCommand, libgio-2.0.so is normally only provided by -devel packages.

- In nsGIOMimeApp::GetCommand, the logic around the check of dlerror() is reversed

- nsGIOMimeApp::GetCommand only returns a single character because cmd is declared incorrectly

- nsGIOMimeApp::Launch should handle empty URI's, as the native GIO API supports this too

- nsGIOService::CreateAppFromCommand uses fragile logic for matching an existing GAppInfo to what we want (as specified in the comment). By comparing brandShortName to the return of g_app_info_get_name, it means that this check will only work where distributors put the display name "Firefox" in their desktop file (ie, "Mozilla Firefox" will break, as will anything else). Also, g_app_info_get_commandline seems inappropriate here, as that returns the entire Exec= line from the desktop file (including the URI placeholders). g_app_info_get_executable seems more appropriate, so I've updated it to return a GAppInfo with the same executable as Firefox.

	Chris Coulson Assignee
	Comment 3 • 14 years ago

This patch ports the protocol handler setting bits to use GIO, by using the new "x-scheme-handler/" mimetypes, and makes the default browser setting work on the latest version of GLib

	Chris Coulson Assignee
	Comment 4 • 14 years ago

This adds GSettings support

	Chris Coulson Assignee
	Comment 5 • 14 years ago

This patch ports the desktop background settings to GSettings, with a GConf fallback

	Chris Coulson Assignee
	Comment 6 • 14 years ago

Hmmm, I guess I need to update patch 4 so that Firefox can still build with pre-GSettings versions of GLib

	Chris Coulson Assignee
	Comment 7 • 14 years ago

Update of the fifth patch to fix a typo

	Chris Coulson Assignee
	Comment 8 • 14 years ago

Updated patch to allow building with older versions of GLib

	Chris Coulson Assignee
	Comment 9 • 14 years ago

Another update to write the correct type to picture-options

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 10 • 14 years ago

Thanks Chris!

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 11 • 14 years ago

+    gchar *fullpath = g_find_program_in_path(tmp);
+    if (fullpath && mAppPath.Equals(fullpath)) {
+      mAppIsInPath = PR_TRUE;
+      g_free(fullpath);
+    }

You're leaking fullpath when !mAppPath.Equals(fullpath)

+    tmp = g_find_program_in_path(launcher);
+    if (!tmp)
+      return PR_FALSE;

You're leaking 'tmp'

Otherwise looks good.

	Chris Coulson Assignee
	Comment 12 • 14 years ago

Ooh, thanks, I should have spotted those before! Will update that in the morning.

Did you want to review the other patches too?

	Chris Coulson Assignee
	Comment 13 • 14 years ago

Oh, actually, in the second case - tmp isn't leaked because it's just a NULL pointer. The first case is definately leaked though, so I'll fix that

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 14 • 14 years ago

(In reply to comment #13)
> Oh, actually, in the second case - tmp isn't leaked because it's just a NULL
> pointer.

It is leaked in the non-NULL case. I didn't quote the right code.

(In reply to comment #12)
> Did you want to review the other patches too?

I could, but Karl Tomlinson is a better reviewer for this stuff.

	Chris Coulson Assignee
	Comment 15 • 14 years ago

Here is patch 1 updated to not leak fullpath.

Note, in the case where tmp is not NULL, it is always free'd at the end of nsGNOMEShellService::GetAppPathFromLauncher() -

+  }
+
+  g_free(tmp);
+  return PR_TRUE;
+}

	Karl Tomlinson (:karlt)
	Comment 16 • 14 years ago

Comment on attachment 490341 [details] [diff] [review]
Patch 2 - Fix nsGIOService

Can you include function names and more context in future patches, please?
Mercurial can be configured to do this automatically as in the [diff] section
here:
https://developer.mozilla.org/en/Installing_Mercurial#Configuration

(In reply to comment #2)
> - nsGIOMimeApp::Launch should handle empty URI's, as the native GIO API
> supports this too

>   char *uri = strdup(PromiseFlatCString(aUri).get());
> 
>-  if (!uri)
>+  if (!uri && !aUri.IsEmpty())
>     return NS_ERROR_OUT_OF_MEMORY;

I don't understand this change.
AIUI strdup returns NULL only on OOM, so the change is unnecessary.
For empty URIs, uri would point to a "\0" string.

   "The  strdup()  function shall return a pointer to a new string on
    success. Otherwise, it shall return a null pointer and set  errno
    to indicate the error."

>+      // If the executable is not absolute, get it's full path
>+      executable = g_find_program_in_path(g_app_info_get_executable(app_info_from_list));

|executable| is not freed.

>+      if (!executable)
>+        executable = strdup(g_app_info_get_executable(app_info_from_list));

Is this necessary?

>+
>+      if (!executable) {
>+        // This GAppInfo has no Exec field
>+        g_object_unref(app_info_from_list);
>+        continue;
>+      }

On "continue", "apps_p = apps_p->next" won't be executed, leading to an
infinite loop or memory access error.

Instead test executable in the next block:

>+      if (strcmp(executable, PromiseFlatCString(cmd).get()) == 0) {

"if (executable && strcmp()) ...

	Karl Tomlinson (:karlt)
	Comment 17 • 14 years ago

For g_find_program_in_path to match cmd, some kind of symbolic link resolution would be helpful, but g_app_info_get_commandline would have had the same problem, so this is really a separate issue.  (It doesn't need to be resolved in this patch.)

	Chris Coulson Assignee
	Comment 18 • 14 years ago

(In reply to comment #16)
> Comment on attachment 490341 [details] [diff] [review]
> Patch 2 - Fix nsGIOService
> 
> Can you include function names and more context in future patches, please?
> Mercurial can be configured to do this automatically as in the [diff] section
> here:
> https://developer.mozilla.org/en/Installing_Mercurial#Configuration
> 

Sure, I didn't know that before. Thanks.

> (In reply to comment #2)
> > - nsGIOMimeApp::Launch should handle empty URI's, as the native GIO API
> > supports this too
> 
> >   char *uri = strdup(PromiseFlatCString(aUri).get());
> > 
> >-  if (!uri)
> >+  if (!uri && !aUri.IsEmpty())
> >     return NS_ERROR_OUT_OF_MEMORY;
> 
> I don't understand this change.
> AIUI strdup returns NULL only on OOM, so the change is unnecessary.
> For empty URIs, uri would point to a "\0" string.
> 
>    "The  strdup()  function shall return a pointer to a new string on
>     success. Otherwise, it shall return a null pointer and set  errno
>     to indicate the error."
> 

That's a misunderstanding on my part - I misunderstood what was really happening when calling that function with EmptyCString() - PromiseFlatCString(aUri).get() returns an allocated string pointing to '\0' rather than a NULL pointer (which would have crashed in strdup anyway). I've reverted that bit now.

> >+      // If the executable is not absolute, get it's full path
> >+      executable = g_find_program_in_path(g_app_info_get_executable(app_info_from_list));
> 
> |executable| is not freed.
>
 
Fixed now.

> >+      if (!executable)
> >+        executable = strdup(g_app_info_get_executable(app_info_from_list));
> 
> Is this necessary?
> 

Thinking about it, probably not. A desktop file with an Exec line which isn't absolute or in the users PATH isn't going to work anyway. I've removed that bit.

> >+
> >+      if (!executable) {
> >+        // This GAppInfo has no Exec field
> >+        g_object_unref(app_info_from_list);
> >+        continue;
> >+      }
> 
> On "continue", "apps_p = apps_p->next" won't be executed, leading to an
> infinite loop or memory access error.
> 
> Instead test executable in the next block:
> 
> >+      if (strcmp(executable, PromiseFlatCString(cmd).get()) == 0) {
> 
> "if (executable && strcmp()) ...

That makes more sense, fixed.

Just testing the updated patch now, and then I will attach it.

	Karl Tomlinson (:karlt)
	Comment 20 • 14 years ago

Comment on attachment 491026 [details] [diff] [review]
Patch 2 - Fix nsGIOService (v2)

>+  // We so this by comparing each GAppInfo's executable with out own

"We *do*"

>+  char *executable;

>+    executable = NULL;

>+      executable = g_find_program_in_path(g_app_info_get_executable(app_info_from_list));

>+    g_free(executable);

In Mozilla's C++ code, we usually try to initialize at declaration (though
that doesn't seem to be happening so much in this file).
Use of |executable| can be entirely contained to the "if (!app_info)" block.
i.e.
      char *executable =
        g_find_program_in_path(g_app_info_get_executable(app_info_from_list));

and move g_free(executable) to the end of the "if (!app_info)" block.

r=me with that nit touched up.

	Chris Coulson Assignee
	Comment 21 • 14 years ago

Thanks. Here is the patch with the 2 additional changes

	Karl Tomlinson (:karlt)
	Comment 22 • 14 years ago

Comment on attachment 490342 [details] [diff] [review]
Patch 3 - Use GIO for protocol handler settings

So far, I've looked at only the GIOService changes:

>+  nsCAutoString contentType(NS_LITERAL_CSTRING("x-scheme-handler/"));

'nsCAutoString contentType("x-scheme-handler/")'
should work here I assume.

>+  g_app_info_set_as_default_for_type(mApp,
>+                                     PromiseFlatCString(contentType).get(),

I expect contentType.get() would be fine here,

>+              PromiseFlatCString(aURIScheme).get(),

but what you have here is correct because aURIScheme is an abstract nsACString
without a get() method.

I notice that (the existing) SetAsDefaultForMimeType only sets anything if the
content_type is already registered, but the behavior in this patch of adding
new content types is correct, right?  i.e.  I assume it makes sense to add new
arbitrary content types?

>+    nsGIOMimeApp *mozApp = new nsGIOMimeApp(app_info);
>+    NS_ENSURE_TRUE(mozApp, NS_ERROR_OUT_OF_MEMORY);

I know this code comes from elsewhere, but Mozilla's age-old myth that
(throwing) new might return NULL is now dead, so NS_ENSURE_TRUE is not needed
here.

>--- a/xpcom/system/nsIGIOService.idl	Fri Nov 12 23:29:13 2010 +0000
>+++ b/xpcom/system/nsIGIOService.idl	Fri Nov 12 23:40:51 2010 +0000
>@@ -62,6 +62,7 @@
>   void launch(in AUTF8String uri);
>   void setAsDefaultForMimeType(in AUTF8String mimeType);
>   void setAsDefaultForFileExtensions(in AUTF8String extensions);
>+  void setAsDefaultForURIScheme(in AUTF8String uriScheme);

nsIGIOMimeApp needs a new uuid for this change
(even though it is backward compatible).

>@@ -87,6 +88,9 @@
>      should not include a leading dot. */
>   AUTF8String        getMimeTypeFromExtension(in AUTF8String extension);
> 
>+  /* Obtain the preferred application for opening a given URI scheme */
>+  nsIGIOMimeApp      getAppForURIScheme(in AUTF8String aURIScheme);
>+

Similarly for nsIGIOService.

	Karl Tomlinson (:karlt)
	Comment 23 • 14 years ago

Comment on attachment 490342 [details] [diff] [review]
Patch 3 - Use GIO for protocol handler settings

>-  if (!gconf)
>+  if (!gconf && !giovfs)

SetDesktopBackground, GetDesktopBackgroundColor, and
SetDesktopBackgroundColorwill need a check that gconf is available.

>+  if (!KeyMatchesAppName(command.get()))
>+    return PR_FALSE; // the handler is disabled or set to another app

IIUC this means "the handler is set to another app".

	Chris Coulson Assignee
	Comment 24 • 14 years ago

Hi,

Thanks for the review, and sorry for the delay (combination of other work and vacation). I'll take a look at this again this week.

	Wolfgang Rosenauer [:wolfiR]
	Comment 25 • 14 years ago

While I've seen this bug before I wasn't aware it contains some bits we are missing already (w/o Gnome 3). Some stuff is already broken in recent Gnome 2 systems and need to be fixed for upcoming distributions asap.
bug 624338 ; bug 624530
I find it a bit weird to mix these things into one bug.

	Chris Coulson Assignee
	Comment 26 • 14 years ago

Here is an updated patch 1 so that it applies again (after http://hg.mozilla.org/mozilla-central/rev/f518eb4285bf)

	Chris Coulson Assignee
	Comment 27 • 14 years ago

Here is an updated patch 3, with the review points incorporated

	Chris Coulson Assignee
	Comment 28 • 14 years ago

Patch 4, un-bitrotted

	Chris Coulson Assignee
	Comment 29 • 14 years ago

Patch 5, un-bitrotted

	Karl Tomlinson (:karlt)
	Comment 30 • 14 years ago

Comment on attachment 503224 [details] [diff] [review]
Patch 1 - Use MOZ_APP_LAUNCHER for default browser executable (v3, un-bitrotted))

(I only reviewed the difference between this and attachment 490545 [details] [diff] [review].)

	Hussam Al-Tayeb
	Comment 31 • 14 years ago

Does usage of libgnomeui fall under this bug too?

	Chris Coulson Assignee
	Comment 32 • 14 years ago

No, this is specifically to address things which just won't work in GNOME 3.0, although, as pointed out by Wolfgang, it also fixes a couple of things that are already broken in GNOME 2.x, but it was easier to fix them in one go rather than attach patches in multiple places.

	Hussam Al-Tayeb
	Comment 33 • 14 years ago

(In reply to comment #32)
> No, this is specifically to address things which just won't work in GNOME 3.0,
> although, as pointed out by Wolfgang, it also fixes a couple of things that are
> already broken in GNOME 2.x, but it was easier to fix them in one go rather
> than attach patches in multiple places.

Ok thanks for the clarification and sorry for the bug spam.

	Karl Tomlinson (:karlt)
	Comment 34 • 14 years ago

I see that g_settings_schema_new() aborts the application if schema is not
installed on the system.

So the only obvious approach I see is to search linearly through an unsorted list of strings describing installed schemas to check whether the schema is installed.
With names "org.gnome.desktop.background", I assume there could be a very
large number of schemas.

Is this API really meant to be used by applications?
Or is there some server we should be prodding?

I can't see a way to add schema names after an application has started.  To ensure that settings can be set for each schema, is the idea that all applications install compiled schema descriptions containing all the schemas that they intend to use, setting XDG_DATA_DIRS or GSETTINGS_SCHEMA_DIR if the schema list is not installed in the default system location?
I'm not clear on whether this works when multiple applications install compiled schema descriptions for the same schema.

	Vincent Untz
	Comment 35 • 13 years ago

(In reply to comment #34)
> I see that g_settings_schema_new() aborts the application if schema is not
> installed on the system.
> 
> So the only obvious approach I see is to search linearly through an unsorted
> list of strings describing installed schemas to check whether the schema is
> installed.
> With names "org.gnome.desktop.background", I assume there could be a very
> large number of schemas.

Well, the idea is that apps rely on those generic schemas to be installed; if you really want to make sure they exist, I'd suggest only checking for one as all org.gnome.desktop.* schemas are distributed together anyway.

> Is this API really meant to be used by applications?

Yes.

> Or is there some server we should be prodding?

No.

> I can't see a way to add schema names after an application has started.  To
> ensure that settings can be set for each schema, is the idea that all
> applications install compiled schema descriptions containing all the schemas
> that they intend to use, setting XDG_DATA_DIRS or GSETTINGS_SCHEMA_DIR if the
> schema list is not installed in the default system location?

No, that's really not what is wanted. The idea is that the system schemas are provided by the system.

I guess your main concern is about the compiled binaries you're distributing, since people can use them but there's no way to add a requirement on the gsettings-desktop-schemas package. Is there any assumption made on the host system for those? Or do you expect them to run on any system?

	Chris Coulson Assignee
	Comment 36 • 13 years ago

(In reply to comment #35)

> I guess your main concern is about the compiled binaries you're distributing,
> since people can use them but there's no way to add a requirement on the
> gsettings-desktop-schemas package. Is there any assumption made on the host
> system for those? Or do you expect them to run on any system?

Note, that the patches here don't make any assumption on the host system. They're designed to gracefully fall back to GConf if the required schemas are not installed, which I think is the desired behaviour

	Karl Tomlinson (:karlt)
	Comment 37 • 13 years ago

(In reply to comment #35)
> I guess your main concern is about the compiled binaries you're distributing,
> since people can use them but there's no way to add a requirement on the
> gsettings-desktop-schemas package. Is there any assumption made on the host
> system for those? Or do you expect them to run on any system?

Yes, binaries distributed from mozilla.com and perhaps distributed by OS vendors.  mozilla.com binaries do not assume a GNOME system and I hope OS packages don't either.  They do assume that GTK is installed.

Currently, if gconf is installed, the browsers checks on start-up whether it is the default browser.  During start-up, I don't want to search through (or have GSettings create) a list of every schema installed.  (This is not such a big issue for setting the default background, but I gather the intention is to migrate all GConf use to GSettings.)

Is there a more efficient way that we can check for gsettings-desktop-schemas?
Perhaps a core GNOME library (and version?) that we are likely to use and also aborts the application when gsettings-desktop-schemas is not installed?
(I guess libgnome-2 is deprecated in GNOME 3.)

What happens when a system schema is deprecated.  Will it continue to be included forever (so that g_settings_schema_new does not abort) but get marked as obsolete somehow?

Maybe searching through an unsorted unbounded schema name list isn't so bad if most applications do this and so all the data is already paged in.  Is there reason to assume this is true?

	Vincent Untz
	Comment 38 • 13 years ago

(In reply to comment #37)
> Currently, if gconf is installed, the browsers checks on start-up whether it is
> the default browser.

(Note that this is deprecated; starting with glib 2.28, you need to check what application is associatied with the x-scheme-handler/http mime type)

> During start-up, I don't want to search through (or have
> GSettings create) a list of every schema installed.  (This is not such a big
> issue for setting the default background, but I gather the intention is to
> migrate all GConf use to GSettings.)

I don't expect g_settings_list_schemas() to be that slow, really: it doesn't even list files in a directory, but open one file with mmap(); this should be a very fast operation. You shouldn't be afraid to use it.

> Is there a more efficient way that we can check for gsettings-desktop-schemas?
> Perhaps a core GNOME library (and version?) that we are likely to use and also
> aborts the application when gsettings-desktop-schemas is not installed?
> (I guess libgnome-2 is deprecated in GNOME 3.)

You could check for the existence of libgnome-desktop-3.so.0, but that's really ugly and I'm sure it will fail somehow in the future. Like, when we bump the soversion of libgnome-desktop-3.

> What happens when a system schema is deprecated.  Will it continue to be
> included forever (so that g_settings_schema_new does not abort) but get marked
> as obsolete somehow?

As far as I know, yes, it will be included forever.

	Karl Tomlinson (:karlt)
	Comment 39 • 13 years ago

(In reply to comment #38)
> (In reply to comment #37)
> > Currently, if gconf is installed, the browsers checks on start-up whether it is
> > the default browser.
> 
> (Note that this is deprecated; starting with glib 2.28, you need to check what
> application is associatied with the x-scheme-handler/http mime type)

Oh, thanks.  I overlooked that we wouldn't be moving that to GSettings.
In that case hopefully we won't need to query GSettings on start up so this doesn't matter (if it is slow).  I'll try to give these patches another look over soon, but have some pressing FF4 things to do first.

	Christian Persch (GNOME) (away; not receiving bug mail)
	Comment 40 • 13 years ago

Comment on attachment 503226 [details] [diff] [review]
Patch 4 - Add nsIGSettingsService to support GSettings (v3)

>+  char **list = (char **)malloc(sizeof(char *)*(length + 1));
[...]
>+    list[i] = (char *)strdup((const char *)NS_ConvertUTF16toUTF8(realString).get());
[...]
>+  g_strfreev(list);

Mismatched allocators here.


SetStringList/GetStringList would probably be much nicer (and easier to implement) if they took/returned a nsIUTF8StringEnumerator instead of nsIArray (see e.g. nsGIOService.cpp).

	Karl Tomlinson (:karlt)
	Comment 41 • 13 years ago

Comment on attachment 503226 [details] [diff] [review]
Patch 4 - Add nsIGSettingsService to support GSettings (v3)

>+[scriptable, uuid(09637d3c-3c07-40b4-aff9-1d2a0f046f3c)]
>+interface nsIGSettingsCollection : nsISupports
>+{
>+  void          setString(in AUTF8String key, in AUTF8String value);
>+  void          setBoolean(in AUTF8String key, in boolean value);
>+  void          setInt(in AUTF8String key, in long value);
>+  void          setStringList(in AUTF8String key, in nsIArray value);
>+  AUTF8String   getString(in AUTF8String key);
>+  boolean       getBoolean(in AUTF8String key);
>+  long          getInt(in AUTF8String key);
>+  nsIArray      getStringList(in AUTF8String key);
>+};

I'm not comfortable providing what looks like generic API but is only safe to
use for some set of key/method combinations that depends on the particular
collection.  With this API, the implementation would need to check that the key is contained in the schema and that value is of the same type as indicated in the schema.  (Unlike GConf, the GSettings functions used here don't make those
checks.)

However, at this stage, GSettings is only used for desktop background.  If
there is no immediate need beyond that, and if "org.gnome.desktop.background"
is a stable well-defined binary API (I don't know how version or feature/key
checking is performed), then accessing that directly from nsGNOMEShellService
is probably the simplest solution.

>+typedef gboolean (*_g_settings_set_string_fn) (GSettings* settings,
>+                                              const char* key,
>+                                              const char* value);
>+typedef gboolean (*_g_settings_set_boolean_fn) (GSettings* settings,
>+                                               const char* key,
>+                                               gboolean value);

I was thinking that instead using g_settings_set(settings, key, format, ...)
could save a few symbol lookups, but, if only "org.gnome.desktop.background"
is needed, then there are not so many variants as to make much difference.

	Bastien Nocera
	Comment 42 • 13 years ago

(In reply to comment #41)
<snip>
> I'm not comfortable providing what looks like generic API but is only safe to
> use for some set of key/method combinations that depends on the particular
> collection.  With this API, the implementation would need to check that the key
> is contained in the schema and that value is of the same type as indicated in
> the schema.  (Unlike GConf, the GSettings functions used here don't make those
> checks.)
> 
> However, at this stage, GSettings is only used for desktop background.  If
> there is no immediate need beyond that, and if "org.gnome.desktop.background"
> is a stable well-defined binary API (I don't know how version or feature/key
> checking is performed), then accessing that directly from nsGNOMEShellService
> is probably the simplest solution.

Just a note that the background key name will change soon (good thing we're still pre-3.0). The current one is broken, as it expects a UTF-8 string for a filename. See https://bugzilla.gnome.org/show_bug.cgi?id=633983

	Bastien Nocera
	Comment 43 • 13 years ago

As sent to desktop-devel-list:

---8<---
The setting to change the background picture has changed names:
https://bugzilla.gnome.org/show_bug.cgi?id=633983

So if you were using "picture-filename" before, you'd need to use "picture-uri" now. Note that gnome-desktop, which is the library setting the background only supports local (file://) URIs.

I've already made the necessary changes to gsettings-desktop-schemas, gnome-desktop and gnome-control-center.
---8<---

Note that this will only affect the next GNOME release, to give time to application developers to make the changes.

	Karl Tomlinson (:karlt)
	Comment 44 • 13 years ago

Thanks for the info.

Sounds like we do need to check the keys then.
I guess g_settings_list_keys() is the way to do that.  If there is a better way, please let us know.  The docs do say "You should probably not be calling this function from "normal" code (since you should already know what keys are in your schema)", but this is not our schema.  Is it not "normal" to use system schemas?

Everything seems to be trying to say that this is the wrong way to do things.
But I don't see a right way.

Given how complex this is to use, a nsIGSettingsService does sound a useful place to concentrate the code.  We could drop the more complex get/setStringList which is unused at this stage.

	Chris Coulson Assignee
	Comment 45 • 13 years ago

(In reply to comment #44)
> 
> Given how complex this is to use, a nsIGSettingsService does sound a useful
> place to concentrate the code.  We could drop the more complex
> get/setStringList which is unused at this stage.

Sure, I don't mind doing this. I only added them really for feature parity with the existing nsIGConfService

(In reply to comment #43)
> As sent to desktop-devel-list:
> 
> ---8<---
> The setting to change the background picture has changed names:

I'm guessing we don't need to support "picture-filename" in Firefox, seeing as this key won't exist in a released version of GNOME?

	Karl Tomlinson (:karlt)
	Comment 46 • 13 years ago

(In reply to comment #45)
> I'm guessing we don't need to support "picture-filename" in Firefox, seeing as
> this key won't exist in a released version of GNOME?

Yes, I agree we shouldn't bother with "picture-filename".

	Karl Tomlinson (:karlt)
	Comment 47 • 13 years ago

Comment on attachment 503226 [details] [diff] [review]
Patch 4 - Add nsIGSettingsService to support GSettings (v3)

A few minor points:

Please unload gioLib in ~nsGSettingsService.
(Helps with our leak detection.)

>+  PRBool res = _g_settings_set_string(mSettings,
>+                                      PromiseFlatCString(aKey).get(),
>+                                      PromiseFlatCString(aValue).get());
>+
>+  return res ? NS_OK : NS_ERROR_FAILURE;

Use gboolean for the local variable "res" from functions that return gboolean.

>+      GSettings *settings = _g_settings_new(PromiseFlatCString(schema).get());
>+      if (!settings)
>+        return NS_ERROR_OUT_OF_MEMORY;
>+      nsGSettingsCollection *mozGSettings = new nsGSettingsCollection(settings);
>+      NS_ENSURE_TRUE(mozGSettings, NS_ERROR_OUT_OF_MEMORY);

|new| won't return NULL, and I think the same applies to g_settings_new.

>         nsIGIOService.idl \
>+	nsIGSettingsService.idl \
>         nsIAccelerometer.idl \

Follow local file style re spaces/tabs.

	Bastien Nocera
	Comment 48 • 13 years ago

(In reply to comment #46)
> (In reply to comment #45)
> > I'm guessing we don't need to support "picture-filename" in Firefox, seeing as
> > this key won't exist in a released version of GNOME?
> 
> Yes, I agree we shouldn't bother with "picture-filename".

Indeed.

	Chris Coulson Assignee
	Comment 49 • 13 years ago

Patch 3 un-bitrotted again.

I'm not sure whether I'm meant to request review again on these.

	Chris Coulson Assignee
	Comment 50 • 13 years ago

Here is an update for the nsIGSettingsService implementation, with the get/set stringlist methods removed, and the other comments addressed.

I'll update the background settings patch tomorrow.

	Karl Tomlinson (:karlt)
	Comment 51 • 13 years ago

Comment on attachment 518476 [details] [diff] [review]
Patch 3 - Use GIO for protocol handler settings (v3)

> I'm not sure whether I'm meant to request review again on these.

Reviews are usually not necessary for merges.  That is unless the merged changes affect the behaviour of the patch.  Perhaps true in this case: your patch had already fixed the bug involved in the merge.

	Karl Tomlinson (:karlt)
	Comment 52 • 13 years ago

Comment on attachment 518477 [details] [diff] [review]
Patch 4 - Add nsIGSettingsService to support GSettings (v4)

Some of comment 41 is still relevant.

AFAIS GSettings is designed for *application* settings.  It assumes
that an application knows the schema for its own settings and so the various
accessor functions assume the application knows which functions are safe to
use and with which arguments.

It seems GSettings has also been used for *system* settings, which is why it
has to be used here.

System setting schemas have changed and will change and so we have to protect
against such changes by checking that the key is in the schema and that the
type specified in the schema matches what we expect.

This checking should be done on the implementation side of
nsIGSettingsCollection.  (I don't want the client to have to work out which
methods and which keys are safe to try.)

	Bastien Nocera
	Comment 53 • 13 years ago

(In reply to comment #52)
> Comment on attachment 518477 [details] [diff] [review]
> Patch 4 - Add nsIGSettingsService to support GSettings (v4)
> 
> Some of comment 41 is still relevant.
> 
> AFAIS GSettings is designed for *application* settings.  It assumes
> that an application knows the schema for its own settings and so the various
> accessor functions assume the application knows which functions are safe to
> use and with which arguments.
> 
> It seems GSettings has also been used for *system* settings, which is why it
> has to be used here.
> 
> System setting schemas have changed and will change

No, they will not change past 3.0. Some might be added, but they should not change. We don't want GNOME 3.0 applications crashing on GNOME 3.2 any more than you do.

> and so we have to protect
> against such changes by checking that the key is in the schema and that the
> type specified in the schema matches what we expect.
> 
> This checking should be done on the implementation side of
> nsIGSettingsCollection.  (I don't want the client to have to work out which
> methods and which keys are safe to try.)

	Hiroyuki Ikezoe (:hiro)
	Comment 54 • 13 years ago

Comment on attachment 491149 [details] [diff] [review]
Patch 2 - Fix nsGIOService (v3)

>+    if (!app_info) {
>+      // If the executable is not absolute, get it's full path
>+      char *executable = g_find_program_in_path(g_app_info_get_executable(app_info_from_list));

Please use gchar* instead of char*. I am not comfortable that the variable which is declared as char* is freed by g_free.
Maybe nobody does not mind it though.

>+      if (executable && strcmp(executable, PromiseFlatCString(cmd).get()) == 0) {

Can't we use g_strcmp0 here?

>+        g_object_ref (app_info_from_list);

Should remove a space in front of paren.

	Karl Tomlinson (:karlt)
	Comment 55 • 13 years ago

Would it make sense to use GnomeBG rather than going directly to GSettings?

http://library.gnome.org/devel/gnome-desktop/2.32/GnomeBG.html
http://library.gnome.org/devel/gnome-desktop3/unstable/GnomeBG.html

	Bastien Nocera
	Comment 56 • 13 years ago

(In reply to comment #55)
> Would it make sense to use GnomeBG rather than going directly to GSettings?
> 
> http://library.gnome.org/devel/gnome-desktop/2.32/GnomeBG.html
> http://library.gnome.org/devel/gnome-desktop3/unstable/GnomeBG.html

You want to replace a stable setting, with a piece of software with no API guarantees, which is only used in actual implementation of drawing the background. No.

Furthermore, gnome-bg has no code to *write* to GSettings.

(In reply to comment #54)
> Comment on attachment 491149 [details] [diff] [review]
> Patch 2 - Fix nsGIOService (v3)
> 
> >+    if (!app_info) {
> >+      // If the executable is not absolute, get it's full path
> >+      char *executable = g_find_program_in_path(g_app_info_get_executable(app_info_from_list));
> 
> Please use gchar* instead of char*. I am not comfortable that the variable
> which is declared as char* is freed by g_free.
> Maybe nobody does not mind it though.

It's a typedef.

	Karl Tomlinson (:karlt)
	Comment 57 • 13 years ago

(In reply to comment #56)
> You want

No.  I asked a question.

> to replace a stable setting,

I don't know why you call this "stable".

How are versioning and deprecated parts of schemas handled?

On my system, I see gtranslator and gitg packages that pull in
gsettings-desktop-schemas.  There is also evince but that may be an error in
the package as I can't see that evince uses the schemas.

That means there are already installed schemas that don't match the post
https://bugzilla.gnome.org/show_bug.cgi?id=633983 schema.

We don't need to be able to set the background with these older schemas but we
need to ensure that the functions and parameters we use are safe.

> with a piece of software with no API guarantees,

This answers my question, thanks.
I failed to notice the GNOME_DESKTOP_USE_UNSTABLE_API check in the header
file.

> Furthermore, gnome-bg has no code to *write* to GSettings.

I thought gnome_bg_save_to_preferences might have done that.

	Karl Tomlinson (:karlt)
	Comment 58 • 13 years ago

What is the distinction between org.gnome.desktop.default-applications.browser and GAppInfo/x-scheme-handler?

	Bastien Nocera
	Comment 59 • 13 years ago

(In reply to comment #57)
> (In reply to comment #56)
> > You want
> 
> No.  I asked a question.
> 
> > to replace a stable setting,
> 
> I don't know why you call this "stable".
> 
> How are versioning and deprecated parts of schemas handled?
> 
> On my system, I see gtranslator and gitg packages that pull in
> gsettings-desktop-schemas.  There is also evince but that may be an error in
> the package as I can't see that evince uses the schemas.
> 
> That means there are already installed schemas that don't match the post
> https://bugzilla.gnome.org/show_bug.cgi?id=633983 schema.
> 
> We don't need to be able to set the background with these older schemas but we
> need to ensure that the functions and parameters we use are safe.

Anything pre-3.0 gsettings-desktop-schemas is very obviously subject to change, but that's what you should target.

> > with a piece of software with no API guarantees,
> 
> This answers my question, thanks.
> I failed to notice the GNOME_DESKTOP_USE_UNSTABLE_API check in the header
> file.
> 
> > Furthermore, gnome-bg has no code to *write* to GSettings.
> 
> I thought gnome_bg_save_to_preferences might have done that.

Yes it does... I wouldn't touch that code if I were you. Nothing actually uses it. FWIW, I (re)wrote most of the background panel in the master control-center.

(In reply to comment #58)
> What is the distinction between org.gnome.desktop.default-applications.browser
> and GAppInfo/x-scheme-handler?

The former was removed and doesn't actually exist anymore (it was a straight GConf port of the schemas), and you should be using GAppInfo to set the default application to x-scheme-handler/http and x-scheme-handler/https.

	Karl Tomlinson (:karlt)
	Comment 60 • 13 years ago

(In reply to comment #59)
> you should be using GAppInfo to set the default
> application to x-scheme-handler/http and x-scheme-handler/https.

Good to know, thanks!

	Jan Horak [:jhorak]
	Comment 61 • 13 years ago

While there hasn't been many changes to desktop background patch I looked at it. I've just changed from picture-file to picture-uri and used NS_GetURLSpecFromFile to obtain valid uri from filename.

	Fernando Herrera
	Comment 62 • 13 years ago

We also need to port the code for checking if a11y is enabled at:

accessible/src/atk/nsApplicationAccessibleWrap.cpp
widget/src/gtk2/nsWindow.cpp

	Christopher Aillon (sabbatical, not receiving bugmail)
	Comment 63 • 13 years ago

Yeah, and the proxy settings too, but first we need to get the existing patchset in!

	Chris Coulson Assignee
	Comment 64 • 13 years ago

jhorak - thanks for that.

Fernando - yes, I will work on the other bits once we have the first 5 patches in. However, based on the discussion since the last review of patch 4, I'm slightly unclear about what I need to do be doing now.

	Karl Tomlinson (:karlt)
	Comment 65 • 13 years ago

(In reply to comment #64)
> However, based on the discussion since the last review of patch 4, I'm
> slightly unclear about what I need to do be doing now.

Comment 52 summarizes the situation.  Firefox is not tied to any particular GNOME version so we can't make the same assumptions that GNOME apps might make.  I would look at gsettings-tool.c for inspiration on how to check which functions are safe to use and with which arguments in the particular schema.

Think about whether it would be simpler to use g_settings_set(settings, key, format, ...) instead of looking for the various g_setting_set_* symbols.

Patches 1 to 3 can land now.

	Chris Coulson Assignee
	Comment 66 • 13 years ago

(In reply to comment #65)
> (In reply to comment #64)
> > However, based on the discussion since the last review of patch 4, I'm
> > slightly unclear about what I need to do be doing now.
> 
> Comment 52 summarizes the situation.  Firefox is not tied to any particular
> GNOME version so we can't make the same assumptions that GNOME apps might make.
>  I would look at gsettings-tool.c for inspiration on how to check which
> functions are safe to use and with which arguments in the particular schema.
> 
> Think about whether it would be simpler to use g_settings_set(settings, key,
> format, ...) instead of looking for the various g_setting_set_* symbols.
> 

Ok, thanks. I'll look at this again tomorrow, it's getting late for me now.

> Patches 1 to 3 can land now.

Excellent, that would be good as those are blocking a couple of other bugs :)

	Christopher Aillon (sabbatical, not receiving bugmail)
	Comment 67 • 13 years ago

(In reply to comment #65)
> Comment 52 summarizes the situation.  Firefox is not tied to any particular
> GNOME version so we can't make the same assumptions that GNOME apps might make.

No, but the user will have many more problems with the desktop if they are using the unstable GNOME, not just Firefox, and this is true in general for unstable software.  I don't think we need to worry about the pre-GNOME 3.0 series.  It guaranteed to be stable for GNOME 3.0+.  People should be running GNOME 3.0 at this point not the unstable series.  I don't think it's worth the effort to add code for the two or three users this might affect.

>  I would look at gsettings-tool.c for inspiration on how to check which
> functions are safe to use and with which arguments in the particular schema.
> 
> Think about whether it would be simpler to use g_settings_set(settings, key,
> format, ...) instead of looking for the various g_setting_set_* symbols.

This might be good for a followup bug, but don't think it should hold up the patch here.

(In reply to comment #52)
> System setting schemas have changed and will change and so we have to protect
> against such changes by checking that the key is in the schema and that the
> type specified in the schema matches what we expect.

Again, it only changed during the unstable series leading up to GNOME 3, but with GNOME 3 it is guaranteed stable.  I really don't think we need to worry about this.

	Karl Tomlinson (:karlt)
	Comment 68 • 13 years ago

(In reply to comment #67)
> No, but the user will have many more problems with the desktop if they are
> using the unstable GNOME, not just Firefox, and this is true in general for
> unstable software.

Users will not have desktop problems if they are not using GNOME.
I don't want the presence of some GNOME components on the system to cause the app to crash.

> I don't think we need to worry about the pre-GNOME 3.0
> series.  It guaranteed to be stable for GNOME 3.0+.

I have no idea what that means.  The GSettings/GConf GTK2/3 situations are just a small part of the evidence that people want things to change.  The only question is when it will change.  Not being prepared for such change would be a mistake.

> I don't think it's worth the
> effort to add code for the two or three users this might affect.

It's unfortunate that the API has been designed so that it is an effort to deal with this.  I don't know the history but it looks like the problem comes from the decision to use an application settings system for system settings.

	Karl Tomlinson (:karlt)
	Comment 69 • 13 years ago

Adding checkin-needed for patches 1 to 3.  In the future, it would be better to separate issues into different bug reports to make following changesets easier.

	Christopher Aillon (sabbatical, not receiving bugmail)
	Comment 70 • 13 years ago

(In reply to comment #68)
> > I don't think we need to worry about the pre-GNOME 3.0
> > series.  It guaranteed to be stable for GNOME 3.0+.
> 
> I have no idea what that means.  The GSettings/GConf GTK2/3 situations are just
> a small part of the evidence that people want things to change.  The only
> question is when it will change.  Not being prepared for such change would be a
> mistake.

Yes, but if so that would be a major library change in eg GNOME 4, which we can only be ready for by modularizing things, as they are already done now.

If the question is whether key names will be changing, no they won't.  They changed between GNOME 3 alpha and GNOME 3 beta, but are now frozen with official GA release of GNOME 3.  If keys were to change at this point, given the fact that it would abort whatever process, there would be bigger problems than Firefox crashing.

	Karl Tomlinson (:karlt)
	Comment 71 • 13 years ago

(In reply to comment #70)
> Yes, but if so that would be a major library change in eg GNOME 4, which we can
> only be ready for by modularizing things, as they are already done now.

What do you mean by "modularizing things"?

> If the question is whether key names will be changing, no they won't.  They
> changed between GNOME 3 alpha and GNOME 3 beta, but are now frozen with
> official GA release of GNOME 3.  If keys were to change at this point, given
> the fact that it would abort whatever process, there would be bigger problems
> than Firefox crashing.

Can you point me at the details of these guarantees please?

Firefox crashing is a big problem to me, but the "bigger" problem in GNOME argument doesn't work because GIO and Gecko are not tied to GNOME.

	(no longer active)
	Comment 72 • 13 years ago

The first three patches landed on cedar:
http://hg.mozilla.org/projects/cedar/rev/500983e631b1
http://hg.mozilla.org/projects/cedar/rev/1c4241b5a0d4
http://hg.mozilla.org/projects/cedar/rev/fdd8cf56674f

	Christopher Aillon (sabbatical, not receiving bugmail)
	Comment 73 • 13 years ago

(In reply to comment #71)
> What do you mean by "modularizing things"?

What we're already doing with nsIGConfService, nsIGsettingsService.  I imagine if things break, it will because we need a nsIGnomeHotNewThing service in the future, not because keys change.

> Can you point me at the details of these guarantees please?
> 
> Firefox crashing is a big problem to me, but the "bigger" problem in GNOME
> argument doesn't work because GIO and Gecko are not tied to GNOME.

Aside from what was already said by Bastien in comment 53 and Vincent in comment 38, and IRC conversations in multiple places, there surprisingly isn't any official reference even though it seems to be generally accepted.  I've pointed this out to Matthias who kindly started this thread: https://mail.gnome.org/archives/release-team/2011-April/msg00223.html

	(no longer active)
	Comment 74 • 13 years ago

http://hg.mozilla.org/mozilla-central/rev/500983e631b1
http://hg.mozilla.org/mozilla-central/rev/1c4241b5a0d4
http://hg.mozilla.org/mozilla-central/rev/fdd8cf56674f

	Chris Coulson Assignee
	Comment 75 • 13 years ago

Ok, here is an updated version of patch 4 with lots of extra sanity checking.

Now, it is impossible to pass values via the nsIGSettingsInterface which would cause the application to be aborted, and an error is returned on all other invalid conditions that wouldn't otherwise cause an abort (eg, an invalid assumption on the type of a key when doing a read).

On all reads, it checks if the key exists in order to avoid aborting on invalid keys. It also uses the generic g_settings_get_value function for reads, and then checks the type of the GVariant is what we expect it to be, so that we can catch errors and propagate them back to the application.

On all writes, it uses the g_settings_range_check function to verify that the given value is of the correct type for the key.

	Karl Tomlinson (:karlt)
	Comment 76 • 13 years ago

Comment on attachment 525775 [details] [diff] [review]
Patch 4 - Add nsIGSettingsService to support GSettings (v5)

This is looking great, thanks!

> On all writes, it uses the g_settings_range_check function to verify that the
> given value is of the correct type for the key.

_range_check is very useful.
That requires glib 2.28 (at run time).  I'm fine with that.

>+#define G_VARIANT_TYPE_INT32        ((const GVariantType *) "i")
>+#define G_VARIANT_TYPE_BOOLEAN      ((const GVariantType *) "b")
>+#define G_VARIANT_TYPE_STRING       ((const GVariantType *) "s")
>+#define G_VARIANT_TYPE_OBJECT_PATH  ((const GVariantType *) "o")
>+#define G_VARIANT_TYPE_SIGNATURE    ((const GVariantType *) "g")

Won't these already be defined on some systems through glib.h?  I assume that
a separate #ifdef G_VARIANT_TYPE_* for each is not necessary and picking one
to test for a single #ifdef block enclosing them all would be fine.

>+  FUNC(g_variant_new_int32, GVariant *, (int value)) \

Stick with "gint32".  Otherwise this is depending on the ABI of the platform.

>+  if (mKeys)
>+    g_strfreev(mKeys);

g_strfreev is null-safe, so no need to test mKeys.

>+  if (!mKeys)
>+    return PR_FALSE;

I don't think this check is necessary.

>+  const char *key = PromiseFlatCString(aKey).get();
>+
>+  for (PRUint32 i = 0; mKeys[i] != NULL; i++) {
>+    if (!g_strcmp0(mKeys[i], key))
>+      return PR_TRUE;
>+  }

AFAIK this is not appropriate usage of PromiseFlatCString as it may create a
temporary buffer that would become invalid.

There's an mKeys[i] null test already, so it is safe to use
(aKey.Equals(mKeys[i])).

>+    g_variant_unref(aValue);

Interesting.  Normally the pattern with floating references is _ref_sink
before the _unref.  It seems a bit strange to remove a reference when the
object isn't owned, but GVariant is implemented to work fine with just the
_unref.

>+NS_IMETHODIMP
>+nsGSettingsCollection::GetString(const nsACString& aKey,
>+                                 nsACString& aResult)
>+{
>+  aResult.Truncate();

The pattern used in xpcom interfaces is that out parameters are not modified
when the method fails, so don't truncate.

>+  const guchar *data = (const guchar *) g_variant_get_data(value);
>+  if (data)
>+    *aResult = *data != 0 ? PR_TRUE : PR_FALSE;
>+  else
>+    *aResult = PR_FALSE;

Are the the serializations from _get_data well defined?

(If not, g_variant_get is an option.)

>+  *aResult = *(PRInt32 *) g_variant_get_data(value);

I think the return value from _get_data should be null-checked.

>+  if (!gioLib) {
>+    gioLib = PR_LoadLibrary("libgio-2.0.so.0");

Need to return early with an error here if this fails.

>+    if (strcmp(schemas[i], PromiseFlatCString(schema).get()) == 0) {

(schema.Equals(schemas[i]))

	Chris Coulson Assignee
	Comment 77 • 13 years ago

Thanks for reviewing.

(In reply to comment #76)
> Comment on attachment 525775 [details] [diff] [review]
> 
> >+#define G_VARIANT_TYPE_INT32        ((const GVariantType *) "i")
> >+#define G_VARIANT_TYPE_BOOLEAN      ((const GVariantType *) "b")
> >+#define G_VARIANT_TYPE_STRING       ((const GVariantType *) "s")
> >+#define G_VARIANT_TYPE_OBJECT_PATH  ((const GVariantType *) "o")
> >+#define G_VARIANT_TYPE_SIGNATURE    ((const GVariantType *) "g")
> 
> Won't these already be defined on some systems through glib.h?  I assume that
> a separate #ifdef G_VARIANT_TYPE_* for each is not necessary and picking one
> to test for a single #ifdef block enclosing them all would be fine.
> 

Yeah, I will fix this to only define them if they aren't already in the headers. I defined them here to avoid needing to bump the build-time requirements on glib (to 2.24)

> >+  FUNC(g_variant_new_int32, GVariant *, (int value)) \
> 
> Stick with "gint32".  Otherwise this is depending on the ABI of the platform.
> 

Ok, will fix that too.

> >+  if (mKeys)
> >+    g_strfreev(mKeys);
> 
> g_strfreev is null-safe, so no need to test mKeys.
> 
 Good catch, I always make this mistake ;)

> >+  if (!mKeys)
> >+    return PR_FALSE;
> 
> I don't think this check is necessary.
> 

Won't it crash when we dereference this later on (mKeys[0])? I'll think about that that, maybe I missed another detail.

> >+  const char *key = PromiseFlatCString(aKey).get();
> >+
> >+  for (PRUint32 i = 0; mKeys[i] != NULL; i++) {
> >+    if (!g_strcmp0(mKeys[i], key))
> >+      return PR_TRUE;
> >+  }
> 
> AFAIK this is not appropriate usage of PromiseFlatCString as it may create a
> temporary buffer that would become invalid.
> 
> There's an mKeys[i] null test already, so it is safe to use
> (aKey.Equals(mKeys[i])).
> 

Ok, I can fix this too.

> >+    g_variant_unref(aValue);
> 
> Interesting.  Normally the pattern with floating references is _ref_sink
> before the _unref.  It seems a bit strange to remove a reference when the
> object isn't owned, but GVariant is implemented to work fine with just the
> _unref.

Yeah, this is a bit strange. I didn't call ref_sink on it, else it wouldn't become owned by gsettings when calling g_settings_set_value, and calling ref_sink immediately before unref seemed a bit wasteful.

> 
> >+NS_IMETHODIMP
> >+nsGSettingsCollection::GetString(const nsACString& aKey,
> >+                                 nsACString& aResult)
> >+{
> >+  aResult.Truncate();
> 
> The pattern used in xpcom interfaces is that out parameters are not modified
> when the method fails, so don't truncate.

Ah, ok. I thought I should return an empty string. I'll fix that.

> 
> >+  const guchar *data = (const guchar *) g_variant_get_data(value);
> >+  if (data)
> >+    *aResult = *data != 0 ? PR_TRUE : PR_FALSE;
> >+  else
> >+    *aResult = PR_FALSE;
> 
> Are the the serializations from _get_data well defined?
> 
> (If not, g_variant_get is an option.)

I'm not too sure about that. I think they are, but perhaps we are safer with g_variant_get

> 
> >+  *aResult = *(PRInt32 *) g_variant_get_data(value);
> 
> I think the return value from _get_data should be null-checked.
> 

Good catch, thanks!

> >+  if (!gioLib) {
> >+    gioLib = PR_LoadLibrary("libgio-2.0.so.0");
> 
> Need to return early with an error here if this fails.
> 

And here :)

> >+    if (strcmp(schemas[i], PromiseFlatCString(schema).get()) == 0) {
> 
> (schema.Equals(schemas[i]))

Will fix that too.

Thanks!

	timeless
	Comment 78 • 13 years ago

>+  if (data)
>+    *aResult = *data != 0 ? PR_TRUE : PR_FALSE;
>+  else
>+    *aResult = PR_FALSE;


*aResult = !!(data && *data);

if you really want to use ?: you can, but there's really no need for if/else

	Chris Coulson Assignee
	Comment 79 • 13 years ago

(In reply to comment #78)
> >+  if (data)
> >+    *aResult = *data != 0 ? PR_TRUE : PR_FALSE;
> >+  else
> >+    *aResult = PR_FALSE;
> 
> 
> *aResult = !!(data && *data);
> 
> if you really want to use ?: you can, but there's really no need for if/else

Thanks.

However, this bit has gone from the updated version anyway (after thinking about one of Karls comments) - I've just converted GetInt and GetBoolean to use g_variant_get_int32 and g_variant_get_boolean respectively (rather than messing around with g_variant_get_data). I've already type-checked the variant, so I'm not sure why I didn't just use these in the first place (and I'm already using g_variant_get_string)

	Chris Coulson Assignee
	Comment 80 • 13 years ago

Here is the updated version.

I've dropped the use of g_variant_get_data entirely. I also had to add a call to KeyExists() in SetValue(), as I just discovered that g_settings_range_check doesn't actually check if the key exists, and will abort if you pass an invalid key.

> >+  if (!mKeys)
> >+    return PR_FALSE;
> 
> I don't think this check is necessary.
> 

I've dropped this too. g_settings_list_keys doesn't return NULL if the schema has no keys.

	Karl Tomlinson (:karlt)
	Comment 81 • 13 years ago

Comment on attachment 523978 [details] [diff] [review]
Patch 5 - Port desktop background settings to GSettings (v5)

(In reply to comment #61)
> While there hasn't been many changes to desktop background patch I looked at
> it. I've just changed from picture-file to picture-uri and used
> NS_GetURLSpecFromFile to obtain valid uri from filename.

There are more differences to v4 than described here.  I suspect they are due
to your patch being in a different order in the series, but this patch doesn't
make quite as much sense without the earlier changes.  Can you update this
patch now that the others in this bug have landed, please?

In SetDesktopBackground(), when WriteImage() fails, it would be better not to
write the non-existant filePath to the settings.  Returning early after
WriteImage() would also save the awkwardness of remembering return values
around the nsILocalFile creation.  All the background_settings->Set* methods
should be kept together so that either all or none of them are called.

I suspect g_filename_from_uri would be simpler than nsILocalFile and
NS_GetURLSpecFromFile.

	Jan Horak [:jhorak]
	Comment 82 • 13 years ago

Sending better version with following changes:
- checking WriteImage function result
- set properties put together
- replaced NS_GetURLSpecFromFile by g_filename_to_uri
- options and colorString strings moved ahead of gsettings code for sharing with gconf code
- sync to trunk

	Karl Tomlinson (:karlt)
	Comment 83 • 13 years ago

Comment on attachment 530595 [details] [diff] [review]
Patch 5 - Port desktop background settings to GSettings (v6)

>+      GError *err = NULL;
>+      gchar *file_uri = g_filename_to_uri(filePath.BeginReading(), NULL, &err);

Normal practice is to use filePath.get() here.

Just pass NULL as the GError argument.

>+      NS_ENSURE_TRUE(file_uri != NULL, NS_ERROR_FAILURE);

We're trying to avoid NS_ENSURE_* macros in newer code because they hide the
flow of the code.  I can tolerate NS_ENSURE_SUCCESS with XPCOM results, but
here I much prefer

  if (!file_uri)
    return NS_ERROR_FAILURE;

	Jan Horak [:jhorak]
	Comment 84 • 13 years ago

- Removed GError and NS_ENSURE_TRUE
- using .get() instead of BeginReading()
- Added missing configure.in changes for minimum gio version.
I hope this could be check-in without review (?)

	Karl Tomlinson (:karlt)
	Comment 85 • 13 years ago

Comment on attachment 531298 [details] [diff] [review]
Patch 5 - Port desktop background settings to GSettings (v7)

(In reply to comment #84)
> - Added missing configure.in changes for minimum gio version.

> -GIO_VERSION=2.0
> +GIO_VERSION=2.18

Where did this come from?
(I don't see any build-time GIO requirements in this patch.)

	Jan Horak [:jhorak]
	Comment 86 • 13 years ago

(In reply to comment #85)
> > -GIO_VERSION=2.0
> > +GIO_VERSION=2.18
> 
> Where did this come from?
> (I don't see any build-time GIO requirements in this patch.)

Oh sorry I confused this patch with https://bugzilla.mozilla.org/show_bug.cgi?id=467168. Reposting patch without configure.in changes.

	Karl Tomlinson (:karlt)
	Comment 87 • 13 years ago

Thank you for everyone's help here.

http://hg.mozilla.org/mozilla-central/rev/8cc9e1405f73
http://hg.mozilla.org/mozilla-central/rev/1d6d08074c41