[Bug 467729] RFE: Add font autoinstallation support

[Bugzilla@Mozilla]

Do not reply to this email. You can add comments to this bug at
https://bugzilla.mozilla.org/show_bug.cgi?id=467729

Karl Tomlinson (:karlt) <karlt at mozbugz.karlt.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Attachment|review?(karlt at mozbugz.karlt |review-
     #8544752 Flags|.net)                       |

--- Comment #31 from Karl Tomlinson (:karlt) <karlt at mozbugz.karlt.net> 2015-02-12 16:44:38 PST ---
Comment on attachment 8544752
  --> https://bugzilla.mozilla.org/attachment.cgi?id=8544752
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.

-- 
Configure bugmail: https://bugzilla.mozilla.org/userprefs.cgi?tab=email

-------------------------------
Product/Component: Firefox :: General



------- You are receiving this mail because: -------
You are on the CC list for the bug.