[rfc][icedtea-web] Remove need for HTML tag scanner in PluginAppletViewer
Adam Domurad
adomurad at redhat.com
Mon Nov 26 12:07:43 PST 2012
On 11/16/2012 09:14 AM, Jiri Vanek wrote:
> 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?
Dropped this change. On further consideration, the explicit null's do
serve a purpose.
>
>> 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 ;-/
Helper method introduced, but overall as it was.
>
>> + 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.
Autoformatted the class.
>
>> +&& !(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.
You're strongly against inner classes, and strongly against exceptions? :)
Good point though, made public & outer.
>
> 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?
Made field be attribute instead.
>> +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
>
>> [.. snipped ..]
>> - 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.
All tests seem fine at my end.
>
>>
>> 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.
> > [..snipped..]
> Except the minor stuff I'm ok with the chage.
>
> J.
Thanks for the look over Jiri and Pavel.
As Pavel suggested I added unit tests for the case where key/values are
empty - and indeed they did not work as intended. This has been fixed.
I have addressed Jiri's comments, removing some style changes that
probably should not have been snuck in :).
I have done additional testing on corner cases, and added C++ side unit
tests. All tests seem to be in order.
Note that the old parsing code seemed to set x and y locations of
applets, but that turned out to not be the case. The ultimate
destination of the x and y parameters was for the
PluginAppletPanelFactory.createPanel function, which oddly enough did
not use them at all. The X and Y fiddling has thus been dropped
entirely, and it does not seem to have affected anything.
Attached is the patch as well as some additional refactoring in a
separate patch (which I can live without, but could be nice).
2012-11-26 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.
* netx/net/sourceforge/jnlp/NetxPanel.java: Use PluginParameters for
attribute lookup
* netx/net/sourceforge/jnlp/PluginBridge.java: Use PluginParameters
for attribute lookup
* netx/net/sourceforge/jnlp/resources/Messages.properties: Add message
for missing code/object attributes.
* netx/net/sourceforge/jnlp/resources/Messages_cs_CZ.properties: Same.
* plugin/icedteanp/IcedTeaNPPlugin.cc: Send escaped 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.
* tests/cpp-unit-tests/IcedTeaPluginUtilsTest.cc: Use CHECK_EQUALS
instead of CHECK.
* tests/netx/unit/net/sourceforge/jnlp/PluginBridgeTest.java: Update
unit tests due to constructor changes.
* netx/net/sourceforge/jnlp/PluginParameterException.java: New, thrown
when code/object attributes are missing.
* netx/net/sourceforge/jnlp/PluginParameters.java: New, Hashtable
wrapper that handles plugin attribute/parameter lookups.
* plugin/icedteanp/java/sun/applet/PluginParameterParser.java: New,
creates PluginParameters from escaped name/values.
* tests/cpp-unit-tests/PluginParametersTest.cc: New, C++ Unit tests for
plugin parameter related functions
* tests/netx/unit/net/sourceforge/jnlp/PluginParametersTest.java: New,
unit tests for PluginParameters class.
* tests/netx/unit/sun/applet/PluginParameterParserTest.java: New, unit
tests for PluginParameterParser class.
Refactoring patch 1, extract PluginAppletPanelFactory:
2012-11-26 Adam Domurad <adomurad at redhat.com>
* plugin/icedteanp/java/sun/applet/PluginAppletViewer.java: Extracted
PluginAppletPanelFactory.
* plugin/icedteanp/java/sun/applet/PluginAppletPanelFactory.java: Moved
into own file.
Refactoring patch 2, new method
PluginAppletViewer#handleInitializationMessage from handleMessage:
2012-11-26 Adam Domurad <adomurad at redhat.com>
* plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
(handleInitializationMessage): New, extracts initialization logic
from PluginAppletViewer.handleMessage.
Happy hacking,
-Adam
-------------- next part --------------
A non-text attachment was scrubbed...
Name: nuke-parser3.patch
Type: text/x-patch
Size: 84561 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20121126/5ba08193/nuke-parser3.patch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: extract-factory.patch
Type: text/x-patch
Size: 16143 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20121126/5ba08193/extract-factory.patch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: extract-initialization.patch
Type: text/x-patch
Size: 8334 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20121126/5ba08193/extract-initialization.patch
More information about the distro-pkg-dev
mailing list