[rfc][icedtea-web] replacing of hardcoded jre path by function

[Adam Domurad]

On 03/19/2013 11:08 AM, Jiri Vanek wrote:
> On 03/12/2013 04:42 PM, Adam Domurad wrote:
>> [ .. snip .. ]
>> The commands vector will not need the c_str(), because it is a vector
>> of std::string (things are much nicer when everything uses
>> std::string, we should move towards that!)
>
> Hmmm.. I can not see the difference...:(

See notes in-line

>
>
> Itteration 2 attached. Thanx for patience!
> J.

Thanks for the new iteration!

> diff -r 1024198de51f plugin/icedteanp/IcedTeaNPPlugin.cc
> --- a/plugin/icedteanp/IcedTeaNPPlugin.cc    Tue Mar 19 15:44:43 2013
> +0100
> +++ b/plugin/icedteanp/IcedTeaNPPlugin.cc    Tue Mar 19 16:02:11 2013
> +0100
> @@ -43,6 +43,7 @@
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> +#include <string>
>  #include <sys/stat.h>
>  #include <sys/types.h>
>  #include <unistd.h>
> @@ -156,8 +157,14 @@
>  // Data directory for plugin.
>  static std::string data_directory;
>
> -// Fully-qualified appletviewer executable.
> -static const char* appletviewer_executable = ICEDTEA_WEB_JRE "/bin/java";
> +// Fully-qualified appletviewer default  executable and rt.jar
> +static const char* appletviewer_default_executable = ICEDTEA_WEB_JRE
> "/bin/java";
> +static const char* appletviewer_default_rtjar = ICEDTEA_WEB_JRE
> "/lib/rt.jar";
> +// Fully-qualified appletviewer default  executable and rt.jar
> +// resolved by methods in runtime. It is expected they will be the
> same during
> +//whole runtime
> +static std::string appletviewer_executable;
> +static std::string appletviewer_rtjar;

You can get rid of these if you just have get_plugin_executable & 
get_plugin_rt_jar return values, and use the return value whenever you 
need it.

>
>  // Applet viewer input channel (needs to be static because it is used
> in plugin_in_pipe_callback)
>  static GIOChannel* in_from_appletviewer = NULL;
> @@ -284,6 +291,16 @@
>
>  #endif
>
> +static std::string get_plugin_executable(std::string& dest){
> +      dest = std::string (appletviewer_default_executable);
> +      return dest;
> +}

Why do you take an argument here ? You should just return the value?

> +
> +static std::string get_plugin_rt_jar(std::string& dest){
> +      dest = std::string (appletviewer_default_rtjar);
> +      return dest;
> +}
> +

Same

>
>  /*
>   * Find first member in GHashTable* depending on version of glib
> @@ -1517,13 +1534,14 @@
>  static NPError
>  plugin_test_appletviewer ()
>  {
> -  PLUGIN_DEBUG ("plugin_test_appletviewer: %s\n",
> appletviewer_executable);
> +
> +  PLUGIN_DEBUG ("plugin_test_appletviewer: %s\n",
> get_plugin_executable(appletviewer_executable).c_str());
>    NPError error = NPERR_NO_ERROR;
>
>    gchar* command_line[3] = { NULL, NULL, NULL };
>    gchar** environment;
>
> -  command_line[0] = g_strdup (appletviewer_executable);
> +  command_line[0] = g_strdup
> (get_plugin_executable(appletviewer_executable).c_str());
>    command_line[1] = g_strdup("-version");
>    command_line[2] = NULL;
>
> @@ -1570,7 +1588,7 @@
>
>    // Construct command line parameters
>
> -  command_line.push_back(appletviewer_executable);
> +
> command_line.push_back(get_plugin_executable(appletviewer_executable).c_str());

As I mentioned earlier, the .c_str() is redundant here and causes an 
unnecessary string copy (if you're curious why, normally std::string can 
be passed around and share string data, but it is not able to do this 
simply given a char*).

>
>    //Add JVM args to command_line
>    for (int i = 0; i < jvm_args->size(); i++)
> @@ -1581,7 +1599,7 @@
>    command_line.push_back(PLUGIN_BOOTCLASSPATH);
>    // set the classpath to avoid using the default (cwd).
>    command_line.push_back("-classpath");
> -  command_line.push_back(ICEDTEA_WEB_JRE "/lib/rt.jar");
> + command_line.push_back(get_plugin_rt_jar(appletviewer_rtjar).c_str());

Same (drop c_str())

>
>    // Enable coverage agent if we are running instrumented plugin
>  #ifdef COVERAGE_AGENT
> @@ -1652,10 +1670,10 @@
>    gchar *output = NULL;
>    std::vector<std::string*>* tokenOutput = NULL;
>
> -  commands.push_back(appletviewer_executable);
> +
> commands.push_back(get_plugin_executable(appletviewer_executable).c_str());

Same (drop c_str())


>    commands.push_back(PLUGIN_BOOTCLASSPATH);
>    commands.push_back("-classpath");
> -  commands.push_back(ICEDTEA_WEB_JRE "/lib/rt.jar");
> + commands.push_back(get_plugin_rt_jar(appletviewer_rtjar).c_str());

Same (drop c_str())

> commands.push_back("net.sourceforge.jnlp.controlpanel.CommandLine");
>    commands.push_back("get");
>    commands.push_back("deployment.plugin.jvm.arguments");
> @@ -2175,11 +2193,11 @@
>      }
>
>    // Set appletviewer_executable.
> -  PLUGIN_DEBUG("Executing java at %s\n", appletviewer_executable);
> +  PLUGIN_DEBUG("Executing java at %s\n",
> get_plugin_executable(appletviewer_executable).c_str());
>    np_error = plugin_test_appletviewer ();
>    if (np_error != NPERR_NO_ERROR)
>      {
> -      fprintf(stderr, "Unable to find java executable %s\n",
> appletviewer_executable);
> +      fprintf(stderr, "Unable to find java executable %s\n",
> get_plugin_executable(appletviewer_executable).c_str());
>        return np_error;
>      }
>
> @@ -2191,7 +2209,7 @@
>
>    plugin_instance_mutex = g_mutex_new ();
>
> -  PLUGIN_DEBUG ("NP_Initialize: using %s\n", appletviewer_executable);
> +  PLUGIN_DEBUG ("NP_Initialize: using %s\n",
> get_plugin_executable(appletviewer_executable).c_str());
>
>    plugin_req_proc = new PluginRequestProcessor();
>    java_req_proc = new JavaMessageSender();


Looks ok otherwise.
Thanks,
-Adam