[RFC][icedtea-web]: Small fix for PR1189 w/ reproducer
Adam Domurad
adomurad at redhat.com
Tue Oct 16 12:36:11 PDT 2012
On 10/12/2012 02:28 PM, Saad Mohammad wrote:
> Hi,
>
> The patches attached includes the reproducer and the fix for PR1189.
> <http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=1189>
>
> CHANGELOG - BUG FIX
> ========================================================================
> 2012-10-12 Saad Mohammad <smohammad at redhat.com>
>
> Fixes PR1189.
> * netx/net/sourceforge/jnlp/PluginBridge.java (PluginBridge):
Nitpick, (PluginBridge) should be on new-line.
> Overrides main-class name with the one specified in the jnlp
> file if applet tag contains a jnlp_href.
> * plugin/icedteanp/java/sun/applet/PluginAppletViewer.java (parse):
> Removes null assigning to atts if code attribute is missing.
>
> CHANGELOG - REPRODUCER
> ========================================================================
> 2012-10-12 Saad Mohammad <smohammad at redhat.com>
>
> Adds reproducer for PR1189.
> *
> tests/reproducers/simple/AppletTagWithMissingCodeAttribute/resources/AppletTagWithMissingCodeAttribute.html:
> Simple webpage which contains an applet tag with no code attribute.
> *
> tests/reproducers/simple/AppletTagWithMissingCodeAttribute/resources/AppletTagWithMissingCodeAttribute.jnlp:
> Jnlp file that is used by the webpages using jnlp_href.
> *
> tests/reproducers/simple/AppletTagWithMissingCodeAttribute/srcs/SimpleApplet.java:
> Simple applet class that outputs a string.
> *
> tests/reproducers/simple/AppletTagWithMissingCodeAttribute/testcases/AppletTagWithMissingCodeAttribute.java:
> Testcase that tests applets without code attribute in html pages.
>
Reproducer review first, comments inline:
> diff --git
> a/tests/reproducers/simple/AppletTagWithMissingCodeAttribute/resources/AppletTagWithMissingCodeAttribute.html
> b/tests/reproducers/simple/AppletTagWithMissingCodeAttribute/resources/AppletTagWithMissingCodeAttribute.html
> new file mode 100644
> --- /dev/null
> +++
> b/tests/reproducers/simple/AppletTagWithMissingCodeAttribute/resources/AppletTagWithMissingCodeAttribute.html
> @@ -0,0 +1,44 @@
> +<!--
> +This file is part of IcedTea.
> +
> +IcedTea is free software; you can redistribute it and/or modify
> +it under the terms of the GNU General Public License as published by
> +the Free Software Foundation; either version 2, or (at your option)
> +any later version.
> +
> +IcedTea is distributed in the hope that it will be useful, but
> +WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with IcedTea; see the file COPYING. If not, write to the
> +Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> Boston, MA
> +02110-1301 USA.
> +
> +Linking this library statically or dynamically with other modules is
> +making a combined work based on this library. Thus, the terms and
> +conditions of the GNU General Public License cover the whole
> +combination.
> +
> +As a special exception, the copyright holders of this library give you
> +permission to link this library with independent modules to produce an
> +executable, regardless of the license terms of these independent
> +modules, and to copy and distribute the resulting executable under
> +terms of your choice, provided that you also meet, for each linked
> +independent module, the terms and conditions of the license of that
> +module. An independent module is a module which is not derived from
> +or based on this library. If you modify this library, you may extend
> +this exception to your version of the library, but you are not
> +obligated to do so. If you do not wish to do so, delete this
> +exception statement from your version.
> +
> + -->
> +<html><head></head><body bgcolor="black">
> +<p>
> + <applet width="800" height="600">
> + <param name="jnlp_href"
> value="AppletTagWithMissingCodeAttribute.jnlp">
> + </applet>
> +</p>
> +</body>
> +</html>
> diff --git
> a/tests/reproducers/simple/AppletTagWithMissingCodeAttribute/resources/AppletTagWithMissingCodeAttribute.jnlp
> b/tests/reproducers/simple/AppletTagWithMissingCodeAttribute/resources/AppletTagWithMissingCodeAttribute.jnlp
> new file mode 100644
> --- /dev/null
> +++
> b/tests/reproducers/simple/AppletTagWithMissingCodeAttribute/resources/AppletTagWithMissingCodeAttribute.jnlp
> @@ -0,0 +1,57 @@
> +<!--
> +This file is part of IcedTea.
> +
> +IcedTea is free software; you can redistribute it and/or modify
> +it under the terms of the GNU General Public License as published by
> +the Free Software Foundation; either version 2, or (at your option)
> +any later version.
> +
> +IcedTea is distributed in the hope that it will be useful, but
> +WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with IcedTea; see the file COPYING. If not, write to the
> +Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> Boston, MA
> +02110-1301 USA.
> +
> +Linking this library statically or dynamically with other modules is
> +making a combined work based on this library. Thus, the terms and
> +conditions of the GNU General Public License cover the whole
> +combination.
> +
> +As a special exception, the copyright holders of this library give you
> +permission to link this library with independent modules to produce an
> +executable, regardless of the license terms of these independent
> +modules, and to copy and distribute the resulting executable under
> +terms of your choice, provided that you also meet, for each linked
> +independent module, the terms and conditions of the license of that
> +module. An independent module is a module which is not derived from
> +or based on this library. If you modify this library, you may extend
> +this exception to your version of the library, but you are not
> +obligated to do so. If you do not wish to do so, delete this
> +exception statement from your version.
> +
> + -->
> +<?xml version="1.0" encoding="utf-8"?>
> +<jnlp spec="1.0" href="AppletTagWithMissingCodeAttribute.jnlp">
> + <information>
> + <title>AppletTagWithMissingCodeAttribute</title>
> + <vendor>IcedTea</vendor>
> + <homepage
> href="http://icedtea.classpath.org/wiki/IcedTea-Web#Testing_IcedTea-Web "/>
> + <description>AppletTagWithMissingCodeAttribute</description>
> + <offline/>
> + </information>
> + <resources>
> + <j2se version="1.4+"/>
> + <jar href="AppletTagWithMissingCodeAttribute.jar"/>
> + </resources>
> + <applet-desc
> + documentBase="."
> + name="SimpleApplet"
> + main-class="SimpleApplet"
> + width="100"
> + height="100">
> + </applet-desc>
> +</jnlp>
> diff --git
> a/tests/reproducers/simple/AppletTagWithMissingCodeAttribute/srcs/SimpleApplet.java
> b/tests/reproducers/simple/AppletTagWithMissingCodeAttribute/srcs/SimpleApplet.java
> new file mode 100644
> --- /dev/null
> +++
> b/tests/reproducers/simple/AppletTagWithMissingCodeAttribute/srcs/SimpleApplet.java
> @@ -0,0 +1,47 @@
> +/* SimpleApplet.java
> +Copyright (C) 2011 Red Hat, Inc.
> +
> +This file is part of IcedTea.
> +
> +IcedTea is free software; you can redistribute it and/or
> +modify it under the terms of the GNU General Public License as
> published by
> +the Free Software Foundation, version 2.
> +
> +IcedTea is distributed in the hope that it will be useful,
> +but WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with IcedTea; see the file COPYING. If not, write to
> +the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> Boston, MA
> +02110-1301 USA.
> +
> +Linking this library statically or dynamically with other modules is
> +making a combined work based on this library. Thus, the terms and
> +conditions of the GNU General Public License cover the whole
> +combination.
> +
> +As a special exception, the copyright holders of this library give you
> +permission to link this library with independent modules to produce an
> +executable, regardless of the license terms of these independent
> +modules, and to copy and distribute the resulting executable under
> +terms of your choice, provided that you also meet, for each linked
> +independent module, the terms and conditions of the license of that
> +module. An independent module is a module which is not derived from
> +or based on this library. If you modify this library, you may extend
> +this exception to your version of the library, but you are not
> +obligated to do so. If you do not wish to do so, delete this
> +exception statement from your version.
> + */
> +
> +import java.applet.Applet;
> +
> + at SuppressWarnings("serial")
> +public class SimpleApplet extends Applet {
> +
> + @Override
> + public void start() {
> + System.out.println("*** APPLET FINISHED ***");
> + }
> +}
>
Neither here nor there, but it would be nice if there was a standard
'simple applet' that just did this, instead of each test that needs an
applet like this having their own.
> diff --git
> a/tests/reproducers/simple/AppletTagWithMissingCodeAttribute/testcases/AppletTagWithMissingCodeAttribute.java
> b/tests/reproducers/simple/AppletTagWithMissingCodeAttribute/testcases/AppletTagWithMissingCodeAttribute.java
> new file mode 100644
> --- /dev/null
> +++
> b/tests/reproducers/simple/AppletTagWithMissingCodeAttribute/testcases/AppletTagWithMissingCodeAttribute.java
> @@ -0,0 +1,58 @@
> +/* AppletTagWithMissingCodeAttribute.java
> + Copyright (C) 2012 Red Hat, Inc.
> +
> +This file is part of IcedTea.
> +
> +IcedTea is free software; you can redistribute it and/or
> +modify it under the terms of the GNU General Public License as
> published by
> +the Free Software Foundation, version 2.
> +
> +IcedTea is distributed in the hope that it will be useful,
> +but WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with IcedTea; see the file COPYING. If not, write to
> +the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> Boston, MA
> +02110-1301 USA.
> +
> +Linking this library statically or dynamically with other modules is
> +making a combined work based on this library. Thus, the terms and
> +conditions of the GNU General Public License cover the whole
> +combination.
> +
> +As a special exception, the copyright holders of this library give you
> +permission to link this library with independent modules to produce an
> +executable, regardless of the license terms of these independent
> +modules, and to copy and distribute the resulting executable under
> +terms of your choice, provided that you also meet, for each linked
> +independent module, the terms and conditions of the license of that
> +module. An independent module is a module which is not derived from
> +or based on this library. If you modify this library, you may extend
> +this exception to your version of the library, but you are not
> +obligated to do so. If you do not wish to do so, delete this
> +exception statement from your version.
> +*/
> +
> +import junit.framework.Assert;
> +import net.sourceforge.jnlp.ProcessResult;
> +import net.sourceforge.jnlp.ServerAccess.AutoClose;
> +import net.sourceforge.jnlp.browsertesting.BrowserTest;
> +import net.sourceforge.jnlp.browsertesting.Browsers;
> +import net.sourceforge.jnlp.closinglisteners.AutoOkClosingListener;
> +import net.sourceforge.jnlp.annotations.TestInBrowsers;
> +import org.junit.Test;
> +
> +public class AppletTagWithMissingCodeAttribute extends BrowserTest {
> +
> + final static String s =
> AutoOkClosingListener.MAGICAL_OK_CLOSING_STRING;
> +
> + @Test
> + @TestInBrowsers(testIn = { Browsers.one })
> + public void EmbeddedAppletWithMissingCodeAttribute() throws
> Exception {
> + ProcessResult pr =
> server.executeBrowser("/AppletTagWithMissingCodeAttribute.html",
> AutoClose.CLOSE_ON_CORRECT_END);
> + Assert.assertTrue("SimpleApplet.class did not correctly
> launched in
I would prefer if it was simply a message that said "APPLET FINISHED" or
so was not found. The fact that it incorrectly launched can be inferred.
> AppletTagWithMissingCodeAttribute.html: " + pr.stdout + pr.stderr,
> + pr.stdout.contains(s));
> + }
> +}
Otherwise looks good.
Implementation next:
> 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
> @@ -109,6 +109,10 @@
> String fileName =
> jarDesc.getLocation().toExternalForm();
> this.jars.add(fileName);
> }
> +
> + if (jnlpFile.isApplet())
> + main = jnlpFile.getApplet().getMainClass();
> +
I would rather consolidate all the logic regarding the main-class-name
as close together as possible.
> } 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.
> @@ -168,6 +172,9 @@
> else
> name = name + " applet";
Probably can move the 'main' logic above here, so its easier to see
among the other main-class-name handling (Just have to check 'useJNLPHref')
>
> + if (main == null)
> + throw new NullPointerException("Main-class could not be
> determined.");
> +
Manually throwing a NPE seems to me less useful than other types of
exceptions, because NPE usually signifies an accidental de-reference.
Shouldn't this be a launch exception or something ?
> if (main.endsWith(".class"))
> main = main.substring(0, main.length() - 6);
>
> diff --git a/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
> b/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
> --- a/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
> +++ b/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
> @@ -2060,7 +2060,6 @@
>
> if (atts.get("code") == null &&
> atts.get("object") == null) {
> statusMsgStream.println(embedRequiresCodeWarning);
> - atts = null;
> }
>
> if (atts.get("width") == null ||
> !isInt(atts.get("width"))) {
Overall looks good,
Thanks,
- Adam
More information about the distro-pkg-dev
mailing list