GNOME Bugzilla – Bug 749977
videosite: Allow looking for alternative resolution scripts
Last modified: 2015-06-11 21:15:54 UTC
.
Created attachment 304082 [details] [review] videosite: Fix URI leak Make sure that "uri" is freed when exiting the function.
Created attachment 304083 [details] [review] videosite: Split off finding the appropriate helper script This also fixes the location of the script when building uninstalled tests.
Created attachment 304084 [details] [review] videosite: Allow looking for alternative resolution scripts Third-parties are allowed to install scripts or binaries in $(libexecdir)/totem-pl-parser/. The first script (as sorted through strcmp()) will be chosen to resolve all possible websites.
Review of attachment 304082 [details] [review]: ++
Review of attachment 304083 [details] [review]: ::: plparse/totem-pl-parser-videosite.c @@ +37,3 @@ + return g_strdup (LIBEXECDIR "/totem-pl-parser-videosite"); +#endif +} This is leaked at both call sites. Why strdup() it?
Review of attachment 304084 [details] [review]: How do third parties choose which number to prefix their binary with? Aren’t they all going to go for ‘1-foo’? Where is this all documented? ::: plparse/totem-pl-parser-videosite.c @@ +36,3 @@ #else + GDir *dir; + GList *list = NULL; Why build a list? Why not just assign to ret iff the value of g_dir_read_name() compares lower than the current value of ret? @@ +46,3 @@ + while ((name = g_dir_read_name (dir)) != NULL) { + if (name[0] == '.') + continue; g_dir_read_name() explicitly excludes ‘.’ and ‘..’.
(In reply to Philip Withnall from comment #5) > Review of attachment 304083 [details] [review] [review]: > > ::: plparse/totem-pl-parser-videosite.c > @@ +37,3 @@ > + return g_strdup (LIBEXECDIR "/totem-pl-parser-videosite"); > +#endif > +} > > This is leaked at both call sites. Why strdup() it? Because it will be variable in the subsequent commit. Fixed the leak though.
Created attachment 304931 [details] [review] videosite: Split off finding the appropriate helper script This also fixes the location of the script when building uninstalled tests.
Created attachment 304932 [details] [review] videosite: Allow looking for alternative resolution scripts Third-parties are allowed to install scripts or binaries in $(libexecdir)/totem-pl-parser/. The first script (as sorted through strcmp()) will be chosen to resolve all possible websites.
(In reply to Philip Withnall from comment #6) > Review of attachment 304084 [details] [review] [review]: > > How do third parties choose which number to prefix their binary with? Aren’t > they all going to go for ‘1-foo’? Likely not... > Where is this all documented? Nowhere, internal easter egg-ish. > ::: plparse/totem-pl-parser-videosite.c > @@ +36,3 @@ > #else > + GDir *dir; > + GList *list = NULL; > > Why build a list? Why not just assign to ret iff the value of > g_dir_read_name() compares lower than the current value of ret? Yep, good idea. > @@ +46,3 @@ > + while ((name = g_dir_read_name (dir)) != NULL) { > + if (name[0] == '.') > + continue; > > g_dir_read_name() explicitly excludes ‘.’ and ‘..’. I also wanted to skip hidden files.
Review of attachment 304931 [details] [review]: ++
Review of attachment 304931 [details] [review]: .
Review of attachment 304932 [details] [review]: ::: plparse/totem-pl-parser-videosite.c @@ +39,3 @@ + char *ret = NULL; + + dir = g_dir_open (LIBEXECDIR "/totem-pl-parser", 0, NULL); A simple comment explaining that the first non-hidden file in the directory (by lexicographic ordering) is chosen would be quite useful. @@ +51,3 @@ + } else if (g_strcmp0 (name, ret) < 0) { + g_free (ret); + ret = g_strdup (name); You could combine these two branches.
Created attachment 305070 [details] [review] videosite: Allow looking for alternative resolution scripts Third-parties are allowed to install scripts or binaries in $(libexecdir)/totem-pl-parser/. The first script (as sorted through strcmp()) will be chosen to resolve all possible websites.
Review of attachment 305070 [details] [review]: ++
Attachment 304082 [details] pushed as 5c21901 - videosite: Fix URI leak Attachment 304931 [details] pushed as b524675 - videosite: Split off finding the appropriate helper script Attachment 305070 [details] pushed as 1147853 - videosite: Allow looking for alternative resolution scripts