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) <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment|review?([email protected] |review- #8544752 Flags|.net) | --- Comment #31 from Karl Tomlinson (:karlt) <[email protected]> 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. _______________________________________________ fonts-bugs mailing list [email protected] https://admin.fedoraproject.org/mailman/listinfo/fonts-bugs http://fonts.fedoraproject.org/
