Re: Notes looking at sweettooth-plugin

["Jasper St. Pierre" <jstpierre mecheye net>]

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?

>  I think we would just stick npapi.h into the gnome-shell tree to
>  avoid sticky build-time dependencies.

Done.

> * The 'funcs' variable should be static.

Done.

> * We should require the origin to be https://extensions.gnome.org

I created a separate #define for it: ALLOW_INSECURE_HTTP.

> * 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.

> * For extra safety, check that the null-terminated length of
>  document.location.href matches the length in the NPString.

Done.

> * You should be checking the return value of funcs.getvalue,
>  funcs.getproperty. I also think it's better not to g_assert()
>  when checking for the right types, just fail cleanly.

Done.

> * 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!)

> * 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...

> * Might be good to have a uuid-validation function that validates
>  to characters that should be in a uuid and call that on all uuids.
>  I don't see anything in the shell code that would obviously have
>  problems with a uuid like '../../foo' but better to catch such things
>  upfront.

Done. "&", "<", ">", "/" and "\\" are now considered invalid
characters in a UUID. Besides that, they are required to be valid
ASCII printable characters, ranging from 32-126 inclusive.

> * I don't particularly like the use of g_return_if_fail() for checking
>  for invalid plugin API use. While I think we can be confident that
>  nobody is going to compile the plugin with -DG_DISABLE_ASSERT,
>  g_return_if_fail() is stylistically supposed to be a debugging aid
>  and not a reliable check.

I've replaced them with regular returns.

> * plugin_object_get_property() doesn't properly propagate back errors
>  from plugin_get_api_version/plugin_get_shell_version

Fixed.

>
> _______________________________________________
> gnome-shell-list mailing list
> gnome-shell-list gnome org
> http://mail.gnome.org/mailman/listinfo/gnome-shell-list
>



-- 
  Jasper