On Fri, 2011-09-02 at 00:24 -0400, Jasper St. Pierre wrote: > On Thu, Sep 1, 2011 at 3:36 PM, Owen Taylor <[email protected]> wrote: > > Some review comments reading the code to sweettooth-plugin: > > > > * I don't see any reason that this shouldn't just live in the > > gnome-shell tree - that would at least reduce the problem > > of the plugin and gnome-shell having incompatible versions. > > > > It would also avoid having packages with the sweettooth-plugin > > name; it's fine if the code deployed on extensions.gnome.org > > has a funky codename, but I don't want people listing software > > on a computer to have to know what sweettooth-plugin is. > > (Distributions can still have a gnome-shell-browser-plugin > > sub-package if they choose.) > > Should I file a bug for gnome-shell?
Yes > > * The origin checking code, as far as I can see, will consider > > http://extensions.gnome.org.mysite.com to be valid. It would be best > > if we can actually use an established URI parser to parse the URL. > > Could just link to libsoup and use soup-uri. > > I chose to use the two properties window.location.hostname and > window.location.protocol instead. Sounds good. > > * It seems like there should be a callback if the shell restarts > > while the plugin is running to allow the webpage to react/restart. > > Do you mean what happens if the DBus name is lost? Relatively easy to > do... (thanks davidz for gdbus!) Yeah, that's what I mean. > > * The handling of G_DBUS_ERROR_NAME_HAS_NO_OWNER in NPP_New > > is leaky and not as intended. > > I'm not sure exactly what you meant here, so... if (!data->proxy) { /* ignore error if the shell is not running, otherwise warn */ if (error->domain != G_DBUS_ERROR || error->code != G_DBUS_ERROR_NAME_HAS_NO_OWNER) { g_warning ("Failed to set up Shell proxy: %s", error->message); g_clear_error (&error); } ret = NPERR_GENERIC_ERROR; } So if error is set and is equal to NAME_IS_NO_OWNER, then we don't warn, and don't clear g_clear_error() (so leak), but with still return an error. - Owen _______________________________________________ gnome-shell-list mailing list [email protected] http://mail.gnome.org/mailman/listinfo/gnome-shell-list
