Re: Notes looking at sweettooth-plugin

[Owen Taylor <otaylor redhat com>]

On Fri, 2011-09-02 at 00:24 -0400, Jasper St. Pierre wrote:
> On Thu, Sep 1, 2011 at 3:36 PM, Owen Taylor <otaylor redhat com> 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