[rfc][icedtea-web] Remove need for HTML tag scanner in PluginAppletViewer
Jiri Vanek
jvanek at redhat.com
Fri Nov 16 06:14:34 PST 2012
On 10/30/2012 09:06 PM, Adam Domurad wrote:
> On 10/26/2012 09:13 AM, Jiri Vanek wrote:
>> On 10/25/2012 10:10 PM, Adam Domurad wrote:
>>> Hi all. This is something I've wanted to do for a while, simply because of the hackish nature of the
>>> applet/object/embed tag parsing code in ITW. Thoughts welcome, I'll still be doing some more testing
>>> on this (looks good so far) but would appreciate feedback. The patch also makes it possible to do
>>> unit tests from classes included in plugin.jar (used to unit test the new
>>> sun.java.PluginAppletAttributes).
>>>
>>> The applet tag information flows like this pre-patch:
>>> - NPAPI parses the tag information
>>> - ITW's C++ side takes in the already-parsed tag information, creates an embed tag (embed tag only)
>>> for ITW to use.
>>> - ITW's java side receives the tag, and scans it using (less than desirable) parsing routines.
>>>
>>> Post-patch:
>>> - NPAPI parses the tag information
>>> - ITW's C++ side generates a simple listing of the name value pairs passed
>>> - ITW's java side parses these name value pairs
>>
>> I like this patch. I found several issues which had come across my mind, but I think during other reviews there will come more of them.
>> Also I'm not 100% capable to verify C code. SO before aprove I will definitely check with Pavel or somebody else can do this. I checked c part only from logical point of view.
>>>
>>> Points of contention:
>>> - PluginAppletViewer#parse had a 'ydisp' variable that has been changed to a static variable, since
>>> the parse method will now only ever handle one applet tag (the old version expected to have
>>> potentially multiple). However I'm not 100% about this because the old version as well only ever
>>> received one applet tag, rendering this effectively to always be 1... I'm not sure if the behaviour
>>> should be 'fixed' this way.
>>
>> Your arguments seemed valid. And although PluginAppletViewer is singleton, making the ydisp variable static looks like it will behave differently. I would recommend empiric testing :)) See how multiple applets are shown on (multiple)page(s) and compare with your new implementation :)
>>>
>>> - The code was made to behave as-it-were as much as possible, meaning it can print a warning about a
>>> "missing code attribute in the embed tag" no matter what tag was used. This is as it was because ITW
>>> would always get passed an embed tag. Feel free to force me to change it:)
>>>
>>> ChangeLog:
>>> 2012-10-25 Adam Domurad <adomurad at redhat.com>
>>>
>>> Remove the applet/embed/object tag parser from ITW. Send the applet
>>> parameters directly from the C++.
>>> * Makefile.am: Allow unit-testing for classes in plugin.jar.
>>> * plugin/icedteanp/IcedTeaNPPlugin.cc: Send quoted parameter
>>> name/values instead of applet tag. Remove some dead code.
>>> * plugin/icedteanp/IcedTeaNPPlugin.h: Rename applet_tag ->
>>> parameters_string.
>>> * plugin/icedteanp/java/sun/applet/PluginAppletViewer.java:
>>> Extract parsing code into its own class.
>>> * plugin/icedteanp/java/sun/applet/PluginAppletAttributes.java:
>>> New, encapsulates the (simplified) attribute parsing logic.
>>> * tests/netx/unit/sun/applet/PluginAppletAttributesTest.java:
>>> Unit tests for parsing logic.
>>>
>>>
>>> nuke-parser.patch
>>>
>>>
>>> diff --git a/Makefile.am b/Makefile.am
>>> --- a/Makefile.am
>>> +++ b/Makefile.am
>>> @@ -882,7 +882,7 @@ stamps/netx-unit-tests-compile.stamp: st
...
>>>
>>> return encoded;
>>
>> This is probably strangest stuff in patch. NPAPI is returning already decoded strings. Why to encode them again?!?!?!
>>
>> Unless I see something very wrong, get rid of this unnecessary middle step!
>>
>>> }
>>>
...
>>> +import java.util.List;
>>> +
>>> +/**
>>> + * Provides parsing for applet attributes passed from the C++ side as quoted,
>>> + * encoded strings. This class is not meant to be initialized
>>
>> No author in ITW sources please:)
>>
>> Please, mke this class as classical object. You can still keep some static (or singleton) utility methods here, but prefer more object way:
>>
>> Hashtable<String, String> atts = PluginAppletAttributes.getPArser().parse(width, height, tag);
>> or
>> Hashtable<String, String> atts = new PluginAppletAttributes().parse(width, height, tag);
>>
>> Although I would like to recomand to wrap the Hashtable<String, String> to Object. Eg ParsedAppletTag. so the above methods wirr return this one.
>> One will thenbe able to access parsedAppletTag.getKey() and will be also able to recieve something else then Strings
>>
>> And again. Get rid of redundant decoding!
>>
>>
>>> + * @author Adam Domurad
>>> + */
>>> +class PluginAppletAttributes {
>>> + /**
>>> + * Decodes the string (converts html escapes into proper characters)
>>> + *
>>> + * @param toDecode The string to decode
>>> + * @return The decoded string
>>> + */
>>> + static String decodeString(String toDecode) {
>>> +
>>> + toDecode = toDecode.replace(">", ">");
>>> + toDecode = toDecode.replace("<", "<");
>>> + toDecode = toDecode.replace(" ", "\n");
>>> + toDecode = toDecode.replace(" ", "\r");
>>> + toDecode = toDecode.replace(""", "\"");
>>> + toDecode = toDecode.replace("&", "&");
>>> +
>>> + return toDecode;
>>> + }
>>> +
>>> + static final boolean isInt(String s) {
>>> + try {
>>> + Integer.parseInt(s);
>>> + return true;
>>> + } catch (NumberFormatException e) {
>>> + return false;
>>> + }
>>> + }
>>
>> :DDD no! :D Although this give sense, I would not recommand usage of exception for directing the flow of code :)
>> My suggestion will be matches \d+ ;)
>> I have also some susspiccion that there is ssDigit method somewhere.
>>
>>> +
>>> + static List<String> extractQuotedStrings(String s) {
>>> + List<String> strs = new ArrayList<String>();
...
>>> + AppletMessageHandler amh = new AppletMessageHandler("appletviewer");
>>> + URL url = new URL(documentBase);
>>> + URLConnection conn = url.openConnection();
>>> + /* The original URL may have been redirected - this
>>> + * sets it to whatever URL/codebase we ended up getting
>>> + */
>>> + url = conn.getURL();
>>> +
>>
>> I would like to see thios in different method. Also the ydisp remains questionable. Imho with yournew approach it will lead to complete removal of it (i'm really afraid of case like three applets per page, and several such a pages)
>>
>>> + Hashtable<String, String> atts = PluginAppletAttributes.parse(width, height, tag);
>>> +
...
>>> + Hashtable<String, String> atts;
>>> +
>>> + atts = PluginAppletAttributes.parse(width, height, codeKeyVal);
>>> + assertEquals("1", atts.get("width"));
>>> + assertEquals("1", atts.get("height"));
>>> +
>>> + //Test that width height are defaulted to in case of not-a-number attributes:
>>> + atts = PluginAppletAttributes.parse(width, height, codeKeyVal + " \"width\" \"NAN\" \"height\" \"NAN\" ");
>>> + assertEquals("1", atts.get("width"));
>>> + assertEquals("1", atts.get("height"));
>>> + }
>>> +
>>> + @Test
>>> + public void testAttributeParseCodeAttribute() {
>>> + final String width = "1", height = "1";
>>> + Hashtable<String, String> atts;
>>> +
>>> + atts = PluginAppletAttributes.parse(width, height, "\"classid\" \"classidValue\" ");
>>> + assertEquals("classidValue", atts.get("code"));
>>> +
>>> + atts = PluginAppletAttributes.parse(width, height, "\"code\" \"java:codeValue\" ");
>>> + assertEquals("codeValue", atts.get("code"));
>>> +
>>> + atts = PluginAppletAttributes.parse(width, height, "\"classid\" \"clsid:classidValue\" ");
>>> + assertEquals(null, atts); //NB: atts == null if theres no object or code attribute
>>> +
>>> + atts = PluginAppletAttributes.parse(width, height, "\"object\" \"objectValue\" ");
>>> + assertEquals("objectValue", atts.get("object"));
>>> + }
>>> +}
>>
>>
>> I do not want to be pedantic, but how deep is coverage of those?
>> And probably tehre will come need to test possible new ParsedAppletTAg class.
>>>
>>
>> Thanx for valuable patch!
>>
>> J.
>>
>
> Thanks for the comments, I hope to have addressed them. Attached is iteration 2, let me know what you think of the direction it is going.
> This patch introduces net.sourceforge.jnlp.PluginParameters as well as sun.applet.PluginParameterParser. I have removed the use of static method as suggested.
>
> The decoded was replaced by a simple escaping.
> The wrapped PluginParameter's class was used where I thought was logical, please comment especially on the placement of this class where Hashtable was previously used.
>
> Thanks for the review & hints,
> - Adam
> nuke-parser2.patch
>
>
> diff --git a/Makefile.am b/Makefile.am
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -882,7 +882,7 @@ stamps/netx-unit-tests-compile.stamp: st
> mkdir -p $(NETX_UNIT_TEST_DIR)&& \
> $(BOOT_DIR)/bin/javac $(IT_JAVACFLAGS) \
> -d $(NETX_UNIT_TEST_DIR) \
> - -classpath $(JUNIT_JAR):$(NETX_DIR)/lib/classes.jar:$(TEST_EXTENSIONS_DIR) \
> + -classpath $(JUNIT_JAR):$(DESTDIR)$(datadir)/$(PACKAGE_NAME)/plugin.jar:$(NETX_DIR)/lib/classes.jar:$(TEST_EXTENSIONS_DIR) \
> @netx-unit-tests-source-files.txt&& \
> mkdir -p stamps&& \
> touch $@
> @@ -912,7 +912,7 @@ stamps/run-netx-unit-tests.stamp: stamps
> done ; \
> cd $(NETX_UNIT_TEST_DIR) ; \
> class_names=`cat $(UNIT_CLASS_NAMES)` ; \
> - CLASSPATH=$(NETX_DIR)/lib/classes.jar:$(JUNIT_JAR):$(JUNIT_RUNNER_JAR):$(TEST_EXTENSIONS_DIR):. \
> + CLASSPATH=$(NETX_DIR)/lib/classes.jar:$(DESTDIR)$(datadir)/$(PACKAGE_NAME)/plugin.jar:$(JUNIT_JAR):$(JUNIT_RUNNER_JAR):$(TEST_EXTENSIONS_DIR):. \
> $(BOOT_DIR)/bin/java -Xbootclasspath:$(RUNTIME) CommandLine $$class_names
> if WITH_XSLTPROC
> $(XSLTPROC) $(TESTS_SRCDIR)/$(REPORT_STYLES_DIRNAME)/logs.xsl $(NETX_UNIT_TEST_DIR)/ServerAccess-logs.xml> $(TESTS_DIR)/logs_unit.html
> diff --git a/netx-dist-tests-whitelist b/netx-dist-tests-whitelist
> --- a/netx-dist-tests-whitelist
> +++ b/netx-dist-tests-whitelist
> @@ -1,1 +1,1 @@
> -.*
> +AppletTakes.*
> diff --git a/netx/net/sourceforge/jnlp/NetxPanel.java b/netx/net/sourceforge/jnlp/NetxPanel.java
> --- a/netx/net/sourceforge/jnlp/NetxPanel.java
> +++ b/netx/net/sourceforge/jnlp/NetxPanel.java
> @@ -28,7 +28,6 @@ import net.sourceforge.jnlp.runtime.JNLP
>
> import java.net.URL;
> import java.util.HashMap;
> -import java.util.Hashtable;
> import java.util.Map;
> import java.util.concurrent.ConcurrentHashMap;
> import java.util.concurrent.ConcurrentMap;
> @@ -43,11 +42,11 @@ import sun.awt.SunToolkit;
> * @author Francis Kung<fkung at redhat.com>
> */
> public class NetxPanel extends AppletViewerPanel {
> - private PluginBridge bridge = null;
> + private final PluginParameters parameters;
> + private PluginBridge bridge;
> private boolean exitOnFailure = true;
> - private AppletInstance appInst = null;
> + private AppletInstance appInst;
Why remove of null?
> private boolean appletAlive;
> - private final String uKey;
>
...looks ok...
> for (String cacheJar : cacheJars) {
> diff --git a/netx/net/sourceforge/jnlp/PluginParameters.java b/netx/net/sourceforge/jnlp/PluginParameters.java
> new file mode 100644
> --- /dev/null
> +++ b/netx/net/sourceforge/jnlp/PluginParameters.java
> @@ -0,0 +1,232 @@
> +/* PluginAppletAttributes -- Provides parsing for applet attributes
...
> +
> + /**
> + * Creates the underlying hash table with the proper overrides.
> + * Ensure all keys are lowercase consistently.
> + *
> + * @param params the properties, before parameter aliasing rules.
> + * @return the resulting parameter table
> + */
The only reasons why you are introducing your own hashmap (backed by classical one) instead of direct backing are handling of case and java_ prefix?
Hmm.. sounds little bit fragile ;-/
> + static Hashtable<String, String> createParameterTable(Map<String, String> rawParams) {
> + Hashtable<String, String> params = new Hashtable<String, String>();
> +
> + for (Map.Entry<String, String> entry : rawParams.entrySet()) {
> + String key = entry.getKey().toLowerCase();
> + String value = entry.getValue();
> + params.put(key, value);
> + }
> +
> + // If there is a classid and no code tag present, transform it to code tag
> + if (params.get("code") == null&& params.get("classid") != null
missing spaces before && ( " "&&" " ) all around. Please fix to java-style.
> +&& !(params.get("classid")).startsWith("clsid:")) {
> + params.put("code", params.get("classid"));
> + }
> +
> + // remove java: from code tag
> + if (params.get("code") != null&& (params.get("code")).startsWith("java:")) {
> + params.put("code", (params.get("code")).substring(5));
> + }
> +
> + // java_* aliases override older names:
> + //http://java.sun.com/j2se/1.4.2/docs/guide/plugin/developer_guide/using_tags.html#in-nav
> + if (params.get("java_code") != null) {
> + params.put("code", (params.get("java_code")));
> + }
> +
> + if (params.get("java_codebase") != null) {
> + params.put("codebase", (params.get("java_codebase")));
> + }
> +
> + if (params.get("java_archive") != null) {
> + params.put("archive", (params.get("java_archive")));
> + }
> +
> + if (params.get("java_object") != null) {
> + params.put("object", (params.get("java_object")));
> + }
> +
> + if (params.get("java_type") != null) {
> + params.put("type", (params.get("java_type")));
> + }
> + return params;
> + }
Persoanlly I'm strongly against inner classes and exceptions. Especially against package-private inner classes.
I would strongly recommend you to move this class to public, outer one, but do as you feel.
Also I'm against localised exceptions, but Here I probably agree.
> +
> + static class PluginParameterException extends RuntimeException {
> + public PluginParameterException(String detail) {
> + super(detail);
> + }
> + }
> +
> + public String toString() {
> + return parameters.toString();
> + }
> +}
> \ No newline at end of file
> diff --git a/netx/net/sourceforge/jnlp/resources/Messages.properties b/netx/net/sourceforge/jnlp/resources/Messages.properties
> --- a/netx/net/sourceforge/jnlp/resources/Messages.properties
> +++ b/netx/net/sourceforge/jnlp/resources/Messages.properties
> @@ -141,6 +141,7 @@ BFileLoc=JNLP file location
> BBadProp=Incorrect property format {0} (should be key=value)
> BBadParam=Incorrect parameter format {0} (should be name=value)
> BNoDir=Directory {0} does not exist.
Isnt field little bit confusing? Attribute perhaps?
> +BNoCodeOrObjectApplet=Applet tag must specify a 'code' or 'object' field.
In cz_CS it will be
+BNoCodeOrObjectApplet=Značka applet musí mít defnován kód nebo objekt - atributy 'code' nebo 'object' chybí.
> RNoResource=Missing Resource: {0}
> RShutdown=This exception to prevent shutdown of JVM, but the process has been terminated.
> RExitTaken=Exit class already set and caller is not exit class.
good :))
Maybe some explanation of : -> ; transformation is worthy
> - * Return string is allocated on the heap. Caller assumes responsibility
> - * for freeing the memory via free()
> + * Escape characters for pssing to Java.
> + * "\n" for new line, "\\" for "\", "\:" for ";"
> */
> -static char*
> -encode_string(char* to_encode)
> -{
> -
> - // Do nothing for an empty string
> - if (to_encode == '\0')
> - return to_encode;
> -
> - // worst case scenario -> all characters are newlines or
> - // returns, each of which translates to 5 substitutions
> - char* encoded = (char*) calloc(((strlen(to_encode)*5)+1), sizeof(char));
> -
> - strcpy(encoded, "");
> -
> - for (int i=0; i< strlen(to_encode); i++)
> +static std::string
> +escape_string(const char* to_encode) {
> + std::string encoded;
> +
> + if (to_encode == NULL)
> {
> - if (to_encode[i] == '\r')
> - encoded = strcat(encoded, " ");
> - else if (to_encode[i] == '\n')
> - encoded = strcat(encoded, " ");
> - else if (to_encode[i] == '>')
> - encoded = strcat(encoded, ">");
> - else if (to_encode[i] == '<')
> - encoded = strcat(encoded, "<");
> - else if (to_encode[i] == '&')
> - encoded = strcat(encoded, "&");
> - else if (to_encode[i] == '"')
> - encoded = strcat(encoded, """);
> + return encoded;
> + }
> +
> + size_t length = strlen(to_encode);
> + for (int i=0; i< length; i++)
> + {
> + if (to_encode[i] == '\n')
> + encoded += "\\n";
> + else if (to_encode[i] == '\\')
> + encoded += "\\\\";
> + else if (to_encode[i] == ';')
> + encoded += "\\:";
> else
> - {
> - char* orig_char = (char*) calloc(2, sizeof(char));
> - orig_char[0] = to_encode[i];
> - orig_char[1] = '\0';
> -
> - strcat(encoded, orig_char);
> -
> - free(orig_char);
> - orig_char = NULL;
> - }
> + encoded += to_encode[i];
> }
>
> return encoded;
> }
>...
>
> import net.sourceforge.jnlp.NetxPanel;
> +import net.sourceforge.jnlp.PluginParameters;
> import net.sourceforge.jnlp.runtime.JNLPClassLoader;
> import sun.awt.AppContext;
> import sun.awt.SunToolkit;
> @@ -124,14 +118,14 @@ class PluginAppletPanelFactory {
>
> public AppletPanel createPanel(PluginStreamHandler streamhandler,
> final int identifier,
> - final long handle, int x, int y,
> + final long handle,
> final URL doc,
> - final Hashtable<String, String> atts) {
> + final PluginParameters params) {
> final NetxPanel panel = AccessController.doPrivileged(new PrivilegedAction<NetxPanel>() {
> public NetxPanel run() {
> - NetxPanel panel = new NetxPanel(doc, atts, false);
> + NetxPanel panel = new NetxPanel(doc, params, false);
> NetxPanel.debug("Using NetX panel");
> - PluginDebug.debug(atts.toString());
> + PluginDebug.debug(params.toString());
> return panel;
> }
> });
> @@ -185,7 +179,7 @@ class PluginAppletPanelFactory {
> try {
> SwingUtilities.invokeAndWait(new Runnable() {
> public void run() {
> - panel.getParent().setSize(Integer.valueOf(atts.get("width")), Integer.valueOf(atts.get("height")));
> + panel.getParent().setSize(params.getWidth(), params.getHeight());
> }
> });
> } catch (InvocationTargetException ite) {
> @@ -480,18 +474,28 @@ public class PluginAppletViewer extends
> int spaceLocation = message.indexOf(' ', "tag".length() + 1);
> String documentBase =
> UrlUtil.decode(message.substring("tag".length() + 1, spaceLocation));
> - String tag = message.substring(spaceLocation + 1);
> + String paramString = message.substring(spaceLocation + 1);
>
> PluginDebug.debug("Handle = ", handle, "\n",
> "Width = ", width, "\n",
> "Height = ", height, "\n",
> "DocumentBase = ", documentBase, "\n",
> - "Tag = ", tag);
> + "Params = ", paramString);
>
> - PluginAppletViewer.parse
> - (identifier, handle, width, height,
> - new StringReader(tag),
> - new URL(documentBase));
> + PluginAppletPanelFactory factory = new PluginAppletPanelFactory();
> + AppletMessageHandler amh = new AppletMessageHandler("appletviewer");
> + URL url = new URL(documentBase);
> + URLConnection conn = url.openConnection();
> + /* The original URL may have been redirected - this
> + * sets it to whatever URL/codebase we ended up getting
> + */
> + url = conn.getURL();
> +
> + PluginParameters params = new PluginParameterParser().parse(width, height, paramString);
> +
> + // Let user know we are starting up
> + streamhandler.write("instance " + identifier + " status " + amh.getMessage("status.start"));
> + factory.createPanel(streamhandler, identifier, handle, url, params);
This several lines scares em a bit if all current tesakses are pasing, then I'm just paranoic. Being you, I woulld not trust me and rerun once more times.
>
> long maxTimeToSleep = APPLET_TIMEOUT;
> appletsLock.lock();
> @@ -695,10 +699,10 @@ public class PluginAppletViewer extends
> // 0 => width, 1=> width_value, 2 => height, 3=> height_value
> String[] dimMsg = message.split(" ");
>
> + final int width = Integer.parseInt(dimMsg[1]);
> final int height = Integer.parseInt(dimMsg[3]);
> - final int width = Integer.parseInt(dimMsg[1]);
^^ ??
>
> - panel.updateSizeInAtts(height, width);
> + panel.updateSizeInAtts(width, height);
This is probably strongly unrelated.
>
> try {
> SwingUtilities.invokeAndWait(new Runnable() {
> @@ -1423,24 +1427,6 @@ public class PluginAppletViewer extends
> }
>
> /**
> - * Decodes the string (converts html escapes into proper characters)
...god that this is away!
> -
> - private static AppletMessageHandler amh = new AppletMessageHandler("appletviewer");
>
> private static void checkConnect(URL url) {
> SecurityManager security = System.getSecurityManager();
> diff --git a/plugin/icedteanp/java/sun/applet/PluginParameterParser.java b/plugin/icedteanp/java/sun/applet/PluginParameterParser.java
> new file mode 100644
> --- /dev/null
> +++ b/plugin/icedteanp/java/sun/applet/PluginParameterParser.java
> @@ -0,0 +1,85 @@
> +package sun.applet;
> +
> +import java.util.HashMap;
> +import java.util.Map;
> +
> +import net.sourceforge.jnlp.PluginParameters;
> +
> +class PluginParameterParser {
> + static private final char DELIMITER_ESCAPE = ':';
> + static private final String KEY_VALUE_DELIMITER = ";";
> +
> + /**
> + * Unescape characters passed from C++.
> + * Specifically, "\n" -> new line, "\\" -> "\", "\:" -> ";"
> + *
> + * @param str The string to unescape
> + * @return The unescaped string
> + */
> + static String unescapeString(String str) {
> + StringBuilder sb = new StringBuilder();
> + for (int i = 0; i< str.length(); i++) {
> + char chr = str.charAt(i);
> + if (chr != '\\') {
> + sb.append(chr);
> + } else {
> + i++; // Skip ahead one
> + chr = str.charAt(i);
> + if (chr == 'n') {
> + sb.append('\n');
> + } else if (chr == '\\') {
> + sb.append('\\');
> + } else if (chr == DELIMITER_ESCAPE) {
> + sb.append(KEY_VALUE_DELIMITER);
> + }
> + }
> + }
> + return sb.toString();
> + }
> +
> + /**
> + * Parse semi-colon delimited key-value pairs.
> + * @param keyvalString the escaped, semicolon-delimited, string
> + * @return a map of the keys to the values
> + */
> + static Map<String, String> parseEscapedKeyValuePairs(String keyvalString) {
> + String[] strs = keyvalString.split(KEY_VALUE_DELIMITER);
> + Map<String, String> attributes = new HashMap<String, String>();
> +
> + for (int i = 0; i< strs.length; i += 2) {
> + String key = unescapeString(strs[i]).toLowerCase();
> + String value = unescapeString(strs[i + 1]);
> + attributes.put(key, value);
> + }
> +
> + return attributes;
> + }
> +
> + static boolean isInt(String s) {
> + return s.matches("^-?\\d+$");
> + }
> +
> + /**
> + * Parsers parameters given a string containing
> + * parameters in quotes.
> + *
> + * @param width default applet width
> + * @param height default applet height
> + * @param parameterString the parameters
> + * @return the attributes in a hash table
> + */
> + public PluginParameters parse(String width,
> + String height, String parameterString) {
> + Map<String, String> params = parseEscapedKeyValuePairs(parameterString);
> +
> + if (params.get("width") == null || !isInt(params.get("width"))) {
> + params.put("width", width);
> + }
> +
> + if (params.get("height") == null || !isInt(params.get("height"))) {
> + params.put("height", height);
> + }
> +
> + return new PluginParameters(params);
> + }
> +}
> diff --git a/tests/netx/unit/net/sourceforge/jnlp/PluginBridgeTest.java b/tests/netx/unit/net/sourceforge/jnlp/PluginBridgeTest.java
> --- a/tests/netx/unit/net/sourceforge/jnlp/PluginBridgeTest.java
> +++ b/tests/netx/unit/net/sourceforge/jnlp/PluginBridgeTest.java
> @@ -29,6 +29,7 @@ import java.net.MalformedURLException;
> import java.net.URL;
> import java.util.HashMap;
> import java.util.Hashtable;
> +import java.util.Map;
>
> import net.sourceforge.jnlp.cache.UpdatePolicy;
>
> @@ -60,14 +61,20 @@ public class PluginBridgeTest {
> }
> }
>
> + static private PluginParameters createValidParamObject() {
> + Map<String, String> params = new HashMap<String, String>();
> + params.put("code", ""); // Avoids an exception being thrown
> + return new PluginParameters(params);
> + }
> +
> @Test
> public void testAbsoluteJNLPHref() throws MalformedURLException, Exception {
> URL codeBase = new URL("http://undesired.absolute.codebase.com");
> String absoluteLocation ="http://absolute.href.com/test.jnlp";
> - Hashtable<String, String> atts = new Hashtable<String, String>();
> - atts.put("jnlp_href", absoluteLocation);
> + PluginParameters params = createValidParamObject();
> + params.put("jnlp_href", absoluteLocation);
> MockJNLPCreator mockCreator = new MockJNLPCreator();
> - PluginBridge pb = new PluginBridge(codeBase, null, "", "", 0, 0, atts, "", mockCreator);
> + PluginBridge pb = new PluginBridge(codeBase, null, "", "", 0, 0, params, mockCreator);
> assertEquals(absoluteLocation, mockCreator.getJNLPHref().toExternalForm());
> }
>
> @@ -75,12 +82,12 @@ public class PluginBridgeTest {
> public void testRelativeJNLPHref() throws MalformedURLException, Exception {
> URL codeBase = new URL("http://desired.absolute.codebase.com/");
> String relativeLocation = "sub/dir/test.jnlp";
> - Hashtable<String, String> atts = new Hashtable<String, String>();
> - atts.put("jnlp_href", relativeLocation);
> + PluginParameters params = createValidParamObject();
> + params.put("jnlp_href", relativeLocation);
> MockJNLPCreator mockCreator = new MockJNLPCreator();
> - PluginBridge pb = new PluginBridge(codeBase, null, "", "", 0, 0, atts, "", mockCreator);
> + PluginBridge pb = new PluginBridge(codeBase, null, "", "", 0, 0, params, mockCreator);
> assertEquals(codeBase.toExternalForm() + relativeLocation,
> - mockCreator.getJNLPHref().toExternalForm());
> + mockCreator.getJNLPHref().toExternalForm());
> }
>
> @Test
> @@ -88,12 +95,12 @@ public class PluginBridgeTest {
> String desiredDomain ="http://desired.absolute.codebase.com";
> URL codeBase = new URL(desiredDomain + "/undesired/sub/dir");
> String relativeLocation = "/app/test/test.jnlp";
> - Hashtable<String, String> atts = new Hashtable<String, String>();
> - atts.put("jnlp_href", relativeLocation);
> + PluginParameters params = createValidParamObject();
> + params.put("jnlp_href", relativeLocation);
> MockJNLPCreator mockCreator = new MockJNLPCreator();
> - PluginBridge pb = new PluginBridge(codeBase, null, "", "", 0, 0, atts, "", mockCreator);
> + PluginBridge pb = new PluginBridge(codeBase, null, "", "", 0, 0, params, mockCreator);
> assertEquals(desiredDomain + relativeLocation,
> - mockCreator.getJNLPHref().toExternalForm());
> + mockCreator.getJNLPHref().toExternalForm());
> }
>
> }
> \ No newline at end of file
> diff --git a/tests/netx/unit/net/sourceforge/jnlp/PluginParametersTest.java b/tests/netx/unit/net/sourceforge/jnlp/PluginParametersTest.java
> new file mode 100644
> --- /dev/null
> +++ b/tests/netx/unit/net/sourceforge/jnlp/PluginParametersTest.java
> @@ -0,0 +1,83 @@
> +package net.sourceforge.jnlp;
> +
> +import static org.junit.Assert.*;
> +
> +import java.util.HashMap;
> +import java.util.Hashtable;
> +import java.util.Map;
> +
> +import org.junit.Test;
> +
> +public class PluginParametersTest {
> +
> + @Test
> + public void testAttributeParseJavaPrefix() {
> + // java_* aliases override older names:
> + //http://java.sun.com/j2se/1.4.2/docs/guide/plugin/developer_guide/using_tags.html#in-nav
> +
> + Map<String, String> rawParams;
> + Hashtable<String, String> params;
> +
> + rawParams = new HashMap<String, String>();
> + rawParams.put("code", "codeValue");
> + rawParams.put("java_code", "java_codeValue");
> + params = PluginParameters.createParameterTable(rawParams);
> +
> + assertEquals("java_codeValue", params.get("code"));
> +
> + rawParams = new HashMap<String, String>();
> + rawParams.put("codebase", "codebaseValue");
> + rawParams.put("java_codebase", "java_codebaseValue");
> + params = PluginParameters.createParameterTable(rawParams);
> +
> + assertEquals("java_codebaseValue", params.get("codebase"));
> +
> + rawParams = new HashMap<String, String>();
> + rawParams.put("archive", "archiveValue");
> + rawParams.put("java_archive", "java_archiveValue");
> + params = PluginParameters.createParameterTable(rawParams);
> +
> + assertEquals("java_archiveValue", params.get("archive"));
> +
> + rawParams = new HashMap<String, String>();
> + rawParams.put("object", "objectValue");
> + rawParams.put("java_object", "java_objectValue");
> + params = PluginParameters.createParameterTable(rawParams);
> +
> + assertEquals("java_objectValue", params.get("object"));
> +
> + rawParams = new HashMap<String, String>();
> + rawParams.put("type", "typeValue");
> + rawParams.put("java_type", "java_typeValue");
> + params = PluginParameters.createParameterTable(rawParams);
> +
> + assertEquals("java_typeValue", params.get("type"));
> + }
> +
> + @Test
> + public void testAttributeParseCodeAttribute() {
> + Map<String, String> rawParams;
> + Hashtable<String, String> params;
> +
> + rawParams = new HashMap<String, String>();
> + rawParams.put("classid", "classidValue");
> + params = PluginParameters.createParameterTable(rawParams);
> + assertEquals("classidValue", params.get("code"));
> +
> + rawParams = new HashMap<String, String>();
> + rawParams.put("code", "java:codeValue");
> + params = PluginParameters.createParameterTable(rawParams);
> + assertEquals("codeValue", params.get("code"));
> +
> + rawParams = new HashMap<String, String>();
> + rawParams.put("classid", "clsid:classidValue");
> + params = PluginParameters.createParameterTable(rawParams);
> + assertEquals(null, params.get("code"));
> +
> + rawParams = new HashMap<String, String>();
> + rawParams.put("object", "objectValue");
> + params = PluginParameters.createParameterTable(rawParams);
> + assertEquals("objectValue", params.get("object"));
> + }
> +
> +}
> diff --git a/tests/netx/unit/sun/applet/PluginParameterParserTest.java b/tests/netx/unit/sun/applet/PluginParameterParserTest.java
> new file mode 100644
> --- /dev/null
> +++ b/tests/netx/unit/sun/applet/PluginParameterParserTest.java
> @@ -0,0 +1,57 @@
> +package sun.applet;
> +
> +import static org.junit.Assert.*;
> +
> +import java.util.Map;
> +
> +import net.sourceforge.jnlp.PluginParameters;
> +
> +import org.junit.Test;
> +
> +public class PluginParameterParserTest {
> +
> + @Test
> + public void testIsInt() {
> + assertFalse(PluginParameterParser.isInt("1.0"));
> + assertFalse(PluginParameterParser.isInt("abc"));
> + assertTrue(PluginParameterParser.isInt("1"));
> + }
> +
> + @Test
> + public void testUnescapeString() {
> + assertEquals("start\n;end\\;",
> + PluginParameterParser.unescapeString("start\\n\\:end\\\\;"));
> + }
> +
> + @Test
> + public void testParseEscapedKeyValuePairs() {
> + final String quotedPartString = "key1;value1;KEY2\\:;value2\\\\;";
> + Map<String, String> params = PluginParameterParser.parseEscapedKeyValuePairs(quotedPartString);
> +
> + assertEquals(params.size(), 2);
> +
> + assertEquals(params.get("key1"), "value1");
> + //ensure key is lowercased
> + assertEquals(params.get("key2;"), "value2\\");
> + }
> +
> + @Test
> + public void testAttributeParseWidthHeightAttributes() {
> + final String width = "1", height = "1";
> + final String codeKeyVal = "code;codeValue;";
> +
> + PluginParameterParser parser = new PluginParameterParser();
> + PluginParameters params;
> +
> + params = parser.parse(width, height, codeKeyVal);
> + assertEquals("1", params.get("width"));
> + assertEquals("1", params.get("height"));
> +
> + //Test that width height are defaulted to in case of not-a-number attributes:
> + params = parser.parse(width, height, codeKeyVal + " width;NAN;height;NAN;");
> + assertEquals("1", params.get("width"));
> + assertEquals("1", params.get("height"));
> + }
> +
> +
> +}
Except the minor stuff I'm ok with the chage.
J.
More information about the distro-pkg-dev
mailing list