[rfc][icedtea-web] bringing applets out of browser (part1)
Jie Kang
jkang at redhat.com
Fri Dec 12 16:12:36 UTC 2014
Hello,
Review in code below:
diff -r 52160fef5621 netx/net/sourceforge/jnlp/JNLPFile.java
--- a/netx/net/sourceforge/jnlp/JNLPFile.java Fri Dec 05 18:21:52 2014 +0100
+++ b/netx/net/sourceforge/jnlp/JNLPFile.java Fri Dec 05 19:57:14 2014 +0100
@@ -294,7 +294,7 @@
[...]
- private static InputStream openURL(URL location, Version version, UpdatePolicy policy) throws IOException {
+ public static InputStream openURL(URL location, Version version, UpdatePolicy policy) throws IOException {
This part might have a number of un-wanted side-effects.
I think you should consider adding a new function: public static String getJNLPFileAsString(...) and use that instead of making this public.
+ OutputController.getLogger().log((debugJnlp==null)?"null":debugJnlp);
Can you fix the spacing here to: (debugJnlp==null) ? "null" : debugJnlp
[...]
+ public String toJnlp(boolean needSecurity, boolean useHref, boolean fix) {
+ if (useJNLPHref && debugJnlp!=null && useHref) {
+ OutputController.getLogger().log("Using debugjnlp as return value toJnlp");
+ if (fix) {
+ return fixCommonIsuses(needSecurity, debugJnlp);
Indentation
+ } else {
[...]
+ s.append(" </resources>\n");
+ s.append(" <applet-desc\n");
+ s.append(" name='").append(getTitle()).append("'\n");
+ s.append(" main-class='").append(getStrippedMain()).append("'\n");
+ s.append(" width='").append(getApplet().getWidth()).append("'\n");
+ s.append(" height='").append(getApplet().getHeight()).append("'>\n");
+ if (!getApplet().getParameters().isEmpty()){
+ Set<Map.Entry<String,String>> prms = getApplet().getParameters().entrySet();
+ for (Map.Entry<String, String> entry : prms) {
+ s.append(" <param name='").append(entry.getKey()).append("' value='").append(entry.getValue()).append("'/>\n");
+ }
+ }
+ s.append(" </applet-desc>\n");
+ s.append("</jnlp>\n");
+ OutputController.getLogger().log("toJnlp generated:");
+ OutputController.getLogger().log(s.toString());
+ return s.toString();
Indentation
[...]
+ public static String strippClkass(String s) {
s/Clkass/Class
[...]
+ private String fixCommonIsuses(boolean needSecurity, String orig) {
[...]
+ static String fixCommonIsuses(boolean needSecurity, String orig, String codebase, String title, String vendor) {
Can you reorder the functions to be together?
[...]
+ //are tested
Can you expand on the comment here: tested for what?
+ static final String SANDBOX_REGEX = toBaseRegex("sandbox", false);
+ static final String CLOSE_INFORMATION_REGEX = toBaseRegex("information",true);
+ static final String SECURITY_REGEX = toBaseRegex("security", false);
[...]
+ OutputController.getLogger().log("no information element Found. Trying to fix");
+ OutputController.getLogger().log("jnlphreff dif not had codebase. Fixing");
+ OutputController.getLogger().log("'.' codebase found. fixing");
+ OutputController.getLogger().log("all-permissions not found and app is signed.");
+ OutputController.getLogger().log("adding sdecurity element");
Please use consistent punctuation and capitalization. s/jnlphreff/jnlphref s/dif/did s/sdecurity/security
[...]
+ private AccessWarningPaneComplexReturn shouldCreateShortcut(ShortcutDesc sd) {
This name is quite hard to understand. What is a ComplexReturn? Can you think of a name that describes exactly what it provides?
Maybe we should consider replacing AccessWarningPane with this class slightly expanded?
[...]
+ HtmlShortuctPanel htmlPanelDesktop;
+ HtmlShortuctPanel htmlPanelMenu;
+ RemeberPanel remberPanel;
s/HtmlShortuctPanel/HtmlShortcutPanel s/RemeberPanel/RememberPanel
[...]
+ JRadioButton byApp = new JRadioButton("rembere by application");
+ JRadioButton byPage = new JRadioButton("rembere by domain");
+ JRadioButton dont = new JRadioButton("don't rember", true);
s/rembere/remember s/rember/remember
[...]
+ byApp.setToolTipText("<html>This application will never ask more permissions questions</html>");
I think a more appropriate message would be "Remember permissions for this application and do not ask again"
+ byPage.setToolTipText("<html>All applications from this domain will stop asking, and will follow your current decission on all permissions</html>");
"Remember permissions for all applications for this domain and do not ask again"
+ dont.setToolTipText("<html>Your decission will be used only for this single permission for this single run</html>");
"Do not remember permissions"
[...]
+ public HtmlShortuctPanel() {
+ super(new FlowLayout(FlowLayout.CENTER, 1, 5));
+ this.setBorder(new EmptyBorder(0, 0, 0, 0));
+ this.add(browser);
+ browsers.setEditable(true);
+ browsers.setSelectedItem(XDesktopEntry.getBrowserBin());
+ this.add(browsers);
+ this.add(jnlpGen);
+ this.add(jnlpHref);
+ this.add(javawsHtml);
+ this.add(fix);
+ browser.setToolTipText("<html>Browser shortcut<br><li>This option will create shortcut to open your browser with current page loaded</li><li>If your browser support offline run, this is the safest option</li></html>");
+ browsers.setToolTipText("<html>browser used for lunching this applet (will lunch icedtea-web later)<br><li>The default browser was preset</li><li>Feel free to include browser of your choice</li></html>");
+ jnlpGen.setToolTipText("<html><li>The jnlp file will be generated from your current html page</li><li>Once you lunch jor shortcut, javaws will lunch this jnlp file</li><li>This applet will then run <b>without</b> the browser</li><li>However experimental, this is working surprisingly well.</li></html>");
+ jnlpHref.setToolTipText("<html>Some aplets are jsut pointing to jnlp file, which is containing actual informations about the resources of this app.<br><li>By selecting this option, this jnlp file will be saved and used for later lunches.</li><li>Javaws will be the luncher, and this applet will run <b>out</b> of browser</li><li>Howeer good htis sounds, this si surprisingly not working</li></html>");
+ javawsHtml.setToolTipText("<html>BY using -html switch, javaws can try to parse html and try to extract applet, and to lunch it out of browser<br><li>highly experimental</li><li>really cool</li><li>not yet implemented</li></html>");
+ fix.setToolTipText("<html>Some jnlps pointed from applet are not designed to be used as jnlp apps<br><li>This will add the known often missing elements to this file (if missing)</li></html>");
+ ButtonGroup bg = new ButtonGroup();
+ bg.add(browser);
+ bg.add(jnlpGen);
+ bg.add(jnlpHref);
+ bg.add(javawsHtml);
+ browser.addActionListener(al);
+ jnlpGen.addActionListener(al);
+ jnlpHref.addActionListener(al);
+ javawsHtml.addActionListener(al);
+ al.actionPerformed(null);
+ this.validate();
+
+ }
Can you add more spacing in this constructor to make it easier to read. As well, it would be great if you could split it into multiple functions with readable names. That way we can understand what each part is doing.
E.g.
public HtmlShortuctPanel() {
addComponents()
setupTooltipTexts()
setupButtonGroup()
setupActionListener()
[...]
}
[..]
diff -r 52160fef5621 netx/net/sourceforge/jnlp/security/dialogs/AccessWarningPaneComplexReturn.java
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/netx/net/sourceforge/jnlp/security/dialogs/AccessWarningPaneComplexReturn.java Fri Dec 05 19:57:14 2014 +0100
@@ -0,0 +1,148 @@
[...]
+
+
+
+
+
+
+
+
Please remove these spaces :)
- Integer buttonIndex;
+ Object buttonIndex;
I think using Object directly is not very safe. How about making a new class to represent this?
[...]
diff -r 52160fef5621 netx/net/sourceforge/jnlp/util/XDesktopEntry.java
--- a/netx/net/sourceforge/jnlp/util/XDesktopEntry.java Fri Dec 05 18:21:52 2014 +0100
+++ b/netx/net/sourceforge/jnlp/util/XDesktopEntry.java Fri Dec 05 19:57:14 2014 +0100
@@ -40,12 +40,14 @@
[...]
+ }catch (Exception ex){
Spacing here
+ OutputController.getLogger().log(ex);
+ }
[...]
+ }catch (NonFileProtocolException ex){
Same as above.
+ OutputController.getLogger().log(ex);
+ //default icon will be used later
+ }
[...]
-
- String origLocation = location.substring("file:".length());
+ }else {
Same as above.
[...]
+ try{
+ URL favico = new URL(
+ file.getCodeBase().getProtocol(),
+ file.getCodeBase().getHost(),
+ file.getCodeBase().getPort(),
+ "/"+FAVICON);
+ JNLPFile.openURL(favico, null, UpdatePolicy.ALWAYS);
+ location = CacheUtil.getCachedResource(favico, null, UpdatePolicy.SESSION)
+ .toString();
+ if (!location.startsWith("file:")) {
+ throw new NonFileProtocolException("Unable to cache icon");
+ }
+ } catch (IOException ex) {
+ //favicon 404 or similar
+ OutputController.getLogger().log(ex);
+ }
Indentation is off.
[...]
- File f = PathsAndFiles.MENUS_DIR.getFile();
+ return findAndVerifyBasicDir(PathsAndFiles.MENUS_DIR.getFile(), " directroy for stroing menu entry cannot be created. You may expect failure");
Same as above
+ if (stringEqualsOption(arg, OptionsDefinitions.OPTIONS.HTML)) {
+ throw new IllegalArgumentException("-html switch is not yet supported");
+ }
Indentation for closing bracket
[...]
+ @Test
+ public void stripClassNoClass() throws Exception {
Indentation for @Test
I like all the tests :D
So far I haven't found any bugs in manual user-testing of simple applets. I don't really expect this to work for complex things 100% of the time though.
If I find anything new I'll let you know :D
My one question is:
What will happen when someone tries to do this and it doesn't work? Please make sure it is a good error message or some information, not just crashing.
Great stuff! :D
Regards,
----- Original Message -----
>
>
> ----- Original Message -----
> > not done - remembering of values from AccessWarningPane and -html switch
> > (all
> > will go as another
> > changesets)
> > not tested - create always setting (without asking user)
> > in next iteration - localization of messages
> >
> > Ufghh.. this is tought one. It is adding possibility to create desktop/menu
> > shortcut which leads to
> > appelt directly, not via browser. To do so, many tweeks to previous patch
> > were needed)
> >
> > long stroy short:
> > set .config/icedtea-web/deployment.properties
> > deployment.javaws.shortcut=ASK_USER
> >
> > and run some appelt in browser
> >
> > Get scared by new desktop shortcut dialogue (there is a lot of explaining
> > tooltips!)
> > And then prize be the light!
> >
> >
> > disclaimer - although I have tested pretty hard, I doubt I have covered all
> > cornercases of jnlp
> > generation. Whatever will be found, I will try to fix now or in another
> > chnagesets...
> > Also note, that appelts using javascript or some long-into-bank applets
> > will
> > probably never work out
> > of browser... But still, majority of applets *will* work. And those old
> > simple applets are target
> > audience of this patch.
> >
> > J.
> >
> > ps: I'm going to speak about this on defconf on start of february, and I'm
> > on
> > vacation 20.12-20.1 so
> > if "anybody" (sorry Jie :) ) can look over it...I will really appreciate:(
> > Sorry for late patch...
> >
>
> Hello,
>
> I just wanted to let you know I've started reviewing and testing, however the
> patch is quite large so it will take me some time.
>
>
> Cheers,
>
>
> --
>
> Jie Kang
>
--
Jie Kang
More information about the distro-pkg-dev
mailing list