[rfc][icedtea-web] bringing applets out of browser (part2)
Jie Kang
jkang at redhat.com
Mon Dec 15 20:23:40 UTC 2014
Hello,
Review in-line with code below:
diff -r b5fa37369ea3 netx/net/sourceforge/jnlp/MalformedXMLParser.java
--- a/netx/net/sourceforge/jnlp/MalformedXMLParser.java Tue Dec 09 10:10:10 2014 +0100
+++ b/netx/net/sourceforge/jnlp/MalformedXMLParser.java Thu Dec 11 17:09:23 2014 +0100
@@ -45,7 +45,6 @@
[...]
- private InputStream xmlizeInputStream(InputStream original) throws ParseException {
+ public static InputStream xmlizeInputStream(InputStream original) throws ParseException {
This change concerns me. I can understand making it public, but why static as well? Can't a user construct an object and call the method from the object?
In Parser.java we have:
Object instance = klass.newInstance();
Method m = klass.getMethod("getRootNode", InputStream.class);
whereas in AppletExtracter.java we have:
+ Class<?> klass = Class.forName(Parser.MALFORMED_PARSER_CLASS);
+ Method m = klass.getMethod("xmlizeInputStream", InputStream.class);
I think you should be consistent. Either instantiate a MalformedXMLParser in all cases, or don't instantiate one ever (make getRootNode static).
diff -r b5fa37369ea3 netx/net/sourceforge/jnlp/NetxPanel.java
--- a/netx/net/sourceforge/jnlp/NetxPanel.java Tue Dec 09 10:10:10 2014 +0100
+++ b/netx/net/sourceforge/jnlp/NetxPanel.java Thu Dec 11 17:09:23 2014 +0100
@@ -67,9 +67,9 @@
+ public void init(PluginBridge bridge) throws LaunchException {
Just wanted to say, I really like this refactor, it makes code easier to read.
diff -r b5fa37369ea3 netx/net/sourceforge/jnlp/PluginBridge.java
--- a/netx/net/sourceforge/jnlp/PluginBridge.java Tue Dec 09 10:10:10 2014 +0100
+++ b/netx/net/sourceforge/jnlp/PluginBridge.java Thu Dec 11 17:09:23 2014 +0100
@@ -49,7 +49,7 @@
*/
public class PluginBridge extends JNLPFile {
Something I've been thinking about for quite a while here is: we probably shouldn't extend JNLPFile, instead we should have an instance of JNLPFile inside the PluginBridge. It would make things much easier to extend when we add features and require more things from the JNLPFile to be used by the PluginBridge. Just a thought.
diff -r b5fa37369ea3 netx/net/sourceforge/jnlp/runtime/AppletExtracter.java
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/netx/net/sourceforge/jnlp/runtime/AppletExtracter.java Thu Dec 11 17:09:23 2014 +0100
@@ -0,0 +1,273 @@
s/Extracter/Extractor/
+ private static final String[] APPLETS = new String[]{
+ "applet", "APPLET", "Applet",
Instead of containing a list of accepted formats for 'applet', shouldn't the parser just accept any form of 'applet'? Just take whatever input string is and make it all lower-case, then check if it is the same as 'applet'. Is it because elements such as 'APPLet' not acceptable?
+ private static class AppletParser {
[...]
+ public AppletParser(Element applet, URL docbase) {
+ this.source = applet;
Why the difference in field name and constructor argument name? I think they should be the same.
Can you also add some comments to what AppletParser is parsing from and what it is parsing into?
+ private URL createCodebase() throws ParseException, MalformedURLException {
+ String written = source.getAttribute("codebase");
Instead of String written, how about String codebase?
diff -r b5fa37369ea3 netx/net/sourceforge/jnlp/runtime/Boot.java
--- a/netx/net/sourceforge/jnlp/runtime/Boot.java Tue Dec 09 10:10:10 2014 +0100
+++ b/netx/net/sourceforge/jnlp/runtime/Boot.java Thu Dec 11 17:09:23 2014 +0100
@@ -16,23 +16,30 @@
[...]
+// int width = e.getComponent().getWidth();
+// int height = e.getComponent().getHeight();
+// np.updateSizeInAtts(height, width);
+// np.setSize(1, 1);
+// np.validate();
+// np.setSize(width, height);
+// np.validate();
+// np.getApplet().resize(width, height);
+// np.getApplet().validate();
Please remove this code if it's no longer needed.
diff -r b5fa37369ea3 plugin/icedteanp/java/sun/applet/PluginMain.java
--- a/plugin/icedteanp/java/sun/applet/PluginMain.java Tue Dec 09 10:10:10 2014 +0100
+++ b/plugin/icedteanp/java/sun/applet/PluginMain.java Thu Dec 11 17:09:23 2014 +0100
@@ -84,7 +84,6 @@
[...]
+ initSecurityContext(streamHandler);
+
PluginAppletViewer.setStreamhandler(streamHandler);
I think there might be extra spaces above.
diff -r b5fa37369ea3 plugin/icedteanp/java/sun/applet/PluginStreamHandler.java
--- a/plugin/icedteanp/java/sun/applet/PluginStreamHandler.java Tue Dec 09 10:10:10 2014 +0100
+++ b/plugin/icedteanp/java/sun/applet/PluginStreamHandler.java Thu Dec 11 17:09:23 2014 +0100
@@ -53,6 +53,20 @@
+ public static final PluginStreamHandler DummyHandler = new PluginStreamHandler(new InputStream() {
+
+ @Override
+ public int read() throws IOException {
+ return 0;
+ }
The line spacing here makes it a little awkward to read. Can you put the 'new InputStream() {' on a new line as well?
+ }, new OutputStream() {
+
+ @Override
+ public void write(int b) throws IOException {
+ //
+ }
+ });
So for above it would become:
+ public static final PluginStreamHandler DummyHandler = new PluginStreamHandler(
+ new InputStream() {
+ @Override
+ public int read() throws IOException {
+ return 0;
+ }
+ }, new OutputStream() {
+ @Override
+ public void write(int b) throws IOException {
+ //
+ }
+ });
Looks nice so far. Will continue manual testing and if I find any errors I will let you know.
Regards,
----- Original Message -----
> On 12/09/2014 03:04 PM, Jiri Vanek wrote:
> > This is first implementation of -html.
> >
>
> rolling on...
> > Well more easy then expected, but not working as expected... So this is
> > moreover preview and work is
> > still in progess (any hint to current impl welcomed!)
> >
> > Well it do what its description says. The param is html page, it finds an
> > applet on it, and then
> > lunch it... So some shortcut to already implemented shortcut, but it was
> > not intended to be.
> >
> > TRight now it have many flaws:
> > 1) generate jnlp instead of runn directly plugin bridge
> > cannot (sometimes)load html resource, beause it is actually on
> > /tmp
> fixed!
> > 2) plugin.jar should be on javaws classpath too:(
> This will probably become a must, but worka aroundable as not-mandatory
> "depndence"
> > 3) too often needs -nosecurity
> Fixed too, but on contrarry, some of my testing aplets (eg jogam one) do
> not have acces to
> classes like String... Hmm.. Investigation in progress.
> >
> > 1) really should be fixed asap (by me, during this changeset) othewrwise it
> > looses sense
> > 2) those is dont know right now, many appelts needs JSObject, but having
> > it, any runs, but more of
> > them fails in runtime later. So it is really question whether to burden
> > javaws with plugin.jar or
> > work differently
> > 3) this s "I dont understan now: because it is doing the same as generated
> > jnlp shortct from
> > previous patch (yes this one is on to of it) . Jnlp shortcut works, but
> > this /tmp one dont... Anyway
> > must be fixed in this changeset or later too.
> >
>
> The patch is much smaller then it loosk. Most changes are only refactroings,
> which make some part of
> logic accessible. Also I have to revisit them, becasue Ithink after I finally
> made tit work, half of
> them is no longer needed.
>
>
> Rejoice!
> J.
>
>
--
Jie Kang
More information about the distro-pkg-dev
mailing list