GNOME Bugzilla – Bug 364022
Parameters are order dependant
Last modified: 2006-10-22 22:25:49 UTC
The way we parse parameters we actually have order-dependant parameters parsing. For example: <embed src="foo.mov" qtsrc="bar.mov"> will work, but: <embed qtsrc="bar.mov" src="foo.mov"> will not. We should: - Fill up a hashtable from the argn/argv, with key, value - Have some convenience functions for parsing booleans (at least, FALSE, false, 0, 1 drives me bananas) - Update hashtable data from the QT URL Extensions (see bug 364017) - Parse the hashtable data as we'd expect to find it (ie. src first, so it can be replaced by qtsrc or filename, base before src so it can be resolved, etc.)
> - Fill up a hashtable from the argn/argv, with key, value My patch (wip) for bug 350297 simply records all the uris in the loop, and moves the processing after the loop; I don't see any need for a hashtable. > - Have some convenience functions for parsing booleans (at least, FALSE, false, > 0, 1 drives me bananas) Yes, me too :)
(In reply to comment #1) > > - Fill up a hashtable from the argn/argv, with key, value > My patch (wip) for bug 350297 simply records all the uris in the loop, and > moves the processing after the loop; I don't see any need for a hashtable. See the patch below. It actually cleans up the code quite a lot (although there are still a large amount of FIXMEs, I must say). > > - Have some convenience functions for parsing booleans (at least, FALSE, false, > > 0, 1 drives me bananas) > Yes, me too :) Done. It also adds: - we support all the images - more readable code to implement the HREF system from bug 364017 - imagepreview support for bug 364020 (but how can we avoid the original src to create a new stream?)
Created attachment 75164 [details] [review] totem-browser-new-parameters-parsing.patch
Unless you find something utterly disturbing with the code, I'll commit this after making a 2.17.1 release.
+ /* We can always play those image types */ We can, but we only need them for the narrowspace and mully plugins, don't we? If so, should be #ifdef'd, IMHO. + args = g_hash_table_new_full (g_str_hash, totem_strcase_equal, + NULL, (GDestroyNotify) g_free); [...] + g_hash_table_insert (args, argn[i], g_strdup (argv[i])); Since you destroy the hashtable in the same function (i.e. where the passed-in argv array is still valid) there's no need to strdup the args; you can just put the argv[i] pointers in there.
(In reply to comment #5) > + /* We can always play those image types */ > > We can, but we only need them for the narrowspace and mully plugins, don't we? > If so, should be #ifdef'd, IMHO. Actually, it was GMP and Mully that used it so far. The NarrowSpace ones usually got an image embedded in a QuickTime file. > + args = g_hash_table_new_full (g_str_hash, totem_strcase_equal, > + NULL, (GDestroyNotify) g_free); > [...] > + g_hash_table_insert (args, argn[i], g_strdup (argv[i])); > > Since you destroy the hashtable in the same function (i.e. where the passed-in > argv array is still valid) there's no need to strdup the args; you can just put > the argv[i] pointers in there. That's true until we get the additional QuickTime URL Extensions working.
2006-10-22 Bastien Nocera <hadess@hadess.net> * browser-plugin/totemPlugin.cpp: gif and jpeg images are supported, rework the parameters parsing so that we are sure which order the parameters are parsed in (Closes: #364022), add the starts of imagepreview support for the DivX plugin