GNOME Bugzilla – Bug 478196
Configure script summary does not show values when built with WebKit.
Last modified: 2007-12-02 20:22:58 UTC
Please describe the problem: epiphany configure script detects gecko flavor = xulrunner even when built with engine=webkit. It also says it is powered by gecko in Help > About. Steps to reproduce: Actual results: Expected results: Does this happen every time? Other information:
Created attachment 96082 [details] [review] Patch for Epiphany bug 478196 I'm really excited to announce that I have created my very first patch for an open source project ever on this bug! Very exciting stuff! I look forward to being able to fix more bugs in the future as I learn more about programming. ~ Nicholas E. Manley
Thanks for the patch :) You're right in finding part of the problem in this code in window-commands.c: + "Powered by %s %s"), rendering_engine, ephy_embed_single_get_backend_name (single) + strlen ("gecko-")); since of course on webkit there won't be a prefix "gecko-". The best way to fix this would however be to change the backend implementations (embed/mozilla/mozilla-embed-single.cpp and embed/webkit/webkit-embed-single.cpp) in the impl_get_backend_name to return a full name (e.g. "Gecko 1.8", "Webkit"), and just do away with the "+strlen("gecko-"))" part here.
I was considering just removing that entire line, but I thought it would be best just to leave it since I am still new to the project and didn't feel comfortable removing someone else's code. I'm sure I will be a bit more useful once I get some more experience and I don't feel as though I am shooting in the dark with every patch I submit. Until then, you'll just have to put up with me. =P
Comment on attachment 96082 [details] [review] Patch for Epiphany bug 478196 Index: embed/ephy-embed-single.c =================================================================== --- embed/ephy-embed-single.c (revision 7495) +++ embed/ephy-embed-single.c (working copy) @@ -306,3 +306,25 @@ EphyEmbedSingleIface *iface = EPHY_EMBED_SINGLE_GET_IFACE (single); return iface->get_backend_name (single); } + +/* + * Reads config.h to determine which backend is being used. + * Consider making the above function deprecated? + */ +const char * +ephy_get_backend_name() +{ + #if defined ( WITH_GECKO_ENGINE ) + #if defined ( HAVE_GECKO_1_7 ) + return "Gecko 1.7"; + #elif defined ( HAVE_GECKO_1_8 ) + return "Gecko 1.8"; + #elif defined ( HAVE_GECKO_1_8_1 ) + return "Gecko 1.8.1"; + #elif defined ( HAVE_GECKO_1_9 ) + return "Gecko 1.9"; + #endif + #elif defined ( WITH_WEBKIT_ENGINE ) + return "WebKit"; + #endif +} Index: src/window-commands.c =================================================================== --- src/window-commands.c (revision 7495) +++ src/window-commands.c (working copy) @@ -823,9 +823,9 @@ shell = ephy_embed_shell_get_default (); single = EPHY_EMBED_SINGLE (ephy_embed_shell_get_embed_single (shell)); - comments = g_strdup_printf (_("Lets you view web pages and find information on the internet.\n" - "Powered by Gecko %s"), - ephy_embed_single_get_backend_name (single) + strlen ("gecko-")); + comments = g_strdup_printf ("Lets you view web pages and find information on the internet.\n" + "Powered by %s", ephy_get_backend_name() ); + licence = g_strjoin ("\n\n", _(licence_part[0]), _(licence_part[1]),
Created attachment 96207 [details] [review] Second revision. Do NOT apply over the first patch. Here is the second revision. The first one is buggy as well as poorly implemented so don't even bother looking at it. The new function, ephy_get_backend_name(), reads the config.h file to determine which rendering engine is being used. It is able to distinguish between Gecko and WebKit, but it improperly detects which version of Gecko is being used because the ./configure script leaves all versions of Gecko (except 1.9) defined. This is a separate bug though and once fixed, this patch will work as expected.
+/* + * Reads config.h to determine which backend is being used. + * Consider making the above function deprecated? + */ I think adding this function isn't the right way. Instead, just the part of the patch in window-commands.c (but still using ephy_embed_single_get_backend_name) should fix this bug, no?
Created attachment 96351 [details] [review] epiphany-bugfix-478196-R3 Okay here is the third revision that does just as you suggest Christian. The reason I wrote my own function is because (as I am unfamiliar with the codebase), the current one seemed to be "all over the place" in that rather than just reading a single file/variable, they have the "shell" variable which is then passed to the function for the "single" variable which is then passed to the function that gets the backend name. If someone were to come in and make more drastic changes than the one I am making, they might be a little disgruntled at having to jump all over the place to understand what is happening in full. I won't question you guys though as I am sure you have your reasons and I am still missing the bigger picture of how the codebase works. Once I get this bug settled though (I at least want to finish what I started), I'll spend some quality time alone studying everything before getting in over my head with any more patches.
Thanks! The reason is that embed/ should be as much as possible backend-independent, and all specific code be in the backend itself.
Committed. I added the missing _( ) to Nicholas' patch to correctly mark the string as translatable. I think that the configure script can be given some love when using WebKit, as the configuration options summary shown at the end of configure --with-engine=webkit is missing values for many elements. I'm not closing this but modifying summary for that reason, and not attaching a patch because configure is black voodoo to me :/
Created attachment 100038 [details] [review] fix empty strings in the output as well as cleanup configure.ac a bit Hi. This patch fixes the issue raised by cosimoc above as well as some minor cleanups. In summary: 1. prepend string comparisons with 'x' (e.g, "x$with_engine" = "xwebkit") to safeguard against empty strings as well as to make sure that variables have a "yes" or "no" value (which is needed by configure flags summary) 2. put conditional in the "action-if-given" parameter of AC_ARG_ENABLE for features that are disabled by default - this removes some conditionals and make some chunks readable be lessening the newlines Let me know if there are any issues.
+if test "x$with_engine" = "xmozilla"; then Please remove all those "x$whatever" changes.
May I know the reason why?
Because it's not necessary. And it makes it hard to see the stuff that's really changed in all the "x" noise.
Created attachment 100058 [details] [review] fix configure summary output Hi chpe. Thanks for the feedback. This is a new patch for the configure summary issue. cheers
+else # with_engine = mozilla + ${enable_desktop_file_plugin='no'} enable_desktop_file_plugin=no +else + ${enable_certificate_manager='no'} Same. + ${enable_spell_checker="no"} Same, twice. +if test "x$with_engine" = "xmozilla"; then Remove the "x" please.
Created attachment 100061 [details] [review] [update] configure summary patch chpe: as per your suggestions. Thanks!
Created attachment 100062 [details] [review] [update] configure summary patch this is the correct one
landed in r7747.