[rfc][icedtea-web] jnlp_href extensions inside HTML applets - PR974

[Adam Domurad]

On 08/15/2013 10:42 AM, Andrew Azores wrote:
[..snip..]

> diff --git a/netx/net/sourceforge/jnlp/ParserSettings.java b/netx/net/sourceforge/jnlp/ParserSettings.java
> --- a/netx/net/sourceforge/jnlp/ParserSettings.java
> +++ b/netx/net/sourceforge/jnlp/ParserSettings.java
> @@ -37,6 +37,9 @@
>
>  package net.sourceforge.jnlp;
>
> +import java.util.Arrays;
> +import java.util.List;
> +
>  /**
>   * Contains settings to be used by the Parser while parsing JNLP files.
>   *
> @@ -44,6 +47,8 @@
>   */
>  public class ParserSettings {
>
> +    private static ParserSettings globalParserSettings = null;
> +
>      private final boolean isStrict;
>      private final boolean extensionAllowed;
>      private final boolean malformedXmlAllowed;
> @@ -75,4 +80,30 @@
>          return isStrict;
>      }
>
> -}
> \ No newline at end of file
> +    /**
> +     * Return the global parser settings in use. If the call getGlobalParserSettings(String[])
> +     * has been made prior to this one, return the same settings. Otherwise, return default
> +     * settings.
> +     */
> +    public static ParserSettings getGlobalParserSettings() {
> +        if (globalParserSettings == null) {
> +            globalParserSettings = new ParserSettings();
> +        }
> +        return globalParserSettings;
> +    }
> +
> +    /**
> +     * Return the ParserSettings to be used according to arguments specified
> +     * at boot on the command line. If no options specified, return default
> +     * settings.
> +     */
> +    public static ParserSettings getGlobalParserSettings(String[] cmdArgs) {

This should be called setGlobalParserSettings in my opinion. It's a 
general rule that if a method has a side-effect, and a return value, you 
should name it according to the side-effect. The logic being that the 
return value is easier to reason about.

(There's a noteworthy example of this from the Java standard library: 
http://docs.oracle.com/javase/7/docs/api/java/lang/Thread.html#interrupted%28%29 
This method should have been called 'clearInterrupt' or similar.)

Anyway I would still somewhat prefer a method that goes from String[] -> 
ParserSettings, as well as a separate 
'setGlobalParserSettings(ParserSettings parserSettings)', but it isn't a 
blocker or anything.


> +        List<String> argList = Arrays.asList(cmdArgs);
> +        boolean strict = argList.contains("-strict");
> +        boolean malformedXmlAllowed = !argList.contains("-xml");
> +
> +        globalParserSettings = new ParserSettings(strict, true, malformedXmlAllowed);
> +        return globalParserSettings;
> +    }
> +
> +}
> diff --git a/netx/net/sourceforge/jnlp/PluginBridge.java b/netx/net/sourceforge/jnlp/PluginBridge.java
> --- a/netx/net/sourceforge/jnlp/PluginBridge.java
> +++ b/netx/net/sourceforge/jnlp/PluginBridge.java
> @@ -26,19 +26,19 @@
>  import java.io.File;
>  import java.io.IOException;
>  import java.io.InputStream;
> +import java.net.MalformedURLException;
>  import java.net.URL;
> -import java.net.MalformedURLException;
> +import java.util.ArrayList;
> +import java.util.Arrays;
>  import java.util.HashSet;
> +import java.util.List;
>  import java.util.Locale;
> -import java.util.List;
> -import java.util.ArrayList;
>  import java.util.Map;
>  import java.util.Set;
>
> +import net.sourceforge.jnlp.runtime.JNLPRuntime;
>  import sun.misc.BASE64Decoder;
>
> -import net.sourceforge.jnlp.runtime.JNLPRuntime;
> -
>  /**
>   * Allows reuse of code that expects a JNLPFile object,
>   * while overriding behaviour specific to applets.
> @@ -47,6 +47,7 @@
>
>      private PluginParameters params;
>      private Set<String> jars = new HashSet<String>();
> +    private List<ExtensionDesc> extensionJars = new ArrayList<ExtensionDesc>();
>      //Folders can be added to the code-base through the archive tag
>      private List<String> codeBaseFolders = new ArrayList<String>();
>      private String[] cacheJars = new String[0];
> @@ -90,6 +91,7 @@
>          this.codeBase = codebase;
>          this.sourceLocation = documentBase;
>          this.params = params;
> +        this.parserSettings = ParserSettings.getGlobalParserSettings();
>
>          if (params.getJNLPHref() != null) {
>              useJNLPHref = true;
> @@ -122,6 +124,9 @@
>                       String fileName = jarDesc.getLocation().toExternalForm();
>                       this.jars.add(fileName);
>                   }
> +
> +                // Store any extensions listed in the JNLP file to be returned later on, namely in getResources()
> +                extensionJars = Arrays.asList(jnlpFile.getResources().getExtensions());
>              } catch (MalformedURLException e) {
>                  // Don't fail because we cannot get the jnlp file. Parameters are optional not required.
>                  // it is the site developer who should ensure that file exist.
> @@ -308,6 +313,8 @@
>                          return result;
>                      } catch (MalformedURLException ex) { /* Ignored */
>                      }
> +                } else if (launchType.equals(ExtensionDesc.class)) {
> +                    return (List<T>) extensionJars; // this list is populated when the PluginBridge is first constructed
>                  }
>                  return sharedResources.getResources(launchType);
>              }
> diff --git a/netx/net/sourceforge/jnlp/runtime/Boot.java b/netx/net/sourceforge/jnlp/runtime/Boot.java
> --- a/netx/net/sourceforge/jnlp/runtime/Boot.java
> +++ b/netx/net/sourceforge/jnlp/runtime/Boot.java
> @@ -214,18 +214,8 @@
>          extra.put("arguments", getOptions("-arg"));
>          extra.put("parameters", getOptions("-param"));
>          extra.put("properties", getOptions("-property"));
> -        boolean strict = false;
> -        boolean malformedXmlAllowed = true;
>
> -        if (null != getOption("-strict")) {
> -            strict = true;
> -        }
> -
> -        if (null != getOption("-xml")) {
> -            malformedXmlAllowed = false;
> -        }
> -
> -        ParserSettings settings = new ParserSettings(strict, true, malformedXmlAllowed);
> +        ParserSettings settings = ParserSettings.getGlobalParserSettings(args);
>
>          try {
>              Launcher launcher = new Launcher(false);

Looks good, approved other than 
s/getGlobalParserSettings(args)/setGlobalParserSettings(args)/ IMO

Unit tests and reproducers approved too.

Good stuff, overall.

Thanks,
-Adam