[rfc][icedtea-web] replacing of hardcoded jre path by function
Jiri Vanek
jvanek at redhat.com
Wed Mar 20 04:02:11 PDT 2013
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: replacingConstWithFunction_3.diff
Type: text/x-patch
Size: 3822 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130320/bcd958fb/replacingConstWithFunction_3.diff
More information about the distro-pkg-dev
mailing list