Last Comment Bug 467729 - RFE: Add font autoinstallation support
: RFE: Add font autoinstallation support
Status: RESOLVED FIXED
[read from comment 40 for documentation]
: dev-doc-needed, intl
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: unspecified
: x86 Linux
: -- enhancement with 2 votes (vote)
: Firefox 38
Assigned To: Frédéric Wang (:fredw)
:
Mentors:
https://github.com/fred-wang/MissingF...
Depends on: 1117742 619521
Blocks: mathml-fonts 1134141 1199921
  Show dependency treegraph
 
Reported: 2008-12-03 00:43 PST by Nicolas Mailhot
Modified: 2015-08-29 07:03 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Patch (12.93 KB, patch)
2014-03-23 14:01 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Review
Patch (14.08 KB, patch)
2014-03-29 05:48 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Review
Add a PackageKit XPCOM API (11.21 KB, patch)
2014-04-27 13:23 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Review
Add a PackageKit XPCOM API (14.28 KB, patch)
2014-04-29 12:45 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Review
Javascript Example to use the PackageKit XPCOM API (978 bytes, application/javascript)
2014-04-30 01:16 PDT, Frédéric Wang (:fredw)
no flags Details
PackageKitTest add-on demonstrating how to use the XPCOM API. (1.66 KB, application/x-xpinstall)
2014-10-31 11:31 PDT, Frédéric Wang (:fredw)
no flags Details
Add a PackageKit XPCOM API (15.54 KB, patch)
2014-10-31 11:36 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Review
Add a PackageKit XPCOM API (15.85 KB, patch)
2014-11-08 07:53 PST, Frédéric Wang (:fredw)
karlt: feedback+
Details | Diff | Review
PackageKitTest add-on demonstrating how to use the XPCOM API. (1.64 KB, application/x-xpinstall)
2014-11-08 07:56 PST, Frédéric Wang (:fredw)
no flags Details
Add a PackageKit XPCOM API (15.65 KB, patch)
2015-01-06 11:44 PST, Frédéric Wang (:fredw)
karlt: review-
Details | Diff | Review
Add a PackageKit XPCOM API (15.30 KB, patch)
2015-02-14 04:27 PST, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Review
Add a PackageKit XPCOM API - follow-up (2.34 KB, patch)
2015-02-16 14:34 PST, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Review
Add a PackageKit XPCOM API (15.56 KB, patch)
2015-02-16 23:59 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Review

User Story
This commit adds a new nsIPackageKitService XPCOM API to be able to perform PackageKit installation requests based on package names, mime types, font-config or GStreamer resources:

http://www.freedesktop.org/software/PackageKit/pk-intro.html
https://help.gnome.org/users/gnome-packagekit/index.html

For the detailed XPCOM API, see

https://hg.mozilla.org/mozilla-central/file/9696d1c4b3ba/xpcom/system/nsIPackageKitService.idl

as well as attachment 8519421 [details] for a sample add-on using that API.

This is only available on Gnome versions of applications (Linux and probably more Unix systems) with a GIO library that is recent enough to support PackageKit (in particular, this is unlikely to be the case for the official Linux builds by Mozilla until bug 627699 is fixed).

Using the new "font-needed" notification described at bug 619521 comment 151 and the installation method introduced here, Linux maintainers should be able to write extensions to support font autoinstallation, as asked in comment 0. See https://github.com/fred-wang/MissingFontsNotifier for a work-in-progress add-on.      
Description Nicolas Mailhot 2008-12-03 00:43:56 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; fr; rv:1.9.0.4) Gecko/2008102920 Firefox/3.0.4
Build Identifier: 

The Linux platform is gaining an on-demand font autoinstallation framework (BSD
and Solaris will probably follow eventually).

http://fedoraproject.org/wiki/Features/AutomaticFontInstallation

Firefox should be plugged into it and use it to request the fonts the pages it renders need. This would have a huge user impact, especially for pages that have strong i18n constrains.

Unlike proposals such as @font-face and EOT the fonts are sourced from trusted user-controlled sources, are properly vetted legal-side, use the latest font versions not old obsolete ones (with bug fixes), and are installed system-wide where other apps can take advantage of them.

Reproducible: Always
Comment 1 Nochum Sossonko [:Natch] 2008-12-03 09:22:03 PST
probably should be core->gfx?
Comment 2 Jesse Ruderman 2008-12-03 17:07:24 PST
I like this idea, as long as it doesn't cause loading http://wikipedia.org/ to toss up a modal dialog for each language I'm missing fonts for ;)
Comment 3 Frédéric Wang (:fredw) 2012-12-20 14:46:19 PST
Making this blocking the MathML font bug.

See http://lists.fedoraproject.org/pipermail/fonts/2012-July/001472.html
Comment 4 Frédéric Wang (:fredw) 2012-12-23 04:59:22 PST
I've done some investigations on PackageKit (http://www.packagekit.org/). If I understand correctly, there seem to be several ways to use it (http://www.packagekit.org/pk-using.html): via the GIO library (http://www.packagekit.org/files/session.c), the libpackagekit library or the raw DBus API. The Mozilla codebase contains some parts relying on the GIO library and other parts on DBus.

It seems to me that the package names are likely to depend on the Linux distributions. Also, I'm wondering if it would be best to use a high-level API (more convenient to use) or the low-level API (to support more platforms).

On the stable Debian version, I see a SessionInstaller package that implements the PackageKit DBus API but does not use libpackagekit (PackageKit is said to be available in the next Debian version but I don't know which package will be used by default). Similarly, I'm wondering if the PackageKit's DBus API is available on mobile platforms and thus would help to solve related bugs like bug 619521 and but 648548. The low-level DBus API seems to be used in FirefoxOS to implement stuff like Bluetooth support etc

BTW, here is the PackageKit DBus API. The basic idea would be to use PackageKit's CreateTransaction and call InstallPackages on the returned object, wait and force a reflow when the fonts are installed.
http://www.packagekit.org/gtk-doc/api-reference.html
http://gitorious.org/packagekit/packagekit/blobs/master/src/org.freedesktop.PackageKit.xml
http://gitorious.org/packagekit/packagekit/blobs/master/src/org.freedesktop.PackageKit.Transaction.xml
Comment 5 Karl Tomlinson (ni?:karlt) 2012-12-23 18:38:10 PST
GIO's GDBus is the new way for GLib/GTK apps such as Gecko apps to use DBus, but Firefox is built against an older GIO which doesn't have GDBus.  dbus-glib is probably the easiest way to use DBus, and Gecko already depends on dbus-glib.  See also Bug 811261 comment 3.

If libpackagekit is as widely available as the PackageKit DBus service then that is an option to consider, but it would have to be used via dlopen.
Comment 6 Nicolas Mailhot 2012-12-24 06:51:33 PST
(In reply to Frédéric Wang (:fredw) from comment #4)

> It seems to me that the package names are likely to depend on the Linux
> distributions.

Fedora-side at least the font provides are generated by fontconfig, and are not distribution-specific (and if those provides are insufficient for some distribution I do hope they'll get the fontconfig provides generator improved instead of inventing their own metadata)
Comment 7 Frédéric Wang (:fredw) 2014-03-23 14:01:17 PDT
Created attachment 8395450 [details] [diff] [review]
Patch

So I tried to unbitrot https://github.com/fred-wang/MozillaCentralPatches/blob/master/download-fonts-1.diff. It seems however, that it's no longer possible to determine if Web fonts are currently loading from mUserFontSet (at least without extending the API). Although I'm not exactly sure whether or not we want to wait these fonts before raising a warning...

This version exposes a scriptable XPCOM interface to install packages via PackageKit, so in theory it could be used from Mozilla add-ons (not tested yet). I guess that could be useful (for example to make the MathML-fonts add-on install the system package). PackageKit seems to have other useful method to install font packages by languages or gstreamer codec.

> GIO's GDBus is the new way for GLib/GTK apps such as Gecko apps to use DBus, but Firefox is built against an older GIO which doesn't have GDBus.

Do you know if this is still true? The patch seems to work for me with my system's GIO. At worse, I guess it could be a compilation define that the Linux distribution maintainers can enable for their Firefox's versions.

> Fedora-side at least the font provides are generated by fontconfig, and are not distribution-specific (and if those provides are insufficient for some distribution I do hope they'll get the fontconfig provides generator improved instead of inventing their own metadata)

For MathML, the plan after bug 407059, bug 947654 and others is to move to OpenType fonts with a MATH table (see https://developer.mozilla.org/en-US/docs/Mozilla_MathML_Project/MathML_Torture_Test for a list of such fonts). In particular, Latin Modern Math is likely to become the default.

At the moment, I've tried with the "InstallPackageNames" method. But for example the Debian package name for STIX is fonts-stix while the one of Fedora is stix-fonts. Do you think there is a less distrib-dependent PackageKit method or would the Linux maintainers have to write patches to modify the list of fonts to install?
Comment 8 Frédéric Wang (:fredw) 2014-03-23 14:05:33 PDT
The DBus interface on the packagekit.org WebSite does not seem up-to-date. Here is the interface I found for the PackageKit installation methods:

https://github.com/nekohayo/gnome-packagekit/blob/master/src/org.freedesktop.PackageKit.xml#L107

So "InstallProvideFiles" seems to be what Nicolas suggests in comment 6.
Comment 9 Karl Tomlinson (ni?:karlt) 2014-03-23 16:20:14 PDT
(In reply to Frédéric Wang (:fredw) from comment #7)
> > GIO's GDBus is the new way for GLib/GTK apps such as Gecko apps to use DBus, but Firefox is built against an older GIO which doesn't have GDBus.
> 
> Do you know if this is still true?

Yes.  Mozilla builds are still against CentOS 6.0 libraries.

There is a project underway to build against GTK+ 3 which will require updating the build systems.  That is at least several months away.
Comment 10 Nicolas Mailhot 2014-03-24 10:53:46 PDT
(In reply to Frédéric Wang (:fredw) from comment #8)
> The DBus interface on the packagekit.org WebSite does not seem up-to-date.
> Here is the interface I found for the PackageKit installation methods:
> 
> https://github.com/nekohayo/gnome-packagekit/blob/master/src/org.freedesktop.
> PackageKit.xml#L107
> 
> So "InstallProvideFiles" seems to be what Nicolas suggests in comment 6.

Yes the actual metadata in Fedora font packages looks like this:

$ rpm -q --provides sil-scheherazade-fonts
config(sil-scheherazade-fonts) = 2.020-1.fc21
font(:lang=aa)
font(:lang=an)
font(:lang=ar)
font(:lang=ay)
font(:lang=az-ir)
font(:lang=bi)
font(:lang=br)
font(:lang=ch)
font(:lang=co)
font(:lang=da)
font(:lang=de)
font(:lang=en)
font(:lang=es)
font(:lang=et)
font(:lang=eu)
font(:lang=fa)
font(:lang=fi)
font(:lang=fil)
font(:lang=fj)
font(:lang=fo)
font(:lang=fr)
font(:lang=fur)
font(:lang=fy)
font(:lang=gd)
font(:lang=gl)
font(:lang=gv)
font(:lang=ho)
font(:lang=ht)
font(:lang=ia)
font(:lang=id)
font(:lang=ie)
font(:lang=io)
font(:lang=is)
font(:lang=it)
font(:lang=jv)
font(:lang=kj)
font(:lang=ks)
font(:lang=ku-iq)
font(:lang=ku-ir)
font(:lang=kwm)
font(:lang=lah)
font(:lang=lb)
font(:lang=li)
font(:lang=mg)
font(:lang=ms)
font(:lang=nb)
font(:lang=nds)
font(:lang=ng)
font(:lang=nl)
font(:lang=nn)
font(:lang=no)
font(:lang=nr)
font(:lang=nso)
font(:lang=oc)
font(:lang=om)
font(:lang=ota)
font(:lang=pa-pk)
font(:lang=pap-an)
font(:lang=pap-aw)
font(:lang=ps-af)
font(:lang=ps-pk)
font(:lang=pt)
font(:lang=rm)
font(:lang=rn)
font(:lang=rw)
font(:lang=sc)
font(:lang=sd)
font(:lang=sg)
font(:lang=sma)
font(:lang=smj)
font(:lang=sn)
font(:lang=so)
font(:lang=sq)
font(:lang=ss)
font(:lang=st)
font(:lang=su)
font(:lang=sv)
font(:lang=sw)
font(:lang=tl)
font(:lang=tn)
font(:lang=ts)
font(:lang=ug)
font(:lang=ur)
font(:lang=uz)
font(:lang=vo)
font(:lang=vot)
font(:lang=wa)
font(:lang=xh)
font(:lang=yap)
font(:lang=za)
font(:lang=zu)
font(scheherazade)
sil-scheherazade-fonts = 2.020-1.fc21

all the font(foo) bits are generated at package build time by fc-query --format '%{=pkgkit}'

The fontconfig maintainer stated at the time he would be amenable to evolve the metadata contents if apps found missing bits once they started using this
Comment 11 Frédéric Wang (:fredw) 2014-03-24 11:02:28 PDT
(In reply to Nicolas Mailhot from comment #10)
> The fontconfig maintainer stated at the time he would be amenable to evolve
> the metadata contents if apps found missing bits once they started using this

Thanks. So for the MATH fonts using "font(latinmodernmath)", "font(stixmath)" etc will be more reliable.
Comment 12 Frédéric Wang (:fredw) 2014-03-24 13:41:39 PDT
(In reply to Frédéric Wang (:fredw) from comment #8)
> So "InstallProvideFiles" seems to be what Nicolas suggests in comment 6.

I meant InstallFontconfigResources...

(In reply to Frédéric Wang (:fredw) from comment #11)
> Thanks. So for the MATH fonts using "font(latinmodernmath)",
> "font(stixmath)" etc will be more reliable.

... but apparently, gnome-packagekit only accepts ":lang=" prefixes, so that won't work for math fonts.
https://git.gnome.org/browse/gnome-packagekit/tree/src/gpk-dbus-task.c#n2254
Comment 13 Frédéric Wang (:fredw) 2014-03-29 05:48:56 PDT
Created attachment 8398927 [details] [diff] [review]
Patch
Comment 14 Masatoshi Kimura [:emk] 2014-03-29 05:58:07 PDT
Is this Linux-specific?
Comment 15 Frédéric Wang (:fredw) 2014-04-26 08:24:18 PDT
So here is a possible plan of action:

* We'll keep the PackageKit XPCOM API of attachment 8398927 [details] [diff] [review] under an #ifdef MOZ_USE_PACKAGE_KIT. This will not be enabled by the official Mozilla builds until GIO is updated, but Linux maintainers could enable it if they wish.

* For the MathML code, instead of calling the PackageKit XPCOM API directly, we'll just send a missing font notification. For script/language, a similar notification will be added in bug 619521.

* For Android, the notification & installation for MathML or script/language will be handled by the front-end code (in 648548).

* We can implement an UI for other front-end codes if necessary (I think Neil was willing to implement one for Seamonkey)

* I'll try to write an add-on for Mozilla products (such as the one in bug 619521) that listens for notifications and tries to do "something clever" with them (i.e. try to call the PackageKit XPCOM API or otherwise display a warning, suggest a download page etc). Linux maintainers can create a package for that add-on and set appropriate dependencies. Keeping this add-on out of Gecko will make easier for people to customize the behavior and to maintain the list of fonts.

Any thoughts?

(In reply to Masatoshi Kimura [:emk] from comment #14)
> Is this Linux-specific?

I think it would be great to have this on other platforms too, but I'm not sure if something like PackageKit is available. At least we could use the notification to display a warning or something.
Comment 16 Frédéric Wang (:fredw) 2014-04-27 13:23:32 PDT
Created attachment 8413426 [details] [diff] [review]
Add a PackageKit XPCOM API
Comment 17 Frédéric Wang (:fredw) 2014-04-27 13:27:39 PDT
(In reply to Nicolas Mailhot from comment #10)
> The fontconfig maintainer stated at the time he would be amenable to evolve
> the metadata contents if apps found missing bits once they started using this

The "font-needed" notifications implemented in bug 619521 uses script tag (I believe it is http://www.unicode.org/iso15924/iso15924-codes.html). I guess it would help if fontconfig supports that too e.g. font(:script=Khmr). Otherwise we'll have to remap the script tags to language codes / package names...
Comment 18 Frédéric Wang (:fredw) 2014-04-29 12:45:09 PDT
Created attachment 8414705 [details] [diff] [review]
Add a PackageKit XPCOM API
Comment 19 Frédéric Wang (:fredw) 2014-04-30 01:16:28 PDT
Created attachment 8415068 [details]
Javascript Example to use the PackageKit XPCOM API

I asked about the :script tag on the FontConfig & PackageKit mailing lists:

http://lists.freedesktop.org/archives/packagekit/2014-April/026252.html
http://lists.freedesktop.org/archives/fontconfig/2014-April/005202.html

To use the patch you need the appropriate gio / packagekit packages installed and to add the following option in your Mozconfig

ac_add_options --enable-packagekit

I attach an example of how to use the Package XPCOM API (here to install tex-gyre and fonts-oflb-asana-math and print the result in the console)
Comment 20 Frédéric Wang (:fredw) 2014-10-31 11:31:01 PDT
Created attachment 8515147 [details]
PackageKitTest add-on demonstrating how to use the XPCOM API.
Comment 21 Frédéric Wang (:fredw) 2014-10-31 11:36:24 PDT
Created attachment 8515150 [details] [diff] [review]
Add a PackageKit XPCOM API

In this version I only use MOZ_ENABLE_GIO as a condition to build the PackageKit XPCOM API. I also tried to follow what was done for GSettingsService to load libgio dynamically and thus avoid the problem with older GIO. This seems to work with the add-on of attachment 8515147 [details].
Comment 22 Frédéric Wang (:fredw) 2014-11-08 07:53:57 PST
Created attachment 8519420 [details] [diff] [review]
Add a PackageKit XPCOM API

This replaces nsIPackageKitServiceCallback by nsIObserver, which is more classical. Also, this may also allow an observer to determine to which install request an install result corresponds (using the aSubject parameter and one nsIPackageKitService instance per transaction).
Comment 23 Frédéric Wang (:fredw) 2014-11-08 07:56:00 PST
Created attachment 8519421 [details]
PackageKitTest add-on demonstrating how to use the XPCOM API.
Comment 24 Karl Tomlinson (ni?:karlt) 2014-11-19 21:38:11 PST
Comment on attachment 8519420 [details] [diff] [review]
Add a PackageKit XPCOM API

Looks like the right approach and nice design.

>+   * @param   An object implementing nsIObserver that will be notified with
>+   *          a message of subject the nsIPackageKitService (this) and topic
>+   *          "packagekit-install". If the installation failed, the message
>+   *          data also contains the error returned by PackageKit.

Is the topic sufficient?  Could the subject be null?
That would mean that some parameters are unnecessary and could be removed.
Comment 25 Frédéric Wang (:fredw) 2014-11-19 23:19:19 PST
(In reply to Karl Tomlinson (:karlt) from comment #24)
> Is the topic sufficient?  Could the subject be null?
> That would mean that some parameters are unnecessary and could be removed.

- I could do as in my previous patches and define my own XPCOM callback class, but IIUC nsIObserver seems quite standard (even when not all the parameters are used). For example in https://bugzilla.mozilla.org/attachment.cgi?id=8520633&action=diff#a/mobile/android/chrome/content/FontLocator.js_sec2 we use one "observe" function to check all the notifications. That's also the approach I've taken for the MissingFontsNotifier add-on https://github.com/fred-wang/MissingFontsNotifier/blob/master/bootstrap.js#L91

- The aSubject parameter could probably be null and so I can avoid passing it to each callback functions. However, if a caller launches two PackageKit commands using different nsIPackageKit instances, the aSubject parameter will allow to determine from which instance the callback is coming (this does not help if the same nsIPackageKit is used though).
Comment 26 Karl Tomlinson (ni?:karlt) 2014-11-20 00:11:17 PST
(In reply to Frédéric Wang (:fredw) from comment #25)
> IIUC nsIObserver seems quite standard (even when not all the
> parameters are used).

Yes, nsIObserver is good.

> - The aSubject parameter could probably be null and so I can avoid passing
> it to each callback functions.

That was what I meant.

> However, if a caller launches two PackageKit
> commands using different nsIPackageKit instances, the aSubject parameter
> will allow to determine from which instance the callback is coming (this
> does not help if the same nsIPackageKit is used though).

There is usually only one instance of each xpcom service.

I assume multiple observers could be used to track the results of different calls, if necessary.
Comment 27 Frédéric Wang (:fredw) 2015-01-06 11:44:33 PST
Created attachment 8544752 [details] [diff] [review]
Add a PackageKit XPCOM API

Addressing comment 24
Comment 28 Frédéric Wang (:fredw) 2015-01-06 23:38:28 PST
A quick question (unrelated to font autoinstallation)... The patch on this bug adds a convenient method to install Gstreamer resources via PackageKit. So I wonder how we currently handle missing Gstreamer resources on Linux and if we could do something with this XPCOM API to automate installation via the package manager?
Comment 29 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2015-01-08 20:50:22 PST
AFAIK we don't do anything to handle missing GStreamer resources.

Maybe we could use this, but I don't think we care that much about helping Linux users install GStreamer codecs.
Comment 30 Karl Tomlinson (ni?:karlt) 2015-01-21 21:10:53 PST
(In reply to Frédéric Wang (:fredw) from comment #28)
> So I wonder how we currently handle missing Gstreamer resources on Linux and
> if we could do something with this XPCOM API to automate installation via
> the package manager?

I guess it would be possible, but perhaps complicated by synchronous web-facing APIs to return info on whether a codec is supported.  At least use of the APIs could be detected and codecs installed before the next page reload.
Comment 31 Karl Tomlinson (ni?:karlt) 2015-02-12 16:44:38 PST
Comment on attachment 8544752 [details] [diff] [review]
Add a PackageKit XPCOM API

This is mostly good, but some of the changes I've requested are a bit fiddly, so I'd like to review again.

>+nsPackageKitService::Init()
>+{
>+#define FUNC(name, type, params) { #name, (nsGDBusFunc *)&_##name },
>+  static const nsGDBusDynamicFunction kGDBusSymbols[] = {

I don't think there's any advantage in having the "static" here.
The function is run only once and position-independent objects mean that the
pointers can't be determined until runtime.  Better I think would be to remove
static, removing any risk of the compiler requesting additional relocations at
start-up.

>+struct InstallPackagesProxyCallData {
>+  nsCOMPtr<nsIObserver> observer;
>+  GDBusProxy* proxy;
>+};

There's no need to keep the GDBusProxy* here as this is provided to the
callback as |source_object|, and that can be cast with reinterpret_cast to
GDBusProxy*.  (g_dbus_proxy_call() will keep a reference to the proxy, so
g_object_unref() can be called on the proxy immediately after calling
g_dbus_proxy_call, if preferred.)

>+static void
>+InstallPackagesNotifyObserver(nsCOMPtr<nsIObserver>& aObserver,
>+                              gchar* aErrorMessage)

Passing by non-const reference implies that the nsCOMPtr might be modified.
It won't be modified here and using a reference in such cases is discouraged.

Instead this method can have a nsIObserver* parameter.  The call sites should
not need changing as nsCOMPtr<T> is implicitly converted to T*.

>+    InstallPackagesProxyCallData* data = new InstallPackagesProxyCallData;
>+    data->observer = userData->observer;

Once this data holds only the observer, there is no need to allocate a new
object just to hold the pointer.  I think what I'd do is

nsIObserver* observer;
userData->observer.forget(observer);

Then pass |observer| to g_dbus_proxy_call() and call observer->Release() in 
InstallPackagesProxyCallCallback where userData is deleted there.

>+                      static_cast<GAsyncReadyCallback>
>+                      (&InstallPackagesProxyCallCallback),

This cast shouldn't be necessary.

>+  gchar** packages = new gchar*[arrayLength + 1];

nsAutoArrayPtr<gchar*> so the delete need not be managed below.

>+    nsAutoCString utf8data = NS_ConvertUTF16toUTF8(data);

NS_ConvertUTF16toUTF8 utf8data(data), or see below.

>+    packages[i] = g_strdup(const_cast<gchar*>(utf8data.get()));

My documentation has "g_strdup (const gchar *str)", which would make the
const_cast unnecessary.

If you like, this can just be

  packages[i] = g_strdup(NS_ConvertUTF16toUTF8(data).get());

(The NS_ConvertUTF16toUTF8 destructor is not run until the complete statement
has been processed.)

>+    parameters = g_variant_new("(u^a&ss)", 0, packages, "hide-finished");

Please explicitly cast the constant '0' as the values are not converted for
the varargs function.  static_cast<guint32>(0)

The & has no effect in g_variant_new() so please remove so as not to imply
that the pointer will continue to be used in the GVariant.

>+                           static_cast<GAsyncReadyCallback>
>+                           (&InstallPackagesProxyNewCallback),

The cast here should be unnecessary.

>+   * @param   An object implementing nsIObserver that will be notified with
>+   *          a message of subject the nsIPackageKitService (this) and topic

Please update this doc now that there is no subject

>+   *          "packagekit-install". If the installation failed, the message
>+   *          data also contains the error returned by PackageKit.

Probably worth saying explicitly that a null data indicates success.
Comment 32 Frédéric Wang (:fredw) 2015-02-14 04:25:53 PST
(In reply to Karl Tomlinson (:karlt) from comment #31)
> >+nsPackageKitService::Init()
> >+{
> >+#define FUNC(name, type, params) { #name, (nsGDBusFunc *)&_##name },
> >+  static const nsGDBusDynamicFunction kGDBusSymbols[] = {
> 
> I don't think there's any advantage in having the "static" here.
> The function is run only once and position-independent objects mean that the
> pointers can't be determined until runtime.  Better I think would be to
> remove
> static, removing any risk of the compiler requesting additional relocations
> at
> start-up.

As I said in comment 21, this was copied from GSettings. However, I don't see any justification for the static keyword on bug 713827.
Comment 33 Frédéric Wang (:fredw) 2015-02-14 04:27:05 PST
Created attachment 8564519 [details] [diff] [review]
Add a PackageKit XPCOM API
Comment 34 Karl Tomlinson (ni?:karlt) 2015-02-15 18:26:59 PST
Comment on attachment 8564519 [details] [diff] [review]
Add a PackageKit XPCOM API

>+  nsAutoArrayPtr<gchar*> packages(new gchar*[arrayLength + 1]);
>+  NS_ENSURE_TRUE(packages, NS_ERROR_OUT_OF_MEMORY);

No need for NS_ENSURE_TRUE here as |new| will not return null.  (It would throw an exception, which would abort the program, if it failed.  The nothrow form of new could be used, but I expect this won't be called with large arrays, and so we can keep things simple here.)
Comment 35 Frédéric Wang (:fredw) 2015-02-16 14:34:44 PST
Created attachment 8565176 [details] [diff] [review]
Add a PackageKit XPCOM API - follow-up

Just a follow-up patch to add missing declarations...
Comment 36 Frédéric Wang (:fredw) 2015-02-16 23:59:20 PST
Created attachment 8565323 [details] [diff] [review]
Add a PackageKit XPCOM API
Comment 38 Ryan VanderMeulen [:RyanVM] 2015-02-17 11:49:41 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/58acfa044b49
Comment 39 Ryan VanderMeulen [:RyanVM] 2015-02-17 18:40:11 PST
https://hg.mozilla.org/mozilla-central/rev/58acfa044b49
Comment 40 Frédéric Wang (:fredw) 2015-02-18 03:16:02 PST
This commit adds a new nsIPackageKitService XPCOM API to be able to perform PackageKit installation requests based on package names, mime types, font-config or GStreamer resources:

http://www.freedesktop.org/software/PackageKit/pk-intro.html
https://help.gnome.org/users/gnome-packagekit/index.html

For the detailed XPCOM API, see

https://hg.mozilla.org/mozilla-central/file/9696d1c4b3ba/xpcom/system/nsIPackageKitService.idl

as well as attachment 8519421 [details] for a sample add-on using that API.

This is only available on Gnome versions of applications (Linux and probably more Unix systems) with a GIO library that is recent enough to support PackageKit (in particular, this is unlikely to be the case for the official Linux builds by Mozilla until bug 627699 is fixed).

Using the new "font-needed" notification described at bug 619521 comment 151 and the installation method introduced here, Linux maintainers should be able to write extensions to support font autoinstallation, as asked in comment 0. See https://github.com/fred-wang/MissingFontsNotifier for a work-in-progress add-on.

Note You need to log in before you can comment on or make changes to this bug.