Re: Notes looking at sweettooth-plugin
Il giorno dom, 11/09/2011 alle 16.40 -0400, Owen Taylor ha scritto: On Sun, 2011-09-11 at 14:57 +0200, Olav Vitters wrote: On Fri, Sep 02, 2011 at 12:24:18AM -0400, Jasper St. Pierre wrote: On Thu, Sep 1, 2011 at 3:36 PM, Owen Taylor otay...@redhat.com wrote: Some review comments reading the code to sweettooth-plugin: What is/will be the status for GNOME 3.2? I'm writing (together with others) the release notes and I'd like to say something about this. even if not for GNOME 3.2, we could put it in the looking forward section (but then really brief) if it is going to be ready, some screenshots would be nice. Ok to give me links and so on or tell me how to create screenshots. The client side stuff is largely there to make the site work, but it's not widely tested yet, and the server side is still very much a work in progress. I think what makes sense is to have the site as a bit of a stealth-beta for this release ... something we're still working on, something we don't advertise as a release feature, but something that you can already use. Which means that extension.gnome.org will be up and running (even if beta) in 3.2? I'm asking because I want to move my extensions to the website as soon as possible, and I'd like to know for how long I'll need to make tarball releases. I already skipped 3.1.90 / 3.1.91, hoping to have the website soon, but distro packagers will need 3.1.92 and 3.2.0 if they have to keep the packages. Giovanni signature.asc Description: This is a digitally signed message part ___ gnome-shell-list mailing list gnome-shell-list@gnome.org http://mail.gnome.org/mailman/listinfo/gnome-shell-list
Re: Notes looking at sweettooth-plugin
On Mon, Sep 12, 2011 at 11:43 AM, Giovanni Campagna scampa.giova...@gmail.com wrote: Il giorno dom, 11/09/2011 alle 16.40 -0400, Owen Taylor ha scritto: On Sun, 2011-09-11 at 14:57 +0200, Olav Vitters wrote: On Fri, Sep 02, 2011 at 12:24:18AM -0400, Jasper St. Pierre wrote: On Thu, Sep 1, 2011 at 3:36 PM, Owen Taylor otay...@redhat.com wrote: Some review comments reading the code to sweettooth-plugin: What is/will be the status for GNOME 3.2? I'm writing (together with others) the release notes and I'd like to say something about this. even if not for GNOME 3.2, we could put it in the looking forward section (but then really brief) if it is going to be ready, some screenshots would be nice. Ok to give me links and so on or tell me how to create screenshots. The client side stuff is largely there to make the site work, but it's not widely tested yet, and the server side is still very much a work in progress. I think what makes sense is to have the site as a bit of a stealth-beta for this release ... something we're still working on, something we don't advertise as a release feature, but something that you can already use. Which means that extension.gnome.org will be up and running (even if beta) in 3.2? I'm asking because I want to move my extensions to the website as soon as possible, and I'd like to know for how long I'll need to make tarball releases. I already skipped 3.1.90 / 3.1.91, hoping to have the website soon, but distro packagers will need 3.1.92 and 3.2.0 if they have to keep the packages. Giovanni For now, keep building tarballs. -- Jasper ___ gnome-shell-list mailing list gnome-shell-list@gnome.org http://mail.gnome.org/mailman/listinfo/gnome-shell-list
Re: Notes looking at sweettooth-plugin
On Fri, Sep 02, 2011 at 12:24:18AM -0400, Jasper St. Pierre wrote: On Thu, Sep 1, 2011 at 3:36 PM, Owen Taylor otay...@redhat.com wrote: Some review comments reading the code to sweettooth-plugin: What is/will be the status for GNOME 3.2? I'm writing (together with others) the release notes and I'd like to say something about this. even if not for GNOME 3.2, we could put it in the looking forward section (but then really brief) if it is going to be ready, some screenshots would be nice. Ok to give me links and so on or tell me how to create screenshots. -- Regards, Olav ___ gnome-shell-list mailing list gnome-shell-list@gnome.org http://mail.gnome.org/mailman/listinfo/gnome-shell-list
Re: Notes looking at sweettooth-plugin
On Sun, 2011-09-11 at 14:57 +0200, Olav Vitters wrote: On Fri, Sep 02, 2011 at 12:24:18AM -0400, Jasper St. Pierre wrote: On Thu, Sep 1, 2011 at 3:36 PM, Owen Taylor otay...@redhat.com wrote: Some review comments reading the code to sweettooth-plugin: What is/will be the status for GNOME 3.2? I'm writing (together with others) the release notes and I'd like to say something about this. even if not for GNOME 3.2, we could put it in the looking forward section (but then really brief) if it is going to be ready, some screenshots would be nice. Ok to give me links and so on or tell me how to create screenshots. The client side stuff is largely there to make the site work, but it's not widely tested yet, and the server side is still very much a work in progress. I think what makes sense is to have the site as a bit of a stealth-beta for this release ... something we're still working on, something we don't advertise as a release feature, but something that you can already use. - Owen ___ gnome-shell-list mailing list gnome-shell-list@gnome.org http://mail.gnome.org/mailman/listinfo/gnome-shell-list
Re: Notes looking at sweettooth-plugin
On Fri, 2011-09-02 at 00:24 -0400, Jasper St. Pierre wrote:
On Thu, Sep 1, 2011 at 3:36 PM, Owen Taylor otay...@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
___
gnome-shell-list mailing list
gnome-shell-list@gnome.org
http://mail.gnome.org/mailman/listinfo/gnome-shell-list
Re: Notes looking at sweettooth-plugin
On Thu, Sep 1, 2011 at 3:36 PM, Owen Taylor otay...@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 ___ gnome-shell-list mailing list gnome-shell-list@gnome.org http://mail.gnome.org/mailman/listinfo/gnome-shell-list
