[RFC][icedtea-web]: Set plugin JVM options in itw-settings

[Omair Majid]

On 10/29/2012 03:30 PM, Saad Mohammad wrote:
> Hi,
> 
> The patch attached contains a new feature allowing users to set plugin JVM
> options through itw-settings. The patch also cleans up itw-settings UI by
> organizing debugging, desktop integration, and security tab into a single tab
> named Advanced Settings.

Please don't mix multiple changes in one patch. I prefer to be able to
review each change (the cleanup and the jvm args) individually.

I like the idea of the JVM args; we should try and get this in ASAP.
Having this work for javaws programs would be nice too.

I am against the cleanup. We had some of this discussion back when
itweb-settings was first created too, and I think it looks better as it
is right now. It seems rather arbitrary to say that certificate
management, for example, is not an advanced task, while enabling
debugging information is.

> diff --git a/netx/net/sourceforge/jnlp/config/DeploymentConfiguration.java b/netx/net/sourceforge/jnlp/config/DeploymentConfiguration.java
> --- a/netx/net/sourceforge/jnlp/config/DeploymentConfiguration.java
> +++ b/netx/net/sourceforge/jnlp/config/DeploymentConfiguration.java
> @@ -158,6 +158,11 @@
>      public static final String KEY_BROWSER_PATH = "deployment.browser.path";
>      public static final String KEY_UPDATE_TIMEOUT = "deployment.javaws.update.timeout";
>  
> +    /*
> +     * Set JVM options for plugin
> +     */
> +    public static final String KEY_PLUGIN_JVM_OPTIONS = "deployment.plugin.jvm.arguments";
> +

Please pick one of "options" and "arguments" and use it in the
string/varaiable/javadoc. I prefer "arguments" since this is really the
command line arguments.

>      public enum ConfigType {
>          System, User
>      }
> diff --git a/netx/net/sourceforge/jnlp/controlpanel/AdvancedSettingsPanel.java b/netx/net/sourceforge/jnlp/controlpanel/AdvancedSettingsPanel.java
> new file mode 100644
> --- /dev/null
> +++ b/netx/net/sourceforge/jnlp/controlpanel/AdvancedSettingsPanel.java
> @@ -0,0 +1,71 @@

Missing license.

> +        //Add panels
> +        for (JPanel eachPanel : panels) {
> +            Dimension d = eachPanel.getMinimumSize();
> +            if (d.height > height)
> +                height = d.height;
> +            if (d.width > width)
> +                width = d.width;
> +
> +            settingsPanel.add(eachPanel);
> +        }
> +
> +        Dimension dim = new Dimension(width + 3, height);
> +        this.setMinimumSize(dim);

Can you avoid setting the sizes explicitly here? I thought a JScrollPane
would size automatically?

> +        //Scroll
> +        JScrollPane scroller = new JScrollPane(settingsPanel);
> +        this.add(scroller);


> diff --git a/netx/net/sourceforge/jnlp/controlpanel/PluginPanel.java b/netx/net/sourceforge/jnlp/controlpanel/PluginPanel.java
> new file mode 100644
> --- /dev/null
> +++ b/netx/net/sourceforge/jnlp/controlpanel/PluginPanel.java
> @@ -0,0 +1,49 @@

Missing license header.

> diff --git a/plugin/icedteanp/IcedTeaNPPlugin.cc b/plugin/icedteanp/IcedTeaNPPlugin.cc
> --- a/plugin/icedteanp/IcedTeaNPPlugin.cc
> +++ b/plugin/icedteanp/IcedTeaNPPlugin.cc

> +  int numberOfCommandsToAllocate = 0;

Please follow the coding conventions in the surrounding code:
int number_of_commands_to_allocate = 0;

>    int cmd_num = 0;
> +
> +  if(jvm_options != NULL){
> +    numberOfCommandsToAllocate++;
> +    printf("Using user specified JVM options: '%s'\n", jvm_options);

Does it make sense to always print this? Maybe PLUGIN_DEBUG should be
used here instead?

> +  }
> +
>    if (plugin_debug)
>    {
> -      command_line = (gchar**) malloc(sizeof(gchar*)*11);
> +      numberOfCommandsToAllocate += 11;
> +      command_line = (gchar**) malloc(sizeof(gchar*) * numberOfCommandsToAllocate);
>        command_line[cmd_num++] = g_strdup(appletviewer_executable);
>        command_line[cmd_num++] = g_strdup(PLUGIN_BOOTCLASSPATH);
>        // set the classpath to avoid using the default (cwd).
>        command_line[cmd_num++] = g_strdup("-classpath");
>        command_line[cmd_num++] = g_strdup_printf("%s/lib/rt.jar", ICEDTEA_WEB_JRE);
> +
> +      if (jvm_options != NULL)
> +        command_line[cmd_num++] = g_strdup(jvm_options);

Would this work with multiple arguments? Suppose I set the args to
"-Xcheck:jni -Xfuture" using itweb-settings. jvm_options will see it and
this code will pass it as a single argument to the JVM, no?

>  
> +  if (jvm_options != NULL){
> +    g_free(jvm_options);
> +    jvm_options = NULL;
> +  }

g_free() on NULL is a no-op. You can just call g_free(jvm_options)
without checking if it's NULL or not.

> +
> +  //Get JVM options set in itw-settings
> +  g_spawn_command_line_sync(cmd_line_str, &output, NULL, NULL, &channel_error);

So we sync-spawn a JVM to find the arguments. That seems rather an
expensive operation. And we do this unconditionally when the first
applet needs to be loaded, delaying the applet startup. Is there a
performance (especially user-visible performance) impact of this? Does
the browser stall when loading the first applet?

> +
> +  //Free memory
> +  g_free(commands);

Command was allocated using malloc(). It should be free'ed using free(),
not g_free().

> diff --git a/plugin/icedteanp/IcedTeaNPPlugin.h b/plugin/icedteanp/IcedTeaNPPlugin.h
> --- a/plugin/icedteanp/IcedTeaNPPlugin.h
> +++ b/plugin/icedteanp/IcedTeaNPPlugin.h
> @@ -136,4 +136,10 @@
>  /* Creates a new scriptable plugin object and returns it */
>  NPObject* allocate_scriptable_object(NPP npp, NPClass *aClass);
>  
> +/* Returns JVM options set in itw-settings */
> +static gchar* get_JVM_options();
> +
> +/* Removes newlines and extra spaces from char */
> +static char* trim_char(char* to_trim);
> +

static functions declared in a header file? Why are you doing this?

Thanks,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681