GNOME Bugzilla – Bug 748395
Update libgdata API usage
Last modified: 2015-06-16 12:00:27 UTC
There are a couple of things in Grilo which need fixing for use with the latest libgdata release (0.17.0), because it has ported from v2 to v3 of the YouTube API. These changes are backwards compatible; this is not an API break. I have tested they compile, but have not had a chance to test with a Grilo client, so that should be done before they are committed. See https://bugzilla.gnome.org/show_bug.cgi?id=687597 for information about the YouTube v3 port.
Created attachment 302273 [details] [review] youtube: Use GDataYouTubeQuery when building GData queries Do not use the parent GDataQuery class. While this worked previously, it is not a valid use of the API, as it prevents any overrides in GDataYouTubeQuery from being used when building query APIs. This will cause queries to break in the transition from Google’s v2 to v3 API, since the GDataYouTubeQuery behaviour has to diverge from GDataQuery.
Created attachment 302274 [details] [review] youtube: Use libgdata helper function to get category list There is no need to reinvent the wheel…especially when that wheel is changing shape from XML to JSON as Google withdraws the deprecated v2 YouTube API for the v3 one.
Review of attachment 302273 [details] [review]: Looks correct to me.
Review of attachment 302274 [details] [review]: The libgdata use looks correct, but ... ::: src/youtube/grl-youtube.c @@ +608,3 @@ + g_error_free (error); + return; + } Don't we need to call bcs->callback in this case? We are leaking 'bcs' here, and we were leaking it before too. I don't really understand why the g_slice_free needs to be in "if (all) {...}".
Created attachment 302591 [details] [review] youtube: Use GDataYouTubeQuery when building GData queries Do not use the parent GDataQuery class. While this worked previously, it is not a valid use of the API, as it prevents any overrides in GDataYouTubeQuery from being used when building query APIs. This will cause queries to break in the transition from Google’s v2 to v3 API, since the GDataYouTubeQuery behaviour has to diverge from GDataQuery.
Created attachment 302592 [details] [review] youtube: Use libgdata helper function to get category list There is no need to reinvent the wheel…especially when that wheel is changing shape from XML to JSON as Google withdraws the deprecated v2 YouTube API for the v3 one.
Created attachment 302593 [details] [review] youtube: Miscellaneous compiler warning fixes
Created attachment 302594 [details] [review] youtube: Fix leak on two errors paths when querying categories When there was an error querying the categories, or when zero categories were returned, the BuildCategorySpec callback would not be called, and then the struct itself would not be freed.
Review of attachment 302591 [details] [review]: Sure.
Review of attachment 302592 [details] [review]: Looks good apart from that. ::: src/youtube/grl-youtube.c @@ +588,3 @@ + BuildCategorySpec *bcs; + GDataAPPCategories *app_categories = NULL; + GList/*<unowned GDataCategory>*/ *categories = NULL; You can put that at the end of the line, after the code, rather than in the middle of it.
Review of attachment 302593 [details] [review]: Sure, though I usually prefer separate patches with the error messages.
Review of attachment 302594 [details] [review]: Looks good.
Pushed with the suggestion from comment #10. Attachment 302591 [details] pushed as fe4253a - youtube: Use GDataYouTubeQuery when building GData queries Attachment 302592 [details] pushed as a207dd0 - youtube: Use libgdata helper function to get category list
Attachment 302593 [details] pushed as 953d0aa - youtube: Miscellaneous compiler warning fixes Attachment 302594 [details] pushed as 4ef9c53 - youtube: Fix leak on two errors paths when querying categories
This broke the Continuous build: http://build.gnome.org/continuous/buildmaster/builds/2015/06/14/27/build/log-grilo-plugins.txt I went ahead and pushed a fix. Maybe you guys could join #testable and watch for build failures after pushing, and help maintain the system?
(In reply to Colin Walters from comment #15) > This broke the Continuous build: > > http://build.gnome.org/continuous/buildmaster/builds/2015/06/14/27/build/log- > grilo-plugins.txt > > I went ahead and pushed a fix. > > Maybe you guys could join #testable and watch for build failures after > pushing, and help maintain the system? Thanks for the fix, though I would imagine that Philip doesn't spend too much time on IRC during week-ends. I know that I rarely do even if I'm hacking on something.
(In reply to Bastien Nocera from comment #16) > (In reply to Colin Walters from comment #15) > > This broke the Continuous build: > > > > http://build.gnome.org/continuous/buildmaster/builds/2015/06/14/27/build/log- > > grilo-plugins.txt > > > > I went ahead and pushed a fix. > > > > Maybe you guys could join #testable and watch for build failures after > > pushing, and help maintain the system? > > Thanks for the fix, though I would imagine that Philip doesn't spend too > much time on IRC during week-ends. I know that I rarely do even if I'm > hacking on something. Bingo. In fact, I’m rarely even on the internet at weekends. There used to be a build.gnome.org RSS feed for each module which would allow failures to be investigated asynchronously — does something like that exist in the new world order? Or some way of having the buildbot automatically open a bug report against a module if it fails to build, and CC in the committer who broke it?