[rfc][icedtea-web] bringing applets out of browser (part2)
Jie Kang
jkang at redhat.com
Thu Dec 18 18:04:51 UTC 2014
----- Original Message -----
> Here it is!
>
> The ultimate maistrwerk :))
>
> Yes. The applets run by javaws -html really run in applets sandbox! An no
> more hacks done! A lof of
> them is now working (nrearly all, except all jaavscript one :(( )
>
> My favourite is: ./javaws -html
> http://www.w3.org/People/mimasa/test/object/java/clock all :)))
>
> I have cleaned the code a lot - now it can be compiled without plugin, but
> plugin.jar is need in
> run-time, if -html is used (otherwise is not)
>
> I smuggeld two refactorings inside - get rid of HashTable. I could not look
> into it anymore... And
> remover Reflect class. It is not used. It can be removed as separate
> changeset without issues. But
> please dont wont me to separate heshtable removal.
>
>
> Except fact that it is buildable, now all (or selected) applets on page can
> be run. Docs will
> probably need a bit of tuning.
>
> Unluckily I had not fixed quite lot of your nits :(( See the replies nline.
>
>
> btw - is there some voulenteer to test it O:) (like try to hack it? and add
> reproducers O:))
>
> On 12/15/2014 09:23 PM, Jie Kang wrote:
> > 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?
> >
>
> Why not? It is perfectly ok. This is utility method converting one stream to
> another. Bounded with
> tagsoup optional dependence. afaik it is best what could be done with this
> method.
Okay.
> > 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).
>
>
> This is very god catch, unluckily on two different cases:
> In parser, we are calling overridden method, to supply modified behavior to
> modified parser.
> In my AppletExtracter i'm really only calling this method, without compile
> time dependence.
I don't see a reason for getRootNode to not be static as well. There are no dependencies for MalformedXMLParser or XMLParser which would require object construction. An instance has no purpose here.
I think you should change getRootNode to static.
On a side note, ideally, MalformedXMLParser shouldn't extend XMLParser, they should both implement a common interface that requires the function 'getRootNode' to be implemented.
>
> >
> >
> > 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.
>
> This would force creation of JNLPinterface, and both those classes to
> implement it. An plugin bridge
> will contains jnlpfile instance anyway. I'm not sure if final result is
> worthy of the huge effort.
I meant something like:
public class PluginBridge {
private JNLPFile jnlpFile;
public PluginBridge(...} {
jnlpFile = ...
}
}
Right now you create a JNLPFile inside of PluginBridge's constructor, grab a few useful things, and then throw it away.
Everytime we need more from the JNLPFile, we have to edit the constructor to grab more of it. As well, since PluginBridge extends JNLPFile, all of the methods of JNLPFile can be called, even though the huge majority of them fail to work. This is hugely unintuitive and breaks the whole idea of objects and implementation inheritance.
The above change would solve these issues quite easily.
Anyways, this isn't related to this patch, just a thought.
If you'd like, I can work on something and post for review?
>
> >
> >
> > 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/
>
> fixed. Actually Ihave split this class to three smaller.
>
> > + 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?
>
> I wish so!
>
> However I dont know any way hw to do case-insensitive match on atributes on
> sax-compliant parsers.
> Instead of hardcoded list, I can do some permutations but afaik this should
> be enough....
I see. Well with a little research, I think the case-sensitivity is intended.
> >
> >
> > + 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.
>
> Well, this was intentional :)
>
> The applet element is passed in, where it is serving as source element for
> any other processing... kept.
>
>
> >
> > Can you also add some comments to what AppletParser is parsing from and
> > what it is parsing into?
>
> done.
> >
> > + 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.
>
> Not exactly. It is the code copypasted from in-browser resize. In time of
> writing I had notestcase
> where to try it. So I would notremove - but - in meantime i made all jogamp
> and similar appelts run
> and found that resizing is working perfectl fien even with my appletcontext
> stub. So removed even
> with listener.
> >
> > 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.
>
>
> removed.
> >
> >
> > 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?
>
> yaaah!!! I was wondering why it looked so nasty :DD Fixed!
> >
> > + }, 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.
A few more nits in-code below:
diff -r 4ac8a8cb0dec netx/net/sourceforge/jnlp/NetxPanel.java
--- a/netx/net/sourceforge/jnlp/NetxPanel.java Wed Dec 17 07:47:31 2014 +0100
+++ b/netx/net/sourceforge/jnlp/NetxPanel.java Thu Dec 18 16:56:52 2014 +0100
@@ -65,11 +65,11 @@
[...]
+ public AppletInstance getAppInst() {
+ return appInst;
+ }
+
+
+
@Override
protected void showAppletException(Throwable t) {
Extra spaces;;
[...]
+
+ }
+
}
Extra spaces;;
diff -r 4ac8a8cb0dec netx/net/sourceforge/jnlp/runtime/Boot.java
--- a/netx/net/sourceforge/jnlp/runtime/Boot.java Wed Dec 17 07:47:31 2014 +0100
+++ b/netx/net/sourceforge/jnlp/runtime/Boot.java Thu Dec 18 16:56:52 2014 +0100
@@ -16,9 +16,10 @@
+ if (optionParser.hasOption(OptionsDefinitions.OPTIONS.HTML)) {
+ List<String> vars = optionParser.getParams(OptionsDefinitions.OPTIONS.HTML);
+ JNLPRuntime.setForksAllowed(false);//needed?
+ ParserSettings settings = init(extra);
+ if (settings == null){
+ return null;
+ }
+ try {
+ OutputController.getLogger().log("Proceeding with html");
+ final URL html = getFileLocation();
+ AppletExtractor axe = new AppletExtractor(html);
+ AppletsFilter filtered = new AppletsFilter(axe.findAppletsOnPage(), html, vars.subList(1, vars.size()));
+ List<AppletParser> applets = filtered.getApplets();
+ // this hack was needed in early phases of the patch. Now it sees to be not neede. Keeping inside to remove after much more testing
+ //will be replaced by regular JNLPRuntime is initialised
+// System.setSecurityManager(new SecurityManager() {
+//
+// @Override
+// public void checkPermission(Permission perm) {
+// //
+// }
+//
+// });
+ final int[] move= new int[]{0,0,0};
+ for (AppletParser appletParser : applets) {
+ //place the applets correctly over screen
+ if (move[0] > 0) {
+ if (move[0] % 2 == 0) {
+ move[1] += 100;
+ } else {
+ move[2] += 100;
+ }
+ }
+ final PluginBridge pb = appletParser.toPluginBridge();
+ final JFrame f = invokePluginMain(pb, html);
+ //close all applets in time
+ f.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
+ //f.setDefaultCloseOperation(JFrame.DISPOSE_ON_CLOSE);
+ SwingUtilities.invokeLater(new Runnable() {
+ @Override
+ public void run() {
+ int x = move[1];
+ int y = move[2];
+ switch (move[0] % 4) {
+ case 0:
+ x=-x;
+ y=y;
+ break;
+ case 1:
+ x=x;
+ y=y;
+ break;
+ case 2:
+ x=x;
+ y=-y;
+ break;
+ case 3:
+ x=-x;
+ y=-y;
+ break;
+ }
+ f.pack();
+ ScreenFinder.centerWindowsToCurrentScreen(f);
+ Rectangle r = f.getBounds();
+ r.x+=x;
+ r.y+=y;
+ f.setBounds(r);
+ f.setVisible(true);
+ }
+ });
+ move[0] ++;
+ }
Biggest nit here: Please refactor this into multiple functions (at least 1).
+ return null;
+ }
+
+
+ extra.put("arguments", optionParser.getParams(OptionsDefinitions.OPTIONS.ARG));
+ extra.put("parameters", optionParser.getParams(OptionsDefinitions.OPTIONS.PARAM));
Extra spaces
diff -r 4ac8a8cb0dec netx/net/sourceforge/jnlp/runtime/html/AppletExtractor.java
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/netx/net/sourceforge/jnlp/runtime/html/AppletExtractor.java Thu Dec 18 16:56:52 2014 +0100
@@ -0,0 +1,164 @@
[...]
+ }
+
+
+
+ public List<Element> findAppletsOnPage() {
+ try{
Extra spaces
diff -r 4ac8a8cb0dec netx/sun/applet/AppletViewerPanelAccess.java
--- a/netx/sun/applet/AppletViewerPanelAccess.java Wed Dec 17 07:47:31 2014 +0100
+++ b/netx/sun/applet/AppletViewerPanelAccess.java Thu Dec 18 16:56:52 2014 +0100
@@ -33,16 +33,21 @@
[...]
}
+
+ @Override
+ public AppletContext getAppletContext() {
Extra space
diff -r 4ac8a8cb0dec plugin/icedteanp/java/sun/applet/PluginMain.java
--- a/plugin/icedteanp/java/sun/applet/PluginMain.java Wed Dec 17 07:47:31 2014 +0100
+++ b/plugin/icedteanp/java/sun/applet/PluginMain.java Thu Dec 18 16:56:52 2014 +0100
@@ -73,10 +73,12 @@
+
+
+
+ public static JFrame javawsHtmlMain(PluginBridge pb, URL html) {
Extra space
Apart from these, everything else looks fine.
Regards
>
>
> Thank you for check!
>
>
> Sorry for not obeying:(
>
--
Jie Kang
More information about the distro-pkg-dev
mailing list