[rfc][icedtea-web] bringing applets out of browser (part2)
Jie Kang
jkang at redhat.com
Thu Dec 18 19:29:14 UTC 2014
----- Original Message -----
>
>
> ----- 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).
Something like:
if (optionParser.hasOption(OptionsDefinitions.OPTIONS.HTML)) {
bootAsHtml(...)
} else {
[...]
}
private void bootAsHtml(...) {
[...]
}
>
>
> + 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
--
Jie Kang
More information about the distro-pkg-dev
mailing list