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

[Adam Domurad]

On 03/20/2013 07:02 AM, Jiri Vanek wrote:
> Looks like we have deal...
> ok?
>
>
> J.
> On 03/19/2013 04:28 PM, Adam Domurad wrote:
>> 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
>
OK to push to HEAD.
-Adam